Re: [PATCH net-next v12 10/15] ax88179_178a: Add AX179A/AX279 multicast configuration

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 0

- [High] ax88179_set_multicast() (drivers/net/usb/ax88179_lib.c:305) now
  only sets AX_RX_CTL_IPE when data->ip_align is non-zero (line 314).
- [Medium] Registering ax88179_set_multicast() as .ndo_set_rx_mode for
  the AX88179A family (drivers/net/usb/ax88179a_devices.c:618) creates a…

--

ax88179_178a: Add AX179A/AX279 multicast configuration

This makes AX_RX_CTL_IPE conditional on the new ip_align field in the
shared ax88179_set_multicast() helper, adds AX_RX_CTL_DROPCRCERR to the
base value, and registers the helper as .ndo_set_rx_mode for the
AX88179A device family.

> diff --git a/drivers/net/usb/ax88179_lib.c b/drivers/net/usb/ax88179_lib.c
> index 3324069e90fc3..708b0b23f1363 100644
> --- a/drivers/net/usb/ax88179_lib.c
> +++ b/drivers/net/usb/ax88179_lib.c
> @@ -305,10 +305,14 @@ 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;
> +
> +	data = dev->driver_priv;
>  
> -	data->rxctl = (AX_RX_CTL_START | AX_RX_CTL_AB | AX_RX_CTL_IPE);
> +	data->rxctl = (AX_RX_CTL_START | AX_RX_CTL_AB | AX_RX_CTL_DROPCRCERR);
> +	if (data->ip_align)
> +		data->rxctl |= AX_RX_CTL_IPE;

[Severity: High]
Can this break RX on the legacy AX88179/AX88178A devices at this commit?

ax88179_set_multicast() is also the rx-mode handler of the older driver:

drivers/net/usb/ax88179_178a.c:ax88179_netdev_ops = {
	...
	.ndo_set_rx_mode	= ax88179_set_multicast,
	...
};

but at this commit ax88179_bind() in ax88179_178a.c only kzalloc's the
private data and never assigns ip_align (grep for ip_align in that file
finds only netdev_alloc_skb_ip_align() calls), so "if (data->ip_align)"
is always false there and the async AX_RX_CTL write at the end of
ax88179_set_multicast() clears AX_RX_CTL_IPE.

That contradicts what ax88179_reset() programs:

drivers/net/usb/ax88179_178a.c:ax88179_reset() {
	...
	/* Configure RX control register => start operation */
	*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;
	...
}

and ax88179_rx_fixup() in the same file still strips the pad
unconditionally, with no ip_align test (unlike ax88179a_devices.c, which
uses "ax179_data->ip_align ? 2 : 0"):

drivers/net/usb/ax88179_178a.c:ax88179_rx_fixup() {
	...
			/* Skip IP alignment pseudo header */
			skb_pull(skb, 2);
	...
		memcpy(ax_skb->data, skb->data + 2, pkt_len);
	...
}

So after the first rx-mode update following reset (dev_set_rx_mode() at
interface open, a promisc/allmulti change, or an unprivileged multicast
join via IP_ADD_MEMBERSHIP or MLD), does every received frame get
delivered shifted by two bytes, with the memcpy() path pulling up to two
bytes from the following packet slot into the frame handed to the stack
while the descriptor-derived CHECKSUM_UNNECESSARY is still applied?

Additional detail from the series: the later commit "ax88179_178a: Add
support for AX88179A/772D/279 EEPROM access" adds
"ax179_data->ip_align = 1;" to ax88179_bind() in ax88179_178a.c, so the
final state of the series is fine. That leaves this commit through
"ax88179_178a: Add ethtool get_drvinfo" / "ax88179_178a: Update driver
name and information" with broken RX on legacy devices. Would it make
sense to move that ip_align assignment into this patch so the tree stays
bisectable?

>  
>  	if (net->flags & IFF_PROMISC) {
>  		data->rxctl |= AX_RX_CTL_PRO;
> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index 481139f71ac26..e77450bc3299f 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c
> @@ -615,6 +615,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]
Does this registration introduce an unsynchronized writer of
ax179_data->rxctl for the AX88179A family?

ax88179a_netdev_ops has neither .ndo_set_rx_mode_async nor
.ndo_change_rx_flags, so __dev_set_rx_mode() calls the handler directly
from dev_set_rx_mode():

net/core/dev_addr_lists.c:dev_set_rx_mode() {
	netif_addr_lock_bh(dev);
	__dev_set_rx_mode(dev);
	netif_addr_unlock_bh(dev);
}

ax88179_set_multicast() then does a multi-step read-modify-write of
data->rxctl (base value, then |= AX_RX_CTL_IPE, then |= PRO/AMALL/AM
with an async multicast-hash URB submitted in between) and finally
programs the register with ax88179_write_cmd_async().

Meanwhile ax88179a_mac_link_up() reads the same field and writes it
synchronously from the phylink resolve path, holding only phylink's
state_mutex:

drivers/net/usb/ax88179a_devices.c:ax88179a_mac_link_up() {
	...
	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, &ax179_data->rxctl);
	...
}

and ax88179a_reset() rewrites both rxctl and the register:

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

The lock sets do not intersect, so can the phylink path latch an
intermediate rxctl value that is missing AX_RX_CTL_AM/AMALL/PRO? And
since one side uses an async URB while the other uses a synchronous
control transfer, can the two AX_RX_CTL writes also complete out of
order and leave the hardware filter without the multicast or promisc
bits until the next filter change or link event?

The same pattern already exists in the legacy driver's
ax88179_link_reset(), but for the AX88179A family this concurrency is
new here, and it is still present at the series tip. Would routing the
filter update through a deferred path (usbnet's EVENT_SET_RX_MODE /
driver_info->set_rx_mode, or .ndo_set_rx_mode_async) put it in the same
process context as the link and reset writers?

>  	.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/20260916-ax88179a-v12-0-60c04c9924a2%40birger-koblitz.de