Re: [PATCH net-next v18 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 3 potential issue(s) to consider:
- [High] net: ax88179a_reset() clobbers user-configured MAC address during resume
- [Medium] net: ax88179a_resume() skips phylink_resume() on hardware read error
- [Medium] ax88179_178a: ignores usbnet_suspend() return value in suspend
--

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

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

The suspend and resume functions are called via the usb_driver structure,
for which the driver has only a single instance. Add wrapper functions
for the different implementations for the AX88179 and AX179A architecutres
in ax88179_lib which calls the 2 implementations in ax88179_178a and
ax88179a_devices, respectively.

> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 071b03b4f145c..ef0856627d08b 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
[ ... ]
> @@ -1356,9 +1359,9 @@ static struct usb_driver ax88179_178a_driver = {
>  	.name =		"ax88179_178a",
>  	.id_table =	products,
>  	.probe =	usbnet_probe,
> -	.suspend =	ax88179_suspend,
> -	.resume =	ax88179_resume,
> -	.reset_resume =	ax88179_resume,
> +	.suspend =	ax88179_suspend_wrapper,

[Severity: Medium]
If usbnet_suspend() aborts an autosuspend (e.g. because of an active TX
queue, returning -EBUSY), ax88179_suspend() ignores the error and continues
to force the MAC and PHY into low-power mode (AX_MEDIUM_RECEIVE_EN disabled,
AX_RX_CTL_STOP, etc.).

This stalls the active network stack because the hardware is stopped but the
USB core reports success to the PM core. The patch refactors this into
ax88179_suspend_wrapper without fixing the underlying bug.

Does this need to be fixed to properly handle usbnet_suspend() return values
in ax88179_suspend() to avoid hardware stalling?

> +	.resume =	ax88179_resume_wrapper,
> +	.reset_resume =	ax88179_resume_wrapper,
>  	.disconnect =	ax88179_disconnect,
>  	.supports_autosuspend = 1,
>  	.disable_hub_initiated_lpm = 1,
[ ... ]
> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index e86cbe3e4400f..e9913a7c70390 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c
[ ... ]
> @@ -135,6 +194,47 @@ static int ax88179a_auto_detach(struct usbnet *dev)
>  	return 0;
>  }
>  
> +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)
> +		goto err_out;

[Severity: Medium]
In ax88179a_resume(), if reading the PHY power state fails (ret < 0),
execution jumps directly to the err_out label.

This bypasses the if (netif_running(dev->net)) { phylink_resume(...) } block.

Because phylink_suspend() was previously called during ax88179a_suspend(),
bypassing the resume call leaves the phylink state machine permanently
suspended. Even if the hardware error was transient, link state updates will
no longer be processed, resulting in a dead network interface that cannot
recover.

Can this be modified so phylink_resume() runs even on hardware error, or is
there another way to prevent the state from becoming stuck?

> +
> +	if (!(reg8 & AX_PHY_POWER)) {
> +		reg8 = AX_PHY_POWER;
> +		ax88179_write_cmd(dev, AX88179A_PHY_POWER, 0, 0, 1, &reg8);
> +		msleep(250);
> +	}
> +	ax88179_write_cmd(dev, AX_FW_MODE, AX_FW_MODE_179A, 0, 0, NULL);
> +
> +	/* Now, that AX_FW_MODE_179A is enabled, the PHY needs a power-cycle.
> +	 * PHY-power is re-enabled in ax88179a_reset()
> +	 */
> +	ax88179_write_u8(dev, AX88179A_PHY_POWER, 0, 0, 0);
> +	msleep(250);
> +
> +	ax88179a_reset(dev);

[Severity: High]
Does this clobber a user-configured MAC address?

During resume, ax88179a_resume() unconditionally calls ax88179a_reset().
ax88179a_reset() calls ax88179_get_mac_addr(dev), which reads the default MAC
address from the hardware EEPROM or DTB and writes it into dev->net->dev_addr.
It then copies this value into dev->net->perm_addr.

If the user had previously configured a custom MAC address, this custom
address is permanently overwritten by the hardware default on every
suspend/resume cycle. Furthermore, modifying perm_addr outside of device
probe violates the invariant that it holds the permanent unalterable
hardware address.

Should the resume path instead restore the existing dev_addr to the device
registers without calling ax88179_get_mac_addr()?

> +
> +	if (netif_running(dev->net)) {
> +		rtnl_lock();
> +		phylink_resume(ax179_data->phylink);
> +		rtnl_unlock();
> +	}
> +
> +err_out:
> +	ax88179_set_pm_mode(dev, false);
> +
> +	return usbnet_resume(intf);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-ax88179a-v18-0-6fa1d4c40422@birger-koblitz.de?part=9