netdev - Re: [PATCH 2/3] net:phy:dp83867: increase SGMII autoneg timer duration

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] [day] [month] [year] [list]

Date:   Fri, 24 May 2019 22:38:00 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Maxim Uvarov <muvarov@...il.com>
Cc:     netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH 2/3] net:phy:dp83867: increase SGMII autoneg timer
 duration

On 24.05.2019 21:55, Maxim Uvarov wrote:
> пт, 24 мая 2019 г. в 20:24, Heiner Kallweit <hkallweit1@...il.com>:
>>
>> On 24.05.2019 12:25, Max Uvarov wrote:
>>> After reset SGMII Autoneg timer is set to 2us (bits 6 and 5 are 01).
>>> That us not enough to finalize autonegatiation on some devices.
>>> Increase this timer duration to maximum supported 16ms.
>>>
>>> Signed-off-by: Max Uvarov <muvarov@...il.com>
>>> ---
>>>  drivers/net/phy/dp83867.c | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>>> index afd31c516cc7..66b0a09ad094 100644
>>> --- a/drivers/net/phy/dp83867.c
>>> +++ b/drivers/net/phy/dp83867.c
>>> @@ -297,6 +297,19 @@ static int dp83867_config_init(struct phy_device *phydev)
>>>                       WARN_ONCE(1, "dp83867: err DP83867_10M_SGMII_CFG\n");
>>>                       return ret;
>>>               }
>>> +
>>> +             /* After reset SGMII Autoneg timer is set to 2us (bits 6 and 5
>>> +              * are 01). That us not enough to finalize autoneg on some
>>> +              * devices. Increase this timer duration to maximum 16ms.
>>> +              */
>> In the public datasheet the bits are described as reserved. However, based on
>> the value, I suppose it's not a timer value but the timer resolution.
> 
> No, it's public:
> http://www.ti.com/lit/ds/symlink/dp83867e.pdf page 72.
> 
I just searched for Dp83867 and found this one:
http://www.ti.com/lit/ds/symlink/dp83867cr.pdf
This PHY seems to have the same ID, but the timer bits are at least not
documented.

> SGMII Auto-Negotiation Timer Duration.
> 
I see, we talk about SGMII aneg, not PHY aneg. My bad.

>>
>>> +             val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4);
>>> +             val &= ~(BIT(5) | BIT(6));
>>> +             ret = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4, val);
>>> +             if (ret) {
>>> +                     WARN_ONCE(1, "dp83867: error config sgmii auto-neg timer\n");
>>> +                     return ret;
>>
>> Same comment as for patch 1.
> 
> Yes, the same answer. I want to capture hardware error then silently
> return error and then debug it.

If you have hardware with buggy MDIO support, then the problem could occur
in any MDIO access. I think then the WARN should be in the MDIO bus ops.

> WARN is more informative then some random "phy not detected" things.
> 
> Max.
> 
>>
>>> +             }
>>> +
>>>       }
>>>
>>>       /* Enable Interrupt output INT_OE in CFG3 register */
>>>
>>
>