Re: [PATCH net-next v17 07/13] ax88179_178a: Add VLAN offload support for AX88179A

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: missing disable logic and brittle XOR toggling in VLAN state machine
- [High] net: VLAN filter initialization clobbers AX88179A_VLAN_ID_CONTROL register
--

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

ax88179_178a: Add VLAN offload support for AX88179A

This commit adds VLAN offload configuration support in netdev_ops for
AX88179A-based chips. The supported features include NETIF_F_HW_VLAN_CTAG_TX,
NETIF_F_HW_VLAN_CTAG_RX, and NETIF_F_HW_VLAN_CTAG_FILTER.

> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c

[ ... ]

> @@ -517,6 +599,58 @@ static int ax88179a_mii_ioctl(struct net_device *net, struct ifreq *rq, int cmd)
>  	return phylink_mii_ioctl(data->phylink, rq, cmd);
>  }
>  
> +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) {
> +		if (features & NETIF_F_HW_VLAN_CTAG_FILTER) {

[Severity: High]
Is there a missing else block here for disabling the filter?

Without an else branch, it appears that disabling NETIF_F_HW_VLAN_CTAG_FILTER
via ethtool will have no effect on the hardware since the disable path is
completely skipped.

> +			for (int i = 0; i < 256; i++) {
> +				/* Address */
> +				tmp = i;
> +				ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_ADDRESS,
> +						  1, 1, &tmp);
> +				/* Data */
> +				ax88179_write_u16(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_DATA0,
> +						  2, 0);
> +				ax88179_write_u8(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL,
> +						 1, AX_VLAN_CONTROL_WE);

[Severity: High]
Does writing AX_VLAN_CONTROL_WE directly clobber the rest of the control
register?

Inside this loop, ax88179_write_u8() blindly overwrites the entire control
register with 0x01 instead of using a read-modify-write. This seems to clear
other active configuration bits in the register, such as AX_VLAN_CONTROL_VSO,
which could silently break RX VLAN hardware acceleration if it is enabled.

> +			}
> +			ret = ax88179_read_cmd(dev, AX_ACCESS_MAC,
> +					       AX88179A_VLAN_ID_CONTROL, 1, 1, &tmp);
> +			if (ret < 0)
> +				return ret;
> +			tmp ^= AX_VLAN_CONTROL_VFE;

[Severity: High]
Does this XOR operation lead to an inverted state if the hardware and OS states
desynchronize?

Using XOR to toggle AX_VLAN_CONTROL_VFE instead of explicitly setting or
clearing the bit based on the features flag means a subsequent state change
could invert the configuration, leading to incorrectly filtered packets.

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

[Severity: High]
Similarly, does this XOR logic for the RX VLAN offload risk state inversion?

If the OS state and hardware state desynchronize, the bit will be flipped
the wrong way, potentially causing packets to be incorrectly untagged or
causing dropped network traffic.

> +		ret = ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL,
> +					1, 1, &tmp);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-ax88179a-v17-0-252814fe35ea@birger-koblitz.de?part=7