Re: [PATCH net v1 1/1] net: usb: asix: forbid runtime PM to avoid PM/MDIO + RTNL deadlock

["Hubert Wiśniewski" <hubert.wisniewski.25632@gmail.com>]

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().
> +	 */
>  	.supports_autosuspend = 1,
>  	.disable_hub_initiated_lpm = 1,
>  };

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>