Re: [PATCH net-next v20 08/13] ax88179_178a: Add AX179A/AX279 multicast configuration

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 2

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] The patch also changes RX behaviour on the existing
  AX88179/AX88178A devices, and the commit message does not say so.
- [Medium] Adding `.ndo_set_rx_mode = ax88179_set_multicast` to
  ax88179a_netdev_ops (ax88179a_devices.c:681) gives…
- [Low] This patch adds `.ndo_set_rx_mode = ax88179_set_multicast` to
  ax88179a_netdev_ops (ax88179a_devices.c:681).
- [Low] ax88179_set_multicast() (ax88179_lib.c:362-364) builds the value
  in a local `rxctl` and publishes it with `WRITE_ONCE(data->rxctl,…

--

ax88179_178a: Add AX179A/AX279 multicast configuration

This patch changes ax88179_set_multicast() to build the RX_CTL value in a
local variable. AX_RX_CTL_IPE is now set only when ip_align is set, and
AX_RX_CTL_DROPCRCERR is always set. The patch also installs
ax88179_set_multicast() as ndo_set_rx_mode for the AX88179A family of
devices.

> diff --git a/drivers/net/usb/ax88179_lib.c b/drivers/net/usb/ax88179_lib.c
> index a314db4c97666..b4b00c8b4b623 100644
> --- a/drivers/net/usb/ax88179_lib.c
> +++ b/drivers/net/usb/ax88179_lib.c
> @@ -321,16 +321,21 @@ int ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom, u8
>  void ax88179_set_multicast(struct net_device *net)
>  {
>  	struct usbnet *dev = netdev_priv(net);
> -	struct ax88179_data *data = dev->driver_priv;
>  	u8 *m_filter = ((u8 *)dev->data);
> +	struct ax88179_data *data;
> +	u16 rxctl = 0;
> +
> +	data = dev->driver_priv;
>  
> -	data->rxctl = (AX_RX_CTL_START | AX_RX_CTL_AB | AX_RX_CTL_IPE);
> +	rxctl = (AX_RX_CTL_START | AX_RX_CTL_AB | AX_RX_CTL_DROPCRCERR);
> +	if (data->ip_align)
> +		rxctl |= AX_RX_CTL_IPE;

[Severity: Medium]
Does this also change RX behaviour on the existing AX88179/AX88178A
devices? The legacy driver uses this helper as its rx_mode handler:

drivers/net/usb/ax88179_178a.c:
	.ndo_set_rx_mode	= ax88179_set_multicast,

Before this patch the helper wrote AX_RX_CTL_START | AX_RX_CTL_AB |
AX_RX_CTL_IPE, without AX_RX_CTL_DROPCRCERR.

The value set in ax88179_reset() only goes into a temporary. It is never
stored in data->rxctl:

drivers/net/usb/ax88179_178a.c:ax88179_reset() {
    ...
	*tmp16 = AX_RX_CTL_DROPCRCERR | AX_RX_CTL_IPE | AX_RX_CTL_START |
		 AX_RX_CTL_AP | AX_RX_CTL_AMALL | AX_RX_CTL_AB;
	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, tmp16);
    ...
}

On open, usbnet_open() runs the reset first. Then
dev_open()->dev_set_rx_mode()->ax88179_set_multicast() overwrites that
value, and ax88179_link_reset() keeps writing data->rxctl back. So legacy
adapters have been running with AX_RX_CTL_DROPCRCERR cleared. Frames with
a bad CRC were then counted in ax88179_rx_fixup():

	if ((*pkt_hdr & (AX_RXHDR_CRC_ERR | AX_RXHDR_DROP_ERR)) ||
	    pkt_len < 2 + ETH_HLEN) {
		dev->net->stats.rx_errors++;

This patch sets AX_RX_CTL_DROPCRCERR for every chip. If the hardware then
drops those frames itself, as the bit name suggests, would legacy users
stop seeing CRC errors in rx_errors?

The commit message says:

  "make sure that AX_RX_CTL_DROPCRCERR is also set to be consistent with
  the initial configuration in ax88179_reset()"

That reset value never survived on legacy devices, so this is a real
behaviour change rather than a consistency cleanup.

The message also leaves out the reason given in the ax88179a_bind()
comment, which says the AX88279 needs both AX_RX_CTL_IPE and
AX_RX_CTL_DROPCRCERR to produce correct RX URBs.

Could the commit message describe the change to legacy devices, or could
that change go in its own patch?

[Severity: Low]
This might not be a bug, but is AX_RX_CTL_AP left out on purpose here?
ax88179a_reset() stores a different baseline in data->rxctl:

drivers/net/usb/ax88179a_devices.c:ax88179a_reset() {
    ...
	rxctl = AX_RX_CTL_DROPCRCERR | AX_RX_CTL_START | AX_RX_CTL_AP |
		AX_RX_CTL_AMALL | AX_RX_CTL_AB;
    ...
}

Now that ndo_set_rx_mode is set for ax88179a,
dev_open()->dev_set_rx_mode()->ax88179_set_multicast() replaces that value
with one that has AX_RX_CTL_AP cleared. ax88179a_mac_link_up() then writes
READ_ONCE(ax179_data->rxctl) back on every link-up. Before this patch
these chips kept AX_RX_CTL_AP set.

The intent is to match the reset-time configuration. Should
ax88179a_reset() drop AX_RX_CTL_AP, or should ax88179_set_multicast() keep
it? The legacy AX88179 has run without AX_RX_CTL_AP after open, but it is
not clear that the same is safe on AX88179A/AX88279 silicon.

[ ... ]

> @@ -351,9 +356,10 @@ void ax88179_set_multicast(struct net_device *net)
>  					AX_MCAST_FLTSIZE, AX_MCAST_FLTSIZE,
>  					m_filter);
>  
> -		data->rxctl |= AX_RX_CTL_AM;
> +		rxctl |= AX_RX_CTL_AM;
>  	}
>  
> +	WRITE_ONCE(data->rxctl, rxctl);
>  	ax88179_write_cmd_async(dev, AX_ACCESS_MAC, AX_RX_CTL,
>  				2, 2, &data->rxctl);

[Severity: Low]
Minor, but the value is already in the local rxctl. Could this pass &rxctl
instead of &data->rxctl? ax88179_write_cmd_async() reads the shared field
back with a plain load:

drivers/net/usb/ax88179_lib.c:ax88179_write_cmd_async() {
    ...
		buf = *((u16 *)data);
    ...
}

Elsewhere, data->rxctl is accessed with WRITE_ONCE()/READ_ONCE()
(ax88179a_reset(), ax88179a_mac_link_up()). If another writer changes the
field between the WRITE_ONCE() and this load, the URB carries a value this
call did not compute, and KCSAN may flag the unmarked access.

ax88179_write_cmd_async() copies into a stack buffer and
usbnet_write_cmd_async() duplicates it, so a pointer to the local should
be safe.

>  }
> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index 4207d2cc2b865..a6b0a3f47118c 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c
> @@ -678,6 +678,7 @@ static const struct net_device_ops ax88179a_netdev_ops = {
>  	.ndo_set_mac_address	= ax88179_set_mac_addr,
>  	.ndo_validate_addr	= eth_validate_addr,
>  	.ndo_eth_ioctl		= ax88179a_mii_ioctl,
> +	.ndo_set_rx_mode	= ax88179_set_multicast,

[Severity: Medium]
Can this new writer of data->rxctl and AX_RX_CTL race with
ax88179a_mac_link_up()?

ax88179a_netdev_ops has no ndo_set_rx_mode_async. So __dev_set_rx_mode()
calls ax88179_set_multicast() directly, in atomic context under
netif_addr_lock_bh(), where it cannot take medium_status_mutex.

ax88179a_mac_link_up() runs from the phylink resolve work and holds
neither rtnl nor the addr lock. It stops RX, then reprograms the RX
framing and bulk-in registers with many synchronous control transfers.
It only takes medium_status_mutex around the final write:

drivers/net/usb/ax88179a_devices.c:ax88179a_mac_link_up() {
    ...
	/* Stop RX/TX for link configuration */
	ax88179_write_u16(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, AX_RX_CTL_STOP);
	ax88179_write_u8(dev, AX_ACCESS_MAC, AX88179A_MAC_PATH, 1, 0);
    ...
	ax88179_write_u16(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, READ_ONCE(ax179_data->rxctl));
	mutex_unlock(&ax179_data->medium_status_mutex);
    ...
}

Two orderings look possible. The first loses an update:

phylink resolve work              rx_mode change (e.g. promisc on)
ax88179a_mac_link_up()
  READ_ONCE(rxctl) -> old
                                  ax88179_set_multicast()
                                    WRITE_ONCE(data->rxctl, new)
                                    async AX_RX_CTL = new
  sync AX_RX_CTL = old

The hardware ends up with the old filter while software holds the new
one. Promiscuous, allmulti or multicast reception would then be missing
until the next rx_mode event.

In the second, the async write includes AX_RX_CTL_START. If it lands
between the AX_RX_CTL_STOP write and the final AX_RX_CTL write, does it
turn the receiver back on while the RX framing and bulk-in settings are
only partly applied? ax88179a_rx_fixup() would then parse URBs built with
those half-applied settings.

Before this patch ax88179a had no ndo_set_rx_mode, so neither ordering
could happen.

Could the rx-mode programming be deferred to process context, for example
through usbnet's EVENT_SET_RX_MODE kevent or ndo_set_rx_mode_async? It
could then be serialized with ax88179a_mac_link_up() under
medium_status_mutex.

>  	.ndo_set_features	= ax88179a_set_features,
>  	.ndo_vlan_rx_add_vid	= ax88179a_vlan_rx_add_vid,
>  	.ndo_vlan_rx_kill_vid	= ax88179a_vlan_rx_kill_vid,

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