netdev - RE: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init

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]

Message-ID: <alpine.DEB.2.21.2005191036500.7651@brian.int1.clnt.de>
Date:   Tue, 19 May 2020 10:41:01 +0200 (CEST)
From:   Jörg Willmann <joe@...t.de>
To:     "Badel, Laurent" <LaurentBadel@...on.com>
cc:     Heiner Kallweit <hkallweit1@...il.com>,
        "fugang.duan@....com" <fugang.duan@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "richard.leitner@...data.com" <richard.leitner@...data.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "alexander.levin@...rosoft.com" <alexander.levin@...rosoft.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "Quette, Arnaud" <ArnaudQuette@...on.com>
Subject: RE: [EXTERNAL]  Re: [PATCH 2/2] Reset PHY in phy_init_hw() before
 interrupt configuration



On Thu, 30 Apr 2020, Badel, Laurent wrote:

> -----Original Message-----
>> From: Heiner Kallweit <hkallweit1@...il.com>
>> Sent: Wednesday, April 29, 2020 7:06 PM
>> To: Badel, Laurent <LaurentBadel@...on.com>; fugang.duan@....com;
>> netdev@...r.kernel.org; andrew@...n.ch; f.fainelli@...il.com;
>> linux@...linux.org.uk; richard.leitner@...data.com;
>> davem@...emloft.net; alexander.levin@...rosoft.com;
>> gregkh@...uxfoundation.org
>> Cc: Quette, Arnaud <ArnaudQuette@...on.com>
>> Subject: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before
>> interrupt configuration
>>
>> On 29.04.2020 11:03, Badel, Laurent wrote:
>>> Description: this patch adds a reset of the PHY in phy_init_hw()
>>> for PHY drivers bearing the PHY_RST_AFTER_CLK_EN flag.
>>>
>>> Rationale: due to the PHY reset reverting the interrupt mask to default,
>>> it is necessary to either perform the reset before PHY configuration,
>>> or re-configure the PHY after reset. This patch implements the former
>>> as it is simpler and more generic.
>>>
>>> Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add
>> phy_reset_after_clk_enable() support")
>>> Signed-off-by: Laurent Badel <laurentbadel@...on.com>
>>>
>>> ---
>>>  drivers/net/phy/phy_device.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>>> index 28e3c5c0e..2cc511364 100644
>>> --- a/drivers/net/phy/phy_device.c
>>> +++ b/drivers/net/phy/phy_device.c
>>> @@ -1082,8 +1082,11 @@ int phy_init_hw(struct phy_device *phydev)
>>>  {
>>>  	int ret = 0;
>>>
>>> -	/* Deassert the reset signal */
>>> -	phy_device_reset(phydev, 0);
>>> +	/* Deassert the reset signal
>>> +	 * If the PHY needs a reset, do it now
>>> +	 */
>>> +	if (!phy_reset_after_clk_enable(phydev))
>>
>> If reset is asserted when entering phy_init_hw(), then
>> phy_reset_after_clk_enable() basically becomes a no-op.
>> Still it should work as expected due to the reset signal being
>> deasserted. It would be worth describing in the comment
>> why the code still works in this case.
>>
>
> Thank you for the comment, this is a very good point.
> I will make sure to include some description when resubmitting.
> I had previously tested this and what I saw was that the first
> time you bring up the interface, the reset is not asserted so
> that phy_reset_after_clk_enable() is effective.
> The subsequent times the interface is brought up, the reset
> is already asserted when entering phy_init_hw(), so that
> it becomes a no-op as you correctly pointed out. However,
> that didn't cause any problem on my board, presumably because
> in that case the clock is already running when the PHY comes
> out of reset.
> I will re-test this carefully against the 'net' tree, though,
> before coming to conclusions.
>
I have two additional things to take into account:
* phy_reset_after_clk_enable() shoulnd't be longer called that way since 
it is now misleading -> the phy is no longer reset after clock enable but 
during hw_init()
* how about fec_resume()? I don't think hw_init() is called then and so 
phy_reset_after_clk_enable() will no longer be called.

>>> +		phy_device_reset(phydev, 0);
>>>
>>>  	if (!phydev->drv)
>>>  		return 0;
>>>
>
>