lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 16 Oct 2019 10:27:55 +0800 From: Yonglong Liu <liuyonglong@...wei.com> To: Heiner Kallweit <hkallweit1@...il.com>, <davem@...emloft.net>, <andrew@...n.ch> CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <linuxarm@...wei.com>, <salil.mehta@...wei.com>, <yisen.zhuang@...wei.com>, <shiju.jose@...wei.com> Subject: Re: [RFC PATCH V2 net] net: phy: Fix "link partner" information disappear issue 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@...wei.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; >> } >> > > > . >
Powered by blists - more mailing lists