[PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Uwe Kleine-König]

Most PHYs signal WoL using an interrupt. So disabling interrupts breaks
WoL at least on PHYs covered by the marvell driver. So skip disabling
irqs on shutdown if WoL is enabled.

While at it also explain the motivation that irqs are disabled at all.

Fixes: e2f016cf7751 ("net: phy: add a shutdown procedure")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

while I'm not sure that disabling interrupts is a good idea in general,
this change at least should fix the WoL case. Note that this change is
only compile tested as next doesn't boot on my test machine (because of
https://git.kernel.org/linus/b3574f579ece24439c90e9a179742c61205fbcfa)
and 6.1 (which is the other kernel I have running) doesn't know about
.wol_enabled. I don't want to delay this fix until I bisected this new
issue.

Assuming this patch is eligible for backporting to stable, maybe point
out that it depends on v6.5-rc1~163^2~286^2~2 ("net: phy: Allow drivers
to always call into ->suspend()"). Didn't try to backport that.

Best regards
Uwe

 drivers/net/phy/phy_device.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 61921d4dbb13..6d1526bdd1d7 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3340,6 +3340,15 @@ static void phy_shutdown(struct device *dev)
 	if (phydev->state == PHY_READY || !phydev->attached_dev)
 		return;
 
+	/* Most phys signal WoL via the irq line. So for these irqs shouldn't be
+	 * disabled.
+	 */
+	if (phydev->wol_enabled)
+		return;
+
+	/* On shutdown disable irqs to prevent an irq storm on systems where the
+	 * irq line is shared by several devices.
+	 */
 	phy_disable_interrupts(phydev);
 }
 
-- 
2.40.1

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Jakub Kicinski]

On Fri,  4 Aug 2023 09:17:57 +0200 Uwe Kleine-König wrote:
> Most PHYs signal WoL using an interrupt. So disabling interrupts breaks
> WoL at least on PHYs covered by the marvell driver. So skip disabling
> irqs on shutdown if WoL is enabled.
> 
> While at it also explain the motivation that irqs are disabled at all.
> 
> Fixes: e2f016cf7751 ("net: phy: add a shutdown procedure")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

What do we do with this one? It sounded like Russell was leaning
towards a revert?

FTR original report:
https://lore.kernel.org/all/20230803181640.yzxsk2xphwryxww4@pengutronix.de/

> while I'm not sure that disabling interrupts is a good idea in general,
> this change at least should fix the WoL case. Note that this change is
> only compile tested as next doesn't boot on my test machine (because of
> https://git.kernel.org/linus/b3574f579ece24439c90e9a179742c61205fbcfa)
> and 6.1 (which is the other kernel I have running) doesn't know about
> .wol_enabled. I don't want to delay this fix until I bisected this new
> issue.
> 
> Assuming this patch is eligible for backporting to stable, maybe point
> out that it depends on v6.5-rc1~163^2~286^2~2 ("net: phy: Allow drivers
> to always call into ->suspend()"). Didn't try to backport that.
> 
> Best regards
> Uwe
> 
>  drivers/net/phy/phy_device.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 61921d4dbb13..6d1526bdd1d7 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3340,6 +3340,15 @@ static void phy_shutdown(struct device *dev)
>  	if (phydev->state == PHY_READY || !phydev->attached_dev)
>  		return;
>  
> +	/* Most phys signal WoL via the irq line. So for these irqs shouldn't be
> +	 * disabled.
> +	 */
> +	if (phydev->wol_enabled)
> +		return;
> +
> +	/* On shutdown disable irqs to prevent an irq storm on systems where the
> +	 * irq line is shared by several devices.
> +	 */
>  	phy_disable_interrupts(phydev);
>  }
>  

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Florian Fainelli]

On 8/8/23 14:53, Jakub Kicinski wrote:
> On Fri,  4 Aug 2023 09:17:57 +0200 Uwe Kleine-König wrote:
>> Most PHYs signal WoL using an interrupt. So disabling interrupts breaks
>> WoL at least on PHYs covered by the marvell driver. So skip disabling
>> irqs on shutdown if WoL is enabled.
>>
>> While at it also explain the motivation that irqs are disabled at all.
>>
>> Fixes: e2f016cf7751 ("net: phy: add a shutdown procedure")
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> What do we do with this one? It sounded like Russell was leaning
> towards a revert?

Yes, though I believe this will create a different kind of regression 
for what Iona was addressing initially. Then it becomes a choice of 
which regression do we consider to be the worst to handle until 
something better comes up.

Russell what are your thoughts?
-- 
Florian

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Russell King (Oracle)]

On Tue, Aug 08, 2023 at 02:59:41PM -0700, Florian Fainelli wrote:
> On 8/8/23 14:53, Jakub Kicinski wrote:
> > On Fri,  4 Aug 2023 09:17:57 +0200 Uwe Kleine-König wrote:
> > > Most PHYs signal WoL using an interrupt. So disabling interrupts breaks
> > > WoL at least on PHYs covered by the marvell driver. So skip disabling
> > > irqs on shutdown if WoL is enabled.
> > > 
> > > While at it also explain the motivation that irqs are disabled at all.
> > > 
> > > Fixes: e2f016cf7751 ("net: phy: add a shutdown procedure")
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > What do we do with this one? It sounded like Russell was leaning
> > towards a revert?
> 
> Yes, though I believe this will create a different kind of regression for
> what Iona was addressing initially. Then it becomes a choice of which
> regression do we consider to be the worst to handle until something better
> comes up.
> 
> Russell what are your thoughts?

In this situation where a fix for a problem is provided which then
causes a regression by fixing that problem, I've seen Linus T state
that it means the fix was incorrect. That seems entirely sensible.

We are, of course, in the situation where reverting the commit
restores the old behaviour and thus fixes a regression, but causes
a regression for another user.

If it is possible to quickly come up with a fix that avoids any
regression to either use case, then that is obviously preferable.
However, if that's not possible, then it seems going back to the
original situation (i.o.w. reverting) is sensible.

Now, the fact is that many PHYs do use their interrupts to signal
that a wake-up happened, and disabling the IRQ from the PHY will
prevent WoL from working. Other PHYs have a separate pin for this.
Two recent examples are AR8035, which only has a single interrupt
pin which covers all interrupts from that PHY, and AR8031 or AR8033
which have a separate WOL_INT pin which might be used - or the
main interrupt pin.

If we hibernate the system, then people how have configured WoL
are going to expect it to work - but disabling the ability for
the PHY to raise an interrupt will prevent it.

So, clearly always disabling PHY interrupts can have a detrimental
effect on the ability to wake a system up using WoL - where the PHY
interrupt is used to signal WoL to the rest of the system.

Now, if waking the system up from hibernation using WoL involves
the PHY asserting its interrupt pin, then the system must be
capable of dealing with the PHY asserting its interrupt while the
system is booting. Remember that the way Linux hibernation works,
that boot is just the same as a regular boot right up through the
normal kernel initialisation. It is only towards the end that the
kernel detects the signature in swap space, and then does the
funky stuff to resume from the saved data.

So, during that boot, the system has to cope with that interrupt
having been asserted by the PHY hardware. Either system firmware
has to recognise that was the wake-up event and deal with it (e.g.
disabling the interrupt source) before passing control to the
kernel, or the kernel has to be able to cope with that interrupt
being stuck at active state until the PHY driver can deal with it.

Obviously, if WoL is not enabled or supported, then disabling the
PHY interrupt should be harmless - but that will have the effect
of masking any issues that a platform may have until PHY based
WoL has been enabled.

Also, don't forget that we have this kexec thing - and the
.shutdown methods will be called just before handing control to
the new kernel.

Uwe's patch solves the problem that he's experiencing - because
it makes the interrupt disabling dependent on the WoL configuration.

However, Ioana's problem would still remain - and enabling WoL on
that platform will make it reappear - and thus it still needs to be
properly fixed.

If that problem is properly fixed, then we don't need to disable
PHY interrupts, which means a revert would be the right approach.

Honestly, I don't know what would be best - and I don't believe we've
heard from Ioana about the problem that was trying to be addressed
(e.g. exactly when it happened and why.)

If I had to guess:
- the PHY in question may be sharing an interrupt with another device
- when that other device probes and claims its interrupt, an interrupt
  storm ensues
- the interrupt layer disables the interrupt input, rendering both the
  PHY and other device unusable.

I think I've covered all the possibilities, all the issues, outcomes,
and the politics as far as Linus T would state. I'm also quite sure
that there will be no way to satisfy everyone!

Bearing in mind that it is holiday season, and we're at -rc5, I
think we should give Ioana a bit more time to respond before we
make a decision. Maybe a little over a week? If we don't hear anything,
then I think following our established policy and reverting would be
the correct way forward.

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

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Ioana Ciornei]

On Tue, Aug 08, 2023 at 11:56:56PM +0100, Russell King (Oracle) wrote:
> On Tue, Aug 08, 2023 at 02:59:41PM -0700, Florian Fainelli wrote:
> > On 8/8/23 14:53, Jakub Kicinski wrote:
> > > On Fri,  4 Aug 2023 09:17:57 +0200 Uwe Kleine-König wrote:
> > > > Most PHYs signal WoL using an interrupt. So disabling interrupts breaks
> > > > WoL at least on PHYs covered by the marvell driver. So skip disabling
> > > > irqs on shutdown if WoL is enabled.
> > > > 
> > > > While at it also explain the motivation that irqs are disabled at all.
> > > > 
> > > > Fixes: e2f016cf7751 ("net: phy: add a shutdown procedure")
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > 
> > > What do we do with this one? It sounded like Russell was leaning
> > > towards a revert?
> > 
> > Yes, though I believe this will create a different kind of regression for
> > what Iona was addressing initially. Then it becomes a choice of which
> > regression do we consider to be the worst to handle until something better
> > comes up.
> > 
> > Russell what are your thoughts?
> 
> In this situation where a fix for a problem is provided which then
> causes a regression by fixing that problem, I've seen Linus T state
> that it means the fix was incorrect. That seems entirely sensible.
> 
> We are, of course, in the situation where reverting the commit
> restores the old behaviour and thus fixes a regression, but causes
> a regression for another user.
> 
> If it is possible to quickly come up with a fix that avoids any
> regression to either use case, then that is obviously preferable.
> However, if that's not possible, then it seems going back to the
> original situation (i.o.w. reverting) is sensible.
> 
> Now, the fact is that many PHYs do use their interrupts to signal
> that a wake-up happened, and disabling the IRQ from the PHY will
> prevent WoL from working. Other PHYs have a separate pin for this.
> Two recent examples are AR8035, which only has a single interrupt
> pin which covers all interrupts from that PHY, and AR8031 or AR8033
> which have a separate WOL_INT pin which might be used - or the
> main interrupt pin.
> 
> If we hibernate the system, then people how have configured WoL
> are going to expect it to work - but disabling the ability for
> the PHY to raise an interrupt will prevent it.
> 
> So, clearly always disabling PHY interrupts can have a detrimental
> effect on the ability to wake a system up using WoL - where the PHY
> interrupt is used to signal WoL to the rest of the system.
> 
> Now, if waking the system up from hibernation using WoL involves
> the PHY asserting its interrupt pin, then the system must be
> capable of dealing with the PHY asserting its interrupt while the
> system is booting. Remember that the way Linux hibernation works,
> that boot is just the same as a regular boot right up through the
> normal kernel initialisation. It is only towards the end that the
> kernel detects the signature in swap space, and then does the
> funky stuff to resume from the saved data.
> 
> So, during that boot, the system has to cope with that interrupt
> having been asserted by the PHY hardware. Either system firmware
> has to recognise that was the wake-up event and deal with it (e.g.
> disabling the interrupt source) before passing control to the
> kernel, or the kernel has to be able to cope with that interrupt
> being stuck at active state until the PHY driver can deal with it.
> 
> Obviously, if WoL is not enabled or supported, then disabling the
> PHY interrupt should be harmless - but that will have the effect
> of masking any issues that a platform may have until PHY based
> WoL has been enabled.
> 
> Also, don't forget that we have this kexec thing - and the
> .shutdown methods will be called just before handing control to
> the new kernel.
> 
> Uwe's patch solves the problem that he's experiencing - because
> it makes the interrupt disabling dependent on the WoL configuration.
> 
> However, Ioana's problem would still remain - and enabling WoL on
> that platform will make it reappear - and thus it still needs to be
> properly fixed.
> 
> If that problem is properly fixed, then we don't need to disable
> PHY interrupts, which means a revert would be the right approach.

Yes, my initial problem would still remain if WoL is enabled on any of
the PHYs that have a shared IRQ line.

The problem is that I find this combination - shared IRQ and WoL enabled
- impossible to make it work. Maybe we could look into denying WoL from
being enabled on PHYs which have a shared interrupt line.

> 
> Honestly, I don't know what would be best - and I don't believe we've
> heard from Ioana about the problem that was trying to be addressed
> (e.g. exactly when it happened and why.)
> 
> If I had to guess:
> - the PHY in question may be sharing an interrupt with another device
> - when that other device probes and claims its interrupt, an interrupt
>   storm ensues
> - the interrupt layer disables the interrupt input, rendering both the
>   PHY and other device unusable.
> 

That's a perfect summary of the problem that I was trying to fix.

The board in question is a LS1021ATSN which has two AR8031 PHYs that
share an interrupt line. In case only one of the PHYs is probed and
there are pending interrupts on the PHY#2 an IRQ storm will happen
since there is no entity to clear the interrupt from PHY#2's registers.
PHY#1's driver will get stuck in .handle_interrupt() indefinitely.

> I think I've covered all the possibilities, all the issues, outcomes,
> and the politics as far as Linus T would state. I'm also quite sure
> that there will be no way to satisfy everyone!
> 
> Bearing in mind that it is holiday season, and we're at -rc5, I
> think we should give Ioana a bit more time to respond before we
> make a decision. Maybe a little over a week? If we don't hear anything,
> then I think following our established policy and reverting would be
> the correct way forward.
> 

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Russell King (Oracle)]

On Wed, Aug 09, 2023 at 05:21:55PM +0300, Ioana Ciornei wrote:
> That's a perfect summary of the problem that I was trying to fix.
> 
> The board in question is a LS1021ATSN which has two AR8031 PHYs that
> share an interrupt line. In case only one of the PHYs is probed and
> there are pending interrupts on the PHY#2 an IRQ storm will happen
> since there is no entity to clear the interrupt from PHY#2's registers.
> PHY#1's driver will get stuck in .handle_interrupt() indefinitely.

So I have two further questions:
1. Is WoL able to be supported on this hardware?
2. AR8031 has a seperate WOL_INT signal that can be used to wake up the
   system. Is this used in the hardware design?

Thanks.

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

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Ioana Ciornei]

On Wed, Aug 09, 2023 at 03:29:17PM +0100, Russell King (Oracle) wrote:
> On Wed, Aug 09, 2023 at 05:21:55PM +0300, Ioana Ciornei wrote:
> > That's a perfect summary of the problem that I was trying to fix.
> > 
> > The board in question is a LS1021ATSN which has two AR8031 PHYs that
> > share an interrupt line. In case only one of the PHYs is probed and
> > there are pending interrupts on the PHY#2 an IRQ storm will happen
> > since there is no entity to clear the interrupt from PHY#2's registers.
> > PHY#1's driver will get stuck in .handle_interrupt() indefinitely.
> 
> So I have two further questions:
> 1. Is WoL able to be supported on this hardware?

I don't know if anyone cares about WoL on the AR8031 PHYs from this
board.

Both of the PHYs are used in conjuction with 2 eTSEC controllers - which
use the driver in drivers/net/ethernet/freescale/gianfar.c.

The Ethernet controller does have WoL support, which means that WoL could
still be supported on the board even though we would forbid WoL on the
AR8031 PHYs.

> 2. AR8031 has a seperate WOL_INT signal that can be used to wake up the
>    system. Is this used in the hardware design?

No, WOL_INT is not connected.

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Russell King (Oracle)]

On Wed, Aug 09, 2023 at 06:44:18PM +0300, Ioana Ciornei wrote:
> On Wed, Aug 09, 2023 at 03:29:17PM +0100, Russell King (Oracle) wrote:
> > On Wed, Aug 09, 2023 at 05:21:55PM +0300, Ioana Ciornei wrote:
> > > That's a perfect summary of the problem that I was trying to fix.
> > > 
> > > The board in question is a LS1021ATSN which has two AR8031 PHYs that
> > > share an interrupt line. In case only one of the PHYs is probed and
> > > there are pending interrupts on the PHY#2 an IRQ storm will happen
> > > since there is no entity to clear the interrupt from PHY#2's registers.
> > > PHY#1's driver will get stuck in .handle_interrupt() indefinitely.
> > 
> > So I have two further questions:
> > 1. Is WoL able to be supported on this hardware?
> 
> I don't know if anyone cares about WoL on the AR8031 PHYs from this
> board.
> 
> Both of the PHYs are used in conjuction with 2 eTSEC controllers - which
> use the driver in drivers/net/ethernet/freescale/gianfar.c.
> 
> The Ethernet controller does have WoL support, which means that WoL could
> still be supported on the board even though we would forbid WoL on the
> AR8031 PHYs.
> 
> > 2. AR8031 has a seperate WOL_INT signal that can be used to wake up the
> >    system. Is this used in the hardware design?
> 
> No, WOL_INT is not connected.

Okay, so we don't need to care about WoL for your hardware setup.

Thinking about this, I wonder whether we could solve your issue by
disabling interrupts when the PHY is probed, rather than disabling
them on shutdown - something like this? (not build tested)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3e9909b30938..4d1a37487923 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3216,6 +3216,8 @@ static int phy_probe(struct device *dev)
 			goto out;
 	}
 
+        phy_disable_interrupts(phydev);
+
 	/* Start out supporting everything. Eventually,
 	 * a controller will attach, and may modify one
 	 * or both of these values
@@ -3333,16 +3335,6 @@ static int phy_remove(struct device *dev)
 	return 0;
 }
 
-static void phy_shutdown(struct device *dev)
-{
-	struct phy_device *phydev = to_phy_device(dev);
-
-	if (phydev->state == PHY_READY || !phydev->attached_dev)
-		return;
-
-	phy_disable_interrupts(phydev);
-}
-
 /**
  * phy_driver_register - register a phy_driver with the PHY layer
  * @new_driver: new phy_driver to register
@@ -3376,7 +3368,6 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)
 	new_driver->mdiodrv.driver.bus = &mdio_bus_type;
 	new_driver->mdiodrv.driver.probe = phy_probe;
 	new_driver->mdiodrv.driver.remove = phy_remove;
-	new_driver->mdiodrv.driver.shutdown = phy_shutdown;
 	new_driver->mdiodrv.driver.owner = owner;
 	new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS;
 
-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Andrew Lunn]

> Thinking about this, I wonder whether we could solve your issue by
> disabling interrupts when the PHY is probed, rather than disabling
> them on shutdown - something like this? (not build tested)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 3e9909b30938..4d1a37487923 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3216,6 +3216,8 @@ static int phy_probe(struct device *dev)
>  			goto out;
>  	}
>  
> +        phy_disable_interrupts(phydev);
> +
>  	/* Start out supporting everything. Eventually,
>  	 * a controller will attach, and may modify one
>  	 * or both of these values

At some point, the interrupt is going to be enabled again. And then
the WoL interrupt will fire. I think some PHY drivers actually need to
see that WoL interrupt. e.g. the marvell driver has this comment:

static int m88e1318_set_wol(struct phy_device *phydev,
                            struct ethtool_wolinfo *wol)
{
....
                /* If WOL event happened once, the LED[2] interrupt pin
                 * will not be cleared unless we reading the interrupt status
                 * register. If interrupts are in use, the normal interrupt
                 * handling will clear the WOL event. Clear the WOL event
                 * before enabling it if !phy_interrupt_is_valid()
                 */

So it seems like just probing the marvell PHY is not enough to clear
the WoL interrupt.

Can we be sure that the other PHY has reached a state it can handle
and clear an interrupt when we come to enable the interrupt? I think
not, especially in cases like NFS root, where the interface will be
put into use as soon as it exists, maybe before the other interface
has probed.

	Andrew

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Florian Fainelli]

On 8/9/2023 9:21 AM, Andrew Lunn wrote:
>> Thinking about this, I wonder whether we could solve your issue by
>> disabling interrupts when the PHY is probed, rather than disabling
>> them on shutdown - something like this? (not build tested)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 3e9909b30938..4d1a37487923 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -3216,6 +3216,8 @@ static int phy_probe(struct device *dev)
>>   			goto out;
>>   	}
>>   
>> +        phy_disable_interrupts(phydev);
>> +
>>   	/* Start out supporting everything. Eventually,
>>   	 * a controller will attach, and may modify one
>>   	 * or both of these values
> 
> At some point, the interrupt is going to be enabled again. And then
> the WoL interrupt will fire. I think some PHY drivers actually need to
> see that WoL interrupt. e.g. the marvell driver has this comment:
> 
> static int m88e1318_set_wol(struct phy_device *phydev,
>                              struct ethtool_wolinfo *wol)
> {
> ....
>                  /* If WOL event happened once, the LED[2] interrupt pin
>                   * will not be cleared unless we reading the interrupt status
>                   * register. If interrupts are in use, the normal interrupt
>                   * handling will clear the WOL event. Clear the WOL event
>                   * before enabling it if !phy_interrupt_is_valid()
>                   */
> 
> So it seems like just probing the marvell PHY is not enough to clear
> the WoL interrupt.
> 
> Can we be sure that the other PHY has reached a state it can handle
> and clear an interrupt when we come to enable the interrupt? I think
> not, especially in cases like NFS root, where the interface will be
> put into use as soon as it exists, maybe before the other interface
> has probed.

Does it really make sense to have the PHY be interrupt driven for this 
specific board configuration if this causes so much hassle?
-- 
Florian

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Russell King (Oracle)]

On Wed, Aug 09, 2023 at 06:21:58PM +0200, Andrew Lunn wrote:
> > Thinking about this, I wonder whether we could solve your issue by
> > disabling interrupts when the PHY is probed, rather than disabling
> > them on shutdown - something like this? (not build tested)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 3e9909b30938..4d1a37487923 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -3216,6 +3216,8 @@ static int phy_probe(struct device *dev)
> >  			goto out;
> >  	}
> >  
> > +        phy_disable_interrupts(phydev);
> > +
> >  	/* Start out supporting everything. Eventually,
> >  	 * a controller will attach, and may modify one
> >  	 * or both of these values
> 
> At some point, the interrupt is going to be enabled again. And then
> the WoL interrupt will fire. I think some PHY drivers actually need to
> see that WoL interrupt. e.g. the marvell driver has this comment:
> 
> static int m88e1318_set_wol(struct phy_device *phydev,
>                             struct ethtool_wolinfo *wol)
> {
> ....
>                 /* If WOL event happened once, the LED[2] interrupt pin
>                  * will not be cleared unless we reading the interrupt status
>                  * register. If interrupts are in use, the normal interrupt
>                  * handling will clear the WOL event. Clear the WOL event
>                  * before enabling it if !phy_interrupt_is_valid()
>                  */
> 
> So it seems like just probing the marvell PHY is not enough to clear
> the WoL interrupt.
> 
> Can we be sure that the other PHY has reached a state it can handle
> and clear an interrupt when we come to enable the interrupt? I think
> not, especially in cases like NFS root, where the interface will be
> put into use as soon as it exists, maybe before the other interface
> has probed.

I suppose the question to Ioana would be - are the two AR8031 PHYs on
the same MDIO bus? If they are, then we're safe, because both will be
probed consecutively (because they're using the same driver.)

I know it would be desirable to have a generic solution to this, but
I don't think that is sanely achievable if we have multiple different
PHYs sharing an interrupt line over multiple different MDIO buses
and multiple different PHY drivers.

So, I'm suggesting we try to do a best-effort solution to solve Ioana's
problem so that we can restore Uwe's WoL behaviour without causing
Ioana's issue to regress. It does mean that if someone has a more weird
setup (such as I describe in my paragraph above) it won't work, but
then it also didn't used to work before Ioana's patch.

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

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Ioana Ciornei]

On Wed, Aug 09, 2023 at 08:15:27PM +0100, Russell King (Oracle) wrote:
> On Wed, Aug 09, 2023 at 06:21:58PM +0200, Andrew Lunn wrote:
> > > Thinking about this, I wonder whether we could solve your issue by
> > > disabling interrupts when the PHY is probed, rather than disabling
> > > them on shutdown - something like this? (not build tested)
> > > 
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index 3e9909b30938..4d1a37487923 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -3216,6 +3216,8 @@ static int phy_probe(struct device *dev)
> > >  			goto out;
> > >  	}
> > >  
> > > +        phy_disable_interrupts(phydev);
> > > +
> > >  	/* Start out supporting everything. Eventually,
> > >  	 * a controller will attach, and may modify one
> > >  	 * or both of these values
> > 
> > At some point, the interrupt is going to be enabled again. And then
> > the WoL interrupt will fire. I think some PHY drivers actually need to
> > see that WoL interrupt. e.g. the marvell driver has this comment:
> > 
> > static int m88e1318_set_wol(struct phy_device *phydev,
> >                             struct ethtool_wolinfo *wol)
> > {
> > ....
> >                 /* If WOL event happened once, the LED[2] interrupt pin
> >                  * will not be cleared unless we reading the interrupt status
> >                  * register. If interrupts are in use, the normal interrupt
> >                  * handling will clear the WOL event. Clear the WOL event
> >                  * before enabling it if !phy_interrupt_is_valid()
> >                  */
> > 
> > So it seems like just probing the marvell PHY is not enough to clear
> > the WoL interrupt.
> > 
> > Can we be sure that the other PHY has reached a state it can handle
> > and clear an interrupt when we come to enable the interrupt? I think
> > not, especially in cases like NFS root, where the interface will be
> > put into use as soon as it exists, maybe before the other interface
> > has probed.
> 
> I suppose the question to Ioana would be - are the two AR8031 PHYs on
> the same MDIO bus? If they are, then we're safe, because both will be
> probed consecutively (because they're using the same driver.)
> 

Yes, the two AR8031 PHYs are on the same MDIO bus.

I just tested your approach to disable the interrupts at phy_probe() and
it seems to be working. I also tested with NFS boot using one of the
PHYs and it's behaving ok.

I think I'm ok with this approach as long as Uwe's problem is also
fixed. I don't know how a wake-on-lan procedure works and if it matters
if the WoL interrupt is lost before the PHY driver gets to know about
it.

Ioana

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Vladimir Oltean]

Hi Uwe,

(I hope the threading won't be broken)

On Fri, Aug 04, 2023 at 09:17:57AM +0200, Uwe Kleine-König wrote:
> Most PHYs signal WoL using an interrupt. So disabling interrupts breaks
> WoL at least on PHYs covered by the marvell driver. So skip disabling
> irqs on shutdown if WoL is enabled.
> 
> While at it also explain the motivation that irqs are disabled at all.
> 
> Fixes: e2f016cf7751 ("net: phy: add a shutdown procedure")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> while I'm not sure that disabling interrupts is a good idea in general,
> this change at least should fix the WoL case. Note that this change is
> only compile tested as next doesn't boot on my test machine (because of
> https://git.kernel.org/linus/b3574f579ece24439c90e9a179742c61205fbcfa)
> and 6.1 (which is the other kernel I have running) doesn't know about
> .wol_enabled. I don't want to delay this fix until I bisected this new
> issue.
> 
> Assuming this patch is eligible for backporting to stable, maybe point
> out that it depends on v6.5-rc1~163^2~286^2~2 ("net: phy: Allow drivers
> to always call into ->suspend()"). Didn't try to backport that.
> 
> Best regards
> Uwe
> 
>  drivers/net/phy/phy_device.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 61921d4dbb13..6d1526bdd1d7 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3340,6 +3340,15 @@ static void phy_shutdown(struct device *dev)
>  	if (phydev->state == PHY_READY || !phydev->attached_dev)
>  		return;
>  
> +	/* Most phys signal WoL via the irq line. So for these irqs shouldn't be
> +	 * disabled.
> +	 */
> +	if (phydev->wol_enabled)
> +		return;
> +
> +	/* On shutdown disable irqs to prevent an irq storm on systems where the
> +	 * irq line is shared by several devices.
> +	 */
>  	phy_disable_interrupts(phydev);
>  }
>  
> -- 
> 2.40.1
> 
> 

I think the idea is not bad and something along these lines might be the
way to go, but I don't think it works (as currently implemented, and
tested by me, prints below).

Upon a quick search, phydev->wol_enabled is only set from phy_suspend(),
and phy_suspend() isn't invoked from the ethnl_set_wol() call stack.

Confirmed this way:

$ ethtool -s end0 wol g # &enet0 in the device tree
$ reboot -f
Rebooting.
[  288.682444] Qualcomm Atheros AR8031/AR8033 mdio@2d24000:02: phy_shutdown: wol_enabled 0
[  288.690935] Qualcomm Atheros AR8031/AR8033 mdio@2d24000:01: phy_shutdown: wol_enabled 0
[  288.736145] reboot: Restarting system

This is on the arch/arm/boot/dts/nxp/ls/ls1021a-tsn.dts, the same board
as the one which Ioana worked on (with the shared AR8031 PHY interrupts).

Sure, it needs to be mentioned that WoL + shared PHY interrupts is not
by any means an impossible combination, but it still won't work reliably.
I guess that's ok temporarily, since WoL requires user opt-in, so in the
default configuration, the LS1021A-TSN is not broken by this change (in
its functional variant).

pw-bot: cr

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Florian Fainelli]

On 8/9/2023 6:58 AM, Vladimir Oltean wrote:
> Hi Uwe,
> 
> (I hope the threading won't be broken)
> 
> On Fri, Aug 04, 2023 at 09:17:57AM +0200, Uwe Kleine-König wrote:
>> Most PHYs signal WoL using an interrupt. So disabling interrupts breaks
>> WoL at least on PHYs covered by the marvell driver. So skip disabling
>> irqs on shutdown if WoL is enabled.
>>
>> While at it also explain the motivation that irqs are disabled at all.
>>
>> Fixes: e2f016cf7751 ("net: phy: add a shutdown procedure")
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>> Hello,
>>
>> while I'm not sure that disabling interrupts is a good idea in general,
>> this change at least should fix the WoL case. Note that this change is
>> only compile tested as next doesn't boot on my test machine (because of
>> https://git.kernel.org/linus/b3574f579ece24439c90e9a179742c61205fbcfa)
>> and 6.1 (which is the other kernel I have running) doesn't know about
>> .wol_enabled. I don't want to delay this fix until I bisected this new
>> issue.
>>
>> Assuming this patch is eligible for backporting to stable, maybe point
>> out that it depends on v6.5-rc1~163^2~286^2~2 ("net: phy: Allow drivers
>> to always call into ->suspend()"). Didn't try to backport that.
>>
>> Best regards
>> Uwe
>>
>>   drivers/net/phy/phy_device.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 61921d4dbb13..6d1526bdd1d7 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -3340,6 +3340,15 @@ static void phy_shutdown(struct device *dev)
>>   	if (phydev->state == PHY_READY || !phydev->attached_dev)
>>   		return;
>>   
>> +	/* Most phys signal WoL via the irq line. So for these irqs shouldn't be
>> +	 * disabled.
>> +	 */
>> +	if (phydev->wol_enabled)
>> +		return;
>> +
>> +	/* On shutdown disable irqs to prevent an irq storm on systems where the
>> +	 * irq line is shared by several devices.
>> +	 */
>>   	phy_disable_interrupts(phydev);
>>   }
>>   
>> -- 
>> 2.40.1
>>
>>
> 
> I think the idea is not bad and something along these lines might be the
> way to go, but I don't think it works (as currently implemented, and
> tested by me, prints below).
> 
> Upon a quick search, phydev->wol_enabled is only set from phy_suspend(),
> and phy_suspend() isn't invoked from the ethnl_set_wol() call stack.

You are right, this was an ill advised suggestion from my side here. In 
principle however we need to have something like this in the shutdown 
routine to have this patch work:

        struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
        struct net_device *netdev = phydev->attached_dev;
        int ret;

        phy_ethtool_get_wol(phydev, &wol);
        phydev->wol_enabled = wol.wolopts || (netdev && 
netdev->wol_enabled);

	if (!phydev->wol_enabled)
		return;

this does make me wonder whether Uwe tested with a prior system 
suspend/resume cycle before shutting down?
-- 
Florian

Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is enabled

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

Hello Florian,

On Wed, Aug 09, 2023 at 08:35:24AM -0700, Florian Fainelli wrote:
> this does make me wonder whether Uwe tested with a prior system
> suspend/resume cycle before shutting down?

No, he didn't. That's why he wrote "Note that this change is
only compile tested as next doesn't boot on my test machine (because of
https://git.kernel.org/linus/b3574f579ece24439c90e9a179742c61205fbcfa)
and 6.1 (which is the other kernel I have running) doesn't know about
.wol_enabled. I don't want to delay this fix until I bisected this new
issue." in the mail containing the patch.

The issue in next is resolved, but I didn't come around to test this
patch yet.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]