[net-next,v2,02/19] net: phy: add a shutdown procedure

Commit Message

Ioana Ciornei Nov. 1, 2020, 12:50 p.m. UTC

From: Ioana Ciornei <[e-mail]>

In case of a board which uses a shared IRQ we can easily end up with an
IRQ storm after a forced reboot.

For example, a 'reboot -f' will trigger a call to the .shutdown()
callbacks of all devices. Because phylib does not implement that hook,
the PHY is not quiesced, thus it can very well leave its IRQ enabled.

At the next boot, if that IRQ line is found asserted by the first PHY
driver that uses it, but _before_ the driver that is _actually_ keeping
the shared IRQ asserted is probed, the IRQ is not going to be
acknowledged, thus it will keep being fired preventing the boot process
of the kernel to continue. This is even worse when the second PHY driver
is a module.

To fix this, implement the .shutdown() callback and disable the
interrupts if these are used.

Note that we are still susceptible to IRQ storms if the previous kernel
exited with a panic or if the bootloader left the shared IRQ active, but
there is absolutely nothing we can do about these cases.

Cc: Alexandru Ardelean <[e-mail]>
Cc: Andre Edich <[e-mail]>
Cc: Antoine Tenart <[e-mail]>
Cc: Baruch Siach <[e-mail]>
Cc: Christophe Leroy <[e-mail]>
Cc: Dan Murphy <[e-mail]>
Cc: Divya Koppera <[e-mail]>
Cc: Florian Fainelli <[e-mail]>
Cc: Hauke Mehrtens <[e-mail]>
Cc: Heiner Kallweit <[e-mail]>
Cc: Jerome Brunet <[e-mail]>
Cc: Kavya Sree Kotagiri <[e-mail]>
Cc: Linus Walleij <[e-mail]>
Cc: Marco Felsch <[e-mail]>
Cc: Marek Vasut <[e-mail]>
Cc: Martin Blumenstingl <[e-mail]>
Cc: Mathias Kresin <[e-mail]>
Cc: Maxim Kochetkov <[e-mail]>
Cc: Michael Walle <[e-mail]>
Cc: Neil Armstrong <[e-mail]>
Cc: Nisar Sayed <[e-mail]>
Cc: Oleksij Rempel <[e-mail]>
Cc: Philippe Schenker <[e-mail]>
Cc: Willy Liu <[e-mail]>
Cc: Yuiko Oshino <[e-mail]>
Signed-off-by: Ioana Ciornei <[e-mail]>
---
Changes in v2:
 - none

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

Comments

Uwe Kleine-König Aug. 3, 2023, 6:16 p.m. UTC | #1

Hello,

this patch became commit e2f016cf775129c050d6c79483073423db15c79a and is
contained in v5.11-rc1.

It broke wake-on-lan on my NAS (an ARM machine with an Armada 370 SoC,
armada-370-netgear-rn104.dts). The used phy driver is marvell.c. I only
report it now as I just upgraded that machine from Debian 11 (with
kernel 5.10.x) to Debian 12 (with kernel 6.1.x).

Commenting out phy_disable_interrupts(...) in v6.1.41's phy_shutdown()
fixes the problem for me.

On Sun, Nov 01, 2020 at 02:50:57PM +0200, Ioana Ciornei wrote:
> In case of a board which uses a shared IRQ we can easily end up with an
> IRQ storm after a forced reboot.
> 
> For example, a 'reboot -f' will trigger a call to the .shutdown()
> callbacks of all devices. Because phylib does not implement that hook,
> the PHY is not quiesced, thus it can very well leave its IRQ enabled.
> 
> At the next boot, if that IRQ line is found asserted by the first PHY
> driver that uses it, but _before_ the driver that is _actually_ keeping
> the shared IRQ asserted is probed, the IRQ is not going to be
> acknowledged, thus it will keep being fired preventing the boot process
> of the kernel to continue. This is even worse when the second PHY driver
> is a module.
> 
> To fix this, implement the .shutdown() callback and disable the
> interrupts if these are used.

I don't know how this should interact with wake-on-lan, but I would
expect that there is a way to fix this without reintroducing the problem
fixed by this change. However I cannot say if this needs fixing in the
generic phy code or the phy driver. Any hints?
 
> Note that we are still susceptible to IRQ storms if the previous kernel
> exited with a panic or if the bootloader left the shared IRQ active, but
> there is absolutely nothing we can do about these cases.

I'd say the bootloader could handle that, knowing that for some machines
changing the bootloader isn't an option.

Best regards
Uwe

Florian Fainelli Aug. 3, 2023, 6:24 p.m. UTC | #2

On 8/3/23 11:16, Uwe Kleine-König wrote:
> Hello,
> 
> this patch became commit e2f016cf775129c050d6c79483073423db15c79a and is
> contained in v5.11-rc1.
> 
> It broke wake-on-lan on my NAS (an ARM machine with an Armada 370 SoC,
> armada-370-netgear-rn104.dts). The used phy driver is marvell.c. I only
> report it now as I just upgraded that machine from Debian 11 (with
> kernel 5.10.x) to Debian 12 (with kernel 6.1.x).
> 
> Commenting out phy_disable_interrupts(...) in v6.1.41's phy_shutdown()
> fixes the problem for me.
> 
> On Sun, Nov 01, 2020 at 02:50:57PM +0200, Ioana Ciornei wrote:
>> In case of a board which uses a shared IRQ we can easily end up with an
>> IRQ storm after a forced reboot.
>>
>> For example, a 'reboot -f' will trigger a call to the .shutdown()
>> callbacks of all devices. Because phylib does not implement that hook,
>> the PHY is not quiesced, thus it can very well leave its IRQ enabled.
>>
>> At the next boot, if that IRQ line is found asserted by the first PHY
>> driver that uses it, but _before_ the driver that is _actually_ keeping
>> the shared IRQ asserted is probed, the IRQ is not going to be
>> acknowledged, thus it will keep being fired preventing the boot process
>> of the kernel to continue. This is even worse when the second PHY driver
>> is a module.
>>
>> To fix this, implement the .shutdown() callback and disable the
>> interrupts if these are used.
> 
> I don't know how this should interact with wake-on-lan, but I would
> expect that there is a way to fix this without reintroducing the problem
> fixed by this change. However I cannot say if this needs fixing in the
> generic phy code or the phy driver. Any hints?

It depends upon what the PHY drivers and underlying hardware are capable 
and willing to do. Some PHY drivers will shutdown the TX path completely 
since you do not need that part to receive Wake-on-LAN packets and pass 
them up to the PHY and/or MAC Wake-on-LAN matching logic. This would 
invite us to let individual PHY drivers make a decision as to what they 
want to do in a .shutdown() routine that would then need to be added to 
each and every driver that wants to do something special. In the absence 
of said routine, you could default to calling phy_disable_interrupts() 
unless the PHY has WoL enabled?

phydev::wol_enabled reflects whether the PHY and/or the MAC has 
Wake-on-LAN enabled which could you could key off to "nullify" what the 
shutdown does.

>   
>> Note that we are still susceptible to IRQ storms if the previous kernel
>> exited with a panic or if the bootloader left the shared IRQ active, but
>> there is absolutely nothing we can do about these cases.
> 
> I'd say the bootloader could handle that, knowing that for some machines
> changing the bootloader isn't an option.

There is also the case of the boot loader not touching any 
PHY/MAC/networking and just booting as fast as possible to the kernel. 
It really becomes a problem that can be distributed against multiple SW 
agents to solve it, though clearly the kernel could do it too, so why 
not keep it there I guess.

Russell King (Oracle) Aug. 3, 2023, 6:49 p.m. UTC | #3

On Thu, Aug 03, 2023 at 11:24:04AM -0700, Florian Fainelli wrote:
> On 8/3/23 11:16, Uwe Kleine-König wrote:
> > Hello,
> > 
> > this patch became commit e2f016cf775129c050d6c79483073423db15c79a and is
> > contained in v5.11-rc1.
> > 
> > It broke wake-on-lan on my NAS (an ARM machine with an Armada 370 SoC,
> > armada-370-netgear-rn104.dts). The used phy driver is marvell.c. I only
> > report it now as I just upgraded that machine from Debian 11 (with
> > kernel 5.10.x) to Debian 12 (with kernel 6.1.x).
> > 
> > Commenting out phy_disable_interrupts(...) in v6.1.41's phy_shutdown()
> > fixes the problem for me.
> > 
> > On Sun, Nov 01, 2020 at 02:50:57PM +0200, Ioana Ciornei wrote:
> > > In case of a board which uses a shared IRQ we can easily end up with an
> > > IRQ storm after a forced reboot.
> > > 
> > > For example, a 'reboot -f' will trigger a call to the .shutdown()
> > > callbacks of all devices. Because phylib does not implement that hook,
> > > the PHY is not quiesced, thus it can very well leave its IRQ enabled.
> > > 
> > > At the next boot, if that IRQ line is found asserted by the first PHY
> > > driver that uses it, but _before_ the driver that is _actually_ keeping
> > > the shared IRQ asserted is probed, the IRQ is not going to be
> > > acknowledged, thus it will keep being fired preventing the boot process
> > > of the kernel to continue. This is even worse when the second PHY driver
> > > is a module.
> > > 
> > > To fix this, implement the .shutdown() callback and disable the
> > > interrupts if these are used.
> > 
> > I don't know how this should interact with wake-on-lan, but I would
> > expect that there is a way to fix this without reintroducing the problem
> > fixed by this change. However I cannot say if this needs fixing in the
> > generic phy code or the phy driver. Any hints?
> 
> It depends upon what the PHY drivers and underlying hardware are capable and
> willing to do. Some PHY drivers will shutdown the TX path completely since
> you do not need that part to receive Wake-on-LAN packets and pass them up to
> the PHY and/or MAC Wake-on-LAN matching logic. This would invite us to let
> individual PHY drivers make a decision as to what they want to do in a
> .shutdown() routine that would then need to be added to each and every
> driver that wants to do something special. In the absence of said routine,
> you could default to calling phy_disable_interrupts() unless the PHY has WoL
> enabled?
> 
> phydev::wol_enabled reflects whether the PHY and/or the MAC has Wake-on-LAN
> enabled which could you could key off to "nullify" what the shutdown does.

If the shutdown method is being called, doesn't that imply that we're
going to be hibernating rather than suspend-to-RAM - because suspend-to
RAM doesn't involve powering down the system, whereas hibernating does.

I think the problem is that when placing the system into hibernate mode
with WoL enabled, there are PHYs which use their interrupt pin to signal
that a WoL event has happened. The problem comes with the aforementioned
commit that the core PHY code now _always_ disables interrupts on the
PHY, even if WoL is active and we're entering hibernate mode.

That's clearly the wrong thing to be doing when people expect WoL to
be able to wake their systems up from hibernate.

The more I think about it, the more I'm coming to the conclusion that
the blamed commit is wrong as it is coded today.

It also occurs to me that systems need to cope with the PHY's INT signal
being asserted through boot, because in the case of WoL-triggered resume
from hibernate mode, the PHY's INT signal will be asserted _while_ the
system is booting up (and remember, recovery from hibernate mode is
essentially no different from a normal system boot with the exception
that the kernel notices a special signature in the swap partition, and
then reloads state from there.)

So, boot has to cope properly with the PHY's interrupt being asserted
in order for WoL from hibernate to work.

It seems to me that a revert of the blamed commit is in order, and a
different approach is needed?

Patch

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5dab6be6fc38..413a0a2c5d51 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2947,6 +2947,13 @@  static int phy_remove(struct device *dev)
 	return 0;
 }
 
+static void phy_shutdown(struct device *dev)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+
+	phy_disable_interrupts(phydev);
+}
+
 /**
  * phy_driver_register - register a phy_driver with the PHY layer
  * @new_driver: new phy_driver to register
@@ -2970,6 +2977,7 @@  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;