mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net v2] net: phylink: allow stopping a suspended instance
@ 2026-10-05 19:09 James Hilliard
  2026-10-05 19:14 ` netdev-bot+sinfo
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: James Hilliard @ 2026-10-05 19:09 UTC (permalink / raw)
  To: netdev, Russell King, Andrew Lunn, Heiner Kallweit,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Russell King (Oracle)
  Cc: linux-kernel, James Hilliard

After a failed MAC resume, a driver must be able to stop phylink without
resuming it and reconfiguring the failed MAC. This is needed when recovery
leaves the interface administratively up but detached until a subsequent
down/up: ndo_stop() must finish the suspended phylink lifetime even though
the MAC could not be restored. The missing transition was identified by
code inspection of the stmmac recovery paths.

For MAC WoL, finish the deferred link-down and clear the WoL disable bit.
Otherwise, avoid repeating PHY, SFP and PCS shutdown, but suspend a PHY
that prepare_resume() powered up for the MAC reset clock.

Retain the saved link state if MAC WoL is suspended again after a failed
resume. For example, fbnic can fail to allocate IRQs on resume and call
phylink_suspend() again on the next system suspend. The carrier is already
off, so taking another snapshot loses the deferred mac_link_down(). Keep
the original snapshot until resume or stop completes, allowing the next
suspend cycle to capture the new link state.

Do not enter MAC WoL suspend on an already stopped instance. The wake
policy can change between a failed resume and the next suspend, for
example when fbnic learns a different BMC presence from firmware. Setting
MAC_WOL alongside STOPPED would leave stop/start gated by MAC_WOL, while
resume would take the MAC-only path without restarting the PHY, SFP or
PCS. Keep the stopped state so resume performs the required full start.

Restore any PHY advertisement reduced by suspend. Track that reduction
separately from explicit driver speed-down requests, so stopping a
suspended instance does not undo a driver's close-time power saving.

Fixes: f97493657c63 ("net: phylink: add suspend/resume support")
Fixes: 4c8925cb9db1 ("net: phylink: fix suspend/resume with WoL enabled and link down")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
Changes in v2:
- Keep stopped instances out of MAC-WoL suspend when the wake policy changes
  after a failed resume, so both resume and stop/start can recover.
- Explain the detached-interface recovery requirement for suspended stop.
- Link to v1: https://patch.msgid.link/20261001-submit-phylink-suspended-stop-v1-v1-1-0669fdb4a93b@gmail.com

To: Russell King <linux@armlinux.org.uk>
To: Andrew Lunn <andrew@lunn.ch>
To: Heiner Kallweit <hkallweit1@gmail.com>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Joakim Zhang <qiangqing.zhang@nxp.com>
To: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/phylink.c | 63 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 1bbcf46c8356..0a62ff1bca02 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -78,6 +78,7 @@ struct phylink {
 
 	bool link_failed;
 	bool suspend_link_up;
+	bool suspend_speed_down;
 	bool force_major_config;
 	bool major_config_failed;
 	bool mac_supports_eee_ops;
@@ -2498,6 +2499,14 @@ void phylink_start(struct phylink *pl)
 }
 EXPORT_SYMBOL_GPL(phylink_start);
 
+static void phylink_restore_suspend_speed(struct phylink *pl)
+{
+	if (pl->suspend_speed_down) {
+		phylink_speed_up(pl);
+		pl->suspend_speed_down = false;
+	}
+}
+
 /**
  * phylink_stop() - stop a phylink instance
  * @pl: a pointer to a &struct phylink returned from phylink_create()
@@ -2509,11 +2518,30 @@ EXPORT_SYMBOL_GPL(phylink_start);
  *
  * This will synchronously bring down the link if the link is not already
  * down (in other words, it will trigger a mac_link_down() method call.)
+ * A suspended instance may be stopped without first calling phylink_resume().
+ * In particular, closing a device after a failed resume must not restart the
+ * link or reconfigure the MAC just to finish shutting it down.
+ * Any PHY advertisement reduced by phylink_suspend() is restored as part
+ * of this transition.
+ * If phylink_prepare_resume() powered up an already stopped PHY, suspend
+ * it again when Wake-on-LAN permits.
  */
 void phylink_stop(struct phylink *pl)
 {
 	ASSERT_RTNL();
 
+	/* Also undo PHY speed control when terminating a suspended instance. */
+	phylink_restore_suspend_speed(pl);
+
+	if (test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state)) {
+		/* A failed MAC resume may have called phylink_prepare_resume()
+		 * and powered the stopped PHY back up to supply its RX clock.
+		 */
+		if (pl->phydev)
+			phy_suspend(pl->phydev);
+		return;
+	}
+
 	if (pl->sfp_bus)
 		sfp_upstream_stop(pl->sfp_bus);
 	if (pl->phydev)
@@ -2526,6 +2554,16 @@ void phylink_stop(struct phylink *pl)
 
 	phylink_run_resolve_and_disable(pl, PHYLINK_DISABLE_STOPPED);
 
+	if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
+		/* Finish the link-down deferred by MAC WoL, without restarting. */
+		flush_work(&pl->resolve);
+		mutex_lock(&pl->state_mutex);
+		if (pl->suspend_link_up)
+			phylink_link_down(pl);
+		__clear_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state);
+		mutex_unlock(&pl->state_mutex);
+	}
+
 	pl->pcs_state = PCS_STATE_DOWN;
 
 	phylink_pcs_disable(pl->pcs);
@@ -2631,14 +2669,22 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
 	if (phylink_mac_supports_wol(pl))
 		mac_wol = !!pl->wolopts_mac;
 
-	if (mac_wol && (!pl->netdev || pl->netdev->ethtool->wol_enabled)) {
+	/* A failed resume may leave the instance stopped even if the wake
+	 * policy now requests MAC WoL. There is no running link to preserve;
+	 * resume must still start the PHY, SFP and PCS in that case.
+	 */
+	if (!test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state) &&
+	    mac_wol && (!pl->netdev || pl->netdev->ethtool->wol_enabled)) {
 		/* Wake-on-Lan enabled, MAC handling */
 		mutex_lock(&pl->state_mutex);
 
+		/* Preserve the pending link-down if a previous resume failed. */
+		if (!test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state))
+			pl->suspend_link_up = phylink_link_is_up(pl);
+
 		/* Stop the resolver bringing the link up */
 		__set_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state);
 
-		pl->suspend_link_up = phylink_link_is_up(pl);
 		if (pl->suspend_link_up) {
 			/* Disable the carrier, to prevent transmit timeouts,
 			 * but one would hope all packets have been sent. This
@@ -2657,8 +2703,10 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
 		phylink_stop(pl);
 	}
 
-	if (phylink_phy_pm_speed_ctrl(pl))
+	if (phylink_phy_pm_speed_ctrl(pl)) {
 		phylink_speed_down(pl, false);
+		pl->suspend_speed_down = true;
+	}
 }
 EXPORT_SYMBOL_GPL(phylink_suspend);
 
@@ -2698,8 +2746,7 @@ void phylink_resume(struct phylink *pl)
 {
 	ASSERT_RTNL();
 
-	if (phylink_phy_pm_speed_ctrl(pl))
-		phylink_speed_up(pl);
+	phylink_restore_suspend_speed(pl);
 
 	if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
 		/* Wake-on-Lan enabled, MAC handling */
@@ -3616,6 +3663,12 @@ int phylink_speed_down(struct phylink *pl, bool sync)
 
 	ASSERT_RTNL();
 
+	/* An explicit request takes over from suspend-time speed control.
+	 * Restore the original advertisement before saving it again, so a
+	 * repeated speed-down cannot replace it with the reduced advertisement.
+	 */
+	phylink_restore_suspend_speed(pl);
+
 	if (!pl->sfp_bus && pl->phydev)
 		ret = phy_speed_down(pl->phydev, sync);
 

---
base-commit: aaaaf87ea99b8766c9a8aa0e71aa42e6bc8a5320
change-id: 20261001-submit-phylink-suspended-stop-v1-09f9b63e6f43

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>


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

* Re: [PATCH net v2] net: phylink: allow stopping a suspended instance
  2026-10-05 19:09 [PATCH net v2] net: phylink: allow stopping a suspended instance James Hilliard
@ 2026-10-05 19:14 ` netdev-bot+sinfo
  2026-10-06  4:42   ` James Hilliard
  2026-10-12 16:06 ` Paolo Abeni
  2026-10-13  1:22 ` Andrew Lunn
  2 siblings, 1 reply; 10+ messages in thread
From: netdev-bot+sinfo @ 2026-10-05 19:14 UTC (permalink / raw)
  To: James Hilliard
  Cc: netdev, Russell King, Andrew Lunn, Heiner Kallweit,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Russell King (Oracle),
	linux-kernel

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

* Re: [PATCH net v2] net: phylink: allow stopping a suspended instance
  2026-10-05 19:14 ` netdev-bot+sinfo
@ 2026-10-06  4:42   ` James Hilliard
  0 siblings, 0 replies; 10+ messages in thread
From: James Hilliard @ 2026-10-06  4:42 UTC (permalink / raw)
  To: netdev-bot+sinfo
  Cc: netdev, Russell King, Andrew Lunn, Heiner Kallweit,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Russell King (Oracle),
	linux-kernel

On Mon, Oct 5, 2026 at 1:14 PM <netdev-bot+sinfo@kernel.org> wrote:
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

Controlled failed-resume/down-up recovery was exercised on Allwinner
H616 hardware with sun8i EMAC1 and an AC300 PHY. This was
fault-injected, not a spontaneous failure.

> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

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

* Re: [PATCH net v2] net: phylink: allow stopping a suspended instance
  2026-10-05 19:09 [PATCH net v2] net: phylink: allow stopping a suspended instance James Hilliard
  2026-10-05 19:14 ` netdev-bot+sinfo
@ 2026-10-12 16:06 ` Paolo Abeni
  2026-10-13  1:22 ` Andrew Lunn
  2 siblings, 0 replies; 10+ messages in thread
From: Paolo Abeni @ 2026-10-12 16:06 UTC (permalink / raw)
  To: James Hilliard, netdev, Russell King, Andrew Lunn,
	Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Russell King (Oracle)
  Cc: linux-kernel

On 10/5/26 21:09, James Hilliard wrote:
> After a failed MAC resume, a driver must be able to stop phylink without
> resuming it and reconfiguring the failed MAC. This is needed when recovery
> leaves the interface administratively up but detached until a subsequent
> down/up: ndo_stop() must finish the suspended phylink lifetime even though
> the MAC could not be restored. The missing transition was identified by
> code inspection of the stmmac recovery paths.
> 
> For MAC WoL, finish the deferred link-down and clear the WoL disable bit.
> Otherwise, avoid repeating PHY, SFP and PCS shutdown, but suspend a PHY
> that prepare_resume() powered up for the MAC reset clock.
> 
> Retain the saved link state if MAC WoL is suspended again after a failed
> resume. For example, fbnic can fail to allocate IRQs on resume and call
> phylink_suspend() again on the next system suspend. The carrier is already
> off, so taking another snapshot loses the deferred mac_link_down(). Keep
> the original snapshot until resume or stop completes, allowing the next
> suspend cycle to capture the new link state.
> 
> Do not enter MAC WoL suspend on an already stopped instance. The wake
> policy can change between a failed resume and the next suspend, for
> example when fbnic learns a different BMC presence from firmware. Setting
> MAC_WOL alongside STOPPED would leave stop/start gated by MAC_WOL, while
> resume would take the MAC-only path without restarting the PHY, SFP or
> PCS. Keep the stopped state so resume performs the required full start.
> 
> Restore any PHY advertisement reduced by suspend. Track that reduction
> separately from explicit driver speed-down requests, so stopping a
> suspended instance does not undo a driver's close-time power saving.
> 
> Fixes: f97493657c63 ("net: phylink: add suspend/resume support")
> Fixes: 4c8925cb9db1 ("net: phylink: fix suspend/resume with WoL enabled and link down")
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
This needs an explicit ack from the phy crew. Also I understand this
is a pre-req for another fix; if so it would be probably better see
both at the same time/in the same series.

/P


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

* Re: [PATCH net v2] net: phylink: allow stopping a suspended instance
  2026-10-05 19:09 [PATCH net v2] net: phylink: allow stopping a suspended instance James Hilliard
  2026-10-05 19:14 ` netdev-bot+sinfo
  2026-10-12 16:06 ` Paolo Abeni
@ 2026-10-13  1:22 ` Andrew Lunn
  2026-10-13 19:46   ` James Hilliard
  2 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2026-10-13  1:22 UTC (permalink / raw)
  To: James Hilliard
  Cc: netdev, Russell King, Heiner Kallweit, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King (Oracle),
	linux-kernel

On Mon, Oct 05, 2026 at 01:09:05PM -0600, James Hilliard wrote:
> After a failed MAC resume, a driver must be able to stop phylink without
> resuming it and reconfiguring the failed MAC. This is needed when recovery
> leaves the interface administratively up but detached until a subsequent
> down/up: ndo_stop() must finish the suspended phylink lifetime even though
> the MAC could not be restored. The missing transition was identified by
> code inspection of the stmmac recovery paths.

I don't particularly like this. It feels like a workaround for a
broken MAC. It would be better to fix the MAC so that resume does not
fail.

As far as i know, this it theoretical, only triggered by fault
injection. If we find a real error situation where the MAC cannot be
resumed, then i will consider it. But until then, i prefer to reject
this patch.

     Andrew

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

* Re: [PATCH net v2] net: phylink: allow stopping a suspended instance
  2026-10-13  1:22 ` Andrew Lunn
@ 2026-10-13 19:46   ` James Hilliard
  2026-10-14 16:02     ` Andrew Lunn
  0 siblings, 1 reply; 10+ messages in thread
From: James Hilliard @ 2026-10-13 19:46 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Russell King, Heiner Kallweit, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King (Oracle),
	linux-kernel

On Mon, Oct 12, 2026 at 7:22 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Oct 05, 2026 at 01:09:05PM -0600, James Hilliard wrote:
> > After a failed MAC resume, a driver must be able to stop phylink without
> > resuming it and reconfiguring the failed MAC. This is needed when recovery
> > leaves the interface administratively up but detached until a subsequent
> > down/up: ndo_stop() must finish the suspended phylink lifetime even though
> > the MAC could not be restored. The missing transition was identified by
> > code inspection of the stmmac recovery paths.
>
> I don't particularly like this. It feels like a workaround for a
> broken MAC. It would be better to fix the MAC so that resume does not
> fail.

Fixing an identified cause of resume failure belongs in the MAC
driver. The intended purpose here is narrower: allow orderly shutdown
after the driver has returned an error, rather than recover the MAC
through phylink.

stmmac_resume() already returns before phylink_resume() when hardware
setup fails. The associated stmmac recovery work keeps the interface
administratively up but detached, allowing an ordinary down/up to
recover it.

That leaves a subsequent ndo_stop() needing to finish the suspended
phylink lifetime. Calling phylink_resume() just to make shutdown
possible would reapply MAC configuration and restart link resolution
before the driver has successfully restored the MAC. That is the
transition this prerequisite was intended to address.

> As far as i know, this it theoretical, only triggered by fault
> injection. If we find a real error situation where the MAC cannot be
> resumed, then i will consider it. But until then, i prefer to reject
> this patch.
>
>      Andrew

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

* Re: [PATCH net v2] net: phylink: allow stopping a suspended instance
  2026-10-13 19:46   ` James Hilliard
@ 2026-10-14 16:02     ` Andrew Lunn
  2026-10-14 17:07       ` James Hilliard
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2026-10-14 16:02 UTC (permalink / raw)
  To: James Hilliard
  Cc: netdev, Russell King, Heiner Kallweit, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King (Oracle),
	linux-kernel

On Tue, Oct 13, 2026 at 01:46:24PM -0600, James Hilliard wrote:
> On Mon, Oct 12, 2026 at 7:22 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Mon, Oct 05, 2026 at 01:09:05PM -0600, James Hilliard wrote:
> > > After a failed MAC resume, a driver must be able to stop phylink without
> > > resuming it and reconfiguring the failed MAC. This is needed when recovery
> > > leaves the interface administratively up but detached until a subsequent
> > > down/up: ndo_stop() must finish the suspended phylink lifetime even though
> > > the MAC could not be restored. The missing transition was identified by
> > > code inspection of the stmmac recovery paths.
> >
> > I don't particularly like this. It feels like a workaround for a
> > broken MAC. It would be better to fix the MAC so that resume does not
> > fail.
> 
> Fixing an identified cause of resume failure belongs in the MAC
> driver. The intended purpose here is narrower: allow orderly shutdown
> after the driver has returned an error, rather than recover the MAC
> through phylink.
> 
> stmmac_resume() already returns before phylink_resume() when hardware
> setup fails.

But why did the hardware setup fail?

This is what i mean about it being a workaround.

    Andrew


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

* Re: [PATCH net v2] net: phylink: allow stopping a suspended instance
  2026-10-14 16:02     ` Andrew Lunn
@ 2026-10-14 17:07       ` James Hilliard
  2026-10-14 17:13         ` Andrew Lunn
  0 siblings, 1 reply; 10+ messages in thread
From: James Hilliard @ 2026-10-14 17:07 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Russell King, Heiner Kallweit, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King (Oracle),
	linux-kernel

On Wed, Oct 14, 2026 at 10:02 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Oct 13, 2026 at 01:46:24PM -0600, James Hilliard wrote:
> > On Mon, Oct 12, 2026 at 7:22 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Mon, Oct 05, 2026 at 01:09:05PM -0600, James Hilliard wrote:
> > > > After a failed MAC resume, a driver must be able to stop phylink without
> > > > resuming it and reconfiguring the failed MAC. This is needed when recovery
> > > > leaves the interface administratively up but detached until a subsequent
> > > > down/up: ndo_stop() must finish the suspended phylink lifetime even though
> > > > the MAC could not be restored. The missing transition was identified by
> > > > code inspection of the stmmac recovery paths.
> > >
> > > I don't particularly like this. It feels like a workaround for a
> > > broken MAC. It would be better to fix the MAC so that resume does not
> > > fail.
> >
> > Fixing an identified cause of resume failure belongs in the MAC
> > driver. The intended purpose here is narrower: allow orderly shutdown
> > after the driver has returned an error, rather than recover the MAC
> > through phylink.
> >
> > stmmac_resume() already returns before phylink_resume() when hardware
> > setup fails.
>
> But why did the hardware setup fail?

The exercised failure was injected, rather than a diagnosed spontaneous
MAC fault. This change comes from auditing the existing error paths for
the deferred stmmac recovery fix.

> This is what i mean about it being a workaround.

The deferred fix addresses a separate software failure: suspend has
already disabled NAPI, but an unsuccessful resume leaves the interface
administratively up. Closing it can then disable NAPI again and hang.
The recovery changes preserve resource ownership so administrative down
can finish and a later open can retry initialization.

That shutdown also has to finish the suspended phylink lifetime.
Without MAC WoL, suspend has already stopped phylink, although
prepare_resume() may have powered the PHY back up. With MAC WoL,
link-down was deferred and the MAC_WOL disable bit remains set.
Suspend-owned advertisement changes also need restoring.

Those are phylink-owned states. Avoiding the second NAPI disable in
stmmac does not resolve them, and calling phylink_resume() just to
permit shutdown would reconfigure the MAC after its setup failed.

The required operation is therefore to terminate the suspended
instance without bringing it back into service. Cleanup must not
depend on first succeeding at the initialization that failed.

The original resume error remains an error, and an identified
MAC/PHY sequencing defect still needs its own fix. This transition
addresses the subsequent shutdown failure; it does not hide or
work around the cause of the unsuccessful resume.

>     Andrew
>

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

* Re: [PATCH net v2] net: phylink: allow stopping a suspended instance
  2026-10-14 17:07       ` James Hilliard
@ 2026-10-14 17:13         ` Andrew Lunn
  2026-10-14 17:31           ` James Hilliard
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2026-10-14 17:13 UTC (permalink / raw)
  To: James Hilliard
  Cc: netdev, Russell King, Heiner Kallweit, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King (Oracle),
	linux-kernel

On Wed, Oct 14, 2026 at 11:07:27AM -0600, James Hilliard wrote:
> On Wed, Oct 14, 2026 at 10:02 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Tue, Oct 13, 2026 at 01:46:24PM -0600, James Hilliard wrote:
> > > On Mon, Oct 12, 2026 at 7:22 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > On Mon, Oct 05, 2026 at 01:09:05PM -0600, James Hilliard wrote:
> > > > > After a failed MAC resume, a driver must be able to stop phylink without
> > > > > resuming it and reconfiguring the failed MAC. This is needed when recovery
> > > > > leaves the interface administratively up but detached until a subsequent
> > > > > down/up: ndo_stop() must finish the suspended phylink lifetime even though
> > > > > the MAC could not be restored. The missing transition was identified by
> > > > > code inspection of the stmmac recovery paths.
> > > >
> > > > I don't particularly like this. It feels like a workaround for a
> > > > broken MAC. It would be better to fix the MAC so that resume does not
> > > > fail.
> > >
> > > Fixing an identified cause of resume failure belongs in the MAC
> > > driver. The intended purpose here is narrower: allow orderly shutdown
> > > after the driver has returned an error, rather than recover the MAC
> > > through phylink.
> > >
> > > stmmac_resume() already returns before phylink_resume() when hardware
> > > setup fails.
> >
> > But why did the hardware setup fail?
> 
> The exercised failure was injected, rather than a diagnosed spontaneous
> MAC fault. This change comes from auditing the existing error paths for
> the deferred stmmac recovery fix.
> 
> > This is what i mean about it being a workaround.
> 
> The deferred fix addresses a separate software failure: suspend has
> already disabled NAPI, but an unsuccessful resume leaves the interface
> administratively up.

But why was the resume unsuccessful?

Sorry, but i've asked this multiple times. You never give a good
answer, so i'm just going to:

NACK.

	Andrew

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

* Re: [PATCH net v2] net: phylink: allow stopping a suspended instance
  2026-10-14 17:13         ` Andrew Lunn
@ 2026-10-14 17:31           ` James Hilliard
  0 siblings, 0 replies; 10+ messages in thread
From: James Hilliard @ 2026-10-14 17:31 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Russell King, Heiner Kallweit, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King (Oracle),
	linux-kernel

On Wed, Oct 14, 2026 at 11:13 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Oct 14, 2026 at 11:07:27AM -0600, James Hilliard wrote:
> > On Wed, Oct 14, 2026 at 10:02 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Tue, Oct 13, 2026 at 01:46:24PM -0600, James Hilliard wrote:
> > > > On Mon, Oct 12, 2026 at 7:22 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > > >
> > > > > On Mon, Oct 05, 2026 at 01:09:05PM -0600, James Hilliard wrote:
> > > > > > After a failed MAC resume, a driver must be able to stop phylink without
> > > > > > resuming it and reconfiguring the failed MAC. This is needed when recovery
> > > > > > leaves the interface administratively up but detached until a subsequent
> > > > > > down/up: ndo_stop() must finish the suspended phylink lifetime even though
> > > > > > the MAC could not be restored. The missing transition was identified by
> > > > > > code inspection of the stmmac recovery paths.
> > > > >
> > > > > I don't particularly like this. It feels like a workaround for a
> > > > > broken MAC. It would be better to fix the MAC so that resume does not
> > > > > fail.
> > > >
> > > > Fixing an identified cause of resume failure belongs in the MAC
> > > > driver. The intended purpose here is narrower: allow orderly shutdown
> > > > after the driver has returned an error, rather than recover the MAC
> > > > through phylink.
> > > >
> > > > stmmac_resume() already returns before phylink_resume() when hardware
> > > > setup fails.
> > >
> > > But why did the hardware setup fail?
> >
> > The exercised failure was injected, rather than a diagnosed spontaneous
> > MAC fault. This change comes from auditing the existing error paths for
> > the deferred stmmac recovery fix.
> >
> > > This is what i mean about it being a workaround.
> >
> > The deferred fix addresses a separate software failure: suspend has
> > already disabled NAPI, but an unsuccessful resume leaves the interface
> > administratively up.
>
> But why was the resume unsuccessful?

There is an independently reported case where resume cannot happen:
an ASIX USB Ethernet adapter unplugged while the system is suspended.

Xu Yang reported this on an i.MX93 EVK, with a warning and call trace
showing phy_stop() called from PHY_HALTED through phylink_stop(),
ax88772_stop(), usbnet_stop() and usbnet_disconnect().[1]

The sequence is:

- Suspend calls phylink_suspend(false), which stops the PHY.
- The adapter is physically removed while suspended.
- USB core skips the disconnected interface's resume callback.
- Disconnect unregisters the netdev, whose stop callback calls
phylink_stop() on the still-suspended instance.

That call sequence remains in the current code. Successful MAC resume
is not an option once the device has been removed.

There is also an existing resume failure path in UCC GETH. On
CONFIG_PPC_85xx, resume rebuilds the controller resources, including RX
buffers allocated using netdev_alloc_skb(). That helper uses GFP_ATOMIC,
so allocation can fail under memory pressure without a hardware fault.

The allocation failure propagates through ucc_geth_startup() to
ucc_geth_init_mac(), whose error cleanup calls ucc_geth_stop() and then
phylink_stop(). With non-MAC-WoL suspend, the PHY was already stopped.
This reaches the transition directly during resume error cleanup,
without the deferred stmmac changes. This second case is from code
inspection, not a hardware reproduction.

These are independent of the injected stmmac failure discussed earlier.
The USB case provides an existing hardware report of why shutdown may
need to proceed without a matching resume.

The earlier discussion favored handling that case in usbnet.[2] That
is a separate question about where to handle the transition, rather
than whether the transition occurs in practice.

These examples most directly support avoiding repeated shutdown of an
already-stopped instance.

[1] https://lore.kernel.org/r/20250806083017.3289300-1-xu.yang_2@nxp.com/
[2] https://lore.kernel.org/r/1c352c2f-c8b3-4cbe-9921-8ba5f0e4b433@lunn.ch/

> Sorry, but i've asked this multiple times. You never give a good
> answer, so i'm just going to:
>
> NACK.
>
>         Andrew

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

end of thread, other threads:[~2026-10-14 17:31 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-05 19:09 [PATCH net v2] net: phylink: allow stopping a suspended instance James Hilliard
2026-10-05 19:14 ` netdev-bot+sinfo
2026-10-06  4:42   ` James Hilliard
2026-10-12 16:06 ` Paolo Abeni
2026-10-13  1:22 ` Andrew Lunn
2026-10-13 19:46   ` James Hilliard
2026-10-14 16:02     ` Andrew Lunn
2026-10-14 17:07       ` James Hilliard
2026-10-14 17:13         ` Andrew Lunn
2026-10-14 17:31           ` James Hilliard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®