On Wed, Feb 03, 2021 at 11:10:45AM +0000, Edd Barrett wrote:
> Hi,
>
> CCing ratchov@ and kettenis@ with some context.
>
> In short: my change broke ugen, which expects to scan up the interface
> range until an interface doesn't exist.
>
> On Wed, Feb 03, 2021 at 06:25:48AM +0100, Marcus Glocker wrote:
> >
> > Index: dev/usb/usbdi.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/usbdi.c,v
> > retrieving revision 1.109
> > diff -u -p -u -p -r1.109 usbdi.c
> > --- dev/usb/usbdi.c 1 Feb 2021 09:21:51 -0000 1.109
> > +++ dev/usb/usbdi.c 2 Feb 2021 06:07:41 -0000
> > @@ -642,6 +642,10 @@ usbd_device2interface_handle(struct usbd
> >
> > if (dev->cdesc == NULL)
> > return (USBD_NOT_CONFIGURED);
> > + if (ifaceno < dev->cdesc->bNumInterfaces) {
> > + *iface = &dev->ifaces[ifaceno];
> > + return (USBD_NORMAL_COMPLETION);
> > + }
> > /*
> > * The correct interface should be at dev->ifaces[ifaceno], but
> > we've
> > * seen non-compliant devices in the wild which present
> > non-contiguous
> >
> > So OK if I commit this fix Edd, Stuart?
>
> I'm OK with it as a quick-fix. At least it will make both of the devices
> in question work.
>
> But in the long run, it's not hard to imagine other non-compliant
> devices which would still be defeated by this code.
>
> Suppose a device presents its contiguous interfaces in reverse order, e.g.:
> [2, 1, 0]. Now suppose a device driver asks for interface 2. We will
> find interface 0, as we never check if it's the right interface and we
> never reach the part of the code that scans the array.
>
> In other words, just because an index exists, doesn't mean it's the right
> interface.
>
> I think (and I'm not much of a kernel hacker, so I reserve the right be wrong)
> the correct solution is to:
>
> * always loop over the array looking for the right interface.
> * change ugen, so that it's scanning resilient to gaps in interface range.
>
Not sure is your above proposition enough. Here is part of dmesg with
some debugging statments for 2 devices which trigger
usbd_device2interface_handle()
See MMM markers.
...
ulpt0 at uhub0 port 3 configuration 1 interface 1 "Samsung Electronics Co.,
Ltd. M2070 Series" rev 2.00/1.00 addr 2
ulpt0: using bi-directional mode
ugen0 at uhub0 port 3 configuration 1 "Samsung Electronics Co., Ltd. M2070
Series" rev 2.00/1.00 addr 2
MMM: USBD_NORMAL_COMPLETION v1 ifaceno=0 bNumInterfaces=2
[usbd_device2interface_handle()|usbdi.c|649]
uhub2 at uhub1 port 1 configuration 1 interface 0 "Advanced Micro Devices Hub"
rev 2.00/0.18 addr 2
umb0 at uhub2 port 3 configuration 1 interface 12 "Sierra Wireless,
Incorporated Sierra Wireless MC7455 Qualcomm\M-. Snapdragon? X7 LTE-A" rev
2.00/0.06 addr 3
ugen1 at uhub2 port 3 configuration 1 "Sierra Wireless, Incorporated Sierra
Wireless MC7455 Qualcomm\M-. Snapdragon? X7 LTE-A" rev 2.00/0.06 addr 3
MMM: USBD_NORMAL_COMPLETION v1 ifaceno=0 bNumInterfaces=5
[usbd_device2interface_handle()|usbdi.c|649]
MMM: USBD_NORMAL_COMPLETION v1 ifaceno=1 bNumInterfaces=5
[usbd_device2interface_handle()|usbdi.c|649]
MMM: USBD_NORMAL_COMPLETION v1 ifaceno=2 bNumInterfaces=5
[usbd_device2interface_handle()|usbdi.c|649]
vscsi0 at root
scsibus2 at vscsi0: 256 targets
...
For ugen1 (belongs to physical device, whih also attaches a umb(4)) we
can see that ifaceno has value of 0, 1, 2, but from lsusb we see that:
(from earliers Marcus Glocker's email)
Index Interface Number
----- ---------------
0 0
1 2
2 3
3 12
4 13
for ifaceno 1 and 2 it will not equal to bInterfaceNumber if we iterate
the for (idx = 0; idx < dev->cdesc->bNumInterfaces; idx++) loop.
I don't have good picture for how all the functions work with each
other, but it does feel like substantial work is needed here, to piece
everything together.
--
Regards,
Mikolaj
