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). 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))