[net PATCH] net: phy: aquantia: fix wrong GENMASK define for LED_PROV_ACT_STRETCH

[net PATCH] net: phy: aquantia: fix wrong GENMASK define for LED_PROV_ACT_STRETCH

[Christian Marangi]

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.

Cc: <stable@vger.kernel.org>
Fixes: 61578f679378 ("net: phy: aquantia: add support for PHY LEDs")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/aquantia/aquantia.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/aquantia/aquantia.h b/drivers/net/phy/aquantia/aquantia.h
index 0c78bfabace5..e2bb66a21589 100644
--- a/drivers/net/phy/aquantia/aquantia.h
+++ b/drivers/net/phy/aquantia/aquantia.h
@@ -76,7 +76,7 @@
 #define VEND1_GLOBAL_LED_PROV_LINK100		BIT(5)
 #define VEND1_GLOBAL_LED_PROV_RX_ACT		BIT(3)
 #define VEND1_GLOBAL_LED_PROV_TX_ACT		BIT(2)
-#define VEND1_GLOBAL_LED_PROV_ACT_STRETCH	GENMASK(0, 1)
+#define VEND1_GLOBAL_LED_PROV_ACT_STRETCH	GENMASK(1, 0)
 
 #define VEND1_GLOBAL_LED_PROV_LINK_MASK		(VEND1_GLOBAL_LED_PROV_LINK100 | \
 						 VEND1_GLOBAL_LED_PROV_LINK1000 | \
-- 
2.48.1

Re: [net PATCH] net: phy: aquantia: fix wrong GENMASK define for LED_PROV_ACT_STRETCH

[Russell King (Oracle)]

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.

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!

Re: [net PATCH] net: phy: aquantia: fix wrong GENMASK define for LED_PROV_ACT_STRETCH

[Christophe JAILLET]

Le 11/05/2025 à 11:57, Russell King (Oracle) a écrit :
> 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.
> 
> 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().
> 

There is a compile time check, but in this case
VEND1_GLOBAL_LED_PROV_ACT_STRETCH looks unused. So it is never expanded 
and compiled.

CJ

Re: [net PATCH] net: phy: aquantia: fix wrong GENMASK define for LED_PROV_ACT_STRETCH

[Jakub Kicinski]

On Sun, 11 May 2025 12:06:38 +0200 Christophe JAILLET wrote:
> There is a compile time check, but in this case
> VEND1_GLOBAL_LED_PROV_ACT_STRETCH looks unused. So it is never expanded 
> and compiled.

Thanks!

Given this the patch is obviously not a fix.

Re: [net PATCH] net: phy: aquantia: fix wrong GENMASK define for LED_PROV_ACT_STRETCH

[Christian Marangi]

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

Re: [net PATCH] net: phy: aquantia: fix wrong GENMASK define for LED_PROV_ACT_STRETCH

[Andrew Lunn]

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.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 61578f679378 ("net: phy: aquantia: add support for PHY LEDs")
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew