Re: umb(4) attachment

2016-06-28 Thread Martin Pieuchot 
On 28/06/16(Tue) 10:22, Mark Kettenis wrote:
> [...] 
> It's already in.  No real fallout as far as I know, except for that
> report about some wierd umass interaction.  But I don't think that's
> actually related to my changes.

It is not related, eric@ showed me the same problem during p2k16.

Re: umb(4) attachment

2016-06-28 Thread Mark Kettenis 
> Date: Tue, 28 Jun 2016 07:01:09 +0200
> From: Martin Pieuchot 
> 
> On 17/06/16(Fri) 22:22, Mark Kettenis wrote:
> > As reported earlier, umb(4) currently doesn't attach to devices that
> > implement both NCM 1.0 and MBIM, such as the Sierra Wireless EM7345
> > that is found in some thinkpads.
> > 
> > The diff below fixes this.  It revamps the way we look up interface
> > descriptors quite a bit.  I removed the unused code for matching
> > devices based on vendor and product ids.  That code got a bit in my
> > way.  It should be possible to bring that back if needed.
> > 
> > With this fix, the EM7345 attaches as:
> > 
> > umb0 at uhub0 port 4 configuration 1 interface 0 "Sierra Wireless Inc. 
> > Sierra Wi
> > reless EM7345 4G LTE" rev 2.00/17.29 addr 2
> > umb0: ignoring invalid segment size 1500
> > umb0: vers 1.0
> > umodem0 at uhub0 port 4 configuration 1 interface 2 "Sierra Wireless Inc. 
> > Sierra Wireless EM7345 4G LTE" rev 2.00/17.29 addr 2
> > umodem0: data interface 3, has no CM over data, has break
> > umodem0: status change notification available
> > ucom0 at umodem0
> > 
> > Note that it still attaches as umodem(4) as well.  That is actually a
> > good thing since it allows me to read out the GPS though that interface.
> > 
> > I believe this code should work on all devices that are properly MBIM
> > compliant.  But of course vendors are notoriously sloppy in providing
> > the right usb interface descriptors for their devices.  So testing is
> > welcome.  If you run into issues, please provide lsusb -v output.
> 
> Any test report?  This looks good to me.

It's already in.  No real fallout as far as I know, except for that
report about some wierd umass interaction.  But I don't think that's
actually related to my changes.

Re: umb(4) attachment

2016-06-27 Thread Martin Pieuchot 
On 17/06/16(Fri) 22:22, Mark Kettenis wrote:
> As reported earlier, umb(4) currently doesn't attach to devices that
> implement both NCM 1.0 and MBIM, such as the Sierra Wireless EM7345
> that is found in some thinkpads.
> 
> The diff below fixes this.  It revamps the way we look up interface
> descriptors quite a bit.  I removed the unused code for matching
> devices based on vendor and product ids.  That code got a bit in my
> way.  It should be possible to bring that back if needed.
> 
> With this fix, the EM7345 attaches as:
> 
> umb0 at uhub0 port 4 configuration 1 interface 0 "Sierra Wireless Inc. Sierra 
> Wi
> reless EM7345 4G LTE" rev 2.00/17.29 addr 2
> umb0: ignoring invalid segment size 1500
> umb0: vers 1.0
> umodem0 at uhub0 port 4 configuration 1 interface 2 "Sierra Wireless Inc. 
> Sierra Wireless EM7345 4G LTE" rev 2.00/17.29 addr 2
> umodem0: data interface 3, has no CM over data, has break
> umodem0: status change notification available
> ucom0 at umodem0
> 
> Note that it still attaches as umodem(4) as well.  That is actually a
> good thing since it allows me to read out the GPS though that interface.
> 
> I believe this code should work on all devices that are properly MBIM
> compliant.  But of course vendors are notoriously sloppy in providing
> the right usb interface descriptors for their devices.  So testing is
> welcome.  If you run into issues, please provide lsusb -v output.

Any test report?  This looks good to me.


> Index: if_umb.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 if_umb.c
> --- if_umb.c  15 Jun 2016 19:39:34 -  1.1
> +++ if_umb.c  17 Jun 2016 20:08:05 -
> @@ -204,48 +204,35 @@ const struct cfattach umb_ca = {
>  
>  int umb_delay = 4000;
>  
> -/*
> - * Normally, MBIM devices are detected by their interface class and subclass.
> - * But for some models that have multiple configurations, it is better to
> - * match by vendor and product id so that we can select the desired
> - * configuration ourselves.
> - *
> - * OTOH, some devices identifiy themself als an MBIM device but fail to speak
> - * the MBIM protocol.
> - */
> -struct umb_products {
> - struct usb_devno dev;
> - int  confno;
> -};
> -const struct umb_products umb_devs[] = {
> - /*
> -  * Add devices here to force them to attach as umb.
> -  * Format: { { VID, PID }, CONFIGNO }
> -  */
> -};
> -
> -#define umb_lookup(vid, pid) \
> - ((const struct umb_products *)usb_lookup(umb_devs, vid, pid))
> -
>  int
>  umb_match(struct device *parent, void *match, void *aux)
>  {
>   struct usb_attach_arg *uaa = aux;
>   usb_interface_descriptor_t *id;
>  
> - if (umb_lookup(uaa->vendor, uaa->product) != NULL)
> - return UMATCH_VENDOR_PRODUCT;
>   if (!uaa->iface)
>   return UMATCH_NONE;
>   if ((id = usbd_get_interface_descriptor(uaa->iface)) == NULL)
>   return UMATCH_NONE;
> - if (id->bInterfaceClass != UICLASS_CDC ||
> - id->bInterfaceSubClass !=
> - UISUBCLASS_MOBILE_BROADBAND_INTERFACE_MODEL ||
> - id->bNumEndpoints != 1)
> +
> + /*
> +  * If this function implements NCM, check if alternate setting
> +  * 1 implements MBIM.
> +  */
> + if (id->bInterfaceClass == UICLASS_CDC &&
> + id->bInterfaceSubClass ==
> + UISUBCLASS_NETWORK_CONTROL_MODEL)
> + id = usbd_find_idesc(uaa->device->cdesc, uaa->ifaceno, 1);
> + if (id == NULL)
>   return UMATCH_NONE;
>  
> - return UMATCH_DEVCLASS_DEVSUBCLASS;
> + if (id->bInterfaceClass == UICLASS_CDC &&
> + id->bInterfaceSubClass ==
> + UISUBCLASS_MOBILE_BROADBAND_INTERFACE_MODEL &&
> + id->bInterfaceProtocol == 0)
> + return UMATCH_IFACECLASS_IFACESUBCLASS_IFACEPROTO;
> +
> + return UMATCH_NONE;
>  }
>  
>  void
> @@ -257,45 +244,54 @@ umb_attach(struct device *parent, struct
>   struct usbd_desc_iter iter;
>   const usb_descriptor_t *desc;
>   int  v;
> + struct usb_cdc_union_descriptor *ud;
>   struct mbim_descriptor *md;
>   int  i;
> - struct usbd_interface *ctrl_iface = NULL;
>   int  ctrl_ep;
> - uint8_t  data_ifaceno;
>   usb_interface_descriptor_t *id;
>   usb_config_descriptor_t *cd;
>   usb_endpoint_descriptor_t *ed;
> + usb_interface_assoc_descriptor_t *ad;
> + int  current_ifaceno = -1;
> + int  data_ifaceno = -1;
>   int  altnum;
>   int  s;
>   struct ifnet *ifp;
>   int  hard_mtu;
>  
>   sc->sc_udev = uaa->device;
> + sc->sc_ctrl_ifaceno = uaa->ifaceno;
>  
> - if (uaa->configno < 0) {
> - /*
> -  * In case the device was matched by VID/PID instead of
> -  * InterfaceClass/InterfaceSubClass, we have to pick the
> -

Re: umb(4) attachment

2016-06-18 Thread Marcus MERIGHI 
Hello, 

the kernel does not compile for me with this patch. 

src/sys/dev/usb/usb.h is missing UISUBCLASS_NETWORK_CONTROL_MODEL. 

What value does it need?

(I get no umb(4) device with a value of 15. Am I supposed to get one
with this hardware and the correct value?)

One OT GPS question below, lsusb -v + dmesg as well.

mark.kette...@xs4all.nl (Mark Kettenis), 2016.06.17 (Fri) 22:22 (CEST):
> As reported earlier, umb(4) currently doesn't attach to devices that
> implement both NCM 1.0 and MBIM, such as the Sierra Wireless EM7345
> that is found in some thinkpads.
> 
> The diff below fixes this.  It revamps the way we look up interface
> descriptors quite a bit.  I removed the unused code for matching
> devices based on vendor and product ids.  That code got a bit in my
> way.  It should be possible to bring that back if needed.
> 
> With this fix, the EM7345 attaches as:
> 
> umb0 at uhub0 port 4 configuration 1 interface 0 "Sierra Wireless Inc. Sierra 
> Wi
> reless EM7345 4G LTE" rev 2.00/17.29 addr 2
> umb0: ignoring invalid segment size 1500
> umb0: vers 1.0
> umodem0 at uhub0 port 4 configuration 1 interface 2 "Sierra Wireless Inc. 
> Sierra Wireless EM7345 4G LTE" rev 2.00/17.29 addr 2
> umodem0: data interface 3, has no CM over data, has break
> umodem0: status change notification available
> ucom0 at umodem0
> 
> Note that it still attaches as umodem(4) as well.  That is actually a
> good thing since it allows me to read out the GPS though that interface.

My /dev/cuaU? all respond to at -> OK. How do you get the GPS data?

> I believe this code should work on all devices that are properly MBIM
> compliant.  But of course vendors are notoriously sloppy in providing
> the right usb interface descriptors for their devices.  So testing is
> welcome.  If you run into issues, please provide lsusb -v output.


Bus 000 Device 001: ID 8086: Intel Corp. 
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass9 Hub
  bDeviceSubClass 0 Unused
  bDeviceProtocol 1 Single TT
  bMaxPacketSize064
  idVendor   0x8086 Intel Corp.
  idProduct  0x 
  bcdDevice1.00
  iManufacturer   1 Intel
  iProduct2 EHCI root hub
  iSerial 0 
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength   25
bNumInterfaces  1
bConfigurationValue 1
iConfiguration  0 
bmAttributes 0x40
  (Missing must-be-set bit!)
  Self Powered
MaxPower0mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   1
  bInterfaceClass 9 Hub
  bInterfaceSubClass  0 Unused
  bInterfaceProtocol  0 Full speed (or root) hub
  iInterface  0 
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81  EP 1 IN
bmAttributes3
  Transfer TypeInterrupt
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0008  1x 8 bytes
bInterval  12
Hub Descriptor:
  bLength  10
  bDescriptorType  41
  nNbrPorts 3
  wHubCharacteristic 0x0002
No power switching (usb 1.0)
Ganged overcurrent protection
TT think time 8 FS bits
  bPwrOn2PwrGood  200 * 2 milli seconds
  bHubContrCurrent  0 milli Ampere
  DeviceRemovable0x00
  PortPwrCtrlMask0x00
 Hub Port Status:
   Port 1: .0503 highspeed power enable connect
   Port 2: .0500 highspeed power
   Port 3: .0500 highspeed power
Device Status: 0x0001
  Self Powered

Bus 000 Device 002: ID 8087:0024 Intel Corp. Integrated Rate Matching Hub
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass9 Hub
  bDeviceSubClass 0 Unused
  bDeviceProtocol 1 Single TT
  bMaxPacketSize064
  idVendor   0x8087 Intel Corp.
  idProduct  0x0024 Integrated Rate Matching Hub
  bcdDevice0.00
  iManufacturer   0 
  iProduct0 
  iSerial 0 
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength   25
bNumInterfaces  1
bConfigurationValue 1
iConfiguration  0 
bmAttributes 0xe0
  Self Powered
  Remote Wakeup
MaxPower0mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   1

umb(4) attachment

2016-06-17 Thread Mark Kettenis 
As reported earlier, umb(4) currently doesn't attach to devices that
implement both NCM 1.0 and MBIM, such as the Sierra Wireless EM7345
that is found in some thinkpads.

The diff below fixes this.  It revamps the way we look up interface
descriptors quite a bit.  I removed the unused code for matching
devices based on vendor and product ids.  That code got a bit in my
way.  It should be possible to bring that back if needed.

With this fix, the EM7345 attaches as:

umb0 at uhub0 port 4 configuration 1 interface 0 "Sierra Wireless Inc. Sierra Wi
reless EM7345 4G LTE" rev 2.00/17.29 addr 2
umb0: ignoring invalid segment size 1500
umb0: vers 1.0
umodem0 at uhub0 port 4 configuration 1 interface 2 "Sierra Wireless Inc. 
Sierra Wireless EM7345 4G LTE" rev 2.00/17.29 addr 2
umodem0: data interface 3, has no CM over data, has break
umodem0: status change notification available
ucom0 at umodem0

Note that it still attaches as umodem(4) as well.  That is actually a
good thing since it allows me to read out the GPS though that interface.

I believe this code should work on all devices that are properly MBIM
compliant.  But of course vendors are notoriously sloppy in providing
the right usb interface descriptors for their devices.  So testing is
welcome.  If you run into issues, please provide lsusb -v output.

Thanks,

Mark


Index: if_umb.c
===
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.1
diff -u -p -r1.1 if_umb.c
--- if_umb.c15 Jun 2016 19:39:34 -  1.1
+++ if_umb.c17 Jun 2016 20:08:05 -
@@ -204,48 +204,35 @@ const struct cfattach umb_ca = {
 
 int umb_delay = 4000;
 
-/*
- * Normally, MBIM devices are detected by their interface class and subclass.
- * But for some models that have multiple configurations, it is better to
- * match by vendor and product id so that we can select the desired
- * configuration ourselves.
- *
- * OTOH, some devices identifiy themself als an MBIM device but fail to speak
- * the MBIM protocol.
- */
-struct umb_products {
-   struct usb_devno dev;
-   int  confno;
-};
-const struct umb_products umb_devs[] = {
-   /*
-* Add devices here to force them to attach as umb.
-* Format: { { VID, PID }, CONFIGNO }
-*/
-};
-
-#define umb_lookup(vid, pid)   \
-   ((const struct umb_products *)usb_lookup(umb_devs, vid, pid))
-
 int
 umb_match(struct device *parent, void *match, void *aux)
 {
struct usb_attach_arg *uaa = aux;
usb_interface_descriptor_t *id;
 
-   if (umb_lookup(uaa->vendor, uaa->product) != NULL)
-   return UMATCH_VENDOR_PRODUCT;
if (!uaa->iface)
return UMATCH_NONE;
if ((id = usbd_get_interface_descriptor(uaa->iface)) == NULL)
return UMATCH_NONE;
-   if (id->bInterfaceClass != UICLASS_CDC ||
-   id->bInterfaceSubClass !=
-   UISUBCLASS_MOBILE_BROADBAND_INTERFACE_MODEL ||
-   id->bNumEndpoints != 1)
+
+   /*
+* If this function implements NCM, check if alternate setting
+* 1 implements MBIM.
+*/
+   if (id->bInterfaceClass == UICLASS_CDC &&
+   id->bInterfaceSubClass ==
+   UISUBCLASS_NETWORK_CONTROL_MODEL)
+   id = usbd_find_idesc(uaa->device->cdesc, uaa->ifaceno, 1);
+   if (id == NULL)
return UMATCH_NONE;
 
-   return UMATCH_DEVCLASS_DEVSUBCLASS;
+   if (id->bInterfaceClass == UICLASS_CDC &&
+   id->bInterfaceSubClass ==
+   UISUBCLASS_MOBILE_BROADBAND_INTERFACE_MODEL &&
+   id->bInterfaceProtocol == 0)
+   return UMATCH_IFACECLASS_IFACESUBCLASS_IFACEPROTO;
+
+   return UMATCH_NONE;
 }
 
 void
@@ -257,45 +244,54 @@ umb_attach(struct device *parent, struct
struct usbd_desc_iter iter;
const usb_descriptor_t *desc;
int  v;
+   struct usb_cdc_union_descriptor *ud;
struct mbim_descriptor *md;
int  i;
-   struct usbd_interface *ctrl_iface = NULL;
int  ctrl_ep;
-   uint8_t  data_ifaceno;
usb_interface_descriptor_t *id;
usb_config_descriptor_t *cd;
usb_endpoint_descriptor_t *ed;
+   usb_interface_assoc_descriptor_t *ad;
+   int  current_ifaceno = -1;
+   int  data_ifaceno = -1;
int  altnum;
int  s;
struct ifnet *ifp;
int  hard_mtu;
 
sc->sc_udev = uaa->device;
+   sc->sc_ctrl_ifaceno = uaa->ifaceno;
 
-   if (uaa->configno < 0) {
-   /*
-* In case the device was matched by VID/PID instead of
-* InterfaceClass/InterfaceSubClass, we have to pick the
-* correct configuration ourself.
-*/
-   uaa->configno = umb_lookup(uaa->vendor, uaa->product)->confno;
-   DPRINTF("%s: switching to config #%d\n", DEVNAM(sc),
-