Messages in this thread | | | Subject | Re: [RFC PATCH net] net: phy: Fix "link partner" information disappear issue | From | Heiner Kallweit <> | Date | Sat, 12 Oct 2019 22:40:39 +0200 |
| |
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) { >>> >> >> >> . >> > >
| |