[PATCH net] net: phy: Don't trigger state machine while in suspend

[PATCH net] net: phy: Don't trigger state machine while in suspend

[Lukas Wunner]

Upon system sleep, mdio_bus_phy_suspend() stops the phy_state_machine(),
but subsequent interrupts may retrigger it:

They may have been left enabled to facilitate wakeup and are not
quiesced until the ->suspend_noirq() phase.  Unwanted interrupts may
hence occur between mdio_bus_phy_suspend() and dpm_suspend_noirq(),
as well as between dpm_resume_noirq() and mdio_bus_phy_resume().

Amend phy_interrupt() to avoid triggering the state machine if the PHY
is suspended.  Signal wakeup instead if the attached net_device or its
parent has been configured as a wakeup source.  (Those conditions are
identical to mdio_bus_phy_may_suspend().)  Postpone handling of the
interrupt until the PHY has resumed.

Before stopping the phy_state_machine() in mdio_bus_phy_suspend(),
wait for a concurrent phy_interrupt() to run to completion.  That is
necessary because phy_interrupt() may have checked the PHY's suspend
status before the system sleep transition commenced and it may thus
retrigger the state machine after it was stopped.

Likewise, after re-enabling interrupt handling in mdio_bus_phy_resume(),
wait for a concurrent phy_interrupt() to complete to ensure that
interrupts which it postponed are properly rerun.

Fixes: 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling")
Link: https://lore.kernel.org/netdev/a5315a8a-32c2-962f-f696-de9a26d30091@samsung.com/
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/net/phy/phy.c        | 23 +++++++++++++++++++++++
 drivers/net/phy/phy_device.c | 23 +++++++++++++++++++++++
 include/linux/phy.h          |  6 ++++++
 3 files changed, 52 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ef62f357b76d..8d3ee3a6495b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -31,6 +31,7 @@
 #include <linux/io.h>
 #include <linux/uaccess.h>
 #include <linux/atomic.h>
+#include <linux/suspend.h>
 #include <net/netlink.h>
 #include <net/genetlink.h>
 #include <net/sock.h>
@@ -976,6 +977,28 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	struct phy_driver *drv = phydev->drv;
 	irqreturn_t ret;
 
+	/* Wakeup interrupts may occur during a system sleep transition.
+	 * Postpone handling until the PHY has resumed.
+	 */
+	if (IS_ENABLED(CONFIG_PM_SLEEP) && phydev->irq_suspended) {
+		struct net_device *netdev = phydev->attached_dev;
+
+		if (netdev) {
+			struct device *parent = netdev->dev.parent;
+
+			if (netdev->wol_enabled)
+				pm_system_wakeup();
+			else if (device_may_wakeup(&netdev->dev))
+				pm_wakeup_dev_event(&netdev->dev, 0, true);
+			else if (parent && device_may_wakeup(parent))
+				pm_wakeup_dev_event(parent, 0, true);
+		}
+
+		phydev->irq_rerun = 1;
+		disable_irq_nosync(irq);
+		return IRQ_HANDLED;
+	}
+
 	mutex_lock(&phydev->lock);
 	ret = drv->handle_interrupt(phydev);
 	mutex_unlock(&phydev->lock);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 431a8719c635..46acddd865a7 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -278,6 +278,15 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
 	if (phydev->mac_managed_pm)
 		return 0;
 
+	/* Wakeup interrupts may occur during the system sleep transition when
+	 * the PHY is inaccessible. Set flag to postpone handling until the PHY
+	 * has resumed. Wait for concurrent interrupt handler to complete.
+	 */
+	if (phy_interrupt_is_valid(phydev)) {
+		phydev->irq_suspended = 1;
+		synchronize_irq(phydev->irq);
+	}
+
 	/* We must stop the state machine manually, otherwise it stops out of
 	 * control, possibly with the phydev->lock held. Upon resume, netdev
 	 * may call phy routines that try to grab the same lock, and that may
@@ -315,6 +324,20 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 	if (ret < 0)
 		return ret;
 no_resume:
+	if (phy_interrupt_is_valid(phydev)) {
+		phydev->irq_suspended = 0;
+		synchronize_irq(phydev->irq);
+
+		/* Rerun interrupts which were postponed by phy_interrupt()
+		 * because they occurred during the system sleep transition.
+		 */
+		if (phydev->irq_rerun) {
+			phydev->irq_rerun = 0;
+			enable_irq(phydev->irq);
+			irq_wake_thread(phydev->irq, phydev);
+		}
+	}
+
 	if (phydev->attached_dev && phydev->adjust_link)
 		phy_start_machine(phydev);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 508f1149665b..b09f7d36cff2 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -572,6 +572,10 @@ struct macsec_ops;
  * @mdix_ctrl: User setting of crossover
  * @pma_extable: Cached value of PMA/PMD Extended Abilities Register
  * @interrupts: Flag interrupts have been enabled
+ * @irq_suspended: Flag indicating PHY is suspended and therefore interrupt
+ *                 handling shall be postponed until PHY has resumed
+ * @irq_rerun: Flag indicating interrupts occurred while PHY was suspended,
+ *             requiring a rerun of the interrupt handler after resume
  * @interface: enum phy_interface_t value
  * @skb: Netlink message for cable diagnostics
  * @nest: Netlink nest used for cable diagnostics
@@ -626,6 +630,8 @@ struct phy_device {
 
 	/* Interrupts are enabled */
 	unsigned interrupts:1;
+	unsigned irq_suspended:1;
+	unsigned irq_rerun:1;
 
 	enum phy_state state;
 
-- 
2.35.2

Re: [PATCH net] net: phy: Don't trigger state machine while in suspend

[Marek Szyprowski]

Hi Lukas,

On 26.05.2022 11:28, Lukas Wunner wrote:
> Upon system sleep, mdio_bus_phy_suspend() stops the phy_state_machine(),
> but subsequent interrupts may retrigger it:
>
> They may have been left enabled to facilitate wakeup and are not
> quiesced until the ->suspend_noirq() phase.  Unwanted interrupts may
> hence occur between mdio_bus_phy_suspend() and dpm_suspend_noirq(),
> as well as between dpm_resume_noirq() and mdio_bus_phy_resume().
>
> Amend phy_interrupt() to avoid triggering the state machine if the PHY
> is suspended.  Signal wakeup instead if the attached net_device or its
> parent has been configured as a wakeup source.  (Those conditions are
> identical to mdio_bus_phy_may_suspend().)  Postpone handling of the
> interrupt until the PHY has resumed.
>
> Before stopping the phy_state_machine() in mdio_bus_phy_suspend(),
> wait for a concurrent phy_interrupt() to run to completion.  That is
> necessary because phy_interrupt() may have checked the PHY's suspend
> status before the system sleep transition commenced and it may thus
> retrigger the state machine after it was stopped.
>
> Likewise, after re-enabling interrupt handling in mdio_bus_phy_resume(),
> wait for a concurrent phy_interrupt() to complete to ensure that
> interrupts which it postponed are properly rerun.
>
> Fixes: 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling")

I'm not sure if this is a right commit here. It revealed the issue, but 
it is not directly related to the net/phy code.

> Link: https://lore.kernel.org/netdev/a5315a8a-32c2-962f-f696-de9a26d30091@samsung.com/
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
>   drivers/net/phy/phy.c        | 23 +++++++++++++++++++++++
>   drivers/net/phy/phy_device.c | 23 +++++++++++++++++++++++
>   include/linux/phy.h          |  6 ++++++
>   3 files changed, 52 insertions(+)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index ef62f357b76d..8d3ee3a6495b 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -31,6 +31,7 @@
>   #include <linux/io.h>
>   #include <linux/uaccess.h>
>   #include <linux/atomic.h>
> +#include <linux/suspend.h>
>   #include <net/netlink.h>
>   #include <net/genetlink.h>
>   #include <net/sock.h>
> @@ -976,6 +977,28 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>   	struct phy_driver *drv = phydev->drv;
>   	irqreturn_t ret;
>   
> +	/* Wakeup interrupts may occur during a system sleep transition.
> +	 * Postpone handling until the PHY has resumed.
> +	 */
> +	if (IS_ENABLED(CONFIG_PM_SLEEP) && phydev->irq_suspended) {
> +		struct net_device *netdev = phydev->attached_dev;
> +
> +		if (netdev) {
> +			struct device *parent = netdev->dev.parent;
> +
> +			if (netdev->wol_enabled)
> +				pm_system_wakeup();
> +			else if (device_may_wakeup(&netdev->dev))
> +				pm_wakeup_dev_event(&netdev->dev, 0, true);
> +			else if (parent && device_may_wakeup(parent))
> +				pm_wakeup_dev_event(parent, 0, true);
> +		}
> +
> +		phydev->irq_rerun = 1;
> +		disable_irq_nosync(irq);
> +		return IRQ_HANDLED;
> +	}
> +
>   	mutex_lock(&phydev->lock);
>   	ret = drv->handle_interrupt(phydev);
>   	mutex_unlock(&phydev->lock);
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 431a8719c635..46acddd865a7 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -278,6 +278,15 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
>   	if (phydev->mac_managed_pm)
>   		return 0;
>   
> +	/* Wakeup interrupts may occur during the system sleep transition when
> +	 * the PHY is inaccessible. Set flag to postpone handling until the PHY
> +	 * has resumed. Wait for concurrent interrupt handler to complete.
> +	 */
> +	if (phy_interrupt_is_valid(phydev)) {
> +		phydev->irq_suspended = 1;
> +		synchronize_irq(phydev->irq);
> +	}
> +
>   	/* We must stop the state machine manually, otherwise it stops out of
>   	 * control, possibly with the phydev->lock held. Upon resume, netdev
>   	 * may call phy routines that try to grab the same lock, and that may
> @@ -315,6 +324,20 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
>   	if (ret < 0)
>   		return ret;
>   no_resume:
> +	if (phy_interrupt_is_valid(phydev)) {
> +		phydev->irq_suspended = 0;
> +		synchronize_irq(phydev->irq);
> +
> +		/* Rerun interrupts which were postponed by phy_interrupt()
> +		 * because they occurred during the system sleep transition.
> +		 */
> +		if (phydev->irq_rerun) {
> +			phydev->irq_rerun = 0;
> +			enable_irq(phydev->irq);
> +			irq_wake_thread(phydev->irq, phydev);
> +		}
> +	}
> +
>   	if (phydev->attached_dev && phydev->adjust_link)
>   		phy_start_machine(phydev);
>   
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 508f1149665b..b09f7d36cff2 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -572,6 +572,10 @@ struct macsec_ops;
>    * @mdix_ctrl: User setting of crossover
>    * @pma_extable: Cached value of PMA/PMD Extended Abilities Register
>    * @interrupts: Flag interrupts have been enabled
> + * @irq_suspended: Flag indicating PHY is suspended and therefore interrupt
> + *                 handling shall be postponed until PHY has resumed
> + * @irq_rerun: Flag indicating interrupts occurred while PHY was suspended,
> + *             requiring a rerun of the interrupt handler after resume
>    * @interface: enum phy_interface_t value
>    * @skb: Netlink message for cable diagnostics
>    * @nest: Netlink nest used for cable diagnostics
> @@ -626,6 +630,8 @@ struct phy_device {
>   
>   	/* Interrupts are enabled */
>   	unsigned interrupts:1;
> +	unsigned irq_suspended:1;
> +	unsigned irq_rerun:1;
>   
>   	enum phy_state state;
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH net] net: phy: Don't trigger state machine while in suspend

[Andrew Lunn]

> +	if (phy_interrupt_is_valid(phydev)) {
> +		phydev->irq_suspended = 0;
> +		synchronize_irq(phydev->irq);
> +
> +		/* Rerun interrupts which were postponed by phy_interrupt()
> +		 * because they occurred during the system sleep transition.
> +		 */
> +		if (phydev->irq_rerun) {
> +			phydev->irq_rerun = 0;
> +			enable_irq(phydev->irq);
> +			irq_wake_thread(phydev->irq, phydev);
> +		}
> +	}

As i said in a previous thread, PHY interrupts are generally level,
not edge. So when you call enable_irq(phydev->irq), doesn't it
immediately fire? You need to first call the handler, and then
re-enable the interrupt.

      Andrew

Re: [PATCH net] net: phy: Don't trigger state machine while in suspend

[Lukas Wunner]

On Mon, Jun 06, 2022 at 03:40:49AM +0200, Andrew Lunn wrote:
> > +	if (phy_interrupt_is_valid(phydev)) {
> > +		phydev->irq_suspended = 0;
> > +		synchronize_irq(phydev->irq);
> > +
> > +		/* Rerun interrupts which were postponed by phy_interrupt()
> > +		 * because they occurred during the system sleep transition.
> > +		 */
> > +		if (phydev->irq_rerun) {
> > +			phydev->irq_rerun = 0;
> > +			enable_irq(phydev->irq);
> > +			irq_wake_thread(phydev->irq, phydev);
> > +		}
> > +	}
> 
> As i said in a previous thread, PHY interrupts are generally level,
> not edge. So when you call enable_irq(phydev->irq), doesn't it
> immediately fire?

Yes, if the interrupt is indeed level and the PHY is capable of
remembering that an interrupt occurred while the system was suspended
or was about to be suspended.

The irq_wake_thread() ensures that the IRQ handler is called,
should one of those conditions *not* be met.

Recall that phylib uses irq_default_primary_handler() as hardirq
handler.  That handler does nothing else but wake the IRQ thread,
which runs phy->handle_interrupt() in task context.

The irq_wake_thread() above likewise wakes the IRQ thread,
i.e. it tells the scheduler to put it on the run queue.

If, as you say, the interrupt is level and fires upon enable_irq(),
the result is that the scheduler is told twice to put the IRQ thread
on the run queue.  Usually this will happen faster than the IRQ thread
actually gets scheduled, so it will only run once.

In the unlikely event that the IRQ thread gets scheduled before the
call to irq_wake_thread(), the IRQ thread will run twice.
However, that's harmless.  IRQ handlers can cope with that.


> You need to first call the handler, and then re-enable the interrupt.

I guess I could call phy_interrupt() before the enable_irq(),
in lieu of irq_wake_thread().

However, it would mean that I'd invoke the IRQ handler behind the back
of the generic irq code.  That doesn't feel quite right.
Calling irq_wake_thread() is the correct way if one wants to be
compliant with the generic irq code's expectations.

If you feel strongly about it I can make that change but I would
advise against it.  Let me know what you think.

Thanks,

Lukas

Re: [PATCH net] net: phy: Don't trigger state machine while in suspend

[Andrew Lunn]

On Mon, Jun 06, 2022 at 07:53:20AM +0200, Lukas Wunner wrote:
> On Mon, Jun 06, 2022 at 03:40:49AM +0200, Andrew Lunn wrote:
> > > +	if (phy_interrupt_is_valid(phydev)) {
> > > +		phydev->irq_suspended = 0;
> > > +		synchronize_irq(phydev->irq);
> > > +
> > > +		/* Rerun interrupts which were postponed by phy_interrupt()
> > > +		 * because they occurred during the system sleep transition.
> > > +		 */
> > > +		if (phydev->irq_rerun) {
> > > +			phydev->irq_rerun = 0;
> > > +			enable_irq(phydev->irq);
> > > +			irq_wake_thread(phydev->irq, phydev);
> > > +		}
> > > +	}
> > 
> > As i said in a previous thread, PHY interrupts are generally level,
> > not edge. So when you call enable_irq(phydev->irq), doesn't it
> > immediately fire?
> 
> Yes, if the interrupt is indeed level and the PHY is capable of
> remembering that an interrupt occurred while the system was suspended
> or was about to be suspended.

It should remember, in the WoL case. It keeps it power etc.

> The irq_wake_thread() ensures that the IRQ handler is called,
> should one of those conditions *not* be met.
> 
> Recall that phylib uses irq_default_primary_handler() as hardirq
> handler.  That handler does nothing else but wake the IRQ thread,
> which runs phy->handle_interrupt() in task context.
> 
> The irq_wake_thread() above likewise wakes the IRQ thread,
> i.e. it tells the scheduler to put it on the run queue.
> 
> If, as you say, the interrupt is level and fires upon enable_irq(),
> the result is that the scheduler is told twice to put the IRQ thread
> on the run queue.  Usually this will happen faster than the IRQ thread
> actually gets scheduled, so it will only run once.
> 
> In the unlikely event that the IRQ thread gets scheduled before the
> call to irq_wake_thread(), the IRQ thread will run twice.
> However, that's harmless.  IRQ handlers can cope with that.

I'm just slightly worried about the IRQ handler returning there was
nothing to do. The IRQ core counts such interrupts, and will disable
the interrupt if nobody says it is actually handling the
interrupts. But it needs to be a few interrupts before this kicks in,
so it should be safe.

One other thought is we should probably get the IRQ Maintainers to
look this patch over. Please could you repost and Cc: them.

Thanks

   Andrew