| Message ID | 1571057797-37602-1-git-send-email-liuyonglong@huawei.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC,V2,net] net: phy: Fix "link partner" information disappear issue | expand |
On 14.10.2019 14:56, 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. > > This patch moves the clear operation of lp_advertising from > phy_start_aneg() to genphy_read_lpa()/genphy_c45_read_lpa(), and > if autoneg on and autoneg not complete, just clear what the > generic functions care about. > > Fixes: 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in genphy_read_status") > Signed-off-by: Yonglong Liu <liuyonglong@huawei.com> > Looks good to me, two small nits below. > --- > change log: > V2: moves the clear operation of lp_advertising from > phy_start_aneg() to genphy_read_lpa()/genphy_c45_read_lpa(), and > if autoneg on and autoneg not complete, just clear what the > generic functions care about. Suggested by Heiner Kallweit. > --- > --- This line seems to be duplicated. > drivers/net/phy/phy-c45.c | 2 ++ > drivers/net/phy/phy.c | 3 --- > drivers/net/phy/phy_device.c | 12 +++++++++++- > 3 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c > index 7935593..a1caeee 100644 > --- a/drivers/net/phy/phy-c45.c > +++ b/drivers/net/phy/phy-c45.c > @@ -323,6 +323,8 @@ int genphy_c45_read_pma(struct phy_device *phydev) > { > int val; > > + linkmode_zero(phydev->lp_advertising); > + > val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1); > if (val < 0) > return val; > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index 119e6f4..105d389b 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -572,9 +572,6 @@ int phy_start_aneg(struct phy_device *phydev) > if (AUTONEG_DISABLE == phydev->autoneg) > phy_sanitize_settings(phydev); > > - /* Invalidate LP advertising flags */ > - linkmode_zero(phydev->lp_advertising); > - > err = phy_config_aneg(phydev); > if (err < 0) > goto out_unlock; > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 9d2bbb1..4b43466 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1787,7 +1787,14 @@ int genphy_read_lpa(struct phy_device *phydev) > { > int lpa, lpagb; > > - if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) { > + if (phydev->autoneg == AUTONEG_ENABLE) { > + if (!phydev->autoneg_complete) { > + mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, > + 0); > + mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0); > + return 0; > + } > + > if (phydev->is_gigabit_capable) { > lpagb = phy_read(phydev, MII_STAT1000); > if (lpagb < 0) > @@ -1816,6 +1823,9 @@ int genphy_read_lpa(struct phy_device *phydev) > > mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa); > } > + else { "} else {" should be on one line. > + linkmode_zero(phydev->lp_advertising); > + } > > return 0; > } >
On 2019/10/16 4:02, Heiner Kallweit wrote: > On 14.10.2019 14:56, 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. >> >> This patch moves the clear operation of lp_advertising from >> phy_start_aneg() to genphy_read_lpa()/genphy_c45_read_lpa(), and >> if autoneg on and autoneg not complete, just clear what the >> generic functions care about. >> >> Fixes: 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in genphy_read_status") >> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com> >> > Looks good to me, two small nits below. > > Thanks! Will fix the two small nits and send it to "net" branch. >> --- >> change log: >> V2: moves the clear operation of lp_advertising from >> phy_start_aneg() to genphy_read_lpa()/genphy_c45_read_lpa(), and >> if autoneg on and autoneg not complete, just clear what the >> generic functions care about. Suggested by Heiner Kallweit. >> --- >> --- > This line seems to be duplicated. > >> drivers/net/phy/phy-c45.c | 2 ++ >> drivers/net/phy/phy.c | 3 --- >> drivers/net/phy/phy_device.c | 12 +++++++++++- >> 3 files changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c >> index 7935593..a1caeee 100644 >> --- a/drivers/net/phy/phy-c45.c >> +++ b/drivers/net/phy/phy-c45.c >> @@ -323,6 +323,8 @@ int genphy_c45_read_pma(struct phy_device *phydev) >> { >> int val; >> >> + linkmode_zero(phydev->lp_advertising); >> + >> val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1); >> if (val < 0) >> return val; >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >> index 119e6f4..105d389b 100644 >> --- a/drivers/net/phy/phy.c >> +++ b/drivers/net/phy/phy.c >> @@ -572,9 +572,6 @@ int phy_start_aneg(struct phy_device *phydev) >> if (AUTONEG_DISABLE == phydev->autoneg) >> phy_sanitize_settings(phydev); >> >> - /* Invalidate LP advertising flags */ >> - linkmode_zero(phydev->lp_advertising); >> - >> err = phy_config_aneg(phydev); >> if (err < 0) >> goto out_unlock; >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index 9d2bbb1..4b43466 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -1787,7 +1787,14 @@ int genphy_read_lpa(struct phy_device *phydev) >> { >> int lpa, lpagb; >> >> - if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) { >> + if (phydev->autoneg == AUTONEG_ENABLE) { >> + if (!phydev->autoneg_complete) { >> + mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, >> + 0); >> + mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0); >> + return 0; >> + } >> + >> if (phydev->is_gigabit_capable) { >> lpagb = phy_read(phydev, MII_STAT1000); >> if (lpagb < 0) >> @@ -1816,6 +1823,9 @@ int genphy_read_lpa(struct phy_device *phydev) >> >> mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa); >> } >> + else { > > "} else {" should be on one line. > >> + linkmode_zero(phydev->lp_advertising); >> + } >> >> return 0; >> } >> > > > . >
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c index 7935593..a1caeee 100644 --- a/drivers/net/phy/phy-c45.c +++ b/drivers/net/phy/phy-c45.c @@ -323,6 +323,8 @@ int genphy_c45_read_pma(struct phy_device *phydev) { int val; + linkmode_zero(phydev->lp_advertising); + val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1); if (val < 0) return val; diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 119e6f4..105d389b 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -572,9 +572,6 @@ int phy_start_aneg(struct phy_device *phydev) if (AUTONEG_DISABLE == phydev->autoneg) phy_sanitize_settings(phydev); - /* Invalidate LP advertising flags */ - linkmode_zero(phydev->lp_advertising); - err = phy_config_aneg(phydev); if (err < 0) goto out_unlock; diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 9d2bbb1..4b43466 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1787,7 +1787,14 @@ int genphy_read_lpa(struct phy_device *phydev) { int lpa, lpagb; - if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) { + if (phydev->autoneg == AUTONEG_ENABLE) { + if (!phydev->autoneg_complete) { + mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, + 0); + mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0); + return 0; + } + if (phydev->is_gigabit_capable) { lpagb = phy_read(phydev, MII_STAT1000); if (lpagb < 0) @@ -1816,6 +1823,9 @@ int genphy_read_lpa(struct phy_device *phydev) mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa); } + else { + linkmode_zero(phydev->lp_advertising); + } return 0; }
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 moves the clear operation of lp_advertising from phy_start_aneg() to genphy_read_lpa()/genphy_c45_read_lpa(), and if autoneg on and autoneg not complete, just clear what the generic functions care about. Fixes: 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in genphy_read_status") Signed-off-by: Yonglong Liu <liuyonglong@huawei.com> --- change log: V2: moves the clear operation of lp_advertising from phy_start_aneg() to genphy_read_lpa()/genphy_c45_read_lpa(), and if autoneg on and autoneg not complete, just clear what the generic functions care about. Suggested by Heiner Kallweit. --- --- drivers/net/phy/phy-c45.c | 2 ++ drivers/net/phy/phy.c | 3 --- drivers/net/phy/phy_device.c | 12 +++++++++++- 3 files changed, 13 insertions(+), 4 deletions(-)