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

[Birger Koblitz <mail@birger-koblitz.de>]

On 07/10/2026 5:05 am, sashiko-bot@kernel.org wrote:
> 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.
> 
Will fix by also checking for netif_running(dev->net) before
resuming phylink.

> [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.
> 
Will fix by also resetting in_pm to false.

>> +		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);
> 
Will fix by also calling usbnet_resume() on error.

> [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:
> 
Will fix by adding pm_runtime_put_noidle() to unbind().

Birger