| Message ID | 1570699808-55313-1-git-send-email-liuyonglong@huawei.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC,net] net: phy: Fix "link partner" information disappear issue | expand |
On 10.10.2019 11:30, Yonglong Liu wrote: > Some drivers just call phy_ethtool_ksettings_set() to set the > links, for those phy drivers that use genphy_read_status(), if > autoneg is on, and the link is up, than execute "ethtool -s > ethx autoneg on" will cause "link partner" information disappear. > > The call trace is phy_ethtool_ksettings_set()->phy_start_aneg() > ->linkmode_zero(phydev->lp_advertising)->genphy_read_status(), > the link didn't change, so genphy_read_status() just return, and > phydev->lp_advertising is zero now. > I think that clearing link partner advertising info in phy_start_aneg() is questionable. If advertising doesn't change then phy_config_aneg() basically is a no-op. Instead we may have to clear the link partner advertising info in genphy_read_lpa() if aneg is disabled or aneg isn't completed (basically the same as in genphy_c45_read_lpa()). Something like: if (!phydev->autoneg_complete) { /* also covers case that aneg is disabled */ linkmode_zero(phydev->lp_advertising); } else if (phydev->autoneg == AUTONEG_ENABLE) { ... } > This patch call genphy_read_lpa() before the link state judgement > to fix this problem. > > Fixes: 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in genphy_read_status") > Signed-off-by: Yonglong Liu <liuyonglong@huawei.com> > --- > drivers/net/phy/phy_device.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 9d2bbb1..ef3073c 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1839,6 +1839,10 @@ int genphy_read_status(struct phy_device *phydev) > if (err) > return err; > > + err = genphy_read_lpa(phydev); > + if (err < 0) > + return err; > + > /* why bother the PHY if nothing can have changed */ > if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link) > return 0; > @@ -1848,10 +1852,6 @@ int genphy_read_status(struct phy_device *phydev) > phydev->pause = 0; > phydev->asym_pause = 0; > > - err = genphy_read_lpa(phydev); > - if (err < 0) > - return err; > - > if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) { > phy_resolve_aneg_linkmode(phydev); > } else if (phydev->autoneg == AUTONEG_DISABLE) { >
On 2019/10/11 3:17, Heiner Kallweit wrote: > On 10.10.2019 11:30, Yonglong Liu wrote: >> Some drivers just call phy_ethtool_ksettings_set() to set the >> links, for those phy drivers that use genphy_read_status(), if >> autoneg is on, and the link is up, than execute "ethtool -s >> ethx autoneg on" will cause "link partner" information disappear. >> >> The call trace is phy_ethtool_ksettings_set()->phy_start_aneg() >> ->linkmode_zero(phydev->lp_advertising)->genphy_read_status(), >> the link didn't change, so genphy_read_status() just return, and >> phydev->lp_advertising is zero now. >> > I think that clearing link partner advertising info in > phy_start_aneg() is questionable. If advertising doesn't change > then phy_config_aneg() basically is a no-op. Instead we may have > to clear the link partner advertising info in genphy_read_lpa() > if aneg is disabled or aneg isn't completed (basically the same > as in genphy_c45_read_lpa()). Something like: > > if (!phydev->autoneg_complete) { /* also covers case that aneg is disabled */ > linkmode_zero(phydev->lp_advertising); > } else if (phydev->autoneg == AUTONEG_ENABLE) { > ... > } > If clear the link partner advertising info in genphy_read_lpa() and genphy_c45_read_lpa(), for the drivers that use genphy_read_status() is ok, but for those drivers that use there own read_status() may have problem, like aqr_read_status(), it will update lp_advertising first, and than call genphy_c45_read_status(), so will cause lp_advertising lost. Another question, please see genphy_c45_read_status(), if clear the link partner advertising info in genphy_c45_read_lpa(), if autoneg is off, phydev->lp_advertising will not clear. >> This patch call genphy_read_lpa() before the link state judgement >> to fix this problem. >> >> Fixes: 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in genphy_read_status") >> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com> >> --- >> drivers/net/phy/phy_device.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index 9d2bbb1..ef3073c 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -1839,6 +1839,10 @@ int genphy_read_status(struct phy_device *phydev) >> if (err) >> return err; >> >> + err = genphy_read_lpa(phydev); >> + if (err < 0) >> + return err; >> + >> /* why bother the PHY if nothing can have changed */ >> if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link) >> return 0; >> @@ -1848,10 +1852,6 @@ int genphy_read_status(struct phy_device *phydev) >> phydev->pause = 0; >> phydev->asym_pause = 0; >> >> - err = genphy_read_lpa(phydev); >> - if (err < 0) >> - return err; >> - >> if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) { >> phy_resolve_aneg_linkmode(phydev); >> } else if (phydev->autoneg == AUTONEG_DISABLE) { >> > > > . >
On 11.10.2019 07:55, Yonglong Liu wrote: > > > On 2019/10/11 3:17, Heiner Kallweit wrote: >> On 10.10.2019 11:30, Yonglong Liu wrote: >>> Some drivers just call phy_ethtool_ksettings_set() to set the >>> links, for those phy drivers that use genphy_read_status(), if >>> autoneg is on, and the link is up, than execute "ethtool -s >>> ethx autoneg on" will cause "link partner" information disappear. >>> >>> The call trace is phy_ethtool_ksettings_set()->phy_start_aneg() >>> ->linkmode_zero(phydev->lp_advertising)->genphy_read_status(), >>> the link didn't change, so genphy_read_status() just return, and >>> phydev->lp_advertising is zero now. >>> >> I think that clearing link partner advertising info in >> phy_start_aneg() is questionable. If advertising doesn't change >> then phy_config_aneg() basically is a no-op. Instead we may have >> to clear the link partner advertising info in genphy_read_lpa() >> if aneg is disabled or aneg isn't completed (basically the same >> as in genphy_c45_read_lpa()). Something like: >> >> if (!phydev->autoneg_complete) { /* also covers case that aneg is disabled */ >> linkmode_zero(phydev->lp_advertising); >> } else if (phydev->autoneg == AUTONEG_ENABLE) { >> ... >> } >> > > If clear the link partner advertising info in genphy_read_lpa() and > genphy_c45_read_lpa(), for the drivers that use genphy_read_status() > is ok, but for those drivers that use there own read_status() may > have problem, like aqr_read_status(), it will update lp_advertising > first, and than call genphy_c45_read_status(), so will cause > lp_advertising lost. > Right, in genphy_read_lpa() we shouldn't clear all lpa bits but only those ones the generic functions care about. Basically the same as in the c45 version. Then a vendor-specific part isn't affected. aqr_read_status() is a good example. It deals with 1Gbps mode that isn't covered by the generic c45 functions. Therefore the 1Gbps-related bits won't be overwritten by the generic functions. > Another question, please see genphy_c45_read_status(), if clear the > link partner advertising info in genphy_c45_read_lpa(), if autoneg is > off, phydev->lp_advertising will not clear. > If autoneg is off, lp_advertising should never be set, so there's nothing to clear. However we may have to look at the case that user switches to fixed speed mode via ethtool. >>> This patch call genphy_read_lpa() before the link state judgement >>> to fix this problem. >>> >>> Fixes: 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in genphy_read_status") >>> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com> >>> --- >>> drivers/net/phy/phy_device.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >>> index 9d2bbb1..ef3073c 100644 >>> --- a/drivers/net/phy/phy_device.c >>> +++ b/drivers/net/phy/phy_device.c >>> @@ -1839,6 +1839,10 @@ int genphy_read_status(struct phy_device *phydev) >>> if (err) >>> return err; >>> >>> + err = genphy_read_lpa(phydev); >>> + if (err < 0) >>> + return err; >>> + >>> /* why bother the PHY if nothing can have changed */ >>> if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link) >>> return 0; >>> @@ -1848,10 +1852,6 @@ int genphy_read_status(struct phy_device *phydev) >>> phydev->pause = 0; >>> phydev->asym_pause = 0; >>> >>> - err = genphy_read_lpa(phydev); >>> - if (err < 0) >>> - return err; >>> - >>> if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) { >>> phy_resolve_aneg_linkmode(phydev); >>> } else if (phydev->autoneg == AUTONEG_DISABLE) { >>> >> >> >> . >> > >
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 9d2bbb1..ef3073c 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1839,6 +1839,10 @@ int genphy_read_status(struct phy_device *phydev) if (err) return err; + err = genphy_read_lpa(phydev); + if (err < 0) + return err; + /* why bother the PHY if nothing can have changed */ if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link) return 0; @@ -1848,10 +1852,6 @@ int genphy_read_status(struct phy_device *phydev) phydev->pause = 0; phydev->asym_pause = 0; - err = genphy_read_lpa(phydev); - if (err < 0) - return err; - if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) { phy_resolve_aneg_linkmode(phydev); } else if (phydev->autoneg == AUTONEG_DISABLE) {
Some drivers just call phy_ethtool_ksettings_set() to set the links, for those phy drivers that use genphy_read_status(), if autoneg is on, and the link is up, than execute "ethtool -s ethx autoneg on" will cause "link partner" information disappear. The call trace is phy_ethtool_ksettings_set()->phy_start_aneg() ->linkmode_zero(phydev->lp_advertising)->genphy_read_status(), the link didn't change, so genphy_read_status() just return, and phydev->lp_advertising is zero now. This patch call genphy_read_lpa() before the link state judgement to fix this problem. Fixes: 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in genphy_read_status") Signed-off-by: Yonglong Liu <liuyonglong@huawei.com> --- drivers/net/phy/phy_device.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)