On Fri, 6 Nov 2020 08:48:47 +0000 David Laight wrote:
> > > +#define SMC_GET_PKT_HDR_STATUS(lp, status)
> > > \
> > > + do { \
> > > + if (SMC_32BIT(lp)) { \
> > > + unsigned int __val = SMC_inl(ioaddr, DATA_REG(lp)); \
> > > + (status) = __val & 0xffff; \
> > > + } else { \
> > > + (status) = SMC_inw(ioaddr, DATA_REG(lp)); \
> > > + } \
> > > + } while (0)
> >
> > This is the original/full macro:
> >
> > #define SMC_GET_PKT_HDR(lp, status, length) \
> > do { \
> > if (SMC_32BIT(lp)) { \
> > unsigned int __val = SMC_inl(ioaddr, DATA_REG(lp)); \
> > (status) = __val & 0xffff; \
> > (length) = __val >> 16; \
> > } else { \
> > (status) = SMC_inw(ioaddr, DATA_REG(lp)); \
> > (length) = SMC_inw(ioaddr, DATA_REG(lp)); \
> > } \
> > } while (0)
> >
> > Note that it reads the same address twice in the else branch.
> >
> > I'm 90% sure we can't remove the read here either so best treat it
> > like the ones in patch 3, right?
>
> One of the two SMC_inw() needs to use 'ioaddr + 2'.
> Probably the one for (length).
>
> The code may also be buggy on BE systems.
More proof that this code is fragile.
Changing IO accesses is not acceptable in a "warning cleanup" patch,
unless it can be tested on real HW.
We can follow up on the issues you see separately, please.
