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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 5 · Low: 4

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In ax88179a_reset(), the new `if (!ax179_data->in_pm)` guard
  (ax88179a_devices.c:1172-1175) skips the whole ax88179_get_mac_addr()…
- [Medium] On resume, ax88179a_reset() re-enables the hardware VLAN
  filter (AX_VLAN_CONTROL_VFE written to AX88179A_VLAN_ID_CONTROL,…
- [Medium] The series is not bisectable.
- [Medium] ax88179a_resume() calls ax88179a_reset() whether or not the
  interface is running (ax88179a_devices.c:207-224).
- [Medium] On every resume, ax88179a_resume() power-cycles the PHY
  through AX88179A_PHY_POWER: it writes 0 at line 221, sleeps 250 ms,…
- [Low] The patch turns off USB runtime PM (autosuspend) for every
  AX88179A/AX88772D/AX88279 device and the commit message does not say…
- [Low] In ax88179a_resume(), if the first control read of
  AX88179A_PHY_POWER fails (lines 207-209), the code jumps to err_out.
- [Low] ax88179a_reset() always arms magic-packet wake (AX_MONITOR_MOD =
  AX_MONITOR_MODE_RWMP, lines 1213-1217).
- [Low] In the WoL branch of ax88179a_suspend() (lines 168-170), the
  return value of ax88179_read_cmd(dev, AX_ACCESS_MAC,…

--

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

This patch adds per-chip suspend/resume function pointers to struct
ax88179_data and sends the usb_driver suspend/resume/reset_resume
callbacks through new wrappers in ax88179_lib.c. It implements
ax88179a_suspend() and ax88179a_resume() for the AX179A family. When
called from resume, ax88179a_reset() now skips the MAC address read and
phylink_start().

> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index f78ad344c1c4f..b077b7153f0c0 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c

[ ... ]

> @@ -1362,9 +1365,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,
> +	.resume =	ax88179_resume_wrapper,
> +	.reset_resume =	ax88179_resume_wrapper,

[Severity: Medium]
Is the series bisectable at this point? From "ax88179_178a: Add HW support
for AX179A-based chips" through "ax88179_178a: Add AX179A/AX279 multicast
configuration", the AX179A-family products entries are bound to this
usb_driver. In those commits .suspend/.resume/.reset_resume still point
at the classic ax88179_suspend()/ax88179_resume().

In those intermediate commits, a system resume runs:

ax88179_resume()
  ax88179_reset()
    mii_nway_restart(&dev->mii)
      mii->mdio_read(...)

ax88179a_bind() never sets dev->mii.mdio_read. Wouldn't the first resume
of an AX88179A/772D/279 device then call a NULL function pointer?

This patch fixes that by sending AX179A devices to ax88179a_resume(), but
the commit message presents the patch only as a new feature. Could the
patches be reordered or merged so that no intermediate commit registers
these callbacks for the new chips? Could the changelog also mention the
fix?

>  	.disconnect =	ax88179_disconnect,
>  	.supports_autosuspend = 1,

[ ... ]

> 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);
> +		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: Low]
What happens if this AX_MEDIUM_STATUS_MODE read fails? For size 2,
ax88179_read_cmd() starts with u16 buf = 0 and copies buf into *data even
on error, so tmp16 ends up as just AX_MEDIUM_RECEIVE_EN.

Writing that value back clears AX_MEDIUM_GIGAMODE, AX_MEDIUM_FULL_DUPLEX,
the flow control enables and AX_MEDIUM_JUMBO_EN. Meanwhile the PHY keeps
its link up for WoL.

Could the MAC medium mode then disagree with the link, so that wake frames
are not received? Should the return value be checked here?

> +
> +		if (priv->chip_version == AX_VERSION_AX88279)
> +			ax88179_write_cmd(dev, AX88179A_WAKEUP_SETTING, 8,
> +					  EPHY_LOW_POWER_EN | S5_WOL_EN
> +					  | S5_WOL_LOW_POWER | 0x8000, 0, NULL);
> +		else
> +			ax88179_write_cmd(dev, AX88179A_WAKEUP_SETTING, 0,
> +					  EPHY_LOW_POWER_EN, 0, NULL);
> +
> +	} else if (priv->chip_version == AX_VERSION_AX88279) {
> +		ax88179_write_cmd(dev, AX88179A_WAKEUP_SETTING, 8, 0x8000, 0, NULL);
> +	}

[Severity: Low]
Is magic packet wake really turned off when WoL is disabled?
ax88179a_reset() always arms it:

	if (ax179_data->chip_version < AX_VERSION_AX88179A)
		*tmp = AX_MONITOR_MODE_PMETYPE | AX_MONITOR_MODE_PMEPOL | AX_MONITOR_MODE_RWMP;
	else
		*tmp = AX_MONITOR_MODE_RWMP;
	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MONITOR_MOD, 1, 1, tmp);

AX_MONITOR_MOD is only rewritten here, inside if (priv->wolopts). The
chip therefore suspends with RWMP still set, even when ethtool reports
WoL as off.

For AX88179A/AX88772D, a WoL suspend also writes AX88179A_WAKEUP_SETTING
with EPHY_LOW_POWER_EN. The else branch only covers AX88279, and nothing
on resume clears the register.

For a running interface, phy_suspend() powers the PHY down, which hides
this. The PHY can still have link during a WoL-off suspend, for example
in the down-interface resume case below. If USB wakeup is enabled in
sysfs in that case, could a magic packet wake the host?

> +
> +	ax88179_set_pm_mode(dev, false);
> +	return 0;
> +}

[ ... ]

> @@ -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: Low]
If this first read fails, the goto skips the AX_FW_MODE_179A write, the
PHY power cycle and all of ax88179a_reset(). err_out still calls
phylink_resume(), which in the non-WoL case starts phylink on hardware
that was never re-initialised. It then does:

	return usbnet_resume(intf);

usbnet_resume() always returns 0, so ret is lost. Would the interface then
look resumed but pass no traffic until it is brought down and up again?
Should the error be propagated here?

> +
> +	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);

[Severity: Medium]
Does this unconditional power cycle lose the PHY configuration when WoL is
enabled? The comment suggests it resets the PHY registers to their
defaults: BMCR, MII_ADVERTISE (including AX_ADVERTISE_2500 on AX88279) and
the EEE advertisement.

With wolopts set, ax88179a_suspend() calls phylink_suspend(pl, true). That
takes the MAC WoL branch, which sets PHYLINK_DISABLE_MAC_WOL and does not
call phylink_stop(). On resume, phylink_resume() then only does:

		phylink_mac_initial_config(pl, true);
		phylink_enable_and_run_resolve(pl, PHYLINK_DISABLE_MAC_WOL);

It never reaches phylink_start()/phy_start(). ax88179a_init_phy() sets
phydev->mac_managed_pm, so MDIO bus PM does not call phy_init_hw() either.

Would the PHY then negotiate using its power-on defaults, while
phydev->advertising and the phylink EEE state still reflect what was set
with ethtool?

> +
> +	ax88179a_reset(dev);

[Severity: Medium]
Should this also be gated on netif_running()? Only phylink_resume() below
checks it. ax88179a_reset() powers the PHY, writes AX_RX_CTL_START, sets
AX_MEDIUM_RECEIVE_EN and arms AX_MONITOR_MODE_RWMP whether or not the
interface is up.

If the interface was down at suspend, ax88179a_stop() had left the
hardware like this:

	phylink_stop(ax179_data->phylink);
	...
	reg16 &= ~AX_MEDIUM_RECEIVE_EN;
	...
	ax88179_write_u16(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 0);

	ax88179_write_u8(dev, AX88179A_PHY_POWER, 0, 0, 0);

After resume, the PHY is powered and can bring up a link with the peer,
and MAC RX is enabled, but phylink is not started. The new comment in
ax88179a_reset(), "phylink_resume() will start phylink, then", does not
hold for down interfaces.

A later suspend of the still-down interface does not power the PHY off
again. With USB wakeup enabled, could RWMP then wake the host while WoL
is off?

[Severity: Medium]
Is the VLAN filter table restored on this path? ax88179a_reset() turns
VFE back on whenever the feature is set:

	if (dev->net->features & NETIF_F_HW_VLAN_CTAG_FILTER)
		*tmp |= AX_VLAN_CONTROL_VFE;

The per-VID bitmap, though, is only written by
ax88179a_vlan_rx_add_vid() and ax88179a_vlan_rx_kill_vid(), and the
driver keeps no software copy of it. ax88179a_set_features() also clears
all 256 entries when it enables the filter, which suggests the table
contents can't be relied on.

The chip can lose its registers across suspend, for example on
reset_resume or when VBUS is cut in S3. Would the hardware filter then
drop frames on the configured VLANs until the VIDs are added again?

> +
> +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);
> +}

[ ... ]

> @@ -802,6 +905,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: Low]
This turns off USB autosuspend for every AX88179A/772D/279 device while it
is bound. The usage count only drops when ax88179a_unbind() calls
pm_runtime_put_noidle().

As the comment says, the new ax88179a_suspend()/ax88179a_resume() take
rtnl_lock() around the phylink calls. An autoresume under RTNL, such as
usbnet_open() -> usb_autopm_get_interface(), would therefore deadlock, so
the new callbacks only work for system sleep.

The commit message only talks about adding the suspend/resume wrappers.
Could it mention that runtime PM is disabled for the AX179A family, and
why? The shared ax88179_178a_driver also still sets
.supports_autosuspend = 1 without any note about this exception.

> +
>  	return 0;

[ ... ]

> @@ -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: Medium]
Does skipping ax88179_get_mac_addr() on resume also skip programming the
unicast address into the chip? ax88179_get_mac_addr() ends with:

	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN, ETH_ALEN,
			  dev->net->dev_addr);

That is the only AX_NODE_ID write on the reset path.

ax88179a_resume() rewrites AX_FW_MODE_179A and power-cycles the PHY, so
the driver seems to expect chip state to be lost, for example on
.reset_resume. In that case AX_NODE_ID would go back to the efuse
default. dev_addr, however, still holds a user-set address (from
ax88179_set_mac_addr()), a DT-provided address or the random fallback.

RX_CTL is programmed with AX_RX_CTL_AP and without AX_RX_CTL_PRO. Would
unicast frames to the interface's own MAC then be filtered out after
resume?

Could the AX_NODE_ID write of dev_addr be kept for the in_pm case, with
only the read/selection step skipped?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-ax88179a-v20-0-37b5a1189983%40birger-koblitz.de