* Re: [PATCH net] net: dsa: microchip: Fix a link check in ksz9477_pcs_read()
2025-10-31 13:05 [PATCH net] net: dsa: microchip: Fix a link check in ksz9477_pcs_read() Dan Carpenter
@ 2025-10-31 14:05 ` Maxime Chevallier
2025-11-06 2:10 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: Maxime Chevallier @ 2025-10-31 14:05 UTC (permalink / raw)
To: Dan Carpenter, Tristram Ha, Harshit Mogalapalli
Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vladimir Oltean,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel, kernel-janitors
Hi Dan,
On 31/10/2025 14:05, Dan Carpenter wrote:
> The BMSR_LSTATUS define is 0x4 but the "p->phydev.link" variable
> is a 1 bit bitfield in a u32. Since 4 doesn't fit in 0-1 range
> it means that ".link" is always set to false. Add a !! to fix
> this.
>
> Fixes: e8c35bfce4c1 ("net: dsa: microchip: Add SGMII port support to KSZ9477 switch")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Interesting issue. I was able to confirm that your patch is
correct by testing on ksz9477 (p->phydev.link stays at 0 no matter
what without this fix), but I was wondering why things weren't
broken even with this bug.
The first thing is that this path is only taken when using 1000BaseX
on that port. I've tested with 1000BaseX, and I'm still getting link
up. That's because we don't do anything at all with p->phydev.link.
The "val" value is returned, and interpreted by the pcs-xpcs.c driver,
who correctly uses "!!(val & BMSR_LSTATUS)", reports the link state to
phylink, and link shows as up/down as expected.
Looking at the code, it seems that p->phydev is almost completely useless,
and is merely used to store the current speed/link/duplex settings.
So all in all your patch is correct and can be merged, but it doesn't
fix a real bug, and the long term thing to do would simply be to get
rid of p->phydev...
Maybe someone from Microchip can comment on that and why we have that
p->phydev, and can we safely remove that ? We could replace it with
3 fields in ksz_port : link, speed and duplex.
Maxime
> ---
> This is from a new static checker warning Harshit and I wrote. Untested.
>
> drivers/net/dsa/microchip/ksz9477.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index d747ea1c41a7..cf67d6377719 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -244,7 +244,7 @@ static int ksz9477_pcs_read(struct mii_bus *bus, int phy, int mmd, int reg)
> p->phydev.link = 0;
> }
> } else if (reg == MII_BMSR) {
> - p->phydev.link = (val & BMSR_LSTATUS);
> + p->phydev.link = !!(val & BMSR_LSTATUS);
> }
> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] net: dsa: microchip: Fix a link check in ksz9477_pcs_read()
2025-10-31 13:05 [PATCH net] net: dsa: microchip: Fix a link check in ksz9477_pcs_read() Dan Carpenter
2025-10-31 14:05 ` Maxime Chevallier
@ 2025-11-06 2:10 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-11-06 2:10 UTC (permalink / raw)
To: Dan Carpenter
Cc: tristram.ha, harshit.m.mogalapalli, woojung.huh, UNGLinuxDriver,
andrew, olteanv, davem, edumazet, kuba, pabeni, maxime.chevallier,
netdev, linux-kernel, kernel-janitors
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Fri, 31 Oct 2025 16:05:01 +0300 you wrote:
> The BMSR_LSTATUS define is 0x4 but the "p->phydev.link" variable
> is a 1 bit bitfield in a u32. Since 4 doesn't fit in 0-1 range
> it means that ".link" is always set to false. Add a !! to fix
> this.
>
> Fixes: e8c35bfce4c1 ("net: dsa: microchip: Add SGMII port support to KSZ9477 switch")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>
> [...]
Here is the summary with links:
- [net] net: dsa: microchip: Fix a link check in ksz9477_pcs_read()
https://git.kernel.org/netdev/net-next/c/c79a02252457
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread