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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <749857e283f04d3b8f84f603fa065cd6@AcuMS.aculab.com>
Date:   Fri, 6 Nov 2020 08:48:47 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Jakub Kicinski' <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>
CC:     netdev <netdev@...r.kernel.org>, Nicolas Pitre <nico@...xnic.net>,
        "Lee Jones" <lee.jones@...aro.org>
Subject: RE: [PATCH net-next v2 1/7] drivers: net: smc91x: Fix set but unused
 W=1 warning

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@...n.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)