Forbid USB runtime PM (autosuspend) for AX88772* in bind.
usbnet enables runtime PM by default in probe, so disabling it via the usb_driver flag is ineffective. For AX88772B, autosuspend shows no measurable power saving in my tests (no link partner, admin up/down). The ~0.453 W -> ~0.248 W reduction on 6.1 comes from phylib powering the PHY off on admin-down, not from USB autosuspend.
With autosuspend active, resume paths may require calling phylink/phylib (caller must hold RTNL) and doing MDIO I/O. Taking RTNL from a USB PM resume can deadlock (RTNL may already be held), and MDIO can attempt a runtime-wake while the USB PM lock is held. Given the lack of benefit and poor test coverage (autosuspend is usually disabled by default in distros), forbid runtime PM here to avoid these hazards.
This affects only AX88772* devices (per-interface in bind). System sleep/resume is unchanged.
Fixes: 4a2c7217cd5a ("net: usb: asix: ax88772: manage PHY PM from MAC") Reported-by: Hubert Wiśniewski hubert.wisniewski.25632@gmail.com Closes: https://lore.kernel.org/all/20220622141638.GE930160@montezuma.acc.umu.se Reported-by: Marek Szyprowski m.szyprowski@samsung.com Closes: https://lore.kernel.org/all/b5ea8296-f981-445d-a09a-2f389d7f6fdd@samsung.com Cc: stable@vger.kernel.org Signed-off-by: Oleksij Rempel o.rempel@pengutronix.de --- Link to the measurement results: https://lore.kernel.org/all/aMkPMa650kfKfmF4@pengutronix.de/ --- drivers/net/usb/asix_devices.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c index 792ddda1ad49..0d341d7e6154 100644 --- a/drivers/net/usb/asix_devices.c +++ b/drivers/net/usb/asix_devices.c @@ -625,6 +625,22 @@ static void ax88772_suspend(struct usbnet *dev) asix_read_medium_status(dev, 1)); }
+/* + * Notes on PM callbacks and locking context: + * + * - asix_suspend()/asix_resume() are invoked for both runtime PM and + * system-wide suspend/resume. For struct usb_driver the ->resume() + * callback does not receive pm_message_t, so the resume type cannot + * be distinguished here. + * + * - The MAC driver must hold RTNL when calling phylink interfaces such as + * phylink_suspend()/resume(). Those calls will also perform MDIO I/O. + * + * - Taking RTNL and doing MDIO from a runtime-PM resume callback (while + * the USB PM lock is held) is fragile. Since autosuspend brings no + * measurable power saving for this device with current driver version, it is + * disabled below. + */ static int asix_suspend(struct usb_interface *intf, pm_message_t message) { struct usbnet *dev = usb_get_intfdata(intf); @@ -919,6 +935,16 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf) if (ret) goto initphy_err;
+ /* Disable USB runtime PM (autosuspend) for this interface. + * Rationale: + * - No measurable power saving from autosuspend for this device. + * - phylink/phylib calls require caller-held RTNL and do MDIO I/O, + * which is unsafe from USB PM resume paths (possible RTNL already + * held, USB PM lock held). + * System suspend/resume is unaffected. + */ + pm_runtime_forbid(&intf->dev); + return 0;
initphy_err: @@ -948,6 +974,10 @@ static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf) phylink_destroy(priv->phylink); ax88772_mdio_unregister(priv); asix_rx_fixup_common_free(dev->driver_priv); + /* Re-allow runtime PM on disconnect for tidiness. The interface + * goes away anyway, but this balances forbid for debug sanity. + */ + pm_runtime_allow(&intf->dev); }
static void ax88178_unbind(struct usbnet *dev, struct usb_interface *intf) @@ -1600,6 +1630,10 @@ static struct usb_driver asix_driver = { .resume = asix_resume, .reset_resume = asix_resume, .disconnect = usbnet_disconnect, + /* usbnet will force supports_autosuspend=1; we explicitly forbid RPM + * per-interface in bind to keep autosuspend disabled for this driver + * by using pm_runtime_forbid(). + */ .supports_autosuspend = 1, .disable_hub_initiated_lpm = 1, };
Hi,
On 17.09.25 11:54, Oleksij Rempel wrote:
With autosuspend active, resume paths may require calling phylink/phylib (caller must hold RTNL) and doing MDIO I/O. Taking RTNL from a USB PM resume can deadlock (RTNL may already be held), and MDIO can attempt a runtime-wake while the USB PM lock is held. Given the lack of benefit and poor test coverage (autosuspend is usually disabled by default in distros), forbid runtime PM here to avoid these hazards.
This reasoning depends on netif_running() returning false during system resume. Is that guaranteed?
Regards Oliver
Hi Oliver,
On Wed, Sep 17, 2025 at 12:10:48PM +0200, Oliver Neukum wrote:
Hi,
On 17.09.25 11:54, Oleksij Rempel wrote:
With autosuspend active, resume paths may require calling phylink/phylib (caller must hold RTNL) and doing MDIO I/O. Taking RTNL from a USB PM resume can deadlock (RTNL may already be held), and MDIO can attempt a runtime-wake while the USB PM lock is held. Given the lack of benefit and poor test coverage (autosuspend is usually disabled by default in distros), forbid runtime PM here to avoid these hazards.
This reasoning depends on netif_running() returning false during system resume. Is that guaranteed?
You’re right - there is no guarantee that netif_running() is false during system resume. This change does not rely on that. If my wording suggested otherwise, I’ll reword the commit message to make it explicit.
1) Runtime PM (autosuspend/autoresume)
Typical chain when user does ip link set dev <if> up while autosuspended: rtnl_newlink (RTNL held) -> __dev_open -> usbnet_open -> usb_autopm_get_interface -> __pm_runtime_resume -> usb_resume_interface -> asix_resume
Here resume happens synchronously under RTNL (and with USB PM locking). If the driver then calls phylink/phylib from resume (caller must hold RTNL; MDIO I/O), we can deadlock or hit PM-lock vs MDIO wake issues.
Patch effect: I forbid runtime PM per-interface in ax88772_bind(). This removes the synchronous autoresume path.
2) System suspend/resume
Typical chain: ... dpm_run_callback (workqueue) -> usb_resume_interface -> asix_resume
This is not under RTNL, and no pm_runtime locking is involved. The patch does not change this path and makes no assumption about netif_running() here.
If helpful, I can rework the commit message.
Best Regards, Oleksij
On 17.09.25 13:52, Oleksij Rempel wrote:
Hi Oliver,
On Wed, Sep 17, 2025 at 12:10:48PM +0200, Oliver Neukum wrote:
Hi,
On 17.09.25 11:54, Oleksij Rempel wrote:
With autosuspend active, resume paths may require calling phylink/phylib (caller must hold RTNL) and doing MDIO I/O. Taking RTNL from a USB PM
This very strongly suggested that the conditional call is the issue.
resume can deadlock (RTNL may already be held), and MDIO can attempt a runtime-wake while the USB PM lock is held. Given the lack of benefit and poor test coverage (autosuspend is usually disabled by default in distros), forbid runtime PM here to avoid these hazards.
This reasoning depends on netif_running() returning false during system resume. Is that guaranteed?
You’re right - there is no guarantee that netif_running() is false during system resume. This change does not rely on that. If my wording suggested otherwise, I’ll reword the commit message to make it explicit.
- Runtime PM (autosuspend/autoresume)
Typical chain when user does ip link set dev <if> up while autosuspended: rtnl_newlink (RTNL held) -> __dev_open -> usbnet_open -> usb_autopm_get_interface -> __pm_runtime_resume -> usb_resume_interface -> asix_resume
Here resume happens synchronously under RTNL (and with USB PM locking). If the driver then calls phylink/phylib from resume (caller must hold RTNL; MDIO I/O), we can deadlock or hit PM-lock vs MDIO wake issues.
Patch effect: I forbid runtime PM per-interface in ax88772_bind(). This removes the synchronous autoresume path.
- System suspend/resume
Typical chain: ... dpm_run_callback (workqueue) -> usb_resume_interface -> asix_resume
This is not under RTNL, and no pm_runtime locking is involved. The patch does not change this path and makes no assumption about netif_running() here.
If helpful, I can rework the commit message.
It would maybe good to include a wording like:
With runtime PM, the driver is forced to resume its device while holding RTNL, if it happens to be suspended. The methods needed to resume the device take RTNL themselves. Thus runtime PM will deadlock.
Regards Oliver
On Wed Sep 17, 2025 at 11:54 AM CEST, Oleksij Rempel wrote:
Forbid USB runtime PM (autosuspend) for AX88772* in bind.
usbnet enables runtime PM by default in probe, so disabling it via the usb_driver flag is ineffective. For AX88772B, autosuspend shows no measurable power saving in my tests (no link partner, admin up/down). The ~0.453 W -> ~0.248 W reduction on 6.1 comes from phylib powering the PHY off on admin-down, not from USB autosuspend.
With autosuspend active, resume paths may require calling phylink/phylib (caller must hold RTNL) and doing MDIO I/O. Taking RTNL from a USB PM resume can deadlock (RTNL may already be held), and MDIO can attempt a runtime-wake while the USB PM lock is held. Given the lack of benefit and poor test coverage (autosuspend is usually disabled by default in distros), forbid runtime PM here to avoid these hazards.
This affects only AX88772* devices (per-interface in bind). System sleep/resume is unchanged.
Fixes: 4a2c7217cd5a ("net: usb: asix: ax88772: manage PHY PM from MAC") Reported-by: Hubert Wiśniewski hubert.wisniewski.25632@gmail.com Closes: https://lore.kernel.org/all/20220622141638.GE930160@montezuma.acc.umu.se Reported-by: Marek Szyprowski m.szyprowski@samsung.com Closes: https://lore.kernel.org/all/b5ea8296-f981-445d-a09a-2f389d7f6fdd@samsung.com Cc: stable@vger.kernel.org Signed-off-by: Oleksij Rempel o.rempel@pengutronix.de
Link to the measurement results: https://lore.kernel.org/all/aMkPMa650kfKfmF4@pengutronix.de/
drivers/net/usb/asix_devices.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c index 792ddda1ad49..0d341d7e6154 100644 --- a/drivers/net/usb/asix_devices.c +++ b/drivers/net/usb/asix_devices.c @@ -625,6 +625,22 @@ static void ax88772_suspend(struct usbnet *dev) asix_read_medium_status(dev, 1)); } +/*
- Notes on PM callbacks and locking context:
- asix_suspend()/asix_resume() are invoked for both runtime PM and
- system-wide suspend/resume. For struct usb_driver the ->resume()
- callback does not receive pm_message_t, so the resume type cannot
- be distinguished here.
- The MAC driver must hold RTNL when calling phylink interfaces such as
- phylink_suspend()/resume(). Those calls will also perform MDIO I/O.
- Taking RTNL and doing MDIO from a runtime-PM resume callback (while
- the USB PM lock is held) is fragile. Since autosuspend brings no
- measurable power saving for this device with current driver version, it is
- disabled below.
- */
static int asix_suspend(struct usb_interface *intf, pm_message_t message) { struct usbnet *dev = usb_get_intfdata(intf); @@ -919,6 +935,16 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf) if (ret) goto initphy_err;
- /* Disable USB runtime PM (autosuspend) for this interface.
* Rationale:* - No measurable power saving from autosuspend for this device.* - phylink/phylib calls require caller-held RTNL and do MDIO I/O,* which is unsafe from USB PM resume paths (possible RTNL already* held, USB PM lock held).* System suspend/resume is unaffected.*/- pm_runtime_forbid(&intf->dev);
- return 0;
initphy_err: @@ -948,6 +974,10 @@ static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf) phylink_destroy(priv->phylink); ax88772_mdio_unregister(priv); asix_rx_fixup_common_free(dev->driver_priv);
- /* Re-allow runtime PM on disconnect for tidiness. The interface
* goes away anyway, but this balances forbid for debug sanity.*/- pm_runtime_allow(&intf->dev);
} static void ax88178_unbind(struct usbnet *dev, struct usb_interface *intf) @@ -1600,6 +1630,10 @@ static struct usb_driver asix_driver = { .resume = asix_resume, .reset_resume = asix_resume, .disconnect = usbnet_disconnect,
- /* usbnet will force supports_autosuspend=1; we explicitly forbid RPM
* per-interface in bind to keep autosuspend disabled for this driver* by using pm_runtime_forbid().*/};
Well, this fixes the issue for me. No suspend/resume -- no deadlock -- no problem. Thanks.
Tested-by: Hubert Wiśniewski hubert.wisniewski.25632@gmail.com
On Wed, Sep 17, 2025 at 11:54:57AM +0200, Oleksij Rempel wrote:
Forbid USB runtime PM (autosuspend) for AX88772* in bind.
usbnet enables runtime PM by default in probe, so disabling it via the usb_driver flag is ineffective. For AX88772B, autosuspend shows no measurable power saving in my tests (no link partner, admin up/down). The ~0.453 W -> ~0.248 W reduction on 6.1 comes from phylib powering the PHY off on admin-down, not from USB autosuspend.
With autosuspend active, resume paths may require calling phylink/phylib (caller must hold RTNL) and doing MDIO I/O. Taking RTNL from a USB PM resume can deadlock (RTNL may already be held), and MDIO can attempt a runtime-wake while the USB PM lock is held. Given the lack of benefit and poor test coverage (autosuspend is usually disabled by default in distros), forbid runtime PM here to avoid these hazards.
This affects only AX88772* devices (per-interface in bind). System sleep/resume is unchanged.
@@ -919,6 +935,16 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf) if (ret) goto initphy_err;
- /* Disable USB runtime PM (autosuspend) for this interface.
* Rationale:* - No measurable power saving from autosuspend for this device.* - phylink/phylib calls require caller-held RTNL and do MDIO I/O,* which is unsafe from USB PM resume paths (possible RTNL already* held, USB PM lock held).* System suspend/resume is unaffected.*/- pm_runtime_forbid(&intf->dev);
Are you aware that the action of pm_runtime_forbid() can be reversed by the user (by writing "auto" to the .../power/control sysfs file)?
To prevent the user from re-enabling runtime PM, you should call pm_runtime_get_noresume() (and then of course pm_runtime_put() or equivalent while unbinding).
Alan Stern
On Wed Sep 17, 2025 at 3:54 PM CEST, Alan Stern wrote:
On Wed, Sep 17, 2025 at 11:54:57AM +0200, Oleksij Rempel wrote:
Forbid USB runtime PM (autosuspend) for AX88772* in bind.
usbnet enables runtime PM by default in probe, so disabling it via the usb_driver flag is ineffective. For AX88772B, autosuspend shows no measurable power saving in my tests (no link partner, admin up/down). The ~0.453 W -> ~0.248 W reduction on 6.1 comes from phylib powering the PHY off on admin-down, not from USB autosuspend.
With autosuspend active, resume paths may require calling phylink/phylib (caller must hold RTNL) and doing MDIO I/O. Taking RTNL from a USB PM resume can deadlock (RTNL may already be held), and MDIO can attempt a runtime-wake while the USB PM lock is held. Given the lack of benefit and poor test coverage (autosuspend is usually disabled by default in distros), forbid runtime PM here to avoid these hazards.
This affects only AX88772* devices (per-interface in bind). System sleep/resume is unchanged.
@@ -919,6 +935,16 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf) if (ret) goto initphy_err;
- /* Disable USB runtime PM (autosuspend) for this interface.
* Rationale:* - No measurable power saving from autosuspend for this device.* - phylink/phylib calls require caller-held RTNL and do MDIO I/O,* which is unsafe from USB PM resume paths (possible RTNL already* held, USB PM lock held).* System suspend/resume is unaffected.*/- pm_runtime_forbid(&intf->dev);
Are you aware that the action of pm_runtime_forbid() can be reversed by the user (by writing "auto" to the .../power/control sysfs file)?
I have tested this. With this patch, it seems that writing "auto" to power/control has no effect -- power/runtime_status remains "active" and the device does not get suspended. But maybe there is a way to force the suspension anyway?
To prevent the user from re-enabling runtime PM, you should call pm_runtime_get_noresume() (and then of course pm_runtime_put() or equivalent while unbinding).
Alan Stern
On Wed, Sep 17, 2025 at 04:31:40PM +0200, Hubert Wiśniewski wrote:
On Wed Sep 17, 2025 at 3:54 PM CEST, Alan Stern wrote:
Are you aware that the action of pm_runtime_forbid() can be reversed by the user (by writing "auto" to the .../power/control sysfs file)?
I have tested this. With this patch, it seems that writing "auto" to power/control has no effect -- power/runtime_status remains "active" and the device does not get suspended. But maybe there is a way to force the suspension anyway?
I don't know exactly what's going on in your particular case. However, if you read the source code for control_store() in drivers/base/power/sysfs.c, you'll see that writing "auto" to the attribute file causes the function to call pm_runtime_allow().
If you turn on CONFIG_PM_ADVANCED_DEBUG there will be extra files in the .../power/ directory, showing some of the other runtime PM values. Perhaps they will help you to figure out what's happening.
Alan Stern
On Wed, Sep 17, 2025 at 11:54:57AM +0200, Oleksij Rempel wrote:
Forbid USB runtime PM (autosuspend) for AX88772* in bind.
[...]
With autosuspend active, resume paths may require calling phylink/phylib (caller must hold RTNL) and doing MDIO I/O. Taking RTNL from a USB PM resume can deadlock (RTNL may already be held), and MDIO can attempt a runtime-wake while the USB PM lock is held.
FWIW, for smsc95xx.c, the MDIO deadlock issue was resolved by commit 7b960c967f2a ("usbnet: smsc95xx: Fix deadlock on runtime resume"). I would assume that something similar would be possible for asix_devices.c as well.
Thanks,
Lukas
linux-stable-mirror@lists.linaro.org
-
Alan Stern -
Hubert Wiśniewski -
Lukas Wunner -
Oleksij Rempel -
Oliver Neukum