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 | \
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().
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
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!
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
linux-stable-mirror@lists.linaro.org
-
Andrew Lunn -
Christian Marangi -
Christophe JAILLET -
Jakub Kicinski -
Russell King (Oracle)