netdev - Re: [PATCH net-next v2] net: smc91x: Refactor SMC

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: <20240601165342.GS491852@kernel.org>
Date: Sat, 1 Jun 2024 17:53:42 +0100
From: Simon Horman <horms@...nel.org>
To: Thorsten Blum <thorsten.blum@...lux.com>
Cc: Nicolas Pitre <nico@...xnic.net>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Breno Leitao <leitao@...ian.org>,
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>,
	Andrew Lunn <andrew@...n.ch>, Arnd Bergmann <arnd@...db.de>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2] net: smc91x: Refactor SMC_* macros

On Fri, May 31, 2024 at 02:01:04PM +0200, Thorsten Blum wrote:
> Use the macro parameter lp directly instead of relying on ioaddr being
> defined in the surrounding scope.
> 
> The macros SMC_CURRENT_BANK(), SMC_SELECT_BANK(), SMC_GET_BASE(), and
> SMC_GET_REV() take an additional parameter ioaddr to use a different
> address if necessary (e.g., as in smc_probe()).
> 
> Relying on implicitly defined variable names in C macros is generally
> considered bad practice and can be avoided by using explicit parameters.
> 
> Compile-tested only.
> 
> Suggested-by: Andrew Lunn <andrew@...n.ch>
> Signed-off-by: Thorsten Blum <thorsten.blum@...lux.com>
> ---
> Changes in v2:
> - Add macro parameter ioaddr where necessary to fix smc_probe() after
>   feedback from Jakub Kicinski
> - Update patch description
> - Link to v1: https://lore.kernel.org/linux-kernel/20240528104421.399885-3-thorsten.blum@toblux.com/
> ---
>  drivers/net/ethernet/smsc/smc91x.c | 132 +++++++++++--------------
>  drivers/net/ethernet/smsc/smc91x.h | 152 ++++++++++++++---------------
>  2 files changed, 131 insertions(+), 153 deletions(-)

Hi Thorsten,

This is a large and repetitive patch, which makes it hard to spot any errors
(I couldn't see any :)

I'm not sure if it is worth doing at this point,
but it might have been worth splitting the patch up,
f.e. by addressing one MACRO at a time, removing
local ioaddr variables as they become unused.

As highlighted bellow, checkpatch flags a lot of issues with
the code updated by this patch. Perhaps they could be addressed
as the lines with issues are updated. Or by patch(es) before
those that make the changes made by this patch. Or some combination
of the two.

...

> diff --git a/drivers/net/ethernet/smsc/smc91x.h b/drivers/net/ethernet/smsc/smc91x.h

...

> @@ -839,64 +839,64 @@ static const char * chip_ids[ 16 ] =  {
>  #define SMC_MUST_ALIGN_WRITE(lp)	SMC_32BIT(lp)
>  
>  #define SMC_GET_PN(lp)						\
> -	(SMC_8BIT(lp)	? (SMC_inb(ioaddr, PN_REG(lp)))	\
> -				: (SMC_inw(ioaddr, PN_REG(lp)) & 0xFF))
> +	(SMC_8BIT(lp)	? (SMC_inb(lp->base, PN_REG(lp)))	\
> +				: (SMC_inw(lp->base, PN_REG(lp)) & 0xFF))
>  
>  #define SMC_SET_PN(lp, x)						\
>  	do {								\
>  		if (SMC_MUST_ALIGN_WRITE(lp))				\
> -			SMC_outl((x)<<16, ioaddr, SMC_REG(lp, 0, 2));	\
> +			SMC_outl((x)<<16, lp->base, SMC_REG(lp, 0, 2));	\

While we are here, I think we can address the whitespace issues
flagged by checkpatch - there should be spaces around '<<'.

Likewise elsewhere.

...

> -#define SMC_CURRENT_BANK(lp)	SMC_inw(ioaddr, BANK_SELECT)
> +#define SMC_CURRENT_BANK(lp, ioaddr)	SMC_inw(ioaddr, BANK_SELECT)

lp is not used in this macro, can it be removed?
Possibly as a follow-up?

Flagged by checkpatch.

>  
> -#define SMC_SELECT_BANK(lp, x)					\
> +#define SMC_SELECT_BANK(lp, x, ioaddr)					\
>  	do {								\
>  		if (SMC_MUST_ALIGN_WRITE(lp))				\
>  			SMC_outl((x)<<16, ioaddr, 12<<SMC_IO_SHIFT);	\
> @@ -904,125 +904,125 @@ static const char * chip_ids[ 16 ] =  {
>  			SMC_outw(lp, x, ioaddr, BANK_SELECT);		\
>  	} while (0)
>  
> -#define SMC_GET_BASE(lp)		SMC_inw(ioaddr, BASE_REG(lp))
> +#define SMC_GET_BASE(lp, ioaddr)	SMC_inw(ioaddr, BASE_REG(lp))
>  
> -#define SMC_SET_BASE(lp, x)	SMC_outw(lp, x, ioaddr, BASE_REG(lp))
> +#define SMC_SET_BASE(lp, x)	SMC_outw(lp, x, lp->base, BASE_REG(lp))

lp is now evaluated twice in this macro.
As the motivation of this patch seems to be about good practice,
perhaps we should avoid introducing a bad one?

Likewise in several other macros.

Flagged by checkpatch.

...

> -#define SMC_GET_CTL(lp)		SMC_inw(ioaddr, CTL_REG(lp))
> +#define SMC_GET_CTL(lp)		SMC_inw(lp->base, CTL_REG(lp))

I don't think this is introduced by this patch,
but perhaps it could be addressed.

Checkpatch says: Macro argument 'lp' may be better as '(lp)' to avoid precedence issues

Likewise elsewhere.

...