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

[David Laight <David.Laight@ACULAB.COM>]

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)