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 -0000 1.1
> +++ if_umb.c 17 Jun 2016 20:08:05 -0000
> @@ -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),
> - uaa->configno);
> - status = usbd_set_config_no(sc->sc_udev, uaa->configno, 1);
> - if (status) {
> - printf("%s: failed to switch to config #%d: %s\n",
> - DEVNAM(sc), uaa->configno, usbd_errstr(status));
> - goto fail;
> - }
> - }
> -
> + /*
> + * Some MBIM hardware does not provide the mandatory CDC Union
> + * Descriptor, so we also look at matching Interface
> + * Association Descriptors to find out the MBIM Data Interface
> + * number.
> + */
> sc->sc_ver_maj = sc->sc_ver_min = -1;
> - usbd_desc_iter_init(sc->sc_udev, &iter);
> hard_mtu = MBIM_MAXSEGSZ_MINVAL;
> + usbd_desc_iter_init(sc->sc_udev, &iter);
> while ((desc = usbd_desc_iter_next(&iter))) {
> + if (desc->bDescriptorType == UDESC_IFACE_ASSOC) {
> + ad = (usb_interface_assoc_descriptor_t *)desc;
> + if (ad->bFirstInterface == uaa->ifaceno &&
> + ad->bInterfaceCount > 1)
> + data_ifaceno = uaa->ifaceno + 1;
> + }
> + if (desc->bDescriptorType == UDESC_INTERFACE) {
> + id = (usb_interface_descriptor_t *)desc;
> + current_ifaceno = id->bInterfaceNumber;
> + continue;
> + }
> + if (current_ifaceno != uaa->ifaceno)
> + continue;
> if (desc->bDescriptorType != UDESC_CS_INTERFACE)
> continue;
> switch (desc->bDescriptorSubtype) {
> + case UDESCSUB_CDC_UNION:
> + ud = (struct usb_cdc_union_descriptor *)desc;
> + data_ifaceno = ud->bSlaveInterface[0];
> + break;
> case UDESCSUB_MBIM:
> md = (struct mbim_descriptor *)desc;
> v = UGETW(md->bcdMBIMVersion);
> @@ -332,39 +328,29 @@ umb_attach(struct device *parent, struct
> goto fail;
> }
>
> - for (i = 0; i < sc->sc_udev->cdesc->bNumInterface; i++) {
> - if (usbd_iface_claimed(sc->sc_udev, i))
> - continue;
> - id = usbd_get_interface_descriptor(&sc->sc_udev->ifaces[i]);
> - if (id == NULL)
> - continue;
> - if (id->bInterfaceClass == UICLASS_CDC &&
> - id->bInterfaceSubClass ==
> - UISUBCLASS_MOBILE_BROADBAND_INTERFACE_MODEL) {
> - ctrl_iface = &sc->sc_udev->ifaces[i];
> - sc->sc_ctrl_ifaceno = id->bInterfaceNumber;
> - usbd_claim_iface(sc->sc_udev, i);
> - } else if (id->bInterfaceClass == UICLASS_CDC_DATA &&
> - id->bInterfaceSubClass == UISUBCLASS_DATA &&
> - id->bInterfaceProtocol == UIPROTO_DATA_MBIM) {
> - sc->sc_data_iface = &sc->sc_udev->ifaces[i];
> - data_ifaceno = id->bInterfaceNumber;
> - usbd_claim_iface(sc->sc_udev, i);
> - }
> - }
> - if (ctrl_iface == NULL) {
> - printf("%s: no control interface found\n", DEVNAM(sc));
> - goto fail;
> - }
> - if (sc->sc_data_iface == NULL) {
> + if (data_ifaceno < 0 || data_ifaceno >= uaa->nifaces) {
> printf("%s: no data interface found\n", DEVNAM(sc));
> goto fail;
> }
> + sc->sc_data_iface = uaa->ifaces[data_ifaceno];
>
> - id = usbd_get_interface_descriptor(ctrl_iface);
> + usbd_claim_iface(sc->sc_udev, uaa->ifaceno);
> + usbd_claim_iface(sc->sc_udev, data_ifaceno);
> +
> + /*
> + * If this is a combined NCM/MBIM function, switch to
> + * alternate setting one to enable MBIM.
> + */
> + id = usbd_get_interface_descriptor(uaa->iface);
> + if (id->bInterfaceClass == UICLASS_CDC &&
> + id->bInterfaceSubClass ==
> + UISUBCLASS_NETWORK_CONTROL_MODEL)
> + usbd_set_interface(uaa->iface, 1);
> +
> + id = usbd_get_interface_descriptor(uaa->iface);
> ctrl_ep = -1;
> for (i = 0; i < id->bNumEndpoints && ctrl_ep == -1; i++) {
> - ed = usbd_interface2endpoint_descriptor(ctrl_iface, i);
> + ed = usbd_interface2endpoint_descriptor(uaa->iface, i);
> if (ed == NULL)
> break;
> if (UE_GET_XFERTYPE(ed->bmAttributes) == UE_INTERRUPT &&
> @@ -376,23 +362,38 @@ umb_attach(struct device *parent, struct
> goto fail;
> }
>
> + /*
> + * For the MBIM Data Interface, select the appropriate
> + * alternate setting by looking for a matching descriptor that
> + * has two endpoints.
> + */
> cd = usbd_get_config_descriptor(sc->sc_udev);
> - id = usbd_get_interface_descriptor(sc->sc_data_iface);
> - altnum = usbd_get_no_alts(cd, id->bInterfaceNumber);
> - if (MBIM_INTERFACE_ALTSETTING >= altnum) {
> - printf("%s: missing alt setting %d for interface #%d\n",
> - DEVNAM(sc), MBIM_INTERFACE_ALTSETTING, data_ifaceno);
> + altnum = usbd_get_no_alts(cd, data_ifaceno);
> + for (i = 0; i < altnum; i++) {
> + id = usbd_find_idesc(cd, data_ifaceno, i);
> + if (id == NULL)
> + continue;
> + if (id->bInterfaceClass == UICLASS_CDC_DATA &&
> + id->bInterfaceSubClass == UISUBCLASS_DATA &&
> + id->bInterfaceProtocol == UIPROTO_DATA_MBIM &&
> + id->bNumEndpoints == 2)
> + break;
> + }
> + if (i == altnum || id == NULL) {
> + printf("%s: missing alt setting for interface #%d\n",
> + DEVNAM(sc), data_ifaceno);
> goto fail;
> }
> - sc->sc_rx_ep = sc->sc_tx_ep = -1;
> - if ((status = usbd_set_interface(sc->sc_data_iface,
> - MBIM_INTERFACE_ALTSETTING))) {
> + status = usbd_set_interface(sc->sc_data_iface, i);
> + if (status) {
> printf("%s: select alt setting %d for interface #%d "
> - "failed: %s\n", DEVNAM(sc), MBIM_INTERFACE_ALTSETTING,
> - data_ifaceno, usbd_errstr(status));
> + "failed: %s\n", DEVNAM(sc), i, data_ifaceno,
> + usbd_errstr(status));
> goto fail;
> }
> +
> id = usbd_get_interface_descriptor(sc->sc_data_iface);
> + sc->sc_rx_ep = sc->sc_tx_ep = -1;
> for (i = 0; i < id->bNumEndpoints; i++) {
> if ((ed = usbd_interface2endpoint_descriptor(sc->sc_data_iface,
> i)) == NULL)
> @@ -420,7 +421,7 @@ umb_attach(struct device *parent, struct
> USB_TASK_TYPE_GENERIC);
> timeout_set(&sc->sc_statechg_timer, umb_statechg_timeout, sc);
>
> - if (usbd_open_pipe_intr(ctrl_iface, ctrl_ep, USBD_SHORT_XFER_OK,
> + if (usbd_open_pipe_intr(uaa->iface, ctrl_ep, USBD_SHORT_XFER_OK,
> &sc->sc_ctrl_pipe, sc, &sc->sc_intr_msg, sizeof (sc->sc_intr_msg),
> umb_intr, USBD_DEFAULT_INTERVAL)) {
> printf("%s: failed to open control pipe\n", DEVNAM(sc));
>
