A question below..
On Wed, Feb 03, 2021 at 11:22:16AM +0000, Mikolaj Kucharski wrote:
> 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.
> >
I would probably ask, what is the meaning of ifaceno?
Is that variable an index in the array or is it bInterfaceNumber?
>From my understanding, with -r1.110 of usbdi.c it's both. From this
email thread, in various devices array index and bInterfaceNumber
don't always have the same value. They don't always match.
What users of usbd_device2interface_handle() function assume,
an array index or bInterfaceNumber for ifaceno argument?
> 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
