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: <20211215110139.GA64001@francesco-nb.int.toradex.com>
Date:   Wed, 15 Dec 2021 12:01:39 +0100
From:   Francesco Dolcini <francesco.dolcini@...adex.com>
To:     Andrew Lunn <andrew@...n.ch>,
        "Russell King (Oracle)" <linux@...linux.org.uk>,
        Joakim Zhang <qiangqing.zhang@....com>
Cc:     "Russell King (Oracle)" <linux@...linux.org.uk>,
        Francesco Dolcini <francesco.dolcini@...adex.com>,
        Philippe Schenker <philippe.schenker@...adex.com>,
        netdev@...r.kernel.org, Joakim Zhang <qiangqing.zhang@....com>,
        "David S . Miller" <davem@...emloft.net>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Fabio Estevam <festevam@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 3/3] net: fec: reset phy on resume after power-up

On Wed, Dec 15, 2021 at 10:36:52AM +0100, Andrew Lunn wrote:
> 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.
This is all correct and will solve the issue, however ...

The problem I see is that nor the phylib nor the PHY driver is aware
that the PHY was powered down, if we unconditionally assert the reset in
the suspend callback in the PHY driver/lib this will affect in a bad
case the most common use case in which we keep the PHY powered in
suspend.

We would have to move the regulator in the PHY driver (phy/micrel.c) to
do it properly.

The reason is that the power consumption in reset is higher in reset
compared to the normal PHY software power down.

This will create a power consumption regression for lot of users.

Doing this into the FEC driver would not have this issue, since we know
if we have a regulator (I guess you saw my one line patch for it).


On Wed, Dec 15, 2021 at 10:29:07AM +0000, Russell King (Oracle) wrote:
> Here's another question which no one seems to have considered. If the
> PHY power source can be controlled, why doesn't the firmware describe
> the power supply for the PHY, and why doesn't the PHY driver control
> the PHY power source? Why is that in the SoC network driver?
Legacy/historical reasons ...

In the first RFC patch for this issue this was mentioned by Philippe,
but than the discussion went into another direction.

As I wrote above if we handle both reset/regulator in the PHY driver it
should work, just a little bit tricky because phy/micrel.c handle a
whole family of phys.


On Wed, Dec 15, 2021 at 10:25:14AM +0000, Joakim Zhang wrote:
> As I mentioned before, both mac and phylib have not taken PHY reset
> into consideration during system suspend/resume scenario. As Andrew
> suggested, you could move this into phy driver suspend function, this
> is a corner case. One point I don't understand, why do you reject to
> assert reset signal during system suspended? 
See my answer to Andrew above, in short asserting the reset without
disabling the regulator will create a regression on the power
consumption.

Any agreement on how to move forward?

 1. add phy_reset_after_power_on() and call it from FEC driver (current
 patchset)
 2. assert phy reset in FEC driver suspend (one line patch from me in
 this thread)
 3. move regulator to phy/micrel.c and assert reset in the phy driver resume
 callback
 4. ?

?

Francesco