Re: [PATCH net-next v12 13/15] ax88179_178a: Update driver name and information

[netdev-bot+sashiko@kernel.org]

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

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

- [Critical] usb: net: sysfs ABI and module name breakage for ax88179
- [Medium] Undisclosed Kconfig symbol rename leaves two in-tree
  defconfigs orphaned.
- [Low] FALSE POSITIVE (retained only per validation policy): the concern
  claims `select AX88796B_PHY` is bogus because the synthetic PHY IDs…
- [Low] The Kconfig prompt/help and MODULE_DESCRIPTION updated by this
  patch still describe the module as Gigabit-only while the device list…
- [Low] Firmware-version producer/consumer mismatch: ax88179a_bind()
  (drivers/net/usb/ax88179a_devices.c:761-766) reads only three bytes…

--

ax88179_178a: Update driver name and information

This renames the Kconfig symbol for the ax88179 driver, extends the Kconfig
prompt and help text with the newly supported chips, and adds selects for
PHYLINK and AX88796B_PHY. It also renames the usb_driver structure and its
.name string and updates MODULE_DESCRIPTION.

> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index 115990b0d9537..4158455a2da76 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
> @@ -195,11 +195,12 @@ config USB_NET_AX8817X
>  	  This driver creates an interface named "ethX", where X depends on
>  	  what other networking devices you have in use.
>  
> -config USB_NET_AX88179_178A
> -	tristate "ASIX AX88179/178A USB 3.0/2.0 to Gigabit Ethernet"
> +config USB_NET_AX88179
> +	tristate "ASIX AX88179/179A/178A USB 3.0/2.0 to Gigabit Ethernet"
>  	depends on USB_USBNET
> -	select CRC32
>  	select PHYLINK
> +	select AX88796B_PHY
> +	select CRC32
>  	default y

[Severity: Medium]
Does this rename leave the two loongarch defconfigs behind?  Both
arch/loongarch/configs/loongson32_defconfig and
arch/loongarch/configs/loongson64_defconfig still carry:

    # CONFIG_USB_NET_AX88179_178A is not set

Since Kconfig silently ignores unknown symbols, and USB_NET_AX88179 keeps
"default y" while both defconfigs have CONFIG_USB_USBNET=m, doesn't
USB_NET_AX88179 come out as m there and the module get built on platforms
that explicitly disabled it?  The same silent flip would happen to any
existing .config with the old symbol disabled when run through
make oldconfig.

Should the two defconfig lines be renamed in the same patch?

The commit message says only:

    Add additionally supported devices to Kconfig description, add
    further requirements such as PHYLINK.

    Update driver name in usb_driver and update MODULE_DESCRIPTION
    to include the additionally supported chips.

The user-visible Kconfig symbol rename from USB_NET_AX88179_178A to
USB_NET_AX88179 is not mentioned at all.  Could the changelog call it out
so backporters know to look for defconfig fallout?

>  	help
>  	  This option adds support for ASIX AX88179 based USB 3.0/2.0
> @@ -207,6 +208,9 @@ config USB_NET_AX88179_178A
>  
>  	  This driver should work with at least the following devices:
>  	    * ASIX AX88179
> +	    * ASIX AX88179A/B
> +	    * ASIX AX88279
> +	    * ASIX AX88772D/E
>  	    * ASIX AX88178A
>  	    * Sitcomm LN-032
>  

[Severity: Low]
This isn't a bug, but the prompt and help text still say Gigabit only,
while the list now includes a Fast Ethernet part and a 2.5G part.  The
prompt reads "ASIX AX88179/179A/178A USB 3.0/2.0 to Gigabit Ethernet" and
the help text says "to Gigabit Ethernet adapters", yet the added entries
are AX88279 and AX88772D/E.

The device table comments in ax88179_178a.c describe them as:

	/* ASIX AX88772D USB 2.0 100Mbit Ethernet */
	/* ASIX AX88279 USB 3.2 2500Mbit Ethernet */

and ax88179a_phylink_setup() in ax88179a_devices.c matches that:

	data->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_100;
	if (data->is_ax88772d)
		data->phylink_config.mac_capabilities |= MAC_10;
	else if (data->chip_version < AX_VERSION_AX88279)
		data->phylink_config.mac_capabilities |= MAC_10 | MAC_1000;
	else
		data->phylink_config.mac_capabilities |= MAC_1000 | MAC_2500FD;

Could the wording be widened to cover 10/100 and 2.5G as well?

> diff --git a/drivers/net/usb/Makefile b/drivers/net/usb/Makefile
> index 2ecead0181eaf..4026f19ecb826 100644
> --- a/drivers/net/usb/Makefile
> +++ b/drivers/net/usb/Makefile
> @@ -12,7 +12,7 @@ obj-$(CONFIG_USB_HSO)		+= hso.o
>  obj-$(CONFIG_USB_LAN78XX)	+= lan78xx.o
>  obj-$(CONFIG_USB_NET_AX8817X)	+= asix.o
>  asix-y := asix_devices.o asix_common.o ax88172a.o
> -obj-$(CONFIG_USB_NET_AX88179_178A)      += ax88179.o
> +obj-$(CONFIG_USB_NET_AX88179)      += ax88179.o
>  ax88179-y := ax88179_178a.o ax88179a_devices.o ax88179_lib.o

[ ... ]

> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 93031b9ee92bc..2601a8875b9e2 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c

[ ... ]

> @@ -1353,7 +1353,7 @@ static struct usb_driver ax88179_178a_driver = {
>  	.disable_hub_initiated_lpm = 1,
>  };
>  
> -module_usb_driver(ax88179_178a_driver);
> +module_usb_driver(ax88179_driver);
>  
> -MODULE_DESCRIPTION("ASIX AX88179/178A based USB 3.0/2.0 Gigabit Ethernet Devices");
> +MODULE_DESCRIPTION("ASIX AX88179/179A/178A based USB 3.0/2.0 Gigabit Ethernet Devices");
>  MODULE_LICENSE("GPL");

[Severity: Low]
Same wording question here: the new MODULE_DESCRIPTION still says
"USB 3.0/2.0 Gigabit Ethernet Devices" although the driver now also binds
the 10/100 AX88772D and the 2.5G AX88279.  Would it be worth updating this
string too?

One more observation, in code this patch does not touch but which comes
from earlier patches in the same series and is still present at the end of
the series:

[Severity: Low]
Is the reported firmware version short one byte?  ax88179a_bind() in
drivers/net/usb/ax88179a_devices.c fills only three entries:

	for (int i = 0; i < 3; i++) {
		ret = ax88179_read_cmd(dev, AX88179A_ACCESS_BL, (0xFD + i),
				       1, 1, &ax179_data->fw_version[i]);
		if (ret < 0)
			ax179_data->fw_version[i] = 0xff;
	}
	netdev_info(dev->net, "AX88179A/279/772D Chip Version: %x, FW: %d.%d.%d.%d\n", ...

but both consumers print four, the probe message above and the ethtool
path in ax88179a_get_drvinfo():

	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]);

struct ax88179_data is allocated with kzalloc_obj(), so fw_version[3] is
always zero rather than firmware data, and ethtool -i reports a trailing
".0" that never came from the device.  Should the fourth byte be read from
its real source, or should the stored and displayed version consistently
use three components?

[ ... ]

> @@ -1341,8 +1341,8 @@ static const struct usb_device_id products[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, products);
>  
> -static struct usb_driver ax88179_178a_driver = {
> -	.name =		"ax88179_178a",
> +static struct usb_driver ax88179_driver = {
> +	.name =		"ax88179",
>  	.id_table =	products,
>  	.probe =	usbnet_probe,
>  	.suspend =	ax88179_suspend_wrapper,

[Severity: Critical]
This renames things that are visible to userspace, and I don't see
any compatibility shim for them.

usb_register() uses struct usb_driver .name verbatim for the driver
directory, so /sys/bus/usb/drivers/ax88179_178a becomes
/sys/bus/usb/drivers/ax88179 with this patch.  Anything that pokes
bind, unbind or new_id under the old path - udev rules, suspend/resume
helper scripts, test harnesses - silently stops finding the directory
after a kernel upgrade.

The module name changes too: the Makefile hunk in this patch keeps
ax88179-y := ax88179_178a.o ax88179a_devices.o ax88179_lib.o, so the
object is now ax88179.ko where it used to be ax88179_178a.ko.  Explicit
modprobe ax88179_178a invocations and any /etc/modprobe.d/ entries
(blacklist ax88179_178a, options ax88179_178a ...) no longer resolve.
Hotplug autoloading should still work since it goes through the
MODULE_DEVICE_TABLE(usb, products) modaliases, but the by-name cases
do not.

Would you consider adding MODULE_ALIAS("ax88179_178a") next to the
updated MODULE_DESCRIPTION so the old module name keeps resolving, or
is the view that the old name is young enough that nothing depends on
it?  Either way, could the changelog state explicitly that the usb
driver .name and the module name change, since right now it only
mentions "Update driver name in usb_driver" without noting the
user-visible consequences?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916-ax88179a-v12-0-60c04c9924a2%40birger-koblitz.de