RE: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled

[Ismail, Mohammad Athari]

> -----Original Message-----
> From: Florian Fainelli <f.fainelli@xxxxxxxxx>
> Sent: Friday, July 9, 2021 12:42 AM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@xxxxxxxxx>;
> Andrew Lunn <andrew@xxxxxxx>
> Cc: Heiner Kallweit <hkallweit1@xxxxxxxxx>; David S . Miller
> <davem@xxxxxxxxxxxxx>; Russell King <linux@xxxxxxxxxxxxxxx>; Jakub
> Kicinski <kuba@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL
> option still enabled
> 
> On 7/8/21 3:10 AM, Ismail, Mohammad Athari wrote:
> >
> >
> >> -----Original Message-----
> >> From: Florian Fainelli <f.fainelli@xxxxxxxxx>
> >> Sent: Thursday, July 8, 2021 10:49 AM
> >> To: Andrew Lunn <andrew@xxxxxxx>; Ismail, Mohammad Athari
> >> <mohammad.athari.ismail@xxxxxxxxx>
> >> Cc: Heiner Kallweit <hkallweit1@xxxxxxxxx>; David S . Miller
> >> <davem@xxxxxxxxxxxxx>; Russell King <linux@xxxxxxxxxxxxxxx>; Jakub
> >> Kicinski <kuba@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> >> linux-kernel@xxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if
> >> WOL option still enabled
> >>
> >>
> >>
> >> On 7/7/2021 6:23 PM, Andrew Lunn wrote:
> >>> On Thu, Jul 08, 2021 at 08:42:53AM +0800,
> >> mohammad.athari.ismail@xxxxxxxxx wrote:
> >>>> From: Mohammad Athari Bin Ismail
> <mohammad.athari.ismail@xxxxxxxxx>
> >>>>
> >>>> When the PHY wakes up from suspend through WOL event, there is a
> >>>> need to reconfigure the WOL if the WOL option still enabled. The
> >>>> main operation is to clear the WOL event status. So that,
> >>>> subsequent WOL event can be triggered properly.
> >>>>
> >>>> This fix is needed especially for the PHY that operates in PHY_POLL
> >>>> mode where there is no handler (such as interrupt handler)
> >>>> available to clear the WOL event status.
> >>>
> >>> I still think this architecture is wrong.
> >>>
> >>> The interrupt pin is wired to the PMIC. Can the PMIC be modelled as
> >>> an interrupt controller? That would allow the interrupt to be
> >>> handled as normal, and would mean you don't need polling, and you
> >>> don't need this hack.
> >>
> >> I have to agree with Andrew here, and if the answer is that you
> >> cannot model this PMIC as an interrupt controller, cannot the
> >> config_init() callback of the driver acknowledge then disable the
> >> interrupts as it normally would if you were cold booting the system?
> >> This would also allow you to properly account for the PHY having woken-
> up the system.
> >
> > Hi Florian,
> >
> > Thank you for the suggestion.
> > If I understand correctly, you are suggesting to acknowledge and clear the
> WOL status in config_init() callback function. Am I correct?
> > If yes, I did try to add a code to clear WOL status in marvell_config_init()
> function (we are using Marvell Alaska 88E1512). But, I found that, if the
> platform wake up from S3(mem) or S4(disk), the config_init() callback
> function is not called. As the result, WOL status not able to be cleared in
> config_init().
> >
> > Please advice if you any suggestion.
> 
> This is presumably that you are seeing with stmmac along with phylink?
> 
> During S3 resume you should be going back to the kernel provided re-entry
> point and resume where we left (warm boot) so
> mdio_bus_phy_resume() should call phy_init_hw() which calls config_init(),
> have you traced if that is somehow not happening?
> 
> During S4 resume (disk), I suppose that you have to involve the boot loader
> to restore the DRAM image from the storage disk, and so that does
> effectively look like a quasi cold boot from the kernel? If so, that should still
> lead to config_init() being called when the PHY is attached, no?

Hi Florian,

This what I understand from the code flow.

With WOL enabled through ethtool, when the system is put into S3 or S4,
this flag netdev->wol_enabled is set true and cause  mdio_bus_phy_may_suspend()
to return false. So, the  phydev->suspended_by_mdio_bus remain as 0 when
exiting from mdio_bus_phy_suspend().

During wake up from S3 or S4, as phydev->suspended_by_mdio_bus remain as 0/false
when mdio_bus_phy_resume() is called, it will jump to no_resume skipping
phy_init_hw() as well as phy_resume().

-Athari-


> --
> Florian