diff mbox series

[net-next,v2,1/7] drivers: net: smc91x: Fix set but unused W=1 warning

Message ID 20201104154858.1247725-2-andrew@lunn.ch
State Changes Requested
Delegated to: David Miller
Headers show
Series smsc W=1 warning fixes | expand

Checks

Context Check Description
jkicinski/stable success Stable not CCed
jkicinski/header_inline success Link
jkicinski/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
jkicinski/checkpatch success total: 0 errors, 0 warnings, 0 checks, 32 lines checked
jkicinski/verify_fixes success Link
jkicinski/kdoc success Errors and warnings before: 6 this patch: 6
jkicinski/build_32bit success Errors and warnings before: 0 this patch: 0
jkicinski/module_param success Was 0 now: 0
jkicinski/verify_signedoff success Link
jkicinski/source_inline success Was 0 now: 0
jkicinski/subject_prefix success Link
jkicinski/tree_selection success Clearly marked for net-next
jkicinski/patch_count success Link
jkicinski/fixes_present success Link
jkicinski/cover_letter success Link

Commit Message

Andrew Lunn Nov. 4, 2020, 3:48 p.m. UTC
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(-)

Comments

Jakub Kicinski Nov. 5, 2020, 10:37 p.m. UTC | #1
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?
David Laight Nov. 6, 2020, 8:48 a.m. UTC | #2
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)
Jakub Kicinski Nov. 6, 2020, 5:42 p.m. UTC | #3
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 mbox series

Patch

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)) {				\