Re: [PATCH net-next v21 03/13] ax88179_178a: Split driver into library and device specific code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] net: usb: ax88179: premature MTU state update in
  ax88179_change_mtu()
- [Medium] net: usb: ax88179: incomplete medium_status_mutex locking
--

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

ax88179_178a: Split driver into library and device specific code

This commit splits the ax88179_178a driver into a shared library and
device-specific code, and renames the module to ax88179 with an alias for
ax88179_178a.

> diff --git a/drivers/net/usb/ax88179_lib.c b/drivers/net/usb/ax88179_lib.c
> new file mode 100644
> index 0000000000000..fbaf12b713b03
> --- /dev/null
> +++ b/drivers/net/usb/ax88179_lib.c

[ ... ]

> +int ax88179_change_mtu(struct net_device *net, int new_mtu)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	struct ax88179_data *data;
> +	u16 tmp16;
> +	int err;
> +
> +	data = dev->driver_priv;
> +	mutex_lock(&data->medium_status_mutex);
> +	WRITE_ONCE(net->mtu, new_mtu);
> +	dev->hard_mtu = net->mtu + net->hard_header_len;
> +	if (net->mtu > 1500) {
> +		err = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> +				       2, 2, &tmp16);
> +		if (err < 0)
> +			goto err_out;
> +		tmp16 |= AX_MEDIUM_JUMBO_EN;
> +		err = ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> +					2, 2, &tmp16);
> +		if (err < 0)
> +			goto err_out;
> +	} else {
> +		err = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> +				       2, 2, &tmp16);
> +		if (err < 0)
> +			goto err_out;
> +		tmp16 &= ~AX_MEDIUM_JUMBO_EN;
> +		err = ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> +					2, 2, &tmp16);
> +		if (err < 0)
> +			goto err_out;
> +	}
> +	mutex_unlock(&data->medium_status_mutex);
> +
> +	/* max qlen depend on hard_mtu and rx_urb_size */
> +	usbnet_update_max_qlen(dev);
> +
> +	return 0;
> +
> +err_out:
> +	mutex_unlock(&data->medium_status_mutex);
> +	return err;
> +}

[Severity: Medium]
Does jumping to err_out leave net->mtu and dev->hard_mtu updated to
new_mtu when a register read or write fails?

When __netif_set_mtu() returns an error, netif_set_mtu_ext() assumes the
driver did not modify net->mtu, so it does not roll the value back or
run NETDEV_CHANGEMTU notifiers, and err_out also skips
usbnet_update_max_qlen().  This leaves net->mtu and dev->hard_mtu out of
sync with AX_MEDIUM_JUMBO_EN and dev->tx_qlen, and a retry with the same
new_mtu returns early in netif_set_mtu_ext() because net->mtu already
equals new_mtu.  Updating net->mtu and dev->hard_mtu only after
ax88179_write_cmd() succeeds would avoid this regression.

[ ... ]

> diff --git a/drivers/net/usb/ax88179_lib.h b/drivers/net/usb/ax88179_lib.h
> new file mode 100644
> index 0000000000000..9111a1395eb2f
> --- /dev/null
> +++ b/drivers/net/usb/ax88179_lib.h

[ ... ]

> +struct ax88179_data {
> +	struct mutex medium_status_mutex;  /* protect AX_MEDIUM_STATUS_MODE */

[Severity: Medium]
Can ax88179_change_mtu() still race with ax88179_link_reset() when
updating AX_MEDIUM_STATUS_MODE?

This patch adds medium_status_mutex and takes it in ax88179_change_mtu(),
but ax88179_link_reset() runs on dev->kevent without holding
medium_status_mutex or rtnl_lock:

CPU0                              kworker
ax88179_change_mtu()              ax88179_link_reset()
                                    if (dev->net->mtu > 1500)
                                      mode |= AX_MEDIUM_JUMBO_EN;
                                    ax88179_write_cmd(...,
                                      AX_RX_BULKIN_QCTRL, ...);
  mutex_lock(&medium_status_mutex)
  WRITE_ONCE(net->mtu, 4000)
  ax88179_read_cmd(...,
    AX_MEDIUM_STATUS_MODE, &tmp16)
  tmp16 |= AX_MEDIUM_JUMBO_EN
  ax88179_write_cmd(...,
    AX_MEDIUM_STATUS_MODE, &tmp16)
  mutex_unlock(&medium_status_mutex)
                                    ax88179_write_cmd(...,
                                      AX_MEDIUM_STATUS_MODE, &mode);
                                      <-- overwrites AX_MEDIUM_JUMBO_EN

In the reverse order, ax88179_change_mtu() can read AX_MEDIUM_STATUS_MODE
before ax88179_link_reset() writes the new link mode and then write back
stale speed and duplex bits.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-ax88179a-v21-0-c2d34410bcbe@birger-koblitz.de?part=3