Re: [PATCH net-next v11 09/15] ax88179_178a: Add VLAN offload support for AX88179A

[netdev-bot+sashiko@kernel.org]

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

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

- [High] ax88179a_set_features() destroys the VLAN control register it
  just programmed: the 256-iteration filter-table clearing loop writes…
- [Medium] Series ordering / bisect breakage that the commit message does
  not disclose: ax88179a_bind() already sets dev->net->features |=…
- [Medium] Undisclosed, unrelated behaviour change in shared library
  code: the hunk in drivers/net/usb/ax88179_lib.c adds…
- [Medium] The newly added `data->rx_checksum = !!(features &
  NETIF_F_RXCSUM)` tracking in ax88179_set_features() is contradicted by…
- [Medium] The new VLAN ndo callbacks and the VLAN part of
  ax88179a_set_features() discard the return value of every…
- [Medium] ax88179a_set_features() programs the VLAN mode bits by XOR-
  toggling the value read from AX88179A_VLAN_ID_CONTROL (`tmp ^=…
- [Medium] The new VLAN helpers and ax88179a_set_features() perform
  1-byte register reads into uninitialized stack variables (`u8 reg8;`…
- [Medium] ax88179_set_features() publishes the new RX-checksum state
  with a plain store to data->rx_checksum from process context (RTNL…

--

ax88179_178a: Add VLAN offload support for AX88179A

This adds ndo_set_features, ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid
for the AX88179A family so that CTAG TX/RX offload and the hardware VLAN
filter table can be configured.  It also adds a line to the shared
ax88179_set_features() that tracks the NETIF_F_RXCSUM state.

No Link: tags were present in the commit header.

A few questions about the new code below.

> diff --git a/drivers/net/usb/ax88179_lib.c b/drivers/net/usb/ax88179_lib.c
> index ea36aef3d3e45..3324069e90fc3 100644
> --- a/drivers/net/usb/ax88179_lib.c
> +++ b/drivers/net/usb/ax88179_lib.c
> @@ -346,6 +346,7 @@ int ax88179_set_features(struct net_device *net, netdev_features_t features)
>  {
>  	u8 tmp;
>  	struct usbnet *dev = netdev_priv(net);
> +	struct ax88179_data *data = dev->driver_priv;
>  	netdev_features_t changed = net->features ^ features;
>  
>  	if (changed & NETIF_F_IP_CSUM) {
> @@ -365,6 +366,7 @@ int ax88179_set_features(struct net_device *net, netdev_features_t features)
>  		tmp ^= AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
>  		       AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
>  		ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, &tmp);
> +		data->rx_checksum = !!(features & NETIF_F_RXCSUM);
>  	}

[Severity: Medium]
This hunk changes RX checksum behaviour in the shared library used by both
ax88179_178a.c and ax88179a_devices.c, while the commit message only
mentions VLAN offload:

    Features supported are:
    NETIF_F_HW_VLAN_CTAG_TX, NETIF_F_HW_VLAN_CTAG_RX
    and NETIF_F_HW_VLAN_CTAG_FILTER.

Previously ax88179_data.rx_checksum was only ever set to 1 in
ax88179a_reset() and never cleared, so ax88179a_rx_fixup() kept calling
ax88179a_rx_checksum() after the user disabled NETIF_F_RXCSUM.  Could this
change be described in the commit message, or split into its own patch?

[Severity: Medium]
Does ax88179a_reset() undo this tracking?  It programs the same register
absolutely and forces the flag back on without consulting net->features:

drivers/net/usb/ax88179a_devices.c:ax88179a_reset() {
	...
	/* Enable checksum offload */
	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
	       AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, tmp);
	ax179_data->rx_checksum = 1;
	...
}

After ethtool -K ethX rx off followed by ip link set ethX down; ip link set
ethX up, is hardware RX checksum offload re-enabled and rx_checksum forced
to 1 while net->features still has NETIF_F_RXCSUM cleared?

There is a second consequence of mixing an absolute write in reset() with
the XOR toggle here: a later ethtool -K ethX rx on reads the already-enabled
AX_RXCOE_* bits, XORs them to zero and disables the checksum engine, yet
sets rx_checksum = 1.  Would ax88179a_rx_fixup() then interpret descriptor
checksum fields produced by a disabled engine?

This is still the case at the end of the series.

[Severity: Medium]
Is the store to data->rx_checksum here synchronized against the reader?
The writer runs in process context under RTNL, while the reader runs from
usbnet's bh work:

drivers/net/usb/ax88179a_devices.c:ax88179a_rx_fixup() {
	...
	if (ax179_data->rx_checksum)
		ax88179a_rx_checksum(ax_skb, pkt_desc);
	...
}

There is no lock, READ_ONCE()/WRITE_ONCE() or barrier on either side.

The ordering also looks inverted for the disable direction: AX_RXCOE_CTL is
reprogrammed first and the flag is cleared afterwards, so frames handled in
that window can still be marked CHECKSUM_UNNECESSARY from descriptor bits
the MAC no longer produces.  Would clearing the flag before disabling the
hardware, and setting it after enabling it, be more correct?

>  
>  	return 0;
> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index 97aad545daee1..4bca801236b08 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c
> @@ -239,6 +239,62 @@ static const struct ethtool_ops ax88179a_ethtool_ops = {
>  	.get_ts_info		= ethtool_op_get_ts_info,
>  };
>  
> +static int ax88179a_vlan_rx_kill_vid(struct net_device *net, __be16 proto, u16 vid)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	u8 vlan_ctrl;
> +	u16 reg16;
> +	u8 reg8;
> +
> +	ax88179_read_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL, 1, 1, &reg8);
> +	vlan_ctrl = reg8;

[Severity: Medium]
Can vlan_ctrl end up holding uninitialized stack data here?  reg8 is
declared without an initializer and the read return value is not checked.
For size == 1, ax88179_read_cmd() passes the caller buffer straight through
without zeroing it:

drivers/net/usb/ax88179_lib.c:ax88179_read_cmd() {
	...
	} else {
		ret = __ax88179_read_cmd(dev, cmd, value, index, size, data);
	}
	...
}

and nothing is copied on a failed transfer:

drivers/net/usb/usbnet.c:__usbnet_read_cmd() {
	...
	if (err > 0 && err <= size) {
		if (data)
			memcpy(data, buf, err);
	...
}

So on an -ENODEV/-ETIMEDOUT/stalled control transfer, is one byte of kernel
stack written back into AX88179A_VLAN_ID_CONTROL, where the device
interprets it as VFE/VSO/RD/WE?  The same pattern is in
ax88179a_vlan_rx_add_vid() and in ax88179a_set_features() (u8 tmp; read,
then tmp ^= AX_VLAN_CONTROL_VSO).  aqc111 initializes its equivalent locals
(u8 reg8 = 0).

> +
> +	/* Address */
> +	reg8 = (vid / 16);
> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_ADDRESS, 1, 1, &reg8);
> +
> +	/* Data */
> +	reg8 = vlan_ctrl | AX_VLAN_CONTROL_RD;
> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL, 1, 1, &reg8);
> +
> +	ax88179_read_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_DATA0, 2, 2, &reg16);
> +	reg16 &= ~(1 << (vid % 16));
> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_DATA0, 2, 2, &reg16);
> +
> +	reg8 = vlan_ctrl | AX_VLAN_CONTROL_WE;
> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL, 1, 1, &reg8);
> +
> +	return 0;
> +}

[Severity: Medium]
Should these callbacks propagate the register access errors instead of
always returning 0?  The 8021q core and __netdev_update_features() use the
return code to decide whether to record the new state.

For the 2-byte case ax88179_read_cmd() zeroes a temporary and copies it out
even when the transfer failed:

drivers/net/usb/ax88179_lib.c:ax88179_read_cmd() {
	...
	if (size == 2) {
		u16 buf = 0;

		ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf);
		le16_to_cpus(&buf);
		*((u16 *)data) = buf;
	}
	...
}

So if the AX88179A_VLAN_ID_DATA0 read fails, reg16 becomes 0 and the
write-back clears the other 15 VIDs sharing that table word, while
ax88179a_vlan_rx_kill_vid() still reports success.  ax88179a_vlan_rx_add_vid()
has the same issue with only the new bit surviving.  A failed address or WE
write leaves the entry unprogrammed and also goes unreported.

ax88179a_set_features() does check ax88179_set_features(), but ignores all
of its own accesses, including the 256 loop iterations.

> +
> +static int ax88179a_vlan_rx_add_vid(struct net_device *net, __be16 proto, u16 vid)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	u8 vlan_ctrl;
> +	u16 reg16;
> +	u8 reg8;
> +
> +	ax88179_read_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL, 1, 1, &reg8);
> +	vlan_ctrl = reg8;
> +

[ ... ]

> +	ax88179_read_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_DATA0, 2, 2, &reg16);
> +	reg16 |= (1 << (vid % 16));
> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_DATA0, 2, 2, &reg16);
> +
> +	reg8 = vlan_ctrl | AX_VLAN_CONTROL_WE;
> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL, 1, 1, &reg8);
> +
> +	return 0;
> +}
> +

[ ... ]

> @@ -499,6 +555,49 @@ static int ax88179a_init_mdio(struct usbnet *dev)
>  	return ret;
>  }
>  
> +static int ax88179a_set_features(struct net_device *net, netdev_features_t features)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	netdev_features_t changed;
> +	int ret;
> +	u8 tmp;
> +
> +	changed = net->features ^ features;
> +
> +	ret = ax88179_set_features(net, features);
> +	if (ret)
> +		return ret;
> +
> +	if (changed & NETIF_F_HW_VLAN_CTAG_FILTER) {
> +		ax88179_read_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL, 1, 1, &tmp);
> +		tmp ^= AX_VLAN_CONTROL_VFE;
> +		ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL, 1, 1, &tmp);
> +		if (features & NETIF_F_HW_VLAN_CTAG_FILTER) {
> +			for (int i = 0; i < 256; i++) {
> +				u16 tmp16 = 0;
> +				/* Address */
> +				tmp = i;
> +				ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_ADDRESS,
> +						  1, 1, &tmp);
> +				/* Data */
> +				ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_DATA0,
> +						  2, 2, &tmp16);
> +				tmp = AX_VLAN_CONTROL_WE;
> +				ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL,
> +						  1, 1, &tmp);
                                  ^^^^

[Severity: High]
Does this bare WE strobe wipe the enable bits in the same register?
AX88179A_VLAN_ID_CONTROL is one byte holding all four bits:

drivers/net/usb/ax88179_lib.h:
	#define AX88179A_VLAN_ID_CONTROL		0x2B
		#define AX_VLAN_CONTROL_WE	0x0001
		#define AX_VLAN_CONTROL_RD	0x0002
		#define AX_VLAN_CONTROL_VSO	0x0010
		#define AX_VLAN_CONTROL_VFE	0x0020

ax88179_write_cmd(dev, ..., 1, 1, &tmp) writes the whole byte, so after
ethtool -K ethX rx-vlan-filter on, does the register end up as 0x01 with
VFE cleared (filter not enabled in hardware) and VSO cleared (RX tag
stripping silently turned off), while dev->features reports both enabled?

Every other site programs this register read-modify-write, including the two
callbacks added by this patch:

	reg8 = vlan_ctrl | AX_VLAN_CONTROL_WE;
	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL, 1, 1, &reg8);

and ax88179a_reset() composes VFE/VSO from net->features.  Should the loop
use vlan_ctrl | AX_VLAN_CONTROL_WE as well?

Note also that the core re-pushes the VIDs after the feature change, and
ax88179a_vlan_rx_add_vid() then ORs WE onto the now-zeroed control value,
so nothing appears to restore VFE/VSO.  The same code is unchanged at the
end of the series.

> +			}
> +		}
> +	}
> +
> +	if (changed & NETIF_F_HW_VLAN_CTAG_RX) {
> +		ax88179_read_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL, 1, 1, &tmp);
> +		tmp ^= AX_VLAN_CONTROL_VSO;
> +		ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL, 1, 1, &tmp);
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Both VLAN mode bits are programmed by XOR-toggling the value read back from
the device rather than being set or cleared from the requested features.
The changed mask only says the requested software bit differs from the
previous software bit, not that the hardware bit still mirrors the old
software state.

Two cases where the register does not mirror net->features:

- right after the clear loop above leaves the control register as 0x01,
  so a following rx-vlan-filter off reads VFE=0 and XORs it to 1, arming
  the hardware filter while userspace is told filtering is off

- before the first ax88179a_reset(), since feature changes are accepted
  while the interface is down

In those cases the XOR inverts the request and the function still returns
success.  Would assigning the bits from features be safer here?

>  
>  static const struct net_device_ops ax88179a_netdev_ops = {
>  	.ndo_open		= usbnet_open,
>  	.ndo_stop		= usbnet_stop,
> @@ -509,6 +608,9 @@ 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		= usbnet_mii_ioctl,
> +	.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,
>  };

[Severity: Medium]
Is the series bisectable across this point?  The earlier commit
"ax88179_178a: Add HW support for AX179A-based chips" already advertises the
filter capability in ax88179a_bind():

	dev->net->features |= NETIF_F_SG | NETIF_F_IP_CSUM |
			      NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM | NETIF_F_TSO |
			      NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX |
			      NETIF_F_HW_VLAN_CTAG_FILTER;

	dev->net->hw_features |= dev->net->features;

but the two ndo callbacks only arrive here, and the core rejects that
combination:

net/core/dev.c:register_netdevice() {
	...
	if (((dev->hw_features | dev->features) &
	     NETIF_F_HW_VLAN_CTAG_FILTER) &&
	    (!dev->netdev_ops->ndo_vlan_rx_add_vid ||
	     !dev->netdev_ops->ndo_vlan_rx_kill_vid)) {
		netdev_WARN(dev, "Buggy VLAN acceleration in driver!\n");
		ret = -EINVAL;
		goto err_uninit;
	}
	...
}

At the parent commit, grepping ax88179a_devices.c for ndo_vlan_rx returns no
match, so does usbnet_probe() -> register_netdev() fail for every AX88179A,
AX88772D and AX88279 device at the intermediate commits?  Could the feature
bits be added in this patch instead, or the ordering noted in the commit
message?

>  
>  static int ax88179a_bind(struct usbnet *dev, struct usb_interface *intf)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914-ax88179a-v11-0-5ea7a925ba6d%40birger-koblitz.de