netdev - Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20251017024229.1959295-1-yicongsrfy@163.com>
Date: Fri, 17 Oct 2025 10:42:29 +0800
From: yicongsrfy@....com
To: michal.pecio@...il.com
Cc: andrew+netdev@...n.ch,
	davem@...emloft.net,
	edumazet@...gle.com,
	kuba@...nel.org,
	linux-usb@...r.kernel.org,
	netdev@...r.kernel.org,
	oliver@...kum.org,
	pabeni@...hat.com
Subject: Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection

On Mon, 13 Oct 2025 11:07:53 +0200, Michal Pecio <michal.pecio@...il.com> wrote:
>
> On Sat, 11 Oct 2025 15:53:13 +0800, yicongsrfy@....com wrote:
> > From: Yi Cong <yicong@...inos.cn>
> >
> > A similar reason was raised in commit ec51fbd1b8a2 ("r8152: add USB
> > device driver for config selection"):
> > Linux prioritizes probing non-vendor-specific configurations.
> >
> > Referring to the implementation of this patch, cfgselect is also
> > used for ax88179 to override the default configuration selection.
> >
> > Signed-off-by: Yi Cong <yicong@...inos.cn>
> >
> > ---
> > v2: fix warning from checkpatch.
> > v5: 1. use KBUILD_MODNAME to obtain the module name.
> >     2. add error handling when usb_register fail.
> >     3. use .choose_configuration instead of .probe.
> >     4. reorder deregister logic.
> > ---
> >  drivers/net/usb/ax88179_178a.c | 68 ++++++++++++++++++++++++++++++++--
> >  1 file changed, 65 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> > index b034ef8a73ea..b6432d414a38 100644
> > --- a/drivers/net/usb/ax88179_178a.c
> > +++ b/drivers/net/usb/ax88179_178a.c
> > @@ -1713,6 +1713,14 @@ static int ax88179_stop(struct usbnet *dev)
> >  	return 0;
> >  }
> >
> > +static int ax88179_probe(struct usb_interface *intf, const struct usb_device_id *i)
> > +{
> > +	if (intf->cur_altsetting->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC)
> > +		return -ENODEV;
> > +
> > +	return usbnet_probe(intf, i);
> > +}
>
> This isn't part of the cfgselector driver being added by this commit
> nor is it documented in the changelog, so why is it here?

> It doesn't seem to be necessary, because USB_DEVICE_AND_INTERFACE_INFO
> matches used by this driver ensure that probe() will only be called on
> interfaces of the correct class 0xff.

I've retested this logic, and indeed, there's no need to add this extra check.
It's already planned to remove this modification in the next patch version.

> > +
> >  static const struct driver_info ax88179_info = {
> >  	.description = "ASIX AX88179 USB 3.0 Gigabit Ethernet",
> >  	.bind = ax88179_bind,
> > @@ -1941,9 +1949,9 @@ static const struct usb_device_id products[] = {
> >  MODULE_DEVICE_TABLE(usb, products);
> >
> >  static struct usb_driver ax88179_178a_driver = {
> > -	.name =		"ax88179_178a",
> > +	.name =		KBUILD_MODNAME,
> >  	.id_table =	products,
> > -	.probe =	usbnet_probe,
> > +	.probe =	ax88179_probe,
> >  	.suspend =	ax88179_suspend,
> >  	.resume =	ax88179_resume,
> >  	.reset_resume =	ax88179_resume,
> > @@ -1952,7 +1960,61 @@ static struct usb_driver ax88179_178a_driver = {
> >  	.disable_hub_initiated_lpm = 1,
> >  };
> >
> > -module_usb_driver(ax88179_178a_driver);
> > +static int ax88179_cfgselector_choose_configuration(struct usb_device *udev)
> > +{
> > +	struct usb_host_config *c;
> > +	int i, num_configs;
> > +
> > +	/* The vendor mode is not always config #1, so to find it out. */
> > +	c = udev->config;
> > +	num_configs = udev->descriptor.bNumConfigurations;
> > +	for (i = 0; i < num_configs; (i++, c++)) {
> > +		struct usb_interface_descriptor	*desc = NULL;
> > +
> > +		if (!c->desc.bNumInterfaces)
> > +			continue;
> > +		desc = &c->intf_cache[0]->altsetting->desc;
> > +		if (desc->bInterfaceClass == USB_CLASS_VENDOR_SPEC)
> > +			break;
> > +	}
> > +
> > +	if (i == num_configs)
> > +		return -ENODEV;
> > +
> > +	return c->desc.bConfigurationValue;
> > +}
>
> I wonder how many copies of this code would justify making it some
> sort of library in usbnet or usbcore?

Yes, there are many similar code instances in the USB subsystem.
However, I'm primarily focused on the networking subsystem,
so my abstraction work here might not be thorough enough.
Hopefully, an experienced USB developer may can optimize this issue.


> > +static struct usb_device_driver ax88179_cfgselector_driver = {
> > +	.name =	KBUILD_MODNAME "-cfgselector",
> > +	.choose_configuration =	ax88179_cfgselector_choose_configuration,
> > +	.id_table = products,
> > +	.generic_subclass = 1,
> > +	.supports_autosuspend = 1,
> > +};
> > +
> > +static int __init ax88179_driver_init(void)
> > +{
> > +	int ret;
> > +
> > +	ret = usb_register_device_driver(&ax88179_cfgselector_driver, THIS_MODULE);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = usb_register(&ax88179_178a_driver);
> > +	if (ret)
> > +		usb_deregister_device_driver(&ax88179_cfgselector_driver);
>
> Any problems if the order of registration is reversed, to ensure that
> the interface driver always exists if the device driver exists?

After swapping the registration order, testing confirms there are no problems.

> > +
> > +	return 0;
>
> return ret perhaps?

Will be fixed in the next version.