Re: Huawei ME906s-158 LTE, cdce(4) vs umb(4)

[Gerhard Roth]

On 3/28/21 3:16 PM, Stuart Henderson wrote:

On 2021/03/28 13:40, Patrick Wildt wrote:

Am Sun, Mar 28, 2021 at 10:53:53AM +0100 schrieb Stuart Henderson:

On 2021/03/25 00:14, Stuart Henderson wrote:

On 2021/03/25 00:30, Patrick Wildt wrote:

Without having looked at anything, it might be worth looking at the most
recent mail in this thread:

'Re: [PATCH] umb(4) fix for X20 (DW5821e) in Dell Latitude 7300'

oh, usb runs through all drivers looking for a VID/PID match before
then running through all looking for a class match? I didn't realise
(thought it would try driver-by-driver with first VID/PID then class)
but that does make sense.

Updated diff below adding my pid/vid and tweaking some comments. My card
attaches to umb with this. I've only added it commented-out to the manual
for now until I see it actually pass traffic.

So this fixes Bryan's, at least improves mine (will look for another
sim / sim-adapter tomorrow), and seems targetted enough to not risk
fallout with existing working devices.

I think this makes sense to commit. OK with me if you'd like to commit
it Gerhard (I think it was your diff originally).

So this isn't enough for the Huawei yet but it doesn't make things
any worse, and helps for DW5821e.

If Gerhard isn't around, can I commit this bit so I'm not wrangling
things which should be separate commits? OK?

Yes, please do.  ok patrick@

Thanks, done.

Since it looks like we're going to need additional quirks to get Huawei
to work and having three separate vid/pid tables seems silly, here's a
diff to change to using a single pid/vid table covering the matches and
the existing fccauth quirk.

Tested with EM7455 (fcc auth required; works as before) and Huawei
(attaches to the correct config descriptor; packet transfer not working
but this is not unexpected).

Hello Stuart,

this is fine with me. ok gerhard@

Index: if_umb.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.38
diff -u -p -r1.38 if_umb.c
--- if_umb.c    28 Mar 2021 12:08:58 -0000      1.38
+++ if_umb.c    28 Mar 2021 13:10:56 -0000
@@ -224,34 +224,34 @@ 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, e.g. to override a class-based match to another
- * driver.
- *
- * OTOH, some devices identify themselves as an MBIM device but fail to speak
- * the MBIM protocol.
- */
-struct umb_products {
+struct umb_quirk {
        struct usb_devno         dev;
-       int                      confno;
+       u_int32_t                umb_flags;
+       int                      umb_confno;
+       int                      umb_match;
  };
-const struct umb_products umb_devs[] = {
-       { { USB_VENDOR_DELL, USB_PRODUCT_DELL_DW5821E }, 2 },
-       { { USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_ME906S }, 3 },
+const struct umb_quirk umb_quirks[] = {
+       { { USB_VENDOR_DELL, USB_PRODUCT_DELL_DW5821E },
+         0,
+         2,
+         UMATCH_VENDOR_PRODUCT
+       },
+
+       { { USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_ME906S },
+         0,
+         3,
+         UMATCH_VENDOR_PRODUCT
+       },
+
+       { { USB_VENDOR_SIERRA, USB_PRODUCT_SIERRA_EM7455 },
+         UMBFLG_FCC_AUTH_REQUIRED,
+         0,
+         0
+       },
  };

#define umb_lookup(vid, pid) \

-       ((const struct umb_products *)usb_lookup(umb_devs, vid, pid))
-
-/*
- * These devices require an "FCC Authentication" command.
- */
-const struct usb_devno umb_fccauth_devs[] = {
-       { USB_VENDOR_SIERRA, USB_PRODUCT_SIERRA_EM7455 },
-};
+       ((const struct umb_quirk *)usb_lookup(umb_quirks, vid, pid))

uint8_t umb_qmi_alloc_cid[] = {

        0x01,
@@ -283,10 +283,12 @@ int
  umb_match(struct device *parent, void *match, void *aux)
  {
        struct usb_attach_arg *uaa = aux;
+       const struct umb_quirk *quirk;
        usb_interface_descriptor_t *id;

- if (umb_lookup(uaa->vendor, uaa->product) != NULL)

-               return UMATCH_VENDOR_PRODUCT;
+       quirk = umb_lookup(uaa->vendor, uaa->product);
+       if (quirk != NULL && quirk->umb_match)
+               return (quirk->umb_match);
        if (!uaa->iface)
                return UMATCH_NONE;
        if ((id = usbd_get_interface_descriptor(uaa->iface)) == NULL)
@@ -317,6 +319,7 @@ umb_attach(struct device *parent, struct
  {
        struct umb_softc *sc = (struct umb_softc *)self;
        struct usb_attach_arg *uaa = aux;
+       const struct umb_quirk *quirk;
        usbd_status status;
        struct usbd_desc_iter iter;
        const usb_descriptor_t *desc;
@@ -339,13 +342,27 @@ umb_attach(struct device *parent, struct
        sc->sc_ctrl_ifaceno = uaa->ifaceno;
        ml_init(&sc->sc_tx_ml);

+ quirk = umb_lookup(uaa->vendor, uaa->product);

+       if (quirk != NULL && quirk->umb_flags) {
+               DPRINTF("%s: setting flags 0x%x from quirk\n", DEVNAM(sc),
+                    quirk->umb_flags);
+               sc->sc_flags |= quirk->umb_flags;
+       }
+
+       /*
+        * 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, e.g. to override a class-based
+        * match to another driver.
+        */
        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;
+               if (quirk == NULL) {
+                       printf("%s: unknown configuration for vid/pid match\n",
+                           DEVNAM(sc));
+                       goto fail;
+               }
+               uaa->configno = quirk->umb_confno;
                DPRINTF("%s: switching to config #%d\n", DEVNAM(sc),
                    uaa->configno);
                status = usbd_set_config_no(sc->sc_udev, uaa->configno, 1);
@@ -357,7 +374,7 @@ umb_attach(struct device *parent, struct
                usbd_delay_ms(sc->sc_udev, 200);

/*

-                * Need to do some manual setups that usbd_probe_and_attach()
+                * Need to do some manual setup that usbd_probe_and_attach()
                 * would do for us otherwise.
                 */
                uaa->nifaces = uaa->device->cdesc->bNumInterfaces;
@@ -435,10 +452,8 @@ umb_attach(struct device *parent, struct
                printf("%s: missing MBIM descriptor\n", DEVNAM(sc));
                goto fail;
        }
-       if (usb_lookup(umb_fccauth_devs, uaa->vendor, uaa->product)) {
-               sc->sc_flags |= UMBFLG_FCC_AUTH_REQUIRED;
+       if (sc->sc_flags & UMBFLG_FCC_AUTH_REQUIRED)
                sc->sc_cid = -1;
-       }

for (i = 0; i < uaa->nifaces; i++) {

                if (usbd_iface_claimed(sc->sc_udev, i))