* [PATCH net v5 0/3] ax88179 driver optimization
@ 2025-10-11 7:53 yicongsrfy
2025-10-11 7:53 ` [PATCH net v5 1/3] net: usb: support quirks in cdc_ncm yicongsrfy
` (2 more replies)
0 siblings, 3 replies; 28+ messages in thread
From: yicongsrfy @ 2025-10-11 7:53 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, oliver
Cc: linux-usb, netdev, Yi Cong
From: Yi Cong <yicong@kylinos.cn>
This series of patches first fixes the issues related to the vendor
driver, then reverts the previous changes to allow the vendor-specific
driver to be loaded.
Yi Cong (3):
net: usb: support quirks in cdc_ncm
net: usb: ax88179_178a: add USB device driver for config selection
Revert "net: usb: ax88179_178a: Bind only to vendor-specific
interface"
drivers/net/usb/ax88179_178a.c | 94 ++++++++++++++++++++++++++++------
drivers/net/usb/cdc_ncm.c | 44 +++++++++++++++-
2 files changed, 121 insertions(+), 17 deletions(-)
Link to v4: https://lore.kernel.org/all/20250930080709.3408463-3-yicongsrfy@163.com/
Changes since v4:
1. Reorder the three patches.
2. Complete the revision based on the previous review comments.
--
2.25.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH net v5 1/3] net: usb: support quirks in cdc_ncm
2025-10-11 7:53 [PATCH net v5 0/3] ax88179 driver optimization yicongsrfy
@ 2025-10-11 7:53 ` yicongsrfy
2025-10-11 7:53 ` [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection yicongsrfy
2025-10-11 7:53 ` [PATCH net v5 3/3] Revert "net: usb: ax88179_178a: Bind only to vendor-specific interface" yicongsrfy
2 siblings, 0 replies; 28+ messages in thread
From: yicongsrfy @ 2025-10-11 7:53 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, oliver
Cc: linux-usb, netdev, Yi Cong
From: Yi Cong <yicong@kylinos.cn>
Some vendors' USB network interface controllers (NICs) may be compatible
with multiple drivers.
I consulted with relevant vendors. Taking the AX88179 chip as an example,
NICs based on this chip may be used across various OS—for instance,
cdc_ncm is used on macOS, while ax88179_178a.ko is the intended driver
on Linux (despite a previous patch having disabled it).
Therefore, the firmware must support multiple protocols.
Currently, both cdc_ncm and ax88179_178a coexist in the Linux kernel.
Supporting both drivers simultaneously leads to the following issues:
1. Inconsistent driver loading order during reboot stress testing:
The order in which drivers are loaded can vary across reboots,
potentially resulting in the unintended driver being loaded. For
example:
[ 4.239893] cdc_ncm 2-1:2.0: MAC-Address: c8:a3:62:ef:99:8e
[ 4.239897] cdc_ncm 2-1:2.0: setting rx_max = 16384
[ 4.240149] cdc_ncm 2-1:2.0: setting tx_max = 16384
[ 4.240583] cdc_ncm 2-1:2.0 usb0: register 'cdc_ncm' at usb-
xxxxx:00-1, CDC NCM, c8:a3:62:ef:99:8e
[ 4.240627] usbcore: registered new interface driver cdc_ncm
[ 4.240908] usbcore: registered new interface driver ax88179_178a
In this case, network connectivity functions, but the cdc_ncm driver is
loaded instead of the expected ax88179_178a.
2. Similar issues during cable plug/unplug testing:
The same race condition can occur when reconnecting the USB device:
[ 79.879922] usb 4-1: new SuperSpeed USB device number 3 using xhci_hcd
[ 79.905168] usb 4-1: New USB device found, idVendor=0b95, idProduct=
1790, bcdDevice= 2.00
[ 79.905185] usb 4-1: New USB device strings: Mfr=1, Product=2,
SerialNumber=3
[ 79.905191] usb 4-1: Product: AX88179B
[ 79.905198] usb 4-1: Manufacturer: ASIX
[ 79.905201] usb 4-1: SerialNumber: 00EF998E
[ 79.915215] ax88179_probe, bConfigurationValue:2
[ 79.952638] cdc_ncm 4-1:2.0: MAC-Address: c8:a3:62:ef:99:8e
[ 79.952654] cdc_ncm 4-1:2.0: setting rx_max = 16384
[ 79.952919] cdc_ncm 4-1:2.0: setting tx_max = 16384
[ 79.953598] cdc_ncm 4-1:2.0 eth0: register 'cdc_ncm' at usb-0000:04:
00.2-1, CDC NCM (NO ZLP), c8:a3:62:ef:99:8e
[ 79.954029] cdc_ncm 4-1:2.0 eth0: unregister 'cdc_ncm' usb-0000:04:
00.2-1, CDC NCM (NO ZLP)
At this point, the network becomes unusable.
To resolve these issues, introduce a *quirks* mechanism into the usbnet
module. By adding chip-specific identification within the generic usbnet
framework, we can skip the usbnet probe process for devices that require a
dedicated driver.
Signed-off-by: Yi Cong <yicong@kylinos.cn>
---
v2: Correct the description of usbnet_quirks.h and modify the code style
v3: Add checking whether the CONFIG_USB_NET_AX88179_178A is enabled
v4: Move quirks from usbnet.ko to cdc_ncm.ko
v5: Move the ignored product information to cdc_ncm.c and delete the
unlikely call in the if judgement
---
drivers/net/usb/cdc_ncm.c | 44 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 43 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 5d123df0a866..7b68706b8a8a 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -2114,10 +2114,52 @@ static const struct usb_device_id cdc_devs[] = {
};
MODULE_DEVICE_TABLE(usb, cdc_devs);
+/* cdc_ncm_ignore_list:
+ * Chip info which already support int vendor specific driver,
+ * and then should be ignored in generic cdc_ncm
+ */
+static const struct usb_device_id cdc_ncm_ignore_list[] = {
+#if IS_ENABLED(CONFIG_USB_NET_AX88179_178A)
+ /* Chips already support in ax88179_178a.c */
+ { USB_DEVICE(0x0b95, 0x1790) },
+ { USB_DEVICE(0x0b95, 0x178a) },
+ { USB_DEVICE(0x04b4, 0x3610) },
+ { USB_DEVICE(0x2001, 0x4a00) },
+ { USB_DEVICE(0x0df6, 0x0072) },
+ { USB_DEVICE(0x04e8, 0xa100) },
+ { USB_DEVICE(0x17ef, 0x304b) },
+ { USB_DEVICE(0x050d, 0x0128) },
+ { USB_DEVICE(0x0930, 0x0a13) },
+ { USB_DEVICE(0x0711, 0x0179) },
+ { USB_DEVICE(0x07c9, 0x000e) },
+ { USB_DEVICE(0x07c9, 0x000f) },
+ { USB_DEVICE(0x07c9, 0x0010) },
+ /* End of support in ax88179_178a.c */
+#endif
+
+ { } /*END*/
+};
+
+static inline bool cdc_ncm_ignore(struct usb_interface *intf)
+{
+ return !!usb_match_id(intf, cdc_ncm_ignore_list);
+}
+
+static int cdc_ncm_probe(struct usb_interface *intf, const struct usb_device_id *prod)
+{
+ /* Should it be ignored? */
+ if (cdc_ncm_ignore(intf)) {
+ dev_dbg(&intf->dev, "cdc_ncm ignore this device!\n");
+ return -ENODEV;
+ }
+
+ return usbnet_probe(intf, prod);
+}
+
static struct usb_driver cdc_ncm_driver = {
.name = "cdc_ncm",
.id_table = cdc_devs,
- .probe = usbnet_probe,
+ .probe = cdc_ncm_probe,
.disconnect = usbnet_disconnect,
.suspend = usbnet_suspend,
.resume = usbnet_resume,
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-11 7:53 [PATCH net v5 0/3] ax88179 driver optimization yicongsrfy
2025-10-11 7:53 ` [PATCH net v5 1/3] net: usb: support quirks in cdc_ncm yicongsrfy
@ 2025-10-11 7:53 ` yicongsrfy
2025-10-13 9:07 ` Michal Pecio
2025-10-11 7:53 ` [PATCH net v5 3/3] Revert "net: usb: ax88179_178a: Bind only to vendor-specific interface" yicongsrfy
2 siblings, 1 reply; 28+ messages in thread
From: yicongsrfy @ 2025-10-11 7:53 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, oliver
Cc: linux-usb, netdev, Yi Cong
From: Yi Cong <yicong@kylinos.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@kylinos.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);
+}
+
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;
+}
+
+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);
+
+ return 0;
+}
+
+static void __exit ax88179_driver_exit(void)
+{
+ usb_deregister_device_driver(&ax88179_cfgselector_driver);
+ usb_deregister(&ax88179_178a_driver);
+}
+
+module_init(ax88179_driver_init);
+module_exit(ax88179_driver_exit);
MODULE_DESCRIPTION("ASIX AX88179/178A based USB 3.0/2.0 Gigabit Ethernet Devices");
MODULE_LICENSE("GPL");
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH net v5 3/3] Revert "net: usb: ax88179_178a: Bind only to vendor-specific interface"
2025-10-11 7:53 [PATCH net v5 0/3] ax88179 driver optimization yicongsrfy
2025-10-11 7:53 ` [PATCH net v5 1/3] net: usb: support quirks in cdc_ncm yicongsrfy
2025-10-11 7:53 ` [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection yicongsrfy
@ 2025-10-11 7:53 ` yicongsrfy
2 siblings, 0 replies; 28+ messages in thread
From: yicongsrfy @ 2025-10-11 7:53 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, oliver
Cc: linux-usb, netdev, Yi Cong
From: Yi Cong <yicong@kylinos.cn>
This reverts commit c67cc4315a8e605ec875bd3a1210a549e3562ddc.
Currently, in the Linux kernel, USB NIC with ASIX chips use the cdc_ncm
driver. However, this driver lacks functionality and performs worse than
the vendor's proprietary driver. In my testing, I have identified the
following issues:
1. The cdc_ncm driver does not support changing the link speed via
ethtool because the corresponding callback function is set to NULL.
2. The CDC protocol does not support retrieving the network duplex status.
3. In TCP_RR and UDP_RR tests, the performance of the cdc_ncm driver
is significantly lower than that of the vendor's driver:
Average of three netperf runs: `netperf -t {TCP/UDP_RR} -H serverIP -l 120`
- cdc_ncm.ko: TCP_RR: 740, UDP_RR: 750
- ax88179_178a.ko: TCP_RR: 8900, UDP_RR: 9200
Signed-off-by: Yi Cong <yicong@kylinos.cn>
---
drivers/net/usb/ax88179_178a.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index b6432d414a38..ec46cab26815 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1893,55 +1893,55 @@ static const struct driver_info at_umc2000sp_info = {
static const struct usb_device_id products[] = {
{
/* ASIX AX88179 10/100/1000 */
- USB_DEVICE_AND_INTERFACE_INFO(0x0b95, 0x1790, 0xff, 0xff, 0),
+ USB_DEVICE(0x0b95, 0x1790),
.driver_info = (unsigned long)&ax88179_info,
}, {
/* ASIX AX88178A 10/100/1000 */
- USB_DEVICE_AND_INTERFACE_INFO(0x0b95, 0x178a, 0xff, 0xff, 0),
+ USB_DEVICE(0x0b95, 0x178a),
.driver_info = (unsigned long)&ax88178a_info,
}, {
/* Cypress GX3 SuperSpeed to Gigabit Ethernet Bridge Controller */
- USB_DEVICE_AND_INTERFACE_INFO(0x04b4, 0x3610, 0xff, 0xff, 0),
+ USB_DEVICE(0x04b4, 0x3610),
.driver_info = (unsigned long)&cypress_GX3_info,
}, {
/* D-Link DUB-1312 USB 3.0 to Gigabit Ethernet Adapter */
- USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x4a00, 0xff, 0xff, 0),
+ USB_DEVICE(0x2001, 0x4a00),
.driver_info = (unsigned long)&dlink_dub1312_info,
}, {
/* Sitecom USB 3.0 to Gigabit Adapter */
- USB_DEVICE_AND_INTERFACE_INFO(0x0df6, 0x0072, 0xff, 0xff, 0),
+ USB_DEVICE(0x0df6, 0x0072),
.driver_info = (unsigned long)&sitecom_info,
}, {
/* Samsung USB Ethernet Adapter */
- USB_DEVICE_AND_INTERFACE_INFO(0x04e8, 0xa100, 0xff, 0xff, 0),
+ USB_DEVICE(0x04e8, 0xa100),
.driver_info = (unsigned long)&samsung_info,
}, {
/* Lenovo OneLinkDock Gigabit LAN */
- USB_DEVICE_AND_INTERFACE_INFO(0x17ef, 0x304b, 0xff, 0xff, 0),
+ USB_DEVICE(0x17ef, 0x304b),
.driver_info = (unsigned long)&lenovo_info,
}, {
/* Belkin B2B128 USB 3.0 Hub + Gigabit Ethernet Adapter */
- USB_DEVICE_AND_INTERFACE_INFO(0x050d, 0x0128, 0xff, 0xff, 0),
+ USB_DEVICE(0x050d, 0x0128),
.driver_info = (unsigned long)&belkin_info,
}, {
/* Toshiba USB 3.0 GBit Ethernet Adapter */
- USB_DEVICE_AND_INTERFACE_INFO(0x0930, 0x0a13, 0xff, 0xff, 0),
+ USB_DEVICE(0x0930, 0x0a13),
.driver_info = (unsigned long)&toshiba_info,
}, {
/* Magic Control Technology U3-A9003 USB 3.0 Gigabit Ethernet Adapter */
- USB_DEVICE_AND_INTERFACE_INFO(0x0711, 0x0179, 0xff, 0xff, 0),
+ USB_DEVICE(0x0711, 0x0179),
.driver_info = (unsigned long)&mct_info,
}, {
/* Allied Telesis AT-UMC2000 USB 3.0/USB 3.1 Gen 1 to Gigabit Ethernet Adapter */
- USB_DEVICE_AND_INTERFACE_INFO(0x07c9, 0x000e, 0xff, 0xff, 0),
+ USB_DEVICE(0x07c9, 0x000e),
.driver_info = (unsigned long)&at_umc2000_info,
}, {
/* Allied Telesis AT-UMC200 USB 3.0/USB 3.1 Gen 1 to Fast Ethernet Adapter */
- USB_DEVICE_AND_INTERFACE_INFO(0x07c9, 0x000f, 0xff, 0xff, 0),
+ USB_DEVICE(0x07c9, 0x000f),
.driver_info = (unsigned long)&at_umc200_info,
}, {
/* Allied Telesis AT-UMC2000/SP USB 3.0/USB 3.1 Gen 1 to Gigabit Ethernet Adapter */
- USB_DEVICE_AND_INTERFACE_INFO(0x07c9, 0x0010, 0xff, 0xff, 0),
+ USB_DEVICE(0x07c9, 0x0010),
.driver_info = (unsigned long)&at_umc2000sp_info,
},
{ },
--
2.25.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-11 7:53 ` [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection yicongsrfy
@ 2025-10-13 9:07 ` Michal Pecio
2025-10-17 2:42 ` yicongsrfy
0 siblings, 1 reply; 28+ messages in thread
From: Michal Pecio @ 2025-10-13 9:07 UTC (permalink / raw)
To: yicongsrfy
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, oliver, linux-usb,
netdev, Yi Cong
On Sat, 11 Oct 2025 15:53:13 +0800, yicongsrfy@163.com wrote:
> From: Yi Cong <yicong@kylinos.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@kylinos.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.
> +
> 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?
> +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?
> +
> + return 0;
return ret perhaps?
> +}
> +
> +static void __exit ax88179_driver_exit(void)
> +{
> + usb_deregister_device_driver(&ax88179_cfgselector_driver);
> + usb_deregister(&ax88179_178a_driver);
> +}
> +
> +module_init(ax88179_driver_init);
> +module_exit(ax88179_driver_exit);
>
> MODULE_DESCRIPTION("ASIX AX88179/178A based USB 3.0/2.0 Gigabit Ethernet Devices");
> MODULE_LICENSE("GPL");
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-13 9:07 ` Michal Pecio
@ 2025-10-17 2:42 ` yicongsrfy
2025-10-17 13:10 ` Alan Stern
0 siblings, 1 reply; 28+ messages in thread
From: yicongsrfy @ 2025-10-17 2:42 UTC (permalink / raw)
To: michal.pecio
Cc: andrew+netdev, davem, edumazet, kuba, linux-usb, netdev, oliver,
pabeni
On Mon, 13 Oct 2025 11:07:53 +0200, Michal Pecio <michal.pecio@gmail.com> wrote:
>
> On Sat, 11 Oct 2025 15:53:13 +0800, yicongsrfy@163.com wrote:
> > From: Yi Cong <yicong@kylinos.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@kylinos.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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-17 2:42 ` yicongsrfy
@ 2025-10-17 13:10 ` Alan Stern
2025-10-17 17:15 ` Michal Pecio
0 siblings, 1 reply; 28+ messages in thread
From: Alan Stern @ 2025-10-17 13:10 UTC (permalink / raw)
To: yicongsrfy
Cc: michal.pecio, andrew+netdev, davem, edumazet, kuba, linux-usb,
netdev, oliver, pabeni
On Fri, Oct 17, 2025 at 10:42:29AM +0800, yicongsrfy@163.com wrote:
> > > + /* 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.
Would it help to have a USB quirks flag that tells the core to prefer
configurations with a USB_CLASS_VENDOR_SPEC interface class when we
choose the device's configuration? Or something similar to that (I'm
not sure exactly what you are looking for)?
Alan Stern
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-17 13:10 ` Alan Stern
@ 2025-10-17 17:15 ` Michal Pecio
2025-10-18 2:27 ` Alan Stern
2025-10-20 10:27 ` Oliver Neukum
0 siblings, 2 replies; 28+ messages in thread
From: Michal Pecio @ 2025-10-17 17:15 UTC (permalink / raw)
To: Alan Stern
Cc: yicongsrfy, andrew+netdev, davem, edumazet, kuba, linux-usb,
netdev, oliver, pabeni
On Fri, 17 Oct 2025 09:10:22 -0400, Alan Stern wrote:
> On Fri, Oct 17, 2025 at 10:42:29AM +0800, yicongsrfy@163.com wrote:
> > > > + /* 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.
>
> Would it help to have a USB quirks flag that tells the core to prefer
> configurations with a USB_CLASS_VENDOR_SPEC interface class when we
> choose the device's configuration? Or something similar to that (I'm
> not sure exactly what you are looking for)?
Either that or just patch usb_choose_configuration() to prefer vendor
config *if* an interface driver is available. But not 100% sure if that
couldn't backfire, so maybe only if the driver asked for it, indeed.
I was looking into making a PoC of that (I have r8152 with two configs)
but there is a snag: r8152-cfgselector bails out if a vendor-specific
check indicates the chip isn't a supported HW revision.
So to to fully replicate r8152-cfgselector, core would neet to get new
"pre_probe" callback in the interface driver. It gets complicated.
A less complicated improvement could be moving the common part of all
cfgselectors into usbnet. Not sure if it's worth it yet for only two?
Regards,
Michal
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-17 17:15 ` Michal Pecio
@ 2025-10-18 2:27 ` Alan Stern
2025-10-18 15:21 ` Michal Pecio
2025-10-20 10:27 ` Oliver Neukum
1 sibling, 1 reply; 28+ messages in thread
From: Alan Stern @ 2025-10-18 2:27 UTC (permalink / raw)
To: Michal Pecio
Cc: yicongsrfy, andrew+netdev, davem, edumazet, kuba, linux-usb,
netdev, oliver, pabeni
On Fri, Oct 17, 2025 at 07:15:11PM +0200, Michal Pecio wrote:
> On Fri, 17 Oct 2025 09:10:22 -0400, Alan Stern wrote:
> > On Fri, Oct 17, 2025 at 10:42:29AM +0800, yicongsrfy@163.com wrote:
> > > 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.
> >
> > Would it help to have a USB quirks flag that tells the core to prefer
> > configurations with a USB_CLASS_VENDOR_SPEC interface class when we
> > choose the device's configuration? Or something similar to that (I'm
> > not sure exactly what you are looking for)?
>
> Either that or just patch usb_choose_configuration() to prefer vendor
> config *if* an interface driver is available. But not 100% sure if that
> couldn't backfire, so maybe only if the driver asked for it, indeed.
>
> I was looking into making a PoC of that (I have r8152 with two configs)
> but there is a snag: r8152-cfgselector bails out if a vendor-specific
> check indicates the chip isn't a supported HW revision.
>
> So to to fully replicate r8152-cfgselector, core would neet to get new
> "pre_probe" callback in the interface driver. It gets complicated.
>
> A less complicated improvement could be moving the common part of all
> cfgselectors into usbnet. Not sure if it's worth it yet for only two?
Without a reasonable clear and quick criterion for deciding when to
favor vendor-specific configs in the USB core, there's little I can do.
Having a quirks flag should help remove some of the indecision, since
such flags are set by hand rather than by an automated procedure. But
I'd still want to have a better idea of exactly what to do when the
quirk flag is set.
Alan Stern
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-18 2:27 ` Alan Stern
@ 2025-10-18 15:21 ` Michal Pecio
2025-10-18 15:36 ` Alan Stern
2025-10-20 9:59 ` Oliver Neukum
0 siblings, 2 replies; 28+ messages in thread
From: Michal Pecio @ 2025-10-18 15:21 UTC (permalink / raw)
To: Alan Stern
Cc: yicongsrfy, andrew+netdev, davem, edumazet, kuba, linux-usb,
netdev, oliver, pabeni
On Fri, 17 Oct 2025 22:27:35 -0400, Alan Stern wrote:
> Without a reasonable clear and quick criterion for deciding when to
> favor vendor-specific configs in the USB core, there's little I can do.
> Having a quirks flag should help remove some of the indecision, since
> such flags are set by hand rather than by an automated procedure. But
> I'd still want to have a better idea of exactly what to do when the
> quirk flag is set.
Existing r8152-cfgselector and the planned ax88179-cfgselector
implement the following logic:
IF a device has particular IDs
(same id_table as in the vendor interface driver)
IF the vendor interface driver is loaded
(ensured by loading it together with cfgselector)
IF the vendor driver supports this device
(calls internal vendor driver code)
THEN select the vendor configuration
It was a PITA, but I have a working proof of concept for r8152.
Still missing is automatic reevaluation of configuration choice when
the vendor driver is loaded after device connection (e.g. by udev).
Those cfgselectors can do it because it seems that registering a new
device (but not interface) driver forces reevaluation.
---
drivers/net/usb/r8152.c | 13 ++++++-------
drivers/usb/core/driver.c | 23 +++++++++++++++++++++++
drivers/usb/core/generic.c | 17 +++++++++++++++--
include/linux/usb.h | 6 ++++++
4 files changed, 50 insertions(+), 9 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index a22d4bb2cf3b..1b016dd81949 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -10020,6 +10020,11 @@ static void rtl8152_disconnect(struct usb_interface *intf)
}
}
+static bool rtl8152_preferred(struct usb_device *udev)
+{
+ return __rtl_get_hw_ver(udev) != RTL_VER_UNKNOWN;
+}
+
/* table of devices that work with this driver */
static const struct usb_device_id rtl8152_table[] = {
/* Realtek */
@@ -10067,6 +10072,7 @@ static struct usb_driver rtl8152_driver = {
.name = MODULENAME,
.id_table = rtl8152_table,
.probe = rtl8152_probe,
+ .preferred = rtl8152_preferred,
.disconnect = rtl8152_disconnect,
.suspend = rtl8152_suspend,
.resume = rtl8152_resume,
@@ -10119,13 +10125,7 @@ static int __init rtl8152_driver_init(void)
{
int ret;
- ret = usb_register_device_driver(&rtl8152_cfgselector_driver, THIS_MODULE);
- if (ret)
- return ret;
-
ret = usb_register(&rtl8152_driver);
- if (ret)
- usb_deregister_device_driver(&rtl8152_cfgselector_driver);
return ret;
}
@@ -10133,7 +10133,6 @@ static int __init rtl8152_driver_init(void)
static void __exit rtl8152_driver_exit(void)
{
usb_deregister(&rtl8152_driver);
- usb_deregister_device_driver(&rtl8152_cfgselector_driver);
}
module_init(rtl8152_driver_init);
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index d29edc7c616a..eaf21c30eac1 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1119,6 +1119,29 @@ void usb_deregister(struct usb_driver *driver)
}
EXPORT_SYMBOL_GPL(usb_deregister);
+/**
+ * usb_driver_preferred - check if this is a preferred interface driver
+ * @drv: interface driver to check (device drivers are ignored)
+ * @udev: the device we are looking up a driver for
+ * Context: must be able to sleep
+ *
+ * TODO locking?
+ */
+bool usb_driver_preferred(struct device_driver *drv, struct usb_device *udev)
+{
+ struct usb_driver *usb_drv;
+
+ if (is_usb_device_driver(drv))
+ return false;
+
+ usb_drv = to_usb_driver(drv);
+
+ return usb_drv->preferred &&
+ usb_device_match_id(udev, usb_drv->id_table) &&
+ usb_drv->preferred(udev);
+}
+EXPORT_SYMBOL_GPL(usb_driver_preferred);
+
/* Forced unbinding of a USB interface driver, either because
* it doesn't support pre_reset/post_reset/reset_resume or
* because it doesn't support suspend/resume.
diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index a48994e11ef3..1923e6f4923b 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -49,11 +49,17 @@ static bool is_uac3_config(struct usb_interface_descriptor *desc)
return desc->bInterfaceProtocol == UAC_VERSION_3;
}
+static int prefer_vendor(struct device_driver *drv, void *data)
+{
+ return usb_driver_preferred(drv, data);
+}
+
int usb_choose_configuration(struct usb_device *udev)
{
int i;
int num_configs;
int insufficient_power = 0;
+ bool class_found = false;
struct usb_host_config *c, *best;
struct usb_device_driver *udriver;
@@ -169,6 +175,12 @@ int usb_choose_configuration(struct usb_device *udev)
#endif
}
+ /* Check if we have a preferred vendor driver for this config */
+ else if (bus_for_each_drv(&usb_bus_type, NULL, (void *) udev, prefer_vendor)) {
+ best = c;
+ break;
+ }
+
/* From the remaining configs, choose the first one whose
* first interface is for a non-vendor-specific class.
* Reason: Linux is more likely to have a class driver
@@ -177,8 +189,9 @@ int usb_choose_configuration(struct usb_device *udev)
USB_CLASS_VENDOR_SPEC &&
(desc && desc->bInterfaceClass !=
USB_CLASS_VENDOR_SPEC)) {
- best = c;
- break;
+ if (!class_found)
+ best = c;
+ class_found = true;
}
/* If all the remaining configs are vendor-specific,
diff --git a/include/linux/usb.h b/include/linux/usb.h
index e85105939af8..1d2c5ebc81ab 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1202,6 +1202,8 @@ extern ssize_t usb_show_dynids(struct usb_dynids *dynids, char *buf);
* @post_reset: Called by usb_reset_device() after the device
* has been reset
* @shutdown: Called at shut-down time to quiesce the device.
+ * @preferred: Check if this driver is preferred over generic class drivers
+ * applicable to the device. May probe device with control transfers.
* @id_table: USB drivers use ID table to support hotplugging.
* Export this with MODULE_DEVICE_TABLE(usb,...). This must be set
* or your driver's probe function will never get called.
@@ -1255,6 +1257,8 @@ struct usb_driver {
void (*shutdown)(struct usb_interface *intf);
+ bool (*preferred)(struct usb_device *udev);
+
const struct usb_device_id *id_table;
const struct attribute_group **dev_groups;
@@ -1267,6 +1271,8 @@ struct usb_driver {
};
#define to_usb_driver(d) container_of_const(d, struct usb_driver, driver)
+extern bool usb_driver_preferred(struct device_driver *drv, struct usb_device *udev);
+
/**
* struct usb_device_driver - identifies USB device driver to usbcore
* @name: The driver name should be unique among USB drivers,
--
2.48.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-18 15:21 ` Michal Pecio
@ 2025-10-18 15:36 ` Alan Stern
2025-10-18 15:56 ` Michal Pecio
2025-10-20 9:59 ` Oliver Neukum
1 sibling, 1 reply; 28+ messages in thread
From: Alan Stern @ 2025-10-18 15:36 UTC (permalink / raw)
To: Michal Pecio
Cc: yicongsrfy, andrew+netdev, davem, edumazet, kuba, linux-usb,
netdev, oliver, pabeni
On Sat, Oct 18, 2025 at 05:21:56PM +0200, Michal Pecio wrote:
> Existing r8152-cfgselector and the planned ax88179-cfgselector
> implement the following logic:
>
> IF a device has particular IDs
> (same id_table as in the vendor interface driver)
>
> IF the vendor interface driver is loaded
> (ensured by loading it together with cfgselector)
>
> IF the vendor driver supports this device
> (calls internal vendor driver code)
>
> THEN select the vendor configuration
>
>
> It was a PITA, but I have a working proof of concept for r8152.
>
> Still missing is automatic reevaluation of configuration choice when
> the vendor driver is loaded after device connection (e.g. by udev).
> Those cfgselectors can do it because it seems that registering a new
> device (but not interface) driver forces reevaluation.
It looks like something else is missing too...
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index d29edc7c616a..eaf21c30eac1 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1119,6 +1119,29 @@ void usb_deregister(struct usb_driver *driver)
> }
> EXPORT_SYMBOL_GPL(usb_deregister);
>
> +/**
> + * usb_driver_preferred - check if this is a preferred interface driver
> + * @drv: interface driver to check (device drivers are ignored)
> + * @udev: the device we are looking up a driver for
> + * Context: must be able to sleep
> + *
> + * TODO locking?
> + */
> +bool usb_driver_preferred(struct device_driver *drv, struct usb_device *udev)
> +{
> + struct usb_driver *usb_drv;
> +
> + if (is_usb_device_driver(drv))
> + return false;
> +
> + usb_drv = to_usb_driver(drv);
> +
> + return usb_drv->preferred &&
> + usb_device_match_id(udev, usb_drv->id_table) &&
> + usb_drv->preferred(udev);
> +}
> +EXPORT_SYMBOL_GPL(usb_driver_preferred);
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index a48994e11ef3..1923e6f4923b 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -49,11 +49,17 @@ static bool is_uac3_config(struct usb_interface_descriptor *desc)
> return desc->bInterfaceProtocol == UAC_VERSION_3;
> }
>
> +static int prefer_vendor(struct device_driver *drv, void *data)
> +{
> + return usb_driver_preferred(drv, data);
> +}
> +
> int usb_choose_configuration(struct usb_device *udev)
> {
> int i;
> int num_configs;
> int insufficient_power = 0;
> + bool class_found = false;
> struct usb_host_config *c, *best;
> struct usb_device_driver *udriver;
>
> @@ -169,6 +175,12 @@ int usb_choose_configuration(struct usb_device *udev)
> #endif
> }
>
> + /* Check if we have a preferred vendor driver for this config */
> + else if (bus_for_each_drv(&usb_bus_type, NULL, (void *) udev, prefer_vendor)) {
> + best = c;
> + break;
> + }
How are prefer_vendor() and usb_driver_preferred() supposed to know
which configuration is being considered?
(Also, is prefer_vendor() really needed? Can't you just pass
usb_driver_preferred as the argument to bus_for_each_drv()? Maybe after
changing the type of its second argument to void * instead of struct
usb_device *?)
Alan Stern
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-18 15:36 ` Alan Stern
@ 2025-10-18 15:56 ` Michal Pecio
2025-10-20 15:56 ` Alan Stern
0 siblings, 1 reply; 28+ messages in thread
From: Michal Pecio @ 2025-10-18 15:56 UTC (permalink / raw)
To: Alan Stern
Cc: yicongsrfy, andrew+netdev, davem, edumazet, kuba, linux-usb,
netdev, oliver, pabeni
On Sat, 18 Oct 2025 11:36:11 -0400, Alan Stern wrote:
> > @@ -169,6 +175,12 @@ int usb_choose_configuration(struct usb_device *udev)
> > #endif
> > }
> >
> > + /* Check if we have a preferred vendor driver for this config */
> > + else if (bus_for_each_drv(&usb_bus_type, NULL, (void *) udev, prefer_vendor)) {
> > + best = c;
> > + break;
> > + }
>
> How are prefer_vendor() and usb_driver_preferred() supposed to know
> which configuration is being considered?
Currently they don't need to know, but this could be added by passing
a temporary struct with more stuff in place of udev.
Really, this whole usb_drv->preferred business could be a simple
boolean flag, if not for r8152 needing to issue control transfers to
the chip to find whether it supports at all.
It seems that ax88179_preferred() could simply always return true.
> (Also, is prefer_vendor() really needed? Can't you just pass
> usb_driver_preferred as the argument to bus_for_each_drv()? Maybe after
> changing the type of its second argument to void * instead of struct
> usb_device *?)
I didn't like the idea of usb_driver_preferred() taking void *.
However, I could cast it to a function taking void * here in generic.c.
The wrapper would become truly necessary if we wanted to pass
config / intf / other stuff to usb_driver_preferred().
Regards,
Michal
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-18 15:21 ` Michal Pecio
2025-10-18 15:36 ` Alan Stern
@ 2025-10-20 9:59 ` Oliver Neukum
2025-10-20 10:48 ` Greg KH
2025-10-20 15:59 ` Michal Pecio
1 sibling, 2 replies; 28+ messages in thread
From: Oliver Neukum @ 2025-10-20 9:59 UTC (permalink / raw)
To: Michal Pecio, Alan Stern
Cc: yicongsrfy, andrew+netdev, davem, edumazet, kuba, linux-usb,
netdev, oliver, pabeni
On 18.10.25 17:21, Michal Pecio wrote:
> index e85105939af8..1d2c5ebc81ab 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1202,6 +1202,8 @@ extern ssize_t usb_show_dynids(struct usb_dynids *dynids, char *buf);
> * @post_reset: Called by usb_reset_device() after the device
> * has been reset
> * @shutdown: Called at shut-down time to quiesce the device.
> + * @preferred: Check if this driver is preferred over generic class drivers
> + * applicable to the device. May probe device with control transfers.
> * @id_table: USB drivers use ID table to support hotplugging.
> * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set
> * or your driver's probe function will never get called.
> @@ -1255,6 +1257,8 @@ struct usb_driver {
>
> void (*shutdown)(struct usb_interface *intf);
>
> + bool (*preferred)(struct usb_device *udev);
I am sorry, but this is a bit clunky. If you really want to
introduce such a method, why not just return the preferred configuration?
Regards
Oliver
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-17 17:15 ` Michal Pecio
2025-10-18 2:27 ` Alan Stern
@ 2025-10-20 10:27 ` Oliver Neukum
1 sibling, 0 replies; 28+ messages in thread
From: Oliver Neukum @ 2025-10-20 10:27 UTC (permalink / raw)
To: Michal Pecio, Alan Stern
Cc: yicongsrfy, andrew+netdev, davem, edumazet, kuba, linux-usb,
netdev, oliver, pabeni
On 17.10.25 19:15, Michal Pecio wrote:
> A less complicated improvement could be moving the common part of all
> cfgselectors into usbnet. Not sure if it's worth it yet for only two?
Please leave usbnet out of this. The selection of a configuration
belongs into core code.
Regards
Oliver
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-20 9:59 ` Oliver Neukum
@ 2025-10-20 10:48 ` Greg KH
2025-10-20 15:59 ` Michal Pecio
1 sibling, 0 replies; 28+ messages in thread
From: Greg KH @ 2025-10-20 10:48 UTC (permalink / raw)
To: Oliver Neukum, Michal Pecio, Alan Stern, yicongsrfy,
andrew+netdev, davem, edumazet, kuba, linux-usb, netdev, oliver,
pabeni
On Mon, Oct 20, 2025 at 11:59:06AM +0200, Oliver Neukum wrote:
> On 18.10.25 17:21, Michal Pecio wrote:
>
> > index e85105939af8..1d2c5ebc81ab 100644
> > --- a/include/linux/usb.h
> > +++ b/include/linux/usb.h
> > @@ -1202,6 +1202,8 @@ extern ssize_t usb_show_dynids(struct usb_dynids *dynids, char *buf);
> > * @post_reset: Called by usb_reset_device() after the device
> > * has been reset
> > * @shutdown: Called at shut-down time to quiesce the device.
> > + * @preferred: Check if this driver is preferred over generic class drivers
> > + * applicable to the device. May probe device with control transfers.
> > * @id_table: USB drivers use ID table to support hotplugging.
> > * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set
> > * or your driver's probe function will never get called.
> > @@ -1255,6 +1257,8 @@ struct usb_driver {
> > void (*shutdown)(struct usb_interface *intf);
> > + bool (*preferred)(struct usb_device *udev);
>
> I am sorry, but this is a bit clunky. If you really want to
> introduce such a method, why not just return the preferred configuration?
And note, this idea has come up many many times over the past 25 years,
ever since we first added USB support to Linux. In the end, it was
always deemed "not going to work" for a variety of real-world reasons.
I suggest reviewing the archives of the mailing list and then, if this
series is resent, documenting why this attempt is different than the
others and why it will now work properly.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-18 15:56 ` Michal Pecio
@ 2025-10-20 15:56 ` Alan Stern
2025-10-20 16:23 ` Michal Pecio
2025-10-21 2:29 ` Yi Cong
0 siblings, 2 replies; 28+ messages in thread
From: Alan Stern @ 2025-10-20 15:56 UTC (permalink / raw)
To: Michal Pecio
Cc: yicongsrfy, andrew+netdev, davem, edumazet, kuba, linux-usb,
netdev, oliver, pabeni
On Sat, Oct 18, 2025 at 05:56:18PM +0200, Michal Pecio wrote:
> On Sat, 18 Oct 2025 11:36:11 -0400, Alan Stern wrote:
> > > @@ -169,6 +175,12 @@ int usb_choose_configuration(struct usb_device *udev)
> > > #endif
> > > }
> > >
> > > + /* Check if we have a preferred vendor driver for this config */
> > > + else if (bus_for_each_drv(&usb_bus_type, NULL, (void *) udev, prefer_vendor)) {
> > > + best = c;
> > > + break;
> > > + }
> >
> > How are prefer_vendor() and usb_driver_preferred() supposed to know
> > which configuration is being considered?
>
> Currently they don't need to know, but this could be added by passing
> a temporary struct with more stuff in place of udev.
>
> Really, this whole usb_drv->preferred business could be a simple
> boolean flag, if not for r8152 needing to issue control transfers to
> the chip to find whether it supports at all.
>
> It seems that ax88179_preferred() could simply always return true.
Instead of all this preferred() stuff, why not have the ax88179 driver's
probe routine check for a different configuration with a vendor-specific
interface? If that other config is present and the chip is the right
type then you can call usb_driver_set_configuration() -- this is exactly
what it's meant for.
Especially with that extra requirement for the chip being the right
kind, this doesn't seem like something the USB core should have to
handle. If other USB networking drivers need to do similar things,
maybe the common part could be put in a library in the usbnet core.
Alan Stern
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-20 9:59 ` Oliver Neukum
2025-10-20 10:48 ` Greg KH
@ 2025-10-20 15:59 ` Michal Pecio
2025-10-21 9:02 ` Oliver Neukum
1 sibling, 1 reply; 28+ messages in thread
From: Michal Pecio @ 2025-10-20 15:59 UTC (permalink / raw)
To: Oliver Neukum
Cc: Alan Stern, yicongsrfy, andrew+netdev, davem, edumazet, kuba,
linux-usb, netdev, oliver, pabeni
On Mon, 20 Oct 2025 11:59:06 +0200, Oliver Neukum wrote:
> On 18.10.25 17:21, Michal Pecio wrote:
>
> > index e85105939af8..1d2c5ebc81ab 100644
> > --- a/include/linux/usb.h
> > +++ b/include/linux/usb.h
> > @@ -1202,6 +1202,8 @@ extern ssize_t usb_show_dynids(struct usb_dynids *dynids, char *buf);
> > * @post_reset: Called by usb_reset_device() after the device
> > * has been reset
> > * @shutdown: Called at shut-down time to quiesce the device.
> > + * @preferred: Check if this driver is preferred over generic class drivers
> > + * applicable to the device. May probe device with control transfers.
> > * @id_table: USB drivers use ID table to support hotplugging.
> > * Export this with MODULE_DEVICE_TABLE(usb,...). This must be set
> > * or your driver's probe function will never get called.
> > @@ -1255,6 +1257,8 @@ struct usb_driver {
> >
> > void (*shutdown)(struct usb_interface *intf);
> >
> > + bool (*preferred)(struct usb_device *udev);
>
> I am sorry, but this is a bit clunky. If you really want to
> introduce such a method, why not just return the preferred
> configuration?
Because I wanted to introduce exactly such a method, rather than one
which returns the configuration ;)
The point was to pull configuration selection *out* of those drivers.
They already do it, and it makes them copy-paste the same trivial loop
which iterates through configs until it finds the vendor interface.
The idea is to have a maximally simple check for a known-good vendor
interface driver before making unfounded assumptions like:
/* From the remaining configs, choose the first one whose
* first interface is for a non-vendor-specific class.
* Reason: Linux is more likely to have a class driver
* than a vendor-specific driver. */
Unfortunately, that's only half the battle. The other half is forcing
configuration reevaluation when such a driver is loaded. I hoped it
would be trivial, but so far it costs me a new bus_for_each_device()
and a whole nontrivial function, while cfgselectors have it for free.
I got my PoC up to feature parity with r8152-cfgselector and it adds
about as much code as it removes (uless more cfgselectors are added).
And of course it's dead weight for those with USB but not USBNET.
drivers/net/usb/r8152.c | 69 ++++++++-------------------------------------------------------------
drivers/usb/core/driver.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/usb/core/generic.c | 26 ++++++++++++++++----------
include/linux/usb.h | 11 ++++++-----
4 files changed, 78 insertions(+), 76 deletions(-)
So not sure if it's worth pursuing.
Regards,
Michal
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-20 15:56 ` Alan Stern
@ 2025-10-20 16:23 ` Michal Pecio
2025-10-20 16:59 ` Alan Stern
2025-10-21 2:29 ` Yi Cong
1 sibling, 1 reply; 28+ messages in thread
From: Michal Pecio @ 2025-10-20 16:23 UTC (permalink / raw)
To: Alan Stern
Cc: yicongsrfy, andrew+netdev, davem, edumazet, kuba, linux-usb,
netdev, oliver, pabeni
On Mon, 20 Oct 2025 11:56:50 -0400, Alan Stern wrote:
> On Sat, Oct 18, 2025 at 05:56:18PM +0200, Michal Pecio wrote:
> > On Sat, 18 Oct 2025 11:36:11 -0400, Alan Stern wrote:
> > > How are prefer_vendor() and usb_driver_preferred() supposed to know
> > > which configuration is being considered?
> >
> > Currently they don't need to know, but this could be added by passing
> > a temporary struct with more stuff in place of udev.
> >
> > Really, this whole usb_drv->preferred business could be a simple
> > boolean flag, if not for r8152 needing to issue control transfers to
> > the chip to find whether it supports at all.
> >
> > It seems that ax88179_preferred() could simply always return true.
>
> Instead of all this preferred() stuff, why not have the ax88179 driver's
> probe routine check for a different configuration with a vendor-specific
> interface? If that other config is present and the chip is the right
> type then you can call usb_driver_set_configuration() -- this is exactly
> what it's meant for.
That could be doable and some code could be shared I guess, but how to
get the probe() routine to run in the first place?
The chip may be in other configuration, without this vendor interface.
If we remove _AND_INTERFACE_INFO, it's still a problem that cdc_ether
may already be bound to the CDC interface in CDC config.
Registering a *device* driver plows through such obstacles, because
core allows device drivers to immediately displace existing drivers.
It seems that this could work, if cdc_ether blacklisting and revert
of _AND_INTERFACE_INFO are applied as suggested in this series.
(But as part of the main commit, to avoid transient regressions).
I wonder if blacklisting is considered necessary evil? Without it, it's
possible that cdc_ether binds for a moment before it's kicked out by
the vendor driver. Looks weird in dmesg, at the very least.
FWIW, my RTL8153 is blacklisted in cdc_ether too. So much for the
promise that cfgselectors will allow users to choose drivers ;)
Regards,
Michal
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-20 16:23 ` Michal Pecio
@ 2025-10-20 16:59 ` Alan Stern
2025-10-21 9:13 ` Oliver Neukum
0 siblings, 1 reply; 28+ messages in thread
From: Alan Stern @ 2025-10-20 16:59 UTC (permalink / raw)
To: Michal Pecio
Cc: yicongsrfy, andrew+netdev, davem, edumazet, kuba, linux-usb,
netdev, oliver, pabeni
On Mon, Oct 20, 2025 at 06:23:27PM +0200, Michal Pecio wrote:
> On Mon, 20 Oct 2025 11:56:50 -0400, Alan Stern wrote:
> > Instead of all this preferred() stuff, why not have the ax88179 driver's
> > probe routine check for a different configuration with a vendor-specific
> > interface? If that other config is present and the chip is the right
> > type then you can call usb_driver_set_configuration() -- this is exactly
> > what it's meant for.
>
> That could be doable and some code could be shared I guess, but how to
> get the probe() routine to run in the first place?
>
> The chip may be in other configuration, without this vendor interface.
> If we remove _AND_INTERFACE_INFO, it's still a problem that cdc_ether
> may already be bound to the CDC interface in CDC config.
>
> Registering a *device* driver plows through such obstacles, because
> core allows device drivers to immediately displace existing drivers.
>
>
> It seems that this could work, if cdc_ether blacklisting and revert
> of _AND_INTERFACE_INFO are applied as suggested in this series.
> (But as part of the main commit, to avoid transient regressions).
>
> I wonder if blacklisting is considered necessary evil? Without it, it's
> possible that cdc_ether binds for a moment before it's kicked out by
> the vendor driver. Looks weird in dmesg, at the very least.
>
> FWIW, my RTL8153 is blacklisted in cdc_ether too. So much for the
> promise that cfgselectors will allow users to choose drivers ;)
Another possibility is simply to give up on handling all of this
automatically in the kernel. The usb_modeswitch program certainly
should be capable of determining when a USB network device ought to
switch to a different configuration; that's very similar to the things
it does already. Maybe userspace is the best place to implement this
stuff.
Furthermore, with usb_modeswitch it's not at all uncommon to have some
drivers bind momentarily before being kicked off. People don't care
about it very much, as long it all happens reliably and automatically.
Alan Stern
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-20 15:56 ` Alan Stern
2025-10-20 16:23 ` Michal Pecio
@ 2025-10-21 2:29 ` Yi Cong
2025-10-21 2:59 ` Alan Stern
1 sibling, 1 reply; 28+ messages in thread
From: Yi Cong @ 2025-10-21 2:29 UTC (permalink / raw)
To: stern
Cc: andrew+netdev, davem, edumazet, kuba, linux-usb, michal.pecio,
netdev, oliver, pabeni
On Mon, 20 Oct 2025 11:56:50 -0400, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Sat, Oct 18, 2025 at 05:56:18PM +0200, Michal Pecio wrote:
> > On Sat, 18 Oct 2025 11:36:11 -0400, Alan Stern wrote:
> > > > @@ -169,6 +175,12 @@ int usb_choose_configuration(struct usb_device *udev)
> > > > #endif
> > > > }
> > > >
> > > > + /* Check if we have a preferred vendor driver for this config */
> > > > + else if (bus_for_each_drv(&usb_bus_type, NULL, (void *) udev, prefer_vendor)) {
> > > > + best = c;
> > > > + break;
> > > > + }
> > >
> > > How are prefer_vendor() and usb_driver_preferred() supposed to know
> > > which configuration is being considered?
> >
> > Currently they don't need to know, but this could be added by passing
> > a temporary struct with more stuff in place of udev.
> >
> > Really, this whole usb_drv->preferred business could be a simple
> > boolean flag, if not for r8152 needing to issue control transfers to
> > the chip to find whether it supports at all.
> >
> > It seems that ax88179_preferred() could simply always return true.
>
> Instead of all this preferred() stuff, why not have the ax88179 driver's
> probe routine check for a different configuration with a vendor-specific
> interface? If that other config is present and the chip is the right
> type then you can call usb_driver_set_configuration() -- this is exactly
> what it's meant for.
I tried calling usb_driver_set_configuration inside driver's probe()
to select the configuration, but my USB network card has three
configurations (bNumConfigurations=3), which causes usb_driver_set_configuration
to be called twice within probe():
```
static int ax88179_probe()
{
if (bConfigurationValue != I_WANT) {
usb_driver_set_configuration(udev, I_WANT)
return -ENODEV;
}
//else really probe
}
```
Although the final result is correct, this approach seems flawed.
This issue does not occur when using choose_configuration.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-21 2:29 ` Yi Cong
@ 2025-10-21 2:59 ` Alan Stern
2025-10-21 6:26 ` Yi Cong
0 siblings, 1 reply; 28+ messages in thread
From: Alan Stern @ 2025-10-21 2:59 UTC (permalink / raw)
To: Yi Cong
Cc: andrew+netdev, davem, edumazet, kuba, linux-usb, michal.pecio,
netdev, oliver, pabeni
On Tue, Oct 21, 2025 at 10:29:25AM +0800, Yi Cong wrote:
> On Mon, 20 Oct 2025 11:56:50 -0400, Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > Instead of all this preferred() stuff, why not have the ax88179 driver's
> > probe routine check for a different configuration with a vendor-specific
> > interface? If that other config is present and the chip is the right
> > type then you can call usb_driver_set_configuration() -- this is exactly
> > what it's meant for.
>
> I tried calling usb_driver_set_configuration inside driver's probe()
> to select the configuration, but my USB network card has three
> configurations (bNumConfigurations=3), which causes usb_driver_set_configuration
> to be called twice within probe():
> ```
> static int ax88179_probe()
> {
> if (bConfigurationValue != I_WANT) {
> usb_driver_set_configuration(udev, I_WANT)
> return -ENODEV;
> }
> //else really probe
> }
> ```
Why is it called twice? The first time probe() runs, it calls
usb_driver_set_configuration() with the config that you want. Then the
second time probe() runs, the config you want has been installed, so
there's no reason to call usb_driver_set_configuration() again.
Unless something is going wrong, that's how it should work. And the
total number of configurations should not matter.
> Although the final result is correct, this approach seems flawed.
This kind of approach is currently being used by usb_modeswitch for a
bunch of devices. The only difference being that the choice of which
config to use is made in userspace, not by a kernel driver.
> This issue does not occur when using choose_configuration.
True, but the discussion in this thread seems to indicate that telling
choose_configuration what to do for some of these network devices would
be extremely awkward.
Alan Stern
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-21 2:59 ` Alan Stern
@ 2025-10-21 6:26 ` Yi Cong
2025-10-21 16:26 ` Alan Stern
0 siblings, 1 reply; 28+ messages in thread
From: Yi Cong @ 2025-10-21 6:26 UTC (permalink / raw)
To: stern
Cc: andrew+netdev, davem, edumazet, kuba, linux-usb, michal.pecio,
netdev, oliver, pabeni
On Mon, 20 Oct 2025 22:59:36 -0400, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Tue, Oct 21, 2025 at 10:29:25AM +0800, Yi Cong wrote:
> > On Mon, 20 Oct 2025 11:56:50 -0400, Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > Instead of all this preferred() stuff, why not have the ax88179 driver's
> > > probe routine check for a different configuration with a vendor-specific
> > > interface? If that other config is present and the chip is the right
> > > type then you can call usb_driver_set_configuration() -- this is exactly
> > > what it's meant for.
> >
> > I tried calling usb_driver_set_configuration inside driver's probe()
> > to select the configuration, but my USB network card has three
> > configurations (bNumConfigurations=3), which causes usb_driver_set_configuration
> > to be called twice within probe():
> > ```
> > static int ax88179_probe()
> > {
> > if (bConfigurationValue != I_WANT) {
> > usb_driver_set_configuration(udev, I_WANT)
> > return -ENODEV;
> > }
> > //else really probe
> > }
> > ```
>
> Why is it called twice? The first time probe() runs, it calls
> usb_driver_set_configuration() with the config that you want. Then the
> second time probe() runs, the config you want has been installed, so
> there's no reason to call usb_driver_set_configuration() again.
>
> Unless something is going wrong, that's how it should work. And the
> total number of configurations should not matter.
It might not be caused by the number of configurations, but rather by
the fact that usb_driver_set_configuration handles configuration changes
through a work queue.
I suspect this is the reason (although I haven't verified it further
—I just observed this behavior and switched to a different implementation
by using choose_configuration):
```
int usb_driver_set_configuration(struct usb_device *udev, int config)
{
...
INIT_WORK(&req->work, driver_set_config_work);
schedule_work(&req->work);
return 0;
}
```
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-20 15:59 ` Michal Pecio
@ 2025-10-21 9:02 ` Oliver Neukum
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Neukum @ 2025-10-21 9:02 UTC (permalink / raw)
To: Michal Pecio, Oliver Neukum
Cc: Alan Stern, yicongsrfy, andrew+netdev, davem, edumazet, kuba,
linux-usb, netdev, pabeni
On 20.10.25 17:59, Michal Pecio wrote:
> On Mon, 20 Oct 2025 11:59:06 +0200, Oliver Neukum wrote:
>> On 18.10.25 17:21, Michal Pecio wrote:
>>> @@ -1255,6 +1257,8 @@ struct usb_driver {
>>>
>>> void (*shutdown)(struct usb_interface *intf);
>>>
>>> + bool (*preferred)(struct usb_device *udev);
>>
>> I am sorry, but this is a bit clunky. If you really want to
>> introduce such a method, why not just return the preferred
>> configuration?
>
> Because I wanted to introduce exactly such a method, rather than one
> which returns the configuration ;)
Well, then I have to state that your patch perfectly implements
your wish. >:->
Would you allow me a follow up question, though? Why have you
developed that wish?
> The point was to pull configuration selection *out* of those drivers.
While I appreciate the goal, it is not clear to me how adding
a method to the generic interface driver template achieves that goal.
In fact this approach seems counterproductive.
In particular a bool will not work for the generic case.
If you really want to make this generic, you'll have to face
the unfortunate possibility that a configuration have
multiple interfaces whose drivers disagree in that regard.
At a minimum you'd have to be able to return a "don't care"
value to compute a reasonable pick.
> They already do it, and it makes them copy-paste the same trivial loop
> which iterates through configs until it finds the vendor interface.
If the concern is simply getting the code centralized (which
is not wrong), then Alan's original proposal of having a flag
(let's not call it a quirk) in usbcore for devices that need
the logic in the heuristic for picking a configuration to be
inverted would seem to be the simplest approach.
> The idea is to have a maximally simple check for a known-good vendor
> interface driver before making unfounded assumptions like:
>
> /* From the remaining configs, choose the first one whose
> * first interface is for a non-vendor-specific class.
> * Reason: Linux is more likely to have a class driver
> * than a vendor-specific driver. */
>
> Unfortunately, that's only half the battle. The other half is forcing
> configuration reevaluation when such a driver is loaded. I hoped it
Exactly. Hence don't put the information that the assumption
must not be made into a driver but into usbcore. Problem avoided.
It looks like this is an issue we are not going to find a perfect
solution for. Hence our priority should be finding the simplest
change. IMHO that's a new quirk just inverting existing logic.
Sure, it is a bit ugly because it depends on the kernel configuration,
but that is what we have a preprocessor for.
Regards
Oliver
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-20 16:59 ` Alan Stern
@ 2025-10-21 9:13 ` Oliver Neukum
2025-10-21 16:33 ` Alan Stern
0 siblings, 1 reply; 28+ messages in thread
From: Oliver Neukum @ 2025-10-21 9:13 UTC (permalink / raw)
To: Alan Stern, Michal Pecio
Cc: yicongsrfy, andrew+netdev, davem, edumazet, kuba, linux-usb,
netdev, pabeni
On 20.10.25 18:59, Alan Stern wrote:
> Another possibility is simply to give up on handling all of this
> automatically in the kernel. The usb_modeswitch program certainly
> should be capable of determining when a USB network device ought to
> switch to a different configuration; that's very similar to the things
> it does already. Maybe userspace is the best place to implement this
> stuff.
That would make usb_modeswitch or yet a new udev component mandatory.
That is the exact opposite of what we would like to achieve.
> Furthermore, with usb_modeswitch it's not at all uncommon to have some
> drivers bind momentarily before being kicked off. People don't care
> about it very much, as long it all happens reliably and automatically.
That is probably not wise in the long run. If the device whose driver
we kick off is a CD-ROM, nobody cares. If it is a network interface,
we'll have to deal with ugly cases like user space already having
sent a DHCP query when we kick the old driver off the interface.
Regards
Oliver
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-21 6:26 ` Yi Cong
@ 2025-10-21 16:26 ` Alan Stern
0 siblings, 0 replies; 28+ messages in thread
From: Alan Stern @ 2025-10-21 16:26 UTC (permalink / raw)
To: Yi Cong
Cc: andrew+netdev, davem, edumazet, kuba, linux-usb, michal.pecio,
netdev, oliver, pabeni
On Tue, Oct 21, 2025 at 02:26:29PM +0800, Yi Cong wrote:
> On Mon, 20 Oct 2025 22:59:36 -0400, Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Tue, Oct 21, 2025 at 10:29:25AM +0800, Yi Cong wrote:
> > > On Mon, 20 Oct 2025 11:56:50 -0400, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > >
> > > > Instead of all this preferred() stuff, why not have the ax88179 driver's
> > > > probe routine check for a different configuration with a vendor-specific
> > > > interface? If that other config is present and the chip is the right
> > > > type then you can call usb_driver_set_configuration() -- this is exactly
> > > > what it's meant for.
> > >
> > > I tried calling usb_driver_set_configuration inside driver's probe()
> > > to select the configuration, but my USB network card has three
> > > configurations (bNumConfigurations=3), which causes usb_driver_set_configuration
> > > to be called twice within probe():
> > > ```
> > > static int ax88179_probe()
> > > {
> > > if (bConfigurationValue != I_WANT) {
> > > usb_driver_set_configuration(udev, I_WANT)
> > > return -ENODEV;
> > > }
> > > //else really probe
> > > }
> > > ```
> >
> > Why is it called twice? The first time probe() runs, it calls
> > usb_driver_set_configuration() with the config that you want. Then the
> > second time probe() runs, the config you want has been installed, so
> > there's no reason to call usb_driver_set_configuration() again.
> >
> > Unless something is going wrong, that's how it should work. And the
> > total number of configurations should not matter.
>
> It might not be caused by the number of configurations, but rather by
> the fact that usb_driver_set_configuration handles configuration changes
> through a work queue.
I don't see why using a work queue should make any difference. If you
really want to find out what's going on, you could add a dump_stack()
call to the probe routine. And maybe a printk at the point where it
calls usb_driver_set_configuration().
Alan Stern
> I suspect this is the reason (although I haven't verified it further
> —I just observed this behavior and switched to a different implementation
> by using choose_configuration):
> ```
> int usb_driver_set_configuration(struct usb_device *udev, int config)
> {
> ...
> INIT_WORK(&req->work, driver_set_config_work);
> schedule_work(&req->work);
> return 0;
> }
> ```
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-21 9:13 ` Oliver Neukum
@ 2025-10-21 16:33 ` Alan Stern
2025-10-22 7:58 ` Oliver Neukum
0 siblings, 1 reply; 28+ messages in thread
From: Alan Stern @ 2025-10-21 16:33 UTC (permalink / raw)
To: Oliver Neukum
Cc: Michal Pecio, yicongsrfy, andrew+netdev, davem, edumazet, kuba,
linux-usb, netdev, pabeni
On Tue, Oct 21, 2025 at 11:13:29AM +0200, Oliver Neukum wrote:
> On 20.10.25 18:59, Alan Stern wrote:
>
> > Another possibility is simply to give up on handling all of this
> > automatically in the kernel. The usb_modeswitch program certainly
> > should be capable of determining when a USB network device ought to
> > switch to a different configuration; that's very similar to the things
> > it does already. Maybe userspace is the best place to implement this
> > stuff.
>
> That would make usb_modeswitch or yet a new udev component mandatory.
> That is the exact opposite of what we would like to achieve.
In the same way that usb_modeswitch or a udev script is already
mandatory for a bunch of other devices?
In this case, it wouldn't be quite as mandatory since the user can
always change the configuration by hand. Some of the things
usb_modeswitch does probably aren't so easy to accomplish.
I agree, it would be great if the kernel could handle all these things
for people. But sometimes it's just a lot easier to do stuff in
userspace.
> > Furthermore, with usb_modeswitch it's not at all uncommon to have some
> > drivers bind momentarily before being kicked off. People don't care
> > about it very much, as long it all happens reliably and automatically.
>
> That is probably not wise in the long run. If the device whose driver
> we kick off is a CD-ROM, nobody cares. If it is a network interface,
> we'll have to deal with ugly cases like user space already having
> sent a DHCP query when we kick the old driver off the interface.
Doesn't the same concern apply every time a network interface goes down?
Alan Stern
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-21 16:33 ` Alan Stern
@ 2025-10-22 7:58 ` Oliver Neukum
2025-10-22 14:28 ` Alan Stern
0 siblings, 1 reply; 28+ messages in thread
From: Oliver Neukum @ 2025-10-22 7:58 UTC (permalink / raw)
To: Alan Stern
Cc: Michal Pecio, yicongsrfy, andrew+netdev, davem, edumazet, kuba,
linux-usb, netdev, pabeni
On 21.10.25 18:33, Alan Stern wrote:
> On Tue, Oct 21, 2025 at 11:13:29AM +0200, Oliver Neukum wrote:
>> On 20.10.25 18:59, Alan Stern wrote:
>>
>>> Another possibility is simply to give up on handling all of this
>>> automatically in the kernel. The usb_modeswitch program certainly
>>> should be capable of determining when a USB network device ought to
>>> switch to a different configuration; that's very similar to the things
>>> it does already. Maybe userspace is the best place to implement this
>>> stuff.
>>
>> That would make usb_modeswitch or yet a new udev component mandatory.
>> That is the exact opposite of what we would like to achieve.
>
> In the same way that usb_modeswitch or a udev script is already
> mandatory for a bunch of other devices?
Arguably broken devices.
> I agree, it would be great if the kernel could handle all these things
> for people. But sometimes it's just a lot easier to do stuff in
> userspace.
Well the kernel does handle them. It just handles them wrong.
You are not proposing to leave devices in the unconfigured state,
are you?
>> That is probably not wise in the long run. If the device whose driver
>> we kick off is a CD-ROM, nobody cares. If it is a network interface,
>> we'll have to deal with ugly cases like user space already having
>> sent a DHCP query when we kick the old driver off the interface.
>
> Doesn't the same concern apply every time a network interface goes down?
It does and that is why spontaneously shutting down network interfaces
in the kernel is a bad idea.
Regards
Oliver
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection
2025-10-22 7:58 ` Oliver Neukum
@ 2025-10-22 14:28 ` Alan Stern
0 siblings, 0 replies; 28+ messages in thread
From: Alan Stern @ 2025-10-22 14:28 UTC (permalink / raw)
To: Oliver Neukum
Cc: Michal Pecio, yicongsrfy, andrew+netdev, davem, edumazet, kuba,
linux-usb, netdev, pabeni
On Wed, Oct 22, 2025 at 09:58:57AM +0200, Oliver Neukum wrote:
> On 21.10.25 18:33, Alan Stern wrote:
> > On Tue, Oct 21, 2025 at 11:13:29AM +0200, Oliver Neukum wrote:
> > > On 20.10.25 18:59, Alan Stern wrote:
> > >
> > > > Another possibility is simply to give up on handling all of this
> > > > automatically in the kernel. The usb_modeswitch program certainly
> > > > should be capable of determining when a USB network device ought to
> > > > switch to a different configuration; that's very similar to the things
> > > > it does already. Maybe userspace is the best place to implement this
> > > > stuff.
> > >
> > > That would make usb_modeswitch or yet a new udev component mandatory.
> > > That is the exact opposite of what we would like to achieve.
> >
> > In the same way that usb_modeswitch or a udev script is already
> > mandatory for a bunch of other devices?
>
> Arguably broken devices.
Perhaps so. That doesn't affect my main point, however. Besides, none
of the possible approaches we have been discussing are truly
_mandatory_, because the user can always force a configuration change
simply by writing to a sysfs file.
> > I agree, it would be great if the kernel could handle all these things
> > for people. But sometimes it's just a lot easier to do stuff in
> > userspace.
>
> Well the kernel does handle them. It just handles them wrong.
:-)
> You are not proposing to leave devices in the unconfigured state,
> are you?
No, I wasn't. But that might not be a bad idea in some cases. If
userspace can do a better job than the kernel at picking a device's
initial configuration, we should stay out of its way.
The trick is to know for which devices -- there may be no general way of
determining this. Particularly if it depends on what out-of-tree
drivers the user has installed.
> > > That is probably not wise in the long run. If the device whose driver
> > > we kick off is a CD-ROM, nobody cares. If it is a network interface,
> > > we'll have to deal with ugly cases like user space already having
> > > sent a DHCP query when we kick the old driver off the interface.
> >
> > Doesn't the same concern apply every time a network interface goes down?
>
> It does and that is why spontaneously shutting down network interfaces
> in the kernel is a bad idea.
If the action is carried out by usb_modeswitch, for example, the program
can be responsible for shutting down the network interface cleanly
before it does the config change.
Alan Stern
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2025-10-22 14:28 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-11 7:53 [PATCH net v5 0/3] ax88179 driver optimization yicongsrfy
2025-10-11 7:53 ` [PATCH net v5 1/3] net: usb: support quirks in cdc_ncm yicongsrfy
2025-10-11 7:53 ` [PATCH net v5 2/3] net: usb: ax88179_178a: add USB device driver for config selection yicongsrfy
2025-10-13 9:07 ` Michal Pecio
2025-10-17 2:42 ` yicongsrfy
2025-10-17 13:10 ` Alan Stern
2025-10-17 17:15 ` Michal Pecio
2025-10-18 2:27 ` Alan Stern
2025-10-18 15:21 ` Michal Pecio
2025-10-18 15:36 ` Alan Stern
2025-10-18 15:56 ` Michal Pecio
2025-10-20 15:56 ` Alan Stern
2025-10-20 16:23 ` Michal Pecio
2025-10-20 16:59 ` Alan Stern
2025-10-21 9:13 ` Oliver Neukum
2025-10-21 16:33 ` Alan Stern
2025-10-22 7:58 ` Oliver Neukum
2025-10-22 14:28 ` Alan Stern
2025-10-21 2:29 ` Yi Cong
2025-10-21 2:59 ` Alan Stern
2025-10-21 6:26 ` Yi Cong
2025-10-21 16:26 ` Alan Stern
2025-10-20 9:59 ` Oliver Neukum
2025-10-20 10:48 ` Greg KH
2025-10-20 15:59 ` Michal Pecio
2025-10-21 9:02 ` Oliver Neukum
2025-10-20 10:27 ` Oliver Neukum
2025-10-11 7:53 ` [PATCH net v5 3/3] Revert "net: usb: ax88179_178a: Bind only to vendor-specific interface" yicongsrfy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).