| Message ID | 20240703132801.623218-2-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1,1/2] net: phy: microchip: lan87xx: reinit PHY after cable test | expand |
On Wed, Jul 03, 2024 at 03:28:01PM +0200, Oleksij Rempel wrote: > Do not report SQI if no link is detected. Otherwise ethtool will show > non zero value even if no cable is attached. > > Fixes: b649695248b15 ("net: phy: LAN87xx: add ethtool SQI support") > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/microchip_t1.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c > index a35528497a576..22530a5b76365 100644 > --- a/drivers/net/phy/microchip_t1.c > +++ b/drivers/net/phy/microchip_t1.c > @@ -840,6 +840,9 @@ static int lan87xx_get_sqi(struct phy_device *phydev) > u8 sqi_value = 0; > int rc; > > + if (!phydev->link) > + return 0; > + > rc = access_ereg(phydev, PHYACC_ATTR_MODE_WRITE, > PHYACC_ATTR_BANK_DSP, T1_COEF_RW_CTL_CFG, 0x0301); > if (rc < 0) > -- > 2.39.2 > > Thanks, Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
On Wed, Jul 03, 2024 at 03:28:01PM +0200, Oleksij Rempel wrote: > Do not report SQI if no link is detected. Otherwise ethtool will show > non zero value even if no cable is attached. > > Fixes: b649695248b15 ("net: phy: LAN87xx: add ethtool SQI support") > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/microchip_t1.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c > index a35528497a576..22530a5b76365 100644 > --- a/drivers/net/phy/microchip_t1.c > +++ b/drivers/net/phy/microchip_t1.c > @@ -840,6 +840,9 @@ static int lan87xx_get_sqi(struct phy_device *phydev) > u8 sqi_value = 0; > int rc; > > + if (!phydev->link) > + return 0; > + Is this the correct place to fix this? Can any PHY report an SQI value if there is no link? Maybe an automotive PHY using T1 and good old fashioned CSMA/CD could report about background noise? But do they? Maybe this should be fixed in linkstate_get_sqi()? Also, maybe it should return -ENETDOWN, not 0. Do we want to say "worse than class A SQI (unstable link)" when in fact the link is "class G SQI (very good link)" once it is up? Andrew
Hi Andrew & Oleksij, > On Wed, Jul 03, 2024 at 03:28:01PM +0200, Oleksij Rempel wrote: > > Do not report SQI if no link is detected. Otherwise ethtool will show > > non zero value even if no cable is attached. > > > > Fixes: b649695248b15 ("net: phy: LAN87xx: add ethtool SQI support") > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > drivers/net/phy/microchip_t1.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/net/phy/microchip_t1.c > b/drivers/net/phy/microchip_t1.c > > index a35528497a576..22530a5b76365 100644 > > --- a/drivers/net/phy/microchip_t1.c > > +++ b/drivers/net/phy/microchip_t1.c > > @@ -840,6 +840,9 @@ static int lan87xx_get_sqi(struct phy_device *phydev) > > u8 sqi_value = 0; > > int rc; > > > > + if (!phydev->link) > > + return 0; > > + > > Is this the correct place to fix this? Can any PHY report an SQI value > if there is no link? Maybe an automotive PHY using T1 and good old > fashioned CSMA/CD could report about background noise? But do they? > > Maybe this should be fixed in linkstate_get_sqi()? > > Also, maybe it should return -ENETDOWN, not 0. Do we want to say > "worse than class A SQI (unstable link)" when in fact the link is > "class G SQI (very good link)" once it is up? I lean to Andew's idea because "SQI values are only valid if link-up condition is present" per OpenAlliance specification of 100Base-T1 Interoperability Test suite. [1] [1] https://opensig.org/automotive-ethernet-specifications/# Thanks. Woojung
diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c index a35528497a576..22530a5b76365 100644 --- a/drivers/net/phy/microchip_t1.c +++ b/drivers/net/phy/microchip_t1.c @@ -840,6 +840,9 @@ static int lan87xx_get_sqi(struct phy_device *phydev) u8 sqi_value = 0; int rc; + if (!phydev->link) + return 0; + rc = access_ereg(phydev, PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP, T1_COEF_RW_CTL_CFG, 0x0301); if (rc < 0)
Do not report SQI if no link is detected. Otherwise ethtool will show non zero value even if no cable is attached. Fixes: b649695248b15 ("net: phy: LAN87xx: add ethtool SQI support") Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/phy/microchip_t1.c | 3 +++ 1 file changed, 3 insertions(+)