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: <20201105143742.047959ed@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 5 Nov 2020 14:37:42 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev <netdev@...r.kernel.org>, Nicolas Pitre <nico@...xnic.net>,
        David Laight <David.Laight@...LAB.COM>,
        Lee Jones <lee.jones@...aro.org>
Subject: Re: [PATCH net-next v2 1/7] drivers: net: smc91x: Fix set but
 unused W=1 warning

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?