* [PATCH v2] net: usb: ax88179_178a: Bind only to vendor-specific interface
@ 2022-05-02 11:06 Hector Martin
2022-05-05 2:30 ` Jakub Kicinski
0 siblings, 1 reply; 4+ messages in thread
From: Hector Martin @ 2022-05-02 11:06 UTC (permalink / raw)
To: Jacky Chou, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: linux-usb, netdev, linux-kernel, Hector Martin
The Anker PowerExpand USB-C to Gigabit Ethernet adapter uses this
chipset, but exposes CDC Ethernet configurations as well as the
vendor specific one. This driver ends up binding first to both CDC
interfaces, tries to instantiate two Ethernet interfaces talking to
the same device, and the result is a nice fireworks show.
Change all the ID matches to specifically match the vendor-specific
interface. By default the device comes up in CDC mode and is bound by
that driver (which works fine); users may switch it to the vendor
interface using sysfs to set bConfigurationValue, at which point the
device actually goes through a reconnect cycle and comes back as a
vendor specific only device, and then this driver binds and works too.
v2: Fixed interface protocol match, commit message.
Signed-off-by: Hector Martin <marcan@marcan.st>
---
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 e2fa56b92685..7c7c2f31d9f1 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1914,55 +1914,55 @@ static const struct driver_info at_umc2000sp_info = {
static const struct usb_device_id products[] = {
{
/* ASIX AX88179 10/100/1000 */
- USB_DEVICE(0x0b95, 0x1790),
+ USB_DEVICE_AND_INTERFACE_INFO(0x0b95, 0x1790, 0xff, 0xff, 0),
.driver_info = (unsigned long)&ax88179_info,
}, {
/* ASIX AX88178A 10/100/1000 */
- USB_DEVICE(0x0b95, 0x178a),
+ USB_DEVICE_AND_INTERFACE_INFO(0x0b95, 0x178a, 0xff, 0xff, 0),
.driver_info = (unsigned long)&ax88178a_info,
}, {
/* Cypress GX3 SuperSpeed to Gigabit Ethernet Bridge Controller */
- USB_DEVICE(0x04b4, 0x3610),
+ USB_DEVICE_AND_INTERFACE_INFO(0x04b4, 0x3610, 0xff, 0xff, 0),
.driver_info = (unsigned long)&cypress_GX3_info,
}, {
/* D-Link DUB-1312 USB 3.0 to Gigabit Ethernet Adapter */
- USB_DEVICE(0x2001, 0x4a00),
+ USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x4a00, 0xff, 0xff, 0),
.driver_info = (unsigned long)&dlink_dub1312_info,
}, {
/* Sitecom USB 3.0 to Gigabit Adapter */
- USB_DEVICE(0x0df6, 0x0072),
+ USB_DEVICE_AND_INTERFACE_INFO(0x0df6, 0x0072, 0xff, 0xff, 0),
.driver_info = (unsigned long)&sitecom_info,
}, {
/* Samsung USB Ethernet Adapter */
- USB_DEVICE(0x04e8, 0xa100),
+ USB_DEVICE_AND_INTERFACE_INFO(0x04e8, 0xa100, 0xff, 0xff, 0),
.driver_info = (unsigned long)&samsung_info,
}, {
/* Lenovo OneLinkDock Gigabit LAN */
- USB_DEVICE(0x17ef, 0x304b),
+ USB_DEVICE_AND_INTERFACE_INFO(0x17ef, 0x304b, 0xff, 0xff, 0),
.driver_info = (unsigned long)&lenovo_info,
}, {
/* Belkin B2B128 USB 3.0 Hub + Gigabit Ethernet Adapter */
- USB_DEVICE(0x050d, 0x0128),
+ USB_DEVICE_AND_INTERFACE_INFO(0x050d, 0x0128, 0xff, 0xff, 0),
.driver_info = (unsigned long)&belkin_info,
}, {
/* Toshiba USB 3.0 GBit Ethernet Adapter */
- USB_DEVICE(0x0930, 0x0a13),
+ USB_DEVICE_AND_INTERFACE_INFO(0x0930, 0x0a13, 0xff, 0xff, 0),
.driver_info = (unsigned long)&toshiba_info,
}, {
/* Magic Control Technology U3-A9003 USB 3.0 Gigabit Ethernet Adapter */
- USB_DEVICE(0x0711, 0x0179),
+ USB_DEVICE_AND_INTERFACE_INFO(0x0711, 0x0179, 0xff, 0xff, 0),
.driver_info = (unsigned long)&mct_info,
}, {
/* Allied Telesis AT-UMC2000 USB 3.0/USB 3.1 Gen 1 to Gigabit Ethernet Adapter */
- USB_DEVICE(0x07c9, 0x000e),
+ USB_DEVICE_AND_INTERFACE_INFO(0x07c9, 0x000e, 0xff, 0xff, 0),
.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(0x07c9, 0x000f),
+ USB_DEVICE_AND_INTERFACE_INFO(0x07c9, 0x000f, 0xff, 0xff, 0),
.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(0x07c9, 0x0010),
+ USB_DEVICE_AND_INTERFACE_INFO(0x07c9, 0x0010, 0xff, 0xff, 0),
.driver_info = (unsigned long)&at_umc2000sp_info,
},
{ },
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2] net: usb: ax88179_178a: Bind only to vendor-specific interface
2022-05-02 11:06 [PATCH v2] net: usb: ax88179_178a: Bind only to vendor-specific interface Hector Martin
@ 2022-05-05 2:30 ` Jakub Kicinski
2022-05-05 12:05 ` Hector Martin
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2022-05-05 2:30 UTC (permalink / raw)
To: Hector Martin
Cc: Jacky Chou, David S. Miller, Eric Dumazet, Paolo Abeni,
linux-usb, netdev, linux-kernel
On Mon, 2 May 2022 20:06:44 +0900 Hector Martin wrote:
> The Anker PowerExpand USB-C to Gigabit Ethernet adapter uses this
> chipset, but exposes CDC Ethernet configurations as well as the
> vendor specific one.
And we have reasons to believe all dongle vendors may have a similar
problem?
> This driver ends up binding first to both CDC
> interfaces, tries to instantiate two Ethernet interfaces talking to
> the same device, and the result is a nice fireworks show.
>
> Change all the ID matches to specifically match the vendor-specific
> interface. By default the device comes up in CDC mode and is bound by
> that driver (which works fine); users may switch it to the vendor
> interface using sysfs to set bConfigurationValue, at which point the
> device actually goes through a reconnect cycle and comes back as a
> vendor specific only device, and then this driver binds and works too.
>
> v2: Fixed interface protocol match, commit message.
>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
> 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 e2fa56b92685..7c7c2f31d9f1 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1914,55 +1914,55 @@ static const struct driver_info at_umc2000sp_info = {
> static const struct usb_device_id products[] = {
> {
> /* ASIX AX88179 10/100/1000 */
> - USB_DEVICE(0x0b95, 0x1790),
> + USB_DEVICE_AND_INTERFACE_INFO(0x0b95, 0x1790, 0xff, 0xff, 0),
> .driver_info = (unsigned long)&ax88179_info,
> },
Should we use USB_CLASS_VENDOR_SPEC and USB_SUBCLASS_VENDOR_SPEC ?
Maybe define a local macro wrapper for USB_DEVICE_AND.. which will
fill those in to avoid long lines?
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] net: usb: ax88179_178a: Bind only to vendor-specific interface
2022-05-05 2:30 ` Jakub Kicinski
@ 2022-05-05 12:05 ` Hector Martin
2022-05-05 16:17 ` Jakub Kicinski
0 siblings, 1 reply; 4+ messages in thread
From: Hector Martin @ 2022-05-05 12:05 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jacky Chou, David S. Miller, Eric Dumazet, Paolo Abeni,
linux-usb, netdev, linux-kernel
On 05/05/2022 11.30, Jakub Kicinski wrote:
> On Mon, 2 May 2022 20:06:44 +0900 Hector Martin wrote:
>> The Anker PowerExpand USB-C to Gigabit Ethernet adapter uses this
>> chipset, but exposes CDC Ethernet configurations as well as the
>> vendor specific one.
>
> And we have reasons to believe all dongle vendors may have a similar
> problem?
Given this is a vendor-specific driver it seems correct to have it only
bind to vendor-specific interfaces. That shouldn't break anything as
long as nobody is implementing this interface with the wrong protocol
IDs (which seems like quite a weird thing to do and fairly unlikely).
FWIW, the one I have has the generic VID/PID, not a custom vendor one.
If you prefer I can change just that one or both of the generic ones.
>> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
>> index e2fa56b92685..7c7c2f31d9f1 100644
>> --- a/drivers/net/usb/ax88179_178a.c
>> +++ b/drivers/net/usb/ax88179_178a.c
>> @@ -1914,55 +1914,55 @@ static const struct driver_info at_umc2000sp_info = {
>> static const struct usb_device_id products[] = {
>> {
>> /* ASIX AX88179 10/100/1000 */
>> - USB_DEVICE(0x0b95, 0x1790),
>> + USB_DEVICE_AND_INTERFACE_INFO(0x0b95, 0x1790, 0xff, 0xff, 0),
>> .driver_info = (unsigned long)&ax88179_info,
>> },
>
> Should we use USB_CLASS_VENDOR_SPEC and USB_SUBCLASS_VENDOR_SPEC ?
> Maybe define a local macro wrapper for USB_DEVICE_AND.. which will
> fill those in to avoid long lines?
Sure, I'll do that!
--
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] net: usb: ax88179_178a: Bind only to vendor-specific interface
2022-05-05 12:05 ` Hector Martin
@ 2022-05-05 16:17 ` Jakub Kicinski
0 siblings, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2022-05-05 16:17 UTC (permalink / raw)
To: Hector Martin
Cc: Jacky Chou, David S. Miller, Eric Dumazet, Paolo Abeni,
linux-usb, netdev, linux-kernel
On Thu, 5 May 2022 21:05:29 +0900 Hector Martin wrote:
> On 05/05/2022 11.30, Jakub Kicinski wrote:
> > On Mon, 2 May 2022 20:06:44 +0900 Hector Martin wrote:
> >> The Anker PowerExpand USB-C to Gigabit Ethernet adapter uses this
> >> chipset, but exposes CDC Ethernet configurations as well as the
> >> vendor specific one.
> >
> > And we have reasons to believe all dongle vendors may have a similar
> > problem?
>
> Given this is a vendor-specific driver it seems correct to have it only
> bind to vendor-specific interfaces. That shouldn't break anything as
> long as nobody is implementing this interface with the wrong protocol
> IDs (which seems like quite a weird thing to do and fairly unlikely).
>
> FWIW, the one I have has the generic VID/PID, not a custom vendor one.
> If you prefer I can change just that one or both of the generic ones.
I have no strong preference, maybe folks more familiar with USB in
general do and will chime in. Seems like a USB question more than
a networking one.
I know we have sort of the opposite problem with Realtek devices where
they bind to the generic driver instead of the vendor one resulting in
loss of functionality and higher power draw.
But that's not 1:1 the problem you're solving. Let's just make sure
the commit message is more explicit about which IDs are required
to fix your system and which are just changed for a good measure,
in case of regressions.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-05 16:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-02 11:06 [PATCH v2] net: usb: ax88179_178a: Bind only to vendor-specific interface Hector Martin
2022-05-05 2:30 ` Jakub Kicinski
2022-05-05 12:05 ` Hector Martin
2022-05-05 16:17 ` Jakub Kicinski
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).