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>
---
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);
 		}
 	}
 
-- 
2.51.0

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);
>  		}
>  	}
>