On 10/5/2020 8:54 AM, Heiner Kallweit wrote:

On 05.10.2020 17:41, Florian Fainelli wrote:

quoted

On 10/5/2020 1:53 AM, Jisheng Zhang wrote:

quoted

On Wed, 30 Sep 2020 13:23:29 -0700 Florian Fainelli wrote:

quoted

On 9/30/2020 1:11 PM, Andrew Lunn wrote:

quoted

On Wed, Sep 30, 2020 at 01:07:19PM -0700, Florian Fainelli wrote:

quoted

On 9/30/2020 12:09 PM, Andrew Lunn wrote:

quoted

On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote:

quoted

Hi,

A GE phy supports pad isolation which can save power in WOL mode. But once the
isolation is enabled, the MAC can't send/receive pkts to/from the phy because
the phy is "isolated". To make the PHY work normally, I need to move the
enabling isolation to suspend hook, so far so good. But the isolation isn't
enabled in system shutdown case, to support this, I want to add shutdown hook
to net phy_driver, then also enable the isolation in the shutdown hook. Is
there any elegant solution?

quoted

Or we can break the assumption: ethernet can still send/receive pkts after
enabling WoL, no?

That is not an easy assumption to break. The MAC might be doing WOL,
so it needs to be able to receive packets.

What you might be able to assume is, if this PHY device has had WOL
enabled, it can assume the MAC does not need to send/receive after
suspend. The problem is, phy_suspend() will not call into the driver
is WOL is enabled, so you have no idea when you can isolate the MAC
from the PHY.

So adding a shutdown in mdio_driver_register() seems reasonable.  But
you need to watch out for ordering. Is the MDIO bus driver still
running?

If your Ethernet MAC controller implements a shutdown callback and that
callback takes care of unregistering the network device which should also
ensure that phy_disconnect() gets called, then your PHY's suspend function
will be called.

Hi Florian

I could be missing something here, but:

phy_suspend does not call into the PHY driver if WOL is enabled. So
Jisheng needs a way to tell the PHY it should isolate itself from the
MAC, and suspend is not that.

I missed that part, that's right if WoL is enabled at the PHY level then
the suspend callback is not called, how about we change that and we
always call the PHY's suspend callback? This would require that we audit

Hi all,

The PHY's suspend callback usually calls genphy_suspend() which will set
BMCR_PDOWN bit, this may break WoL. I think this is one the reason why
we ignore the phydrv->suspend() when WoL is enabled. If we goes to this
directly, it looks like we need to change each phy's suspend implementation,
I.E if WoL is enabled, ignore genphy_suspend() and do possible isolation;
If WoL is disabled, keep the code path as is.

So compared with the shutdown hook, which direction is better?

I believe you will have an easier time to add an argument to the PHY driver suspend's function to indicate the WoL status, or to move down the check for WoL being enabled/supported compared to adding support for shutdown into the MDIO bus layer, and then PHY drivers.

Maybe the shutdown callback of mdio_bus_type could be implemented.
It could iterate over all PHY's on the bus, check for WoL (similar to
mdio_bus_phy_may_suspend) and do whatever is needed.
Seems to me to be the most generic way.

OK and we optionally call into a PHY device's shutdown function if 
defined so it can perform PHY specific work? That would work for me.
-- 
Florian