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, };