netdev - Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 20 Mar 2019 15:08:29 +0800
From:   Phil Reid <preid@...ctromag.com.au>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        liweihang <liweihang@...ilicon.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        "dongsheng.wang@...-semitech.com" <dongsheng.wang@...-semitech.com>,
        "cphealy@...il.com" <cphealy@...il.com>,
        "clemens.gruber@...ruber.com" <clemens.gruber@...ruber.com>,
        "nbd@....name" <nbd@....name>,
        "harini.katakam@...inx.com" <harini.katakam@...inx.com>
Subject: Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

On 20/03/2019 2:39 pm, Heiner Kallweit wrote:
> On 20.03.2019 06:16, Phil Reid wrote:
>> On 20/03/2019 11:37 am, Florian Fainelli wrote:
>>>
>>>
>>> On 3/19/2019 7:34 PM, liweihang wrote:
>>>> Hi all,
>>>>
>>>> I've met a similar issue and sent an email to discuss about it before:
>>>> Question about setting speed and duplex failed after auto-negotiation disabled on marvell phy
>>>>
>>>> d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset
>>>> I reverted this patch and the auto-negotiation works ok.
>>>>
>>>> Florian, could you please read my previous email and give me some advice?
>>>
>>> If you can copy the patch author on that email the next time that will
>>> help expedite things.
>>>
>>> So the problem seems to come from the fact that unless the BCMR_RESET
>>> bit is written, then m88e1121_config_aneg_rgmii_delays() has no effect,
>>> does that sound like what you are observing?
>>>
>>> Does the following work for you (Phil and yourself)?
>>>
>>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
>>> index 3ccba37bd6dd..6a1ea4c2042a 100644
>>> --- a/drivers/net/phy/marvell.c
>>> +++ b/drivers/net/phy/marvell.c
>>> @@ -448,6 +448,10 @@ static int m88e1121_config_aneg(struct phy_device
>>> *phydev)
>>>                   err = m88e1121_config_aneg_rgmii_delays(phydev);
>>>                   if (err < 0)
>>>                           return err;
>>> +
>>> +               err = genphy_soft_reset(phydev);
>>> +               if (err < 0)
>>> +                       return err;
>>>           }
>>>
>>>           err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
>>>
>>
>>
>> G'day Florian,
>>
>> Nope that didn't work for me.
>> But based on that patch and liweihang email I found the following works for me:
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 46c8672..de71aef 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1827,7 +1827,13 @@ int genphy_soft_reset(struct phy_device *phydev)
>>   {
>>          int ret;
>>
>> -       ret = phy_write(phydev, MII_BMCR, BMCR_RESET);
>> +       phydev_err(phydev, "genphy_soft_reset");
>> +
>> +       ret = phy_read(phydev, MII_BMCR);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ret = phy_write(phydev, MII_BMCR, ret | BMCR_RESET);
> 
> Hmm, that would mean in your case some set bit needs to be preserved.
> Usually that's not needed. Could you please check which value is read
> from MII_BMCR?
> 

The relevant log is:
[    2.513985] bus: 'mdio_bus': driver_probe_device: matched device stmmac-0:00 with driver mv88e6085
[    2.513995] bus: 'mdio_bus': really_probe: probing driver mv88e6085 with device stmmac-0:00
[    2.514011] mv88e6085 stmmac-0:00: no pinctrl handle
[    2.514040] mv88e6085 stmmac-0:00: GPIO lookup for consumer reset
[    2.514048] mv88e6085 stmmac-0:00: using device tree for GPIO lookup
[    2.514097] mv88e6085 stmmac-0:00: using lookup tables for GPIO lookup
[    2.514106] mv88e6085 stmmac-0:00: No GPIO consumer reset found
[    2.514251] mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell 88E6352, revision 1
[    2.884936] mv88e6085 stmmac-0:00 lan1 (uninitialized): PHY [!soc!ethernet@...02000!mdio!switch0@...dio:02] driver [Marvell 88E1540]
[    2.905230] mv88e6085 stmmac-0:00 lan2 (uninitialized): PHY [!soc!ethernet@...02000!mdio!switch0@...dio:03] driver [Marvell 88E1540]
[    2.927978] mv88e6085 stmmac-0:00 lan3 (uninitialized): PHY [!soc!ethernet@...02000!mdio!switch0@...dio:04] driver [Marvell 88E1540]
[    2.941852] driver: 'mv88e6085': driver_bound: bound to device 'stmmac-0:00'
[    2.941900] bus: 'mdio_bus': really_probe: bound device stmmac-0:00 to driver mv88e6085
[    8.447971] mv88e6085 stmmac-0:00 lan2: configuring for phy/ link mode
[    8.462773] mv88e6085 stmmac-0:00 lan3: configuring for phy/ link mode
[    8.476192] mv88e6085 stmmac-0:00 lan1: configuring for phy/ link mode
[    8.488751] mv88e6085 stmmac-0:00 wifi: configuring for fixed/ link mode
[    8.511087] mv88e6085 stmmac-0:00 wifi: Link is Up - 100Mbps/Full - flow control off
[    8.553462] Marvell 88E1540 !soc!ethernet@...02000!mdio!switch0@...dio:03: genphy_soft_reset 00001140
[    8.564818] Marvell 88E1540 !soc!ethernet@...02000!mdio!switch0@...dio:04: genphy_soft_reset 00001140
[    8.588558] Marvell 88E1540 !soc!ethernet@...02000!mdio!switch0@...dio:02: genphy_soft_reset 00001140
[   12.330708] mv88e6085 stmmac-0:00 lan2: Link is Up - 1Gbps/Full - flow control rx/tx

A read of the 88e6352 docs indicates that the reset bit must be written
simultaneously with the auto negotiation bit. (bit 12)
This also seems to be the case for the link speed and I think the
duplex mode.

-- 
Regards
Phil