Re: [PATCH net-next v17 09/13] ax88179_178a: Add Suspend/resume support for AX88179A/772D/279

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] pm: pm_runtime_get_noresume usage count leak on unbind
- [High] net: ax88179a: in_pm state leak prevents PHY recovery on suspend error
- [Medium] net: ax88179a: unbalanced phylink_resume on suspend error path
- [Medium] net: ax88179a: missing usbnet_resume on resume error path
--

commit a41c6d9493f6bfa6db95add83f5f79963fe546ba
Author: Birger Koblitz <mail@birger-koblitz.de>

ax88179_178a: Add Suspend/resume support for AX88179A/772D/279

This commit adds suspend and resume functionality for the AX88179A, AX88772D,
and AX88279 architectures. It implements wrapper functions in the shared
library to call the specific suspend and resume implementations.

> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index e592382692c74..aa5f2d6e42e76 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c
[ ... ]
> @@ -126,6 +128,62 @@ static void ax88179a_status(struct usbnet *dev, struct urb *urb)
>  	phylink_mac_interrupt(data->phylink);
>  }
>  
> +static int ax88179a_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> +	struct usbnet *dev = usb_get_intfdata(intf);
> +	struct ax88179_data *priv;
> +	u8 tmp8 = 0;
> +	u16 tmp16;
> +	int ret;
> +
> +	priv = dev->driver_priv;
> +	ax88179_set_pm_mode(dev, true);
> +
> +	if (netif_running(dev->net)) {
> +		rtnl_lock();
> +		phylink_suspend(priv->phylink, !!priv->wolopts);
> +		rtnl_unlock();
> +	}
> +
> +	ret = usbnet_suspend(intf, message);
> +	if (ret) {
> +		rtnl_lock();
> +		phylink_resume(priv->phylink);

[Severity: Medium]
Could this unconditionally start the phylink state machine on an interface
that is administratively down?

In ax88179a_suspend(), phylink_suspend() is only called if netif_running()
is true, but if usbnet_suspend() subsequently fails, the error recovery path
here calls phylink_resume() regardless.

[Severity: High]
Does this error path leak the in_pm state?

During ax88179a_suspend(), in_pm is set to true. If usbnet_suspend() returns
an error, the function returns early without resetting in_pm to false via
ax88179_set_pm_mode(). If an autosuspend or system suspend is aborted due to
this error, subsequent calls to ax88179a_reset() will permanently skip
phylink_start() because in_pm is incorrectly left as true.

> +		rtnl_unlock();
> +		return ret;
> +	}
> +
> +	/* Enable WoL */
[ ... ]
> +static int ax88179a_resume(struct usb_interface *intf)
> +{
> +	struct usbnet *dev = usb_get_intfdata(intf);
> +	struct ax88179_data *ax179_data;
> +	u8 reg8 = 0;
> +	int ret;
> +
> +	ax179_data = dev->driver_priv;
> +	ax88179_set_pm_mode(dev, true);
> +
> +	ret = ax88179_read_cmd(dev, AX88179A_PHY_POWER, 0, 0, 1, &reg8);
> +	if (ret < 0) {
> +		ax88179_set_pm_mode(dev, false);
> +		return ret;

[Severity: Medium]
If ax88179_read_cmd() fails, does returning early here leave the
dev->suspend_count permanently unbalanced?

The usbnet_suspend() function increments dev->suspend_count and stops the
queues. Because this error path bypasses usbnet_resume(), the count is never
decremented.

> +	}
> +	if (!(reg8 & AX_PHY_POWER)) {
[ ... ]
> @@ -784,6 +886,13 @@ static int ax88179a_bind(struct usbnet *dev, struct usb_interface *intf)
>  	if (ret)
>  		goto initphy_err;
>  
> +	/* Keep this interface runtime-PM active by taking a usage ref.
> +	 * Prevents runtime suspend while bound and avoids resume paths
> +	 * that could deadlock (autoresume under RTNL while USB PM lock
> +	 * is held, phylink/MDIO wants RTNL).
> +	 */
> +	pm_runtime_get_noresume(&intf->dev);

[Severity: High]
Does this leak the PM usage counter?

The usage reference is acquired here to prevent runtime autosuspend, but the
unbind path (ax88179a_unbind()) does not seem to drop this reference via
pm_runtime_put_noidle(). Rebinding the driver or initiating a software
unbind/bind cycle could repeatedly leak the PM reference, permanently
preventing the device and its parent USB hubs from entering runtime suspend.

> +
>  	return 0;
>  
>  initphy_err:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-ax88179a-v17-0-252814fe35ea@birger-koblitz.de?part=9