linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled
@ 2021-07-08  0:42 mohammad.athari.ismail
  2021-07-08  1:23 ` Andrew Lunn
  0 siblings, 1 reply; 12+ messages in thread
From: mohammad.athari.ismail @ 2021-07-08  0:42 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, David S . Miller, Russell King,
	Jakub Kicinski, Florian Fainelli
  Cc: netdev, linux-kernel, mohammad.athari.ismail

From: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>

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.

Fixes: 611d779af7ca ("net: phy: fix MDIO bus PM PHY resuming")
Signed-off-by: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>
---
 drivers/net/phy/phy_device.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5d5f9a9ee768..d68703ce03b1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -295,6 +295,7 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
 
 static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 {
+	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
 	struct phy_device *phydev = to_phy_device(dev);
 	int ret;
 
@@ -314,6 +315,13 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 	if (ret < 0)
 		return ret;
 no_resume:
+	/* If the PHY has WOL option still enabled, reconfigure the WOL mainly
+	 * to clear the WOL event status.
+	 */
+	phy_ethtool_get_wol(phydev, &wol);
+	if (wol.wolopts)
+		phy_ethtool_set_wol(phydev, &wol);
+
 	if (phydev->attached_dev && phydev->adjust_link)
 		phy_start_machine(phydev);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled
  2021-07-08  0:42 [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled mohammad.athari.ismail
@ 2021-07-08  1:23 ` Andrew Lunn
  2021-07-08  2:48   ` Florian Fainelli
  2021-07-08 10:02   ` Ismail, Mohammad Athari
  0 siblings, 2 replies; 12+ messages in thread
From: Andrew Lunn @ 2021-07-08  1:23 UTC (permalink / raw)
  To: mohammad.athari.ismail
  Cc: Heiner Kallweit, David S . Miller, Russell King, Jakub Kicinski,
	Florian Fainelli, netdev, linux-kernel

On Thu, Jul 08, 2021 at 08:42:53AM +0800, mohammad.athari.ismail@intel.com wrote:
> From: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>
> 
> 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.

	Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled
  2021-07-08  1:23 ` Andrew Lunn
@ 2021-07-08  2:48   ` Florian Fainelli
  2021-07-08 10:10     ` Ismail, Mohammad Athari
  2021-07-08 10:02   ` Ismail, Mohammad Athari
  1 sibling, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2021-07-08  2:48 UTC (permalink / raw)
  To: Andrew Lunn, mohammad.athari.ismail
  Cc: Heiner Kallweit, David S . Miller, Russell King, Jakub Kicinski,
	netdev, linux-kernel



On 7/7/2021 6:23 PM, Andrew Lunn wrote:
> On Thu, Jul 08, 2021 at 08:42:53AM +0800, mohammad.athari.ismail@intel.com wrote:
>> From: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>
>>
>> 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.
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled
  2021-07-08  1:23 ` Andrew Lunn
  2021-07-08  2:48   ` Florian Fainelli
@ 2021-07-08 10:02   ` Ismail, Mohammad Athari
  2021-07-08 13:11     ` Andrew Lunn
  1 sibling, 1 reply; 12+ messages in thread
From: Ismail, Mohammad Athari @ 2021-07-08 10:02 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Heiner Kallweit, David S . Miller, Russell King, Jakub Kicinski,
	Florian Fainelli, netdev, linux-kernel



> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, July 8, 2021 9:24 AM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>; David S . Miller
> <davem@davemloft.net>; Russell King <linux@armlinux.org.uk>; Jakub Kicinski
> <kuba@kernel.org>; Florian Fainelli <f.fainelli@gmail.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL
> option still enabled
> 
> On Thu, Jul 08, 2021 at 08:42:53AM +0800, mohammad.athari.ismail@intel.com
> wrote:
> > From: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>
> >
> > 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.

Hi Andrew,

In our platform, the PHY interrupt pin is not connected to Host CPU. So, the CPU couldn`t service the PHY interrupt.  The PHY interrupt pin is connected to a power management controller (PMC) as a HW wake up signal. The PMC itself couldn't act as interrupt controller to service the PHY interrupt.

During WOL event, the WOL signal is sent to PMC through the PHY interrupt pin to wake up the PMC. Then, the PMC will wake up the Host CPU and the whole system.

Therefore, for our platform, PHY_POLL mode is chosen in PHY driver.

-Athari-

> 
> 	Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled
  2021-07-08  2:48   ` Florian Fainelli
@ 2021-07-08 10:10     ` Ismail, Mohammad Athari
  2021-07-08 16:41       ` Florian Fainelli
  0 siblings, 1 reply; 12+ messages in thread
From: Ismail, Mohammad Athari @ 2021-07-08 10:10 UTC (permalink / raw)
  To: Florian Fainelli, Andrew Lunn
  Cc: Heiner Kallweit, David S . Miller, Russell King, Jakub Kicinski,
	netdev, linux-kernel



> -----Original Message-----
> From: Florian Fainelli <f.fainelli@gmail.com>
> Sent: Thursday, July 8, 2021 10:49 AM
> To: Andrew Lunn <andrew@lunn.ch>; Ismail, Mohammad Athari
> <mohammad.athari.ismail@intel.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>; David S . Miller
> <davem@davemloft.net>; Russell King <linux@armlinux.org.uk>; Jakub Kicinski
> <kuba@kernel.org>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> 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@intel.com wrote:
> >> From: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>
> >>
> >> 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.

-Athari- 

> --
> Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled
  2021-07-08 10:02   ` Ismail, Mohammad Athari
@ 2021-07-08 13:11     ` Andrew Lunn
  2021-07-08 23:11       ` Ismail, Mohammad Athari
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2021-07-08 13:11 UTC (permalink / raw)
  To: Ismail, Mohammad Athari
  Cc: Heiner Kallweit, David S . Miller, Russell King, Jakub Kicinski,
	Florian Fainelli, netdev, linux-kernel

> Hi Andrew,
> 
> In our platform, the PHY interrupt pin is not connected to Host CPU. So, the CPU couldn`t service the PHY interrupt.  The PHY interrupt pin is connected to a power management controller (PMC) as a HW wake up signal. The PMC itself couldn't act as interrupt controller to service the PHY interrupt.
> 
> During WOL event, the WOL signal is sent to PMC through the PHY interrupt pin to wake up the PMC. Then, the PMC will wake up the Host CPU and the whole system.

How is the PMC connected to the host? LPC? At wake up can you ask it
why it woke you up? What event it was, power restored, power button
press, or WOL? Can the PMC generate interrupts over the LPC? What PMC
is it? Is there a datasheet for it?

Getting your architecture correct will also solve your S3/S4 problems.

    Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled
  2021-07-08 10:10     ` Ismail, Mohammad Athari
@ 2021-07-08 16:41       ` Florian Fainelli
  2021-07-08 23:20         ` Ismail, Mohammad Athari
  2021-07-09  9:57         ` Russell King (Oracle)
  0 siblings, 2 replies; 12+ messages in thread
From: Florian Fainelli @ 2021-07-08 16:41 UTC (permalink / raw)
  To: Ismail, Mohammad Athari, Andrew Lunn
  Cc: Heiner Kallweit, David S . Miller, Russell King, Jakub Kicinski,
	netdev, linux-kernel

On 7/8/21 3:10 AM, Ismail, Mohammad Athari wrote:
> 
> 
>> -----Original Message-----
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Sent: Thursday, July 8, 2021 10:49 AM
>> To: Andrew Lunn <andrew@lunn.ch>; Ismail, Mohammad Athari
>> <mohammad.athari.ismail@intel.com>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>; David S . Miller
>> <davem@davemloft.net>; Russell King <linux@armlinux.org.uk>; Jakub Kicinski
>> <kuba@kernel.org>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
>> 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@intel.com wrote:
>>>> From: Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>
>>>>
>>>> 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?
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled
  2021-07-08 13:11     ` Andrew Lunn
@ 2021-07-08 23:11       ` Ismail, Mohammad Athari
  0 siblings, 0 replies; 12+ messages in thread
From: Ismail, Mohammad Athari @ 2021-07-08 23:11 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Heiner Kallweit, David S . Miller, Russell King, Jakub Kicinski,
	Florian Fainelli, netdev, linux-kernel



> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, July 8, 2021 9:11 PM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>; David S . Miller
> <davem@davemloft.net>; Russell King <linux@armlinux.org.uk>; Jakub
> Kicinski <kuba@kernel.org>; Florian Fainelli <f.fainelli@gmail.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL
> option still enabled
> 
> > Hi Andrew,
> >
> > In our platform, the PHY interrupt pin is not connected to Host CPU. So, the
> CPU couldn`t service the PHY interrupt.  The PHY interrupt pin is connected to
> a power management controller (PMC) as a HW wake up signal. The PMC
> itself couldn't act as interrupt controller to service the PHY interrupt.
> >
> > During WOL event, the WOL signal is sent to PMC through the PHY interrupt
> pin to wake up the PMC. Then, the PMC will wake up the Host CPU and the
> whole system.
> 
> How is the PMC connected to the host? LPC? At wake up can you ask it why it
> woke you up? What event it was, power restored, power button press, or
> WOL? Can the PMC generate interrupts over the LPC? What PMC is it? Is
> there a datasheet for it?
> 
> Getting your architecture correct will also solve your S3/S4 problems.

Hi Andrew,

I'll try to get more info from our architecture design team.

-Athari-

> 
>     Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled
  2021-07-08 16:41       ` Florian Fainelli
@ 2021-07-08 23:20         ` Ismail, Mohammad Athari
  2021-07-08 23:22           ` Florian Fainelli
  2021-07-09  9:57         ` Russell King (Oracle)
  1 sibling, 1 reply; 12+ messages in thread
From: Ismail, Mohammad Athari @ 2021-07-08 23:20 UTC (permalink / raw)
  To: Florian Fainelli, Andrew Lunn
  Cc: Heiner Kallweit, David S . Miller, Russell King, Jakub Kicinski,
	netdev, linux-kernel



> -----Original Message-----
> From: Florian Fainelli <f.fainelli@gmail.com>
> Sent: Friday, July 9, 2021 12:42 AM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>;
> Andrew Lunn <andrew@lunn.ch>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>; David S . Miller
> <davem@davemloft.net>; Russell King <linux@armlinux.org.uk>; Jakub
> Kicinski <kuba@kernel.org>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> 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@gmail.com>
> >> Sent: Thursday, July 8, 2021 10:49 AM
> >> To: Andrew Lunn <andrew@lunn.ch>; Ismail, Mohammad Athari
> >> <mohammad.athari.ismail@intel.com>
> >> Cc: Heiner Kallweit <hkallweit1@gmail.com>; David S . Miller
> >> <davem@davemloft.net>; Russell King <linux@armlinux.org.uk>; Jakub
> >> Kicinski <kuba@kernel.org>; netdev@vger.kernel.org;
> >> linux-kernel@vger.kernel.org
> >> 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@intel.com wrote:
> >>>> From: Mohammad Athari Bin Ismail
> <mohammad.athari.ismail@intel.com>
> >>>>
> >>>> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled
  2021-07-08 23:20         ` Ismail, Mohammad Athari
@ 2021-07-08 23:22           ` Florian Fainelli
  2021-07-09  1:30             ` Andrew Lunn
  0 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2021-07-08 23:22 UTC (permalink / raw)
  To: Ismail, Mohammad Athari, Andrew Lunn
  Cc: Heiner Kallweit, David S . Miller, Russell King, Jakub Kicinski,
	netdev, linux-kernel

On 7/8/21 4:20 PM, Ismail, Mohammad Athari wrote:
> 
> 
>> -----Original Message-----
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Sent: Friday, July 9, 2021 12:42 AM
>> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>;
>> Andrew Lunn <andrew@lunn.ch>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>; David S . Miller
>> <davem@davemloft.net>; Russell King <linux@armlinux.org.uk>; Jakub
>> Kicinski <kuba@kernel.org>; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> 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@gmail.com>
>>>> Sent: Thursday, July 8, 2021 10:49 AM
>>>> To: Andrew Lunn <andrew@lunn.ch>; Ismail, Mohammad Athari
>>>> <mohammad.athari.ismail@intel.com>
>>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>; David S . Miller
>>>> <davem@davemloft.net>; Russell King <linux@armlinux.org.uk>; Jakub
>>>> Kicinski <kuba@kernel.org>; netdev@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org
>>>> 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@intel.com wrote:
>>>>>> From: Mohammad Athari Bin Ismail
>> <mohammad.athari.ismail@intel.com>
>>>>>>
>>>>>> 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().

Ah yes you are right, we just skip resume in that case. OK let me think
about it some more.
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled
  2021-07-08 23:22           ` Florian Fainelli
@ 2021-07-09  1:30             ` Andrew Lunn
  0 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2021-07-09  1:30 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Ismail, Mohammad Athari, Heiner Kallweit, David S . Miller,
	Russell King, Jakub Kicinski, netdev, linux-kernel

> Ah yes you are right, we just skip resume in that case. OK let me think
> about it some more.

The point here is, it is an interrupt, from the perspective of the PHY
hardware and its driver. But the interrupt handler is never being
called because the interrupt output from the chip is not causing an
actual interrupt. Fix that, and your problem goes away. Or you need to
add a whole new mechanism that you are using the interrupt hardware in
the PHY some something else than an actual interrupt.

    Andrew


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled
  2021-07-08 16:41       ` Florian Fainelli
  2021-07-08 23:20         ` Ismail, Mohammad Athari
@ 2021-07-09  9:57         ` Russell King (Oracle)
  1 sibling, 0 replies; 12+ messages in thread
From: Russell King (Oracle) @ 2021-07-09  9:57 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Ismail, Mohammad Athari, Andrew Lunn, Heiner Kallweit,
	David S . Miller, Jakub Kicinski, netdev, linux-kernel

On Thu, Jul 08, 2021 at 09:41:45AM -0700, Florian Fainelli wrote:
> 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?

Don't forget that when resuming from S4, we effectively boot the kernel
normally, then check whether we are resuming, before we then start
loading the resume image, suspend the current kernel, shuffle the pages
around, and resume the original kernel.

What that means is that a PHY will see the effects of a normal kernel
boot before we resume the original kernel. If a PHY driver (e.g.)
disables features such as WOL and we do not restore these settings on
resume, then those settings will be reset.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-07-09  9:57 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-08  0:42 [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL option still enabled mohammad.athari.ismail
2021-07-08  1:23 ` Andrew Lunn
2021-07-08  2:48   ` Florian Fainelli
2021-07-08 10:10     ` Ismail, Mohammad Athari
2021-07-08 16:41       ` Florian Fainelli
2021-07-08 23:20         ` Ismail, Mohammad Athari
2021-07-08 23:22           ` Florian Fainelli
2021-07-09  1:30             ` Andrew Lunn
2021-07-09  9:57         ` Russell King (Oracle)
2021-07-08 10:02   ` Ismail, Mohammad Athari
2021-07-08 13:11     ` Andrew Lunn
2021-07-08 23:11       ` Ismail, Mohammad Athari

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).