Re: [PATCH net-next v19 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] net: Missing MAC address hardware restore during device resume
- [Medium] net: Unchecked read from hardware registers clobbers state on suspend
- [Medium] net: usb: ax88179_178a: dropped error code in ax88179a_resume()
- [Medium] net: ax88179_178a: unaligned memory access in ax88179a_reset()
--

commit 899b3b4428f2b801666bb023701af9a492e4e1b0
Author: Birger Koblitz <mail@birger-koblitz.de>
Subject: 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/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index a6b0a3f47118c..01d6a35f68b2c 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c
[ ... ]
> @@ -126,6 +128,63 @@ 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)
> +{
[ ... ]
> +	/* Enable WoL */
> +	if (priv->wolopts) {
> +		ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MONITOR_MOD, 1, 1, &tmp8);
> +		tmp8 &= ~(AX_MONITOR_MODE_RWLC | AX_MONITOR_MODE_RWMP);

[Severity: Medium]
What happens if ax88179_read_cmd() fails due to a transient USB error?

It appears tmp8 could be zeroed out by the failure, which would cause the
subsequent bitwise operations and ax88179_write_cmd() to clear all other
configured bits in the AX_MONITOR_MOD register, potentially corrupting the
device state.

> +		if (priv->wolopts & WAKE_PHY)
> +			tmp8 |= AX_MONITOR_MODE_RWLC;
> +		if (priv->wolopts & WAKE_MAGIC)
> +			tmp8 |= AX_MONITOR_MODE_RWMP;
> +
> +		ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MONITOR_MOD, 1, 1, &tmp8);
> +
> +		ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, 2, 2, &tmp16);
> +		tmp16 |= AX_MEDIUM_RECEIVE_EN;

[Severity: Medium]
Similarly, could an unchecked failure of ax88179_read_cmd() here result in
tmp16 being written back as just AX_MEDIUM_RECEIVE_EN, unintentionally
clearing other bits in AX_MEDIUM_STATUS_MODE?

> +		ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, 2, 2, &tmp16);
> +
[ ... ]
> +static int ax88179a_resume(struct usb_interface *intf)
> +{
[ ... ]
> +	ax88179_write_u8(dev, AX88179A_PHY_POWER, 0, 0, 0);
> +	msleep(250);
> +
> +	ax88179a_reset(dev);

[Severity: Medium]
Could this new call to ax88179a_reset() during resume trigger unaligned
memory accesses?

Looking at ax88179a_reset(), the driver casts a 1-byte aligned u8 array to a
u16 pointer:

	u8 buf[5];
	tmp16 = (u16 *)buf;
	*tmp16 = AX_MEDIUM_RECEIVE_EN | ...

Is there a risk this could cause alignment faults on architectures that do
not support unaligned accesses natively?

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

[Severity: Medium]
If the hardware initialization fails (for example, if ax88179_read_cmd fails
and jumps to err_out), the error code in ret seems to be discarded here
because this function always returns the result of usbnet_resume(intf), which
is 0.

Should we return ret to properly inform the PM core of the failure?

> +}
[ ... ]
> @@ -1055,9 +1166,13 @@ static int ax88179a_reset(struct usbnet *dev)
>  	ax88179_write_u8(dev, AX_ACCESS_MAC, AX88179A_MAC_SWP_CTRL, 1, 0);
>  	ax88179_write_u8(dev, AX_ACCESS_MAC, AX88179A_MAC_TX_HDR_CKSUM, 1, 0);
>  
> -	/* Read MAC address from DTB or asix chip */
> -	ax88179_get_mac_addr(dev);
> -	memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
> +	/* Read MAC address from DTB or asix chip, except when called
> +	 * from resume context.
> +	 */
> +	if (!ax179_data->in_pm) {
> +		ax88179_get_mac_addr(dev);
> +		memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);
> +	}

[Severity: High]
Does skipping ax88179_get_mac_addr() during resume prevent the hardware MAC
address from being restored?

After a reset_resume, hardware state is lost. If ax88179_get_mac_addr() is
skipped because in_pm is true, it seems the software dev_addr is never
programmed back into the hardware AX_NODE_ID registers, leaving the hardware
filter with its EEPROM default MAC.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010-ax88179a-v19-0-7475173cfb56@birger-koblitz.de?part=9