Re: AW: add usb option device

[Johan Hovold]

On Sat, Dec 03, 2016 at 07:47:11PM +0100, Giuseppe Lippolis wrote:
> Dear All,
> 
> > If possible you want to tell the option driver which interfaces to 
> > bind to (white-listing) rather than specifying which not to bind to 
> > (black-listing). The latter typically means probing all interfaces, 
> > checking the black list, and then bailing out for unsupported 
> > interfaces.
> 
> I try to use the blacklist to specify which interface not to bind:
> 
>   static const struct option_blacklist_info dlink_dwm_158_ blacklist = {
>   .reserved = BIT(0) | BIT(1),
>   };
> 
>   { USB_DEVICE (0x2001, 0x7d04),
>   .driver_info = (kernel_ulong_t)_dwm_158_blacklist },
> 
> The result seems positive, after boot:

> T:  Bus=01 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  2 Spd=480  MxCh= 0
> D:  Ver= 2.00 Cls=02(comm.) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
> P:  Vendor=2001 ProdID=7d04 Rev= 3.00
> S:  Manufacturer=D-Link,Inc
> S:  Product=D-Link DWM-158
> C:* #Ifs= 7 Cfg#= 1 Atr=a0 MxPwr=500mA
> A:  FirstIf#= 0 IfCount= 2 Cls=02(comm.) Sub=06 Prot=00
> I:* If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=06 Prot=00 Driver=cdc_ether
> E:  Ad=88(I) Atr=03(Int.) MxPS=  64 Ivl=125us
> I:  If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> I:* If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
> E:  Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> I:* If#= 2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=02 Prot=01 Driver=option
> E:  Ad=87(I) Atr=03(Int.) MxPS=  64 Ivl=125us
> E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> I:* If#= 3 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
> E:  Ad=83(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> I:* If#= 4 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
> E:  Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> I:* If#= 5 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
> E:  Ad=85(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=05(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> I:* If#= 6 Alt= 0 #EPs= 2 Cls=08(stor.) Sub=06 Prot=50 Driver=(none)
> E:  Ad=86(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=06(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms

Yes, that would work, but if you look at the interface definitions above
you see that simply matching all interfaces with class 0xff (i.e.
white-listing) would suffice and is therefore preferred.

> On the kernel v4.8.12, I see similar d-link devices with the same
> VENDOR_ID declared in this way:
> 
>   { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d01, 0xff, 0x02, 0x01) },
> /* D-Link DWM-156 (variant) */
>   { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d01, 0xff, 0x00, 0x00) },
> /* D-Link DWM-156 (variant) */
>   { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d02, 0xff, 0x02, 0x01) },
>   { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d02, 0xff, 0x00, 0x00) },
>   { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d03, 0xff, 0x02, 0x01) },
>   { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d03, 0xff, 0x00, 0x00) },
>   { USB_DEVICE_INTERFACE_CLASS(0x2001, 0x7e19, 0xff), 
> /* D-Link DWM-221 B1 */
> .driver_info = (kernel_ulong_t)_intf4_blacklist },
> 
> Shall I declare a 
>   #define DLINK_VENDOR_ID 0x2001
> Or use the hardcoded style ?

Just stick to the hard-coded 0x2001 for now. There are two (or three)
other VIDs used for D-Link in option and it looks like one of them is
really the PCI vendor id.

Either way, we can clean that up later.

> Another question when is appropriate to use USB_DEVICE and when
> USB_DEVICE_INTERFACE_CLASS ?

So if you just use USB_DEVICE_INTERFACE_CLASS() you don't need to do any
blacklisting at all:

{ USB_DEVICE_INTERFACE_CLASS(0x2001, 0x7d04, 0xff) },

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

AW: AW: add usb option device

[Giuseppe Lippolis]

Dear All,

> If possible you want to tell the option driver which interfaces to 
> bind to (white-listing) rather than specifying which not to bind to 
> (black-listing). The latter typically means probing all interfaces, 
> checking the black list, and then bailing out for unsupported 
> interfaces.

I try to use the blacklist to specify which interface not to bind:

static const struct option_blacklist_info dlink_dwm_158_ blacklist = {
.reserved = BIT(0) | BIT(1),
};

{ USB_DEVICE (0x2001, 0x7d04),
.driver_info = (kernel_ulong_t)_dwm_158_blacklist },

The result seems positive, after boot:

T:  Bus=01 Lev=00 Prnt=00 Port=00 Cnt=00 Dev#=  1 Spd=480  MxCh= 1
B:  Alloc=  0/800 us ( 0%), #Int=  0, #Iso=  0
D:  Ver= 2.00 Cls=09(hub  ) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
P:  Vendor=1d6b ProdID=0002 Rev= 4.04
S:  Manufacturer=Linux 4.4.35 ehci_hcd
S:  Product=EHCI Host Controller
S:  SerialNumber=101c.ehci
C:* #Ifs= 1 Cfg#= 1 Atr=e0 MxPwr=  0mA
I:* If#= 0 Alt= 0 #EPs= 1 Cls=09(hub  ) Sub=00 Prot=00 Driver=hub
E:  Ad=81(I) Atr=03(Int.) MxPS=   4 Ivl=256ms

T:  Bus=01 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  2 Spd=480  MxCh= 0
D:  Ver= 2.00 Cls=02(comm.) Sub=00 Prot=00 MxPS=64 #Cfgs=  1
P:  Vendor=2001 ProdID=7d04 Rev= 3.00
S:  Manufacturer=D-Link,Inc
S:  Product=D-Link DWM-158
C:* #Ifs= 7 Cfg#= 1 Atr=a0 MxPwr=500mA
A:  FirstIf#= 0 IfCount= 2 Cls=02(comm.) Sub=06 Prot=00
I:* If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=06 Prot=00 Driver=cdc_ether
E:  Ad=88(I) Atr=03(Int.) MxPS=  64 Ivl=125us
I:  If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
I:* If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether
E:  Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
I:* If#= 2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=02 Prot=01 Driver=option
E:  Ad=87(I) Atr=03(Int.) MxPS=  64 Ivl=125us
E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E:  Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
I:* If#= 3 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
E:  Ad=83(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E:  Ad=03(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
I:* If#= 4 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
E:  Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E:  Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
I:* If#= 5 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=00 Prot=00 Driver=option
E:  Ad=85(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E:  Ad=05(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
I:* If#= 6 Alt= 0 #EPs= 2 Cls=08(stor.) Sub=06 Prot=50 Driver=(none)
E:  Ad=86(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
E:  Ad=06(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms


On the kernel v4.8.12, I see similar d-link devices with the same VENDOR_ID 
declared in this way:

{ USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d01, 0xff, 0x02, 0x01) },
/* D-Link DWM-156 (variant) */
{ USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d01, 0xff, 0x00, 0x00) },
/* D-Link DWM-156 (variant) */
{ USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d02, 0xff, 0x02, 0x01) },
{ USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d02, 0xff, 0x00, 0x00) },
{ USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d03, 0xff, 0x02, 0x01) },
{ USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d03, 0xff, 0x00, 0x00) },
{ USB_DEVICE_INTERFACE_CLASS(0x2001, 0x7e19, 0xff), 
/* D-Link DWM-221 B1 */
  .driver_info = (kernel_ulong_t)_intf4_blacklist },

Shall I declare a 
#define DLINK_VENDOR_ID 0x2001
Or use the hardcoded style ?

Another question when is appropriate to use USB_DEVICE and when 
USB_DEVICE_INTERFACE_CLASS ?

Thanks.



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: AW: add usb option device

[Dan Williams]

On Wed, 2016-11-16 at 22:11 +0100, Giuseppe Lippolis wrote:
> > 
> > > 
> > > This will make option grab all the ports, as shown by your dmesg
> > > output.  But USB interfaces 0 and 1 are actually cdc-ether and
> > > should
> > > *not* be grabbed by option.
> 
> I also have another curiosity:
> Why the driver architecture doesn't recognize autonomously the cdc-
> ether
> Interface and only gets the serial ports?

I'm not 100% sure, but driver loading order is undependable enough (for
good reason) to be effectively random.  So you must do the right thing
in each driver for ID matching or you will sometimes get the wrong
result.  This also prevents people from being lazy :)

But also, it's not just about automatically loading.  You can
rmmod/modprobe any driver at any time (as long as it's a module and not
built-in) and if you don't do the right thing for ID matching, this
sequence ends up with the wrong driver bound:

rmmod cdc-ether
rmmod option
modprobe option
modprobe cdc-ether

Now option is bound to the ether port and cdc-ether can't bind to it.

The point is, drivers should *only* claim interfaces and devices they
actually should drive.  They should not claim interfaces they should
not or cannot drive.

Dan

> Thanks,
> Bye.
> 
> > 
> > -Ursprüngliche Nachricht-
> > Von: Oliver Neukum [mailto:oneu...@suse.com]
> > Gesendet: Mittwoch, 16. November 2016 21:57
> > An: Dan Williams 
> > Cc: Giuseppe Lippolis ; linux-...@vger.kern
> > el.org
> > Betreff: Re: add usb option device
> > 
> > On Wed, 2016-11-16 at 11:49 -0600, Dan Williams wrote:
> > > 
> > > This will make option grab all the ports, as shown by your dmesg
> > > output.  But USB interfaces 0 and 1 are actually cdc-ether and
> > > should
> > > *not* be grabbed by option.
> > > 
> > > You want to limit option to grabbing bInterfaceClass=255 to make
> > > sure
> > > it only gets the serial ports.
> > 
> > Hi,
> > 
> > what happens if you actually use these ethernet interfaces while
> > somebody
> > uses the serial interfaces?
> > 
> > Regards
> > Oliver
> > 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

AW: add usb option device

[Giuseppe Lippolis]

> > This will make option grab all the ports, as shown by your dmesg
> > output.  But USB interfaces 0 and 1 are actually cdc-ether and should
> > *not* be grabbed by option.

I also have another curiosity:
Why the driver architecture doesn't recognize autonomously the cdc-ether
Interface and only gets the serial ports?

Thanks,
Bye.

> -Ursprüngliche Nachricht-
> Von: Oliver Neukum [mailto:oneu...@suse.com]
> Gesendet: Mittwoch, 16. November 2016 21:57
> An: Dan Williams 
> Cc: Giuseppe Lippolis ; linux-usb@vger.kernel.org
> Betreff: Re: add usb option device
> 
> On Wed, 2016-11-16 at 11:49 -0600, Dan Williams wrote:
> > This will make option grab all the ports, as shown by your dmesg
> > output.  But USB interfaces 0 and 1 are actually cdc-ether and should
> > *not* be grabbed by option.
> >
> > You want to limit option to grabbing bInterfaceClass=255 to make sure
> > it only gets the serial ports.
> 
> Hi,
> 
> what happens if you actually use these ethernet interfaces while somebody
> uses the serial interfaces?
> 
>   Regards
>   Oliver
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

AW: add usb option device

[Giuseppe Lippolis]

Dear All,
thanks for the very interesting discussion.

> > This will make option grab all the ports, as shown by your dmesg
> > output.  But USB interfaces 0 and 1 are actually cdc-ether and should
> > *not* be grabbed by option.
> >
> > You want to limit option to grabbing bInterfaceClass=255 to make sure
> > it only gets the serial ports.
>

Actually the device have 2 cdc_ether interface and the others are serial comm.
This is the bootlog of the original oem firmware:

hub 2-0:1.0: USB hub found
hub 2-0:1.0: 1 port detected
usb 2-1: new high speed USB device using rt3xxx-ehci and address 2
usb 2-1: configuration #1 chosen from 1 choice
usbcore: registered new interface driver cdc_ether
usbcore: registered new interface driver usbserial
drivers/usb/serial/usb-serial.c: USB Serial support registered for generic
usbserial_generic 2-1:1.2: generic converter detected
usb 2-1: generic converter now attached to ttyUSB0
usbserial_generic 2-1:1.3: generic converter detected
usb 2-1: generic converter now attached to ttyUSB1
usbserial_generic 2-1:1.4: generic converter detected
usb 2-1: generic converter now attached to ttyUSB2
usbserial_generic 2-1:1.5: generic converter detected
usb 2-1: generic converter now attached to ttyUSB3
usbserial_generic 2-1:1.6: generic converter detected
usb 2-1: generic converter now attached to ttyUSB4
usbcore: registered new interface driver usbserial_generic
drivers/usb/serial/usb-serial.c: USB Serial Driver core

and at the end of the modem configuration:
usbnet0   Link encap:Ethernet  HWaddr xx:xx:xx:xx:xx:xx
  inet addr:77.25.169.193  Bcast:77.25.169.195  Mask:255.255.255.252
  inet6 addr: fe80:::feaa:/64 Scope:Link
  UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
  RX packets:6 errors:0 dropped:0 overruns:0 frame:0
  TX packets:8 errors:0 dropped:0 overruns:0 carrier:0
  collisions:0 txqueuelen:1000
  RX bytes:272 (272.0 B)  TX bytes:600 (600.0 B)

Therefore I also consider convenient preserve the first two interface for the 
cdc_ether.
Can you please clarify me the differences between black and white list and how 
to configure it?

> Also, is it really a D-Link DWM-158?  That appears to be a USB dongle- type
> device, while what's in the DWR-512 is a PCI-E minicard that looks like a ZTE
> MF210, from the FCC pictures.

How is interested in having more info about the modem and the router can read 
the description page I compiled here:
https://wiki.openwrt.org/inbox/d-link/d-link_dwr-512_b

I will make additional test with different driver configuration and I will come 
back to you.

Bye.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

AW: add usb option device

[Giuseppe Lippolis]

> Your email client ate the tabs and spit out spaces and line-wrapped the
> patch, making it impossible to apply.
> 
> Also, you need to make it against the latest kernel tree, 4.4 is really
old.
> 
> And finally, there is no good subject: line, or description of the patch,
or a
> signed-off-by: line, all of which are described in the document I pointed
you
> at.
> 
> Please fix this up and resend.

I add now a new post making a patch on the linux 4.8.8 :

[PATCH] add DWR-158 in usb option
http://marc.info/?l=linux-usb=147924122203290=2

I hope now is better.

Bye.


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

AW: add usb option device

[Giuseppe Lippolis]

Here it is:

--- a/linux-4.4.23/drivers/usb/serial/option.c2016-09-30
10:20:43.0 +0200
+++ b/linux-4.4.30/drivers/usb/serial/option.c   2016-11-14
21:01:15.738450136 +0100
@@ -306,6 +306,9 @@ static void option_instat_callback(struc
 #define DLINK_PRODUCT_DWM_652_U5   0xce16
 #define DLINK_PRODUCT_DWM_652_U5A  0xce1e

+#define DLINK_ATL_VENDOR_ID0x2001
+#define DLINK_PRODUCT_DWM_158  0x7d04
+
 #define QISDA_VENDOR_ID0x1da5
 #define QISDA_PRODUCT_H21_4512 0x4512
 #define QISDA_PRODUCT_H21_4523 0x4523
@@ -1812,6 +1815,8 @@ static const struct usb_device_id option
{ USB_DEVICE(DLINK_VENDOR_ID, DLINK_PRODUCT_DWM_652) },
{ USB_DEVICE(ALINK_VENDOR_ID, DLINK_PRODUCT_DWM_652_U5) }, /* Yes,
ALINK_VENDOR_ID */
{ USB_DEVICE(ALINK_VENDOR_ID, DLINK_PRODUCT_DWM_652_U5A) },
+   { USB_DEVICE(DLINK_ATL_VENDOR_ID, DLINK_PRODUCT_DWM_158) },
+
{ USB_DEVICE(QISDA_VENDOR_ID, QISDA_PRODUCT_H21_4512) },
{ USB_DEVICE(QISDA_VENDOR_ID, QISDA_PRODUCT_H21_4523) },
{ USB_DEVICE(QISDA_VENDOR_ID, QISDA_PRODUCT_H20_4515) },


bye

> -Ursprüngliche Nachricht-
> Von: Greg KH [mailto:gre...@linuxfoundation.org]
> Gesendet: Dienstag, 15. November 2016 20:19
> An: Giuseppe Lippolis 
> Cc: linux-usb@vger.kernel.org
> Betreff: Re: add usb option device
> 
> On Tue, Nov 15, 2016 at 08:13:56PM +0100, Giuseppe Lippolis wrote:
> > Dear all,
> > I'm porting the Dlink DWR-512 device to LEDE (embedded linux).
> > This device embed a 3G modem connected through the usb bus .
> > The modem work properly with the option kernel module.
> >
> > I added these line in the
> >
> > #define DLINK_PRODUCT_DWM_652  0x3e04
> > #define DLINK_PRODUCT_DWM_652_U5  0xce16
> > #define DLINK_PRODUCT_DWM_652_U5A   0xce1e
> >
> > +   #define DLINK_ATL_VENDOR_ID 0x2001
> > +   #define DLINK_PRODUCT_DWM_158  0x7d04
> >
> > #define QISDA_VENDOR_ID 0x1da5
> > #define QISDA_PRODUCT_H21_4512  0x4512
> >
> > [...]
> >
> > { USB_DEVICE(ALINK_VENDOR_ID, DLINK_PRODUCT_DWM_652_U5) },
> /*
> > Yes, ALINK_VENDOR_ID */
> > { USB_DEVICE(ALINK_VENDOR_ID, DLINK_PRODUCT_DWM_652_U5A)
> },
> > + { USB_DEVICE(DLINK_ATL_VENDOR_ID, DLINK_PRODUCT_DWM_158)
> },
> > { USB_DEVICE(QISDA_VENDOR_ID, QISDA_PRODUCT_H21_4512) },
> > { USB_DEVICE(QISDA_VENDOR_ID, QISDA_PRODUCT_H21_4523) },
> 
> Great, can you provide a patch for this in a format that we can apply it
in?
> The file, Documentation/SubmittingPatches will show you what you need to
> do.  If you have any questions after reading that, please let us know.
> 
> thanks,
> 
> greg k-h

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html