netdev - Re: [PATCH net-next v2 3/4] net: phy: micrel: Replace hardcoded pages with defines

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: <aIKbaS8ASndR7Xe_@shell.armlinux.org.uk>
Date: Thu, 24 Jul 2025 21:45:29 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc: andrew@...n.ch, hkallweit1@...il.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	richardcochran@...il.com, o.rempel@...gutronix.de,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 3/4] net: phy: micrel: Replace hardcoded
 pages with defines

On Thu, Jul 24, 2025 at 10:08:25PM +0200, Horatiu Vultur wrote:
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index b04c471c11a4a..d20f028106b7d 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -2788,6 +2788,13 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev,
>  	return ret;
>  }
>  
> +#define LAN_EXT_PAGE_0					0
> +#define LAN_EXT_PAGE_1					1
> +#define LAN_EXT_PAGE_2					2
> +#define LAN_EXT_PAGE_4					4
> +#define LAN_EXT_PAGE_5					5
> +#define LAN_EXT_PAGE_31					31

I don't see the point of this change. This is almost as bad as:

#define ZERO 0
#define ONE 1
#define TWO 2
#define THREE 3
...
#define ONE_HUNDRED_AND_FIFTY_FIVE 155
etc

It doesn't give us any new information, and just adds extra clutter,
making the code less readable.

The point of using register definitions is to describe the purpose
of the number, giving the number a meaning, not to just hide the
number because we don't want to see such things in C code.

I'm sorry if you were asked to do this in v1, but I think if you
were asked to do it, it would've been assuming that the definitions
could be more meaningful.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!