Re: [PATCH net-next v20 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 6 potential
issues to consider:
- [High] net: usb: ax88179_178a: missing MAC address restore in
  ax88179a_reset()
- [High] net: usb: ax88179_178a: PHY power-cycle in ax88179a_resume()
  breaks WoL resume
- [Medium] net: usb: ax88179_178a: ax88179a_resume() powers on PHY and
  MAC when netdev down
- [Medium] net: usb: ax88179_178a: unchecked register reads in
  ax88179a_suspend()
- [Medium] net: usb: ax88179_178a: dropped error code in
  ax88179a_resume()
- [Medium] net: usb: ax88179_178a: unaligned u16 access in
  ax88179a_reset()
--

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

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

This commit adds suspend and resume callbacks for AX88179A, AX88772D, and
AX88279 devices and dispatches USB power management operations through
per-device wrapper callbacks in ax88179_lib.

> 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)
> +{
> +	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 && netif_running(dev->net)) {
> +		rtnl_lock();
> +		phylink_resume(priv->phylink);
> +		rtnl_unlock();
> +		ax88179_set_pm_mode(dev, false);
> +		return ret;
> +	}
> +
> +	/* 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);
> +		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;
> +		ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, 2, 2, &tmp16);

[Severity: Medium]
What happens if either ax88179_read_cmd() call fails here?  On a read error,
tmp8 remains 0 and ax88179_read_cmd() zeroes tmp16, so the subsequent
ax88179_write_cmd() calls overwrite AX_MONITOR_MOD and AX_MEDIUM_STATUS_MODE
with zeroed base values and clear the active medium configuration bits.

[ ... ]
> @@ -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;
> +
> +	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: Medium]
Does ax88179a_reset() perform unaligned u16 accesses through tmp16?

drivers/net/usb/ax88179a_devices.c:ax88179a_reset() {
	...
	u16 *tmp16;
	u8 buf[5];
	u16 rxctl;
	u8 *tmp;

	tmp16 = (u16 *)buf;
	...
	*tmp16 = AX_MEDIUM_RECEIVE_EN | AX_MEDIUM_TXFLOW_CTRLEN |
		 AX_MEDIUM_RXFLOW_CTRLEN | AX_MEDIUM_FULL_DUPLEX;
	if (!ax179_data->is_ax88772d)
		*tmp16 |= AX_MEDIUM_GIGAMODE;
	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, 2, 2, tmp16);
	...
}

buf is a 1-byte-aligned u8 array on the stack, so dereferencing tmp16 in
ax88179a_reset() and inside ax88179_write_cmd() can fault on architectures
that trap on unaligned accesses.  Using separate u8 and u16 stack variables
(or ax88179_write_u16()) avoids aliasing buf.

> +
> +err_out:
> +	if (netif_running(dev->net)) {
> +		rtnl_lock();
> +		phylink_resume(ax179_data->phylink);
> +		rtnl_unlock();
> +	}

[Severity: Medium]
When the interface is administratively down (!netif_running(dev->net)), does
ax88179a_resume() leave the PHY powered on and the MAC receiver enabled?

When the interface is down, ax88179a_init_phy() or ax88179a_stop() leaves
the PHY suspended/powered off and AX_MEDIUM_RECEIVE_EN and AX_RX_CTL
cleared.  Calling ax88179a_reset() unconditionally during resume sets
AX_PHY_POWER, starts AX_RX_CTL, and enables AX_MEDIUM_RECEIVE_EN, so the
hardware stays powered on and brings up the physical link while the netdev
is down.

> +
> +	ax88179_set_pm_mode(dev, false);
> +
> +	return usbnet_resume(intf);
> +}

[Severity: Medium]
Does jumping to err_out when reading AX88179A_PHY_POWER fails drop the error
code in ret?  err_out skips hardware initialization and ax88179a_reset(),
but returns usbnet_resume(intf) (which returns 0), reporting a successful
resume to the USB core while the device remains uninitialized.

[ ... ]
> @@ -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]
When ax179_data->in_pm is true, does skipping ax88179_get_mac_addr() leave
the hardware AX_NODE_ID register unprogrammed after resume or reset_resume?

ax88179_get_mac_addr() is also what writes dev->net->dev_addr to AX_NODE_ID:

drivers/net/usb/ax88179_lib.c:ax88179_get_mac_addr() {
	...
	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN, ETH_ALEN,
			  dev->net->dev_addr);
}

If the chip loses power during suspend or goes through reset_resume, any MAC
address set via ax88179_set_mac_addr(), read from Device Tree, or generated
randomly is lost in AX_NODE_ID, causing the hardware unicast filter
(AX_RX_CTL_AP) to drop incoming unicast frames.  Should an else branch write
dev->net->dev_addr to AX_NODE_ID when ax179_data->in_pm is true?

[ ... ]
> @@ -1117,7 +1232,11 @@ static int ax88179a_reset(struct usbnet *dev)
>  	/* Ensure EEE state is off, consistent with phylink setup */
>  	ax88179_write_cmd(dev, AX_GPHY_CTL, AX_GPHY_EEE_CTRL, false, 0, NULL);
>  
> -	phylink_start(ax179_data->phylink);
> +	/* ax88179a_reset() may also be called from resume context,
> +	 * phylink_resume() will start phylink, then.
> +	 */
> +	if (!ax179_data->in_pm)
> +		phylink_start(ax179_data->phylink);

[Severity: High]
When Wake-on-LAN is enabled, does phylink_resume() actually call
phylink_start() after ax88179a_resume() power-cycles the PHY?

In ax88179a_suspend(), calling phylink_suspend(priv->phylink,
!!priv->wolopts) with WoL enabled sets PHYLINK_DISABLE_MAC_WOL and does not
call phylink_stop() or phy_stop().  On resume, ax88179a_resume()
unconditionally power-cycles the PHY (wiping volatile PHY registers and
dropping the physical link), but phylink_resume() takes the
PHYLINK_DISABLE_MAC_WOL branch instead of calling phylink_start():

drivers/net/phy/phylink.c:phylink_resume() {
	...
	if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
		...
		phylink_mac_initial_config(pl, true);

		/* Re-enable and re-resolve the link parameters */
		phylink_enable_and_run_resolve(pl, PHYLINK_DISABLE_MAC_WOL);
	} else {
		phylink_start(pl);
	}
}

Because phy_start() is not called on that path, phylib does not reprogram
advertised link modes or EEE settings into the freshly reset PHY, and
phylink_resolve() immediately restores carrier using the stale pre-suspend
phy_state.link while the PHY link is down.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-ax88179a-v20-0-37b5a1189983@birger-koblitz.de?part=9