netdev - Re: [PATCH net-next 3/3] net: fec: reset phy on resume after power-up

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: <Ybm3NDeq96TSjh+k@lunn.ch>
Date:   Wed, 15 Dec 2021 10:36:52 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Francesco Dolcini <francesco.dolcini@...adex.com>
Cc:     Philippe Schenker <philippe.schenker@...adex.com>,
        netdev@...r.kernel.org, Joakim Zhang <qiangqing.zhang@....com>,
        "David S . Miller" <davem@...emloft.net>,
        Russell King <linux@...linux.org.uk>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Fabio Estevam <festevam@...il.com>,
        Fugang Duan <fugang.duan@....com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 3/3] net: fec: reset phy on resume after power-up

On Tue, Dec 14, 2021 at 11:35:48PM +0100, Francesco Dolcini wrote:
> Hello Andrew,
> 
> On Tue, Dec 14, 2021 at 07:54:54PM +0100, Andrew Lunn wrote:
> > What i don't particularly like about this is that the MAC driver is
> > doing it. Meaning if this PHY is used with any other MAC, the same
> > code needs adding there.
> This is exactly the same case as phy_reset_after_clk_enable() [1][2], to
> me it does not look that bad.
> 
> > So maybe in the phy driver, add a suspend handler, which asserts the
> > reset. This call here will take it out of reset, so applying the reset
> > you need?
> Asserting the reset in the phylib in suspend path is a bad idea, in the
> general case in which the PHY is powered in suspend the
> power-consumption is likely to be higher if the device is in reset
> compared to software power-down using the BMCR register (at least for
> the PHY datasheet I checked).

Maybe i don't understand your hardware.

You have a regulator providing power of the PHY.

You have a reset, i guess a GPIO, connected to the reset pin of the
PHY.

What you could do is:

PHY driver suspend handler does a phy_device_reset(ndev->phydev, 1)
to put the PHY into reset.

MAC driver disables the regulator.

Power consumption should now be 0, since it does not have any power.

On resume, the MAC enables the regulator. At this point, the PHY gets
power, but is still held in reset. It is now consuming power, but not
doing anything. The MAC calls phy_hw_init(), which calls
phy_device_reset(ndev->phydev, 0), taking the PHY out of reset.

Hopefully, this release from reset is enough to make the PHY work.

Doing it like this also addresses Russell point. phy_hw_init() is not
putting the device into reset, it is only taking it out of reset, if
it happens to be already in reset. So we are not slowing down link up
for everybody.

    Andrew