Re: [PATCH net-next v20 10/13] ax88179_178a: Add ethtool get_drvinfo

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] ax88179a_get_drvinfo() (drivers/net/usb/ax88179a_devices.c)
  always overwrites info->fw_version with the cached…

--

ax88179_178a: Add ethtool get_drvinfo

This commit adds ax88179a_get_drvinfo() as the get_drvinfo ethtool
callback. It uses the usbnet defaults and then, for AX88179A and newer
chips, overwrites fw_version with the four firmware bytes cached at
bind time.

> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index 01d6a35f68b2c..69095110e7043 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c
> @@ -235,6 +235,20 @@ static int ax88179a_resume(struct usb_interface *intf)
>  	return usbnet_resume(intf);
>  }
>  
> +static void ax88179a_get_drvinfo(struct net_device *net, struct ethtool_drvinfo *info)
> +{
> +	struct ax88179_data *priv = netdev2data(net);
> +
> +	/* Inherit standard device info */
> +	usbnet_get_drvinfo(net, info);
> +	if (priv->chip_version < AX_VERSION_AX88179A)
> +		return;
> +
> +	snprintf(info->fw_version, sizeof(info->fw_version), "%d.%d.%d.%d",
> +		 priv->fw_version[0], priv->fw_version[1],
> +		 priv->fw_version[2], priv->fw_version[3]);

[Severity: Low]
Could this report a firmware version that the device never returned?

In ax88179a_bind(), a failed firmware read is replaced with 0xff and
bind keeps going:

drivers/net/usb/ax88179a_devices.c:ax88179a_bind() {
    ...
	for (int i = 0; i < 3; i++) {
		ret = ax88179_read_cmd(dev, AX88179A_ACCESS_BL, AX88179A_SW_VERSION + i,
				       1, 1, &ax179_data->fw_version[i]);
		if (ret < 0)
			ax179_data->fw_version[i] = 0xff;
	}
	ret = ax88179_read_cmd(dev, AX88179A_ACCESS_BL, AX88179A_SW_REVISION,
			       1, 1, &ax179_data->fw_version[3]);
	if (ret < 0)
		ax179_data->fw_version[3] = 0xff;
    ...
}

If any of these USB control transfers fail, ethtool -i would show
"255.255.255.255", or something mixed like "1.2.255.3" when only some
reads fail. That string also replaces the driver_info->description
default that usbnet_get_drvinfo() has just written.

Before this patch the 0xff sentinel only appeared in the netdev_info()
line printed by bind.

Would it make sense to record whether the reads succeeded and, when
they did not, skip the overwrite (keeping the usbnet default) or report
something like "unknown"?

aqc111_get_drvinfo() in the same directory also reports firmware bytes
without checking for read errors.

> +}
> +

[ ... ]

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