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] [day] [month] [year] [list]
Message-ID: <CY4PR1701MB187802CE3A01E94EF5659922DF190@CY4PR1701MB1878.namprd17.prod.outlook.com>
Date:   Mon, 26 Oct 2020 10:21:08 +0000
From:   "Badel, Laurent" <LaurentBadel@...on.com>
To:     Jörg Willmann <joe@...t.de>
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



> 

-----------------------------
Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la Longeraie 7, 1110, Morges, Switzerland 

-----------------------------

-----Original Message-----
> From: Jörg Willmann <joe@...t.de>
> Sent: Tuesday, May 19, 2020 10:41 AM
> To: Badel, Laurent <LaurentBadel@...on.com>
> Cc: Heiner Kallweit <hkallweit1@...il.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; 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.
> 

My apologies, in the midst of modifying and preparing a new patch for this issue
I now realize that I seem to have failed to reply to your comments. Thank you
very much for your time and efforts reviewing the patch; I have taken these into
account and indeed there also seems to be an issue with fec_resume that I have
now addressed.

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