Message ID | 20170927213103.11987-1-aleksander@aleksander.es |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Series | rndis_host: support Novatel Verizon USB730L | expand |
From: Aleksander Morgado <aleksander@aleksander.es> Date: Wed, 27 Sep 2017 23:31:03 +0200 > I'm not sure if binding this logic to a specific vid:pid (1410:9030) > would be more appropriate here, or if it's ok to just bind > class/subclass/protocol (as in the activesync case). Let me know > what you think. I don't have enough USB Networking knowledge to make a decision here. Some things seem confusing. For example, if we should be matching USB_CLASS_MISC, subclass=4, protocol=1 for RNDIS over Ethernet, then we why aren't we also matching USB_CLASS_MISC, subclass=4, protocol=2 for RNDIS over wireless and instead are matching "Remote RNDIS" in the form of USB_CLASS_WIRELSS, subclass=1, protocol=3? I really don't understand any of this magic :-) So someone more knowledgable needs to review this.
David Miller <davem@davemloft.net> writes: > From: Aleksander Morgado <aleksander@aleksander.es> > Date: Wed, 27 Sep 2017 23:31:03 +0200 > >> I'm not sure if binding this logic to a specific vid:pid (1410:9030) >> would be more appropriate here, or if it's ok to just bind >> class/subclass/protocol (as in the activesync case). Let me know >> what you think. > > I don't have enough USB Networking knowledge to make a decision here. > > Some things seem confusing. For example, if we should be matching > USB_CLASS_MISC, subclass=4, protocol=1 for RNDIS over Ethernet, then > we why aren't we also matching USB_CLASS_MISC, subclass=4, protocol=2 > for RNDIS over wireless and instead are matching "Remote RNDIS" in > the form of USB_CLASS_WIRELSS, subclass=1, protocol=3? > > I really don't understand any of this magic :-) > > So someone more knowledgable needs to review this. Let me just say that I am not qualified. But since this needs a review, I am going to offer my view anyway. FWIW, I don't think *anyone* understand this magic... I certainly don't. We can pretty much ignore the USB-IF and any specs, since that is what the vendors appear to do. They provide device specific drivers for Windows, so all they care about is that their device "works" with their driver. But in Linux we prefer to create drivers for device classes whenever we can, to avoid having to add every single device by ID. So we try to guess future patterns based on the devices we have observed, even when there is no clear spec. This is what Aleksander does here. He has a device with a 'Cls=ef(misc ) Sub=04 Prot=01' function. This device works with the rndis_host driver. That is all we know. We cannot prove that a class match is correct. But it does make sense to try it. At least we know that this works for one device. Adding anything else, e.g. based on the table at http://www.usb.org/developers/defined_class/#BaseClassEFh , is a bit more risky. We don't know if a driver will work with *any* such device until we've actually seen one. This is just my opinion, and probably full of bogus assumptions as usual. I was sort of hoping that some expert would speak up so I didn't have to :-) But FWIW: Reviewed-by: Bjørn Mork <bjorn@mork.no> Bjørn
From: Bjørn Mork <bjorn@mork.no> Date: Tue, 03 Oct 2017 16:01:15 +0200 > We can pretty much ignore the USB-IF and any specs, since that is what > the vendors appear to do. They provide device specific drivers for > Windows, so all they care about is that their device "works" with their > driver. > > But in Linux we prefer to create drivers for device classes whenever we > can, to avoid having to add every single device by ID. So we try to > guess future patterns based on the devices we have observed, even when > there is no clear spec. This is what Aleksander does here. He has a > device with a 'Cls=ef(misc ) Sub=04 Prot=01' function. This device > works with the rndis_host driver. That is all we know. > > We cannot prove that a class match is correct. But it does make sense to > try it. At least we know that this works for one device. > > Adding anything else, e.g. based on the table at > http://www.usb.org/developers/defined_class/#BaseClassEFh , is a bit > more risky. We don't know if a driver will work with *any* such device > until we've actually seen one. > > This is just my opinion, and probably full of bogus assumptions as > usual. I was sort of hoping that some expert would speak up so I didn't > have to :-) Ok ;-) > But FWIW: > > Reviewed-by: Bjørn Mork <bjorn@mork.no> So I'll apply this for now, thanks for your feedback.
Am Dienstag, den 03.10.2017, 16:01 +0200 schrieb Bjørn Mork: > Adding anything else, e.g. based on the table at > http://www.usb.org/developers/defined_class/#BaseClassEFh , is a bit > more risky. We don't know if a driver will work with *any* such device > until we've actually seen one. > > This is just my opinion, and probably full of bogus assumptions as > usual. I was sort of hoping that some expert would speak up so I didn't > have to :-) Outside the vendors, there is nobody. Regards Oliver
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c index 8ab281b478f2..2df0bcc6d30b 100644 --- a/drivers/net/usb/cdc_ether.c +++ b/drivers/net/usb/cdc_ether.c @@ -54,11 +54,19 @@ static int is_wireless_rndis(struct usb_interface_descriptor *desc) desc->bInterfaceProtocol == 3); } +static int is_novatel_rndis(struct usb_interface_descriptor *desc) +{ + return (desc->bInterfaceClass == USB_CLASS_MISC && + desc->bInterfaceSubClass == 4 && + desc->bInterfaceProtocol == 1); +} + #else #define is_rndis(desc) 0 #define is_activesync(desc) 0 #define is_wireless_rndis(desc) 0 +#define is_novatel_rndis(desc) 0 #endif @@ -150,7 +158,8 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf) */ rndis = (is_rndis(&intf->cur_altsetting->desc) || is_activesync(&intf->cur_altsetting->desc) || - is_wireless_rndis(&intf->cur_altsetting->desc)); + is_wireless_rndis(&intf->cur_altsetting->desc) || + is_novatel_rndis(&intf->cur_altsetting->desc)); memset(info, 0, sizeof(*info)); info->control = intf; diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c index a151f267aebb..b807c91abe1d 100644 --- a/drivers/net/usb/rndis_host.c +++ b/drivers/net/usb/rndis_host.c @@ -632,6 +632,10 @@ static const struct usb_device_id products [] = { /* RNDIS for tethering */ USB_INTERFACE_INFO(USB_CLASS_WIRELESS_CONTROLLER, 1, 3), .driver_info = (unsigned long) &rndis_info, +}, { + /* Novatel Verizon USB730L */ + USB_INTERFACE_INFO(USB_CLASS_MISC, 4, 1), + .driver_info = (unsigned long) &rndis_info, }, { }, // END };
Treat the ef/04/01 interface class/subclass/protocol combination used by the Novatel Verizon USB730L (1410:9030) as a possible RNDIS interface. T: Bus=01 Lev=02 Prnt=02 Port=01 Cnt=02 Dev#= 17 Spd=480 MxCh= 0 D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 3 P: Vendor=1410 ProdID=9030 Rev=03.10 S: Manufacturer=Novatel Wireless S: Product=MiFi USB730L S: SerialNumber=0123456789ABCDEF C: #Ifs= 3 Cfg#= 1 Atr=80 MxPwr=500mA I: If#= 0 Alt= 0 #EPs= 1 Cls=ef(misc ) Sub=04 Prot=01 Driver=rndis_host I: If#= 1 Alt= 0 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=rndis_host I: If#= 2 Alt= 0 #EPs= 1 Cls=03(HID ) Sub=00 Prot=00 Driver=usbhid Once the network interface is brought up, the user just needs to run a DHCP client to get IP address and routing setup. As a side note, other Novatel Verizon USB730L models with the same vid:pid end up exposing a standard ECM interface which doesn't require any other kernel update to make it work. Signed-off-by: Aleksander Morgado <aleksander@aleksander.es> --- Hey, I'm not sure if binding this logic to a specific vid:pid (1410:9030) would be more appropriate here, or if it's ok to just bind class/subclass/protocol (as in the activesync case). Let me know what you think. --- drivers/net/usb/cdc_ether.c | 11 ++++++++++- drivers/net/usb/rndis_host.c | 4 ++++ 2 files changed, 14 insertions(+), 1 deletion(-) -- 2.14.1