| Message ID | 20201104154858.1247725-2-andrew@lunn.ch |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | smsc W=1 warning fixes | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for net-next |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/kdoc | success | Errors and warnings before: 6 this patch: 6 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
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.
diff --git a/drivers/net/ethernet/smsc/smc91x.c b/drivers/net/ethernet/smsc/smc91x.c index f6b73afd1879..61333939a73e 100644 --- a/drivers/net/ethernet/smsc/smc91x.c +++ b/drivers/net/ethernet/smsc/smc91x.c @@ -703,7 +703,7 @@ static void smc_tx(struct net_device *dev) { struct smc_local *lp = netdev_priv(dev); void __iomem *ioaddr = lp->base; - unsigned int saved_packet, packet_no, tx_status, pkt_len; + unsigned int saved_packet, packet_no, tx_status; DBG(3, dev, "%s\n", __func__); @@ -720,7 +720,7 @@ static void smc_tx(struct net_device *dev) /* read the first word (status word) from this packet */ SMC_SET_PTR(lp, PTR_AUTOINC | PTR_READ); - SMC_GET_PKT_HDR(lp, tx_status, pkt_len); + SMC_GET_PKT_HDR_STATUS(lp, tx_status); DBG(2, dev, "TX STATUS 0x%04x PNR 0x%02x\n", tx_status, packet_no); diff --git a/drivers/net/ethernet/smsc/smc91x.h b/drivers/net/ethernet/smsc/smc91x.h index 387539a8094b..705d9d6ebaa5 100644 --- a/drivers/net/ethernet/smsc/smc91x.h +++ b/drivers/net/ethernet/smsc/smc91x.h @@ -1056,6 +1056,16 @@ static const char * chip_ids[ 16 ] = { } \ } while (0) +#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) + #define SMC_PUSH_DATA(lp, p, l) \ do { \ if (SMC_32BIT(lp)) { \
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> --- drivers/net/ethernet/smsc/smc91x.c | 4 ++-- drivers/net/ethernet/smsc/smc91x.h | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-)