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 <[email protected]>
>>
> 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;
>> }
>>
>
>
> .
>
