Re: [RFC] net: phy: add shutdown hook to struct phy_driver

[Jisheng Zhang]

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


> 
> On 9/30/2020 1:11 PM, Andrew Lunn wrote:
> > On Wed, Sep 30, 2020 at 01:07:19PM -0700, Florian Fainelli wrote:  
> >>
> >>
> >> On 9/30/2020 12:09 PM, Andrew Lunn wrote:  
> >>> On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote:  
> >>>> 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?  
> >>>  
> >>>> 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?

Thanks in advance,
Jisheng

> every driver that defines both .suspend and .set_wol but there are not
> that many.
> 
> Adding an additional callback to the PHY driver does not really scale
> and it would require us to be extra careful and also plumb the MDIO bus
> shutdown.
> --
> Florian