linux-kernel - Re: [net PATCH] net: phy: aquantia: fix wrong GENMASK define for LED_PROV_ACT

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: <682076b3.5d0a0220.2603de.62cd@mx.google.com>
Date: Sun, 11 May 2025 12:06:41 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Daniel Golle <daniel@...rotopia.org>,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [net PATCH] net: phy: aquantia: fix wrong GENMASK define for
 LED_PROV_ACT_STRETCH

On Sun, May 11, 2025 at 10:57:10AM +0100, Russell King (Oracle) wrote:
> On Sun, May 11, 2025 at 11:06:17AM +0200, Christian Marangi wrote:
> > In defining VEND1_GLOBAL_LED_PROV_ACT_STRETCH there was a typo where the
> > GENMASK definition was swapped.
> > 
> > Fix it to prevent any kind of misconfiguration if ever this define will
> > be used in the future.
> 
> I thought GENMASK() was supposed to warn about this kind of thing. I've
> questioned in the past whether GENMASK() is better than defining fields
> with hex numbers, and each time I see another repeat of this exact case,
> I re-question whether GENMASK() actually gives much benefit over hex
> numbers because it's just too easy to get the two arguments to
> GENMASK() swapped and it's never obvious that's happened.
>

Maybe there are warning but since this define wasn't actually used they
are not triggered?

Honestly GENMASK is a saviour as from the dev point it's much easier to
understand the mask this way than raw hex.

Also most of the programming documentation (or at least the good one)
always use this pattern of defining a table with range of bits soo
translating that to the driver with the define is only a matter of
copying the range number.

It's also worth to consider that converting bit range to raw hex might
also introduce error and probably nobody would ever notice them compared
to the much clear GENMASK macro.

Aside from this, if no check are placed for GENMASK macro then they
should be easy to implement? Simple logic should be applied like

GENMASK(x, y)

x > y should be always true.

Actually I wonder...

with GENMASK(0, 1) what kind of mask is getting created?

> I don't remember there being a dribble of patches in the past
> correcting bitfields defined using hex numbers, but that seems common
> with GENMASK().
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

-- 
	Ansuel