| Message ID | 20201104154858.1247725-1-andrew@lunn.ch |
|---|---|
| Headers | show |
| Series | smsc W=1 warning fixes | expand |
On Wed, 4 Nov 2020 16:48:52 +0100 Andrew Lunn wrote: > drivers/net/ethernet/smsc/smc91x.c:706:51: warning: variable ‘pkt_len’ set but not used [-Wunused-but-set-variable] > 706 | unsigned int saved_packet, packet_no, tx_status, pkt_len; > > Add a new macro for getting fields out of the header, which only gets > the status, not the length which in this case is not needed. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> Sorry I missed something on v1 > +#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?
From: Jakub Kicinski > Sent: 05 November 2020 22:38 > On Wed, 4 Nov 2020 16:48:52 +0100 Andrew Lunn wrote: > > drivers/net/ethernet/smsc/smc91x.c:706:51: warning: variable ‘pkt_len’ set but not used [-Wunused- > but-set-variable] > > 706 | unsigned int saved_packet, packet_no, tx_status, pkt_len; > > > > Add a new macro for getting fields out of the header, which only gets > > the status, not the length which in this case is not needed. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > Sorry I missed something on v1 > > > +#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. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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.
Fixup various W=1 warnings, and then add COMPILE_TEST support, which explains why these where missed on the previous pass. One of these patches added as a new checkpatch warning, by copy/pasting an existing macro and slightly modifying it. The code already has lots of checkpatch warnings, so one more is not going to make that much difference. v2: Use while (0) Rework buffer alignment to make it clearer Andrew Lunn (7): drivers: net: smc91x: Fix set but unused W=1 warning drivers: net: smc91x: Fix missing kerneldoc reported by W=1 drivers: net: smc911x: Work around set but unused status drivers: net: smc911x: Fix set but unused status because of DBG macro drivers: net: smc911x: Fix passing wrong number of parameters to DBG() macro drivers: net: smc911x: Fix cast from pointer to integer of different size drivers: net: smsc: Add COMPILE_TEST support drivers/net/ethernet/smsc/Kconfig | 6 +++--- drivers/net/ethernet/smsc/smc911x.c | 19 +++++++++++-------- drivers/net/ethernet/smsc/smc91x.c | 10 ++++++++-- drivers/net/ethernet/smsc/smc91x.h | 10 ++++++++++ 4 files changed, 32 insertions(+), 13 deletions(-)