On Wed, 19 Aug 2020 12:02:23 +0200
Martin Pieuchot <m...@openbsd.org> wrote:
> On 18/08/20(Tue) 18:53, Marcus Glocker wrote:
> > On Wed, 12 Aug 2020 21:39:15 +0200
> > Marcus Glocker <mar...@nazgul.ch> wrote:
> >
> > > jmc was so nice to send me his trouble device over to do some
> > > further investigations. Just some updates on what I've noticed
> > > today:
> > >
> > > - The issue isn't specific to xhci(4). I also see the same issue
> > > on some of my ehci(4) machines when attaching this device.
> > >
> > > - It seems like the device gets in to an 'corrupted state' after
> > > running a couple of control transfer against it. Initially they
> > > work fine, with smaller and larger transfer sizes, and at one
> > > point the device hangs up and doesn't recover until re-attaching
> > > it. While on some ehci(4) machines the uhidev(4) attach works
> > > fine, after running lsusb against the device, I see transfer
> > > errors coming up again; On xhci(4) namely XHCI_CODE_TXERR.
> > >
> > > - Attaching an USB 2.0 hub doesn't make any difference, no matter
> > > if attached to an xhci(4) or an ehci(4) controller.
> > >
> > > Not sure what is going wrong with this little beast ...
> >
> > OK, I give up :-) Following my summary report.
> >
> > This device seems to have issues with two control request types:
> >
> > - UR_GET_STATUS, not called for this device from the kernel in
> > the default code path. But e.g. 'lsusb -v' will call it.
> >
> > - UR_SET_IDLE, as called in uhidev_attach().
> >
> > UR_GET_STATUS will stall the device for good on *all* controller
> > drivers.
>
> Does this also happen when the device attaches as ugen(4)? If yes
> that would rules out concurrency issues that might happen when using
> lsusb(1) while other transfers are in fly. To test you need to
> disable the current attaching driver in ukc.
Yes, it does also happen when attaching the device to ugen(4).
But honestly, I was playing around yesterday evening a bit further with
this device, and I noticed that the device also stalls with lsusb when I
remove the get status and get report request in the lsusb code.
Therefore I need to correct my statement, saying instead that *some*
request in lsusb makes the device stall as well. What I just found in
the lsusb ChangeLog:
Added (somewhat dummy) Set_Protocol and Set_Idle requests to stream
dumping setup.
I'll try to confirm if the stall really happens there. At least that
would be in line with our findings in the kernel.
> > UR_SET_IDLE works only on ehci(4) - Don't ask me why.
> > On all the other controller drivers the following UR_GET_REPORT
> > request will fail, stalling the device as well. I tried all kind
> > of things to get the UR_SET_IDLE request working on xhci(4), but
> > without any luck.
>
> Does the device respond to GET_IDLE?
No, same behaviour as for SET_IDLE, returns with USBD_STALLED.
> It it a timing problem? How much time does the device need to be
> idle? Does introducing a delay before and/or after usbd_set_idle()
> change the behavior?
I already tried 100ms delay before and after set idle, no change.
> Did you try passing a non-0 duration parameter to the SET_IDLE
> command?
Yes, I also already tried with various non-0 duration parameters, no
change.
> Taking a step back, why does a uaudio(4) needs a UR_SET_IDLE? This
> tells the device to only respond to IN interrupt transfers when new
> events occur, right? Does all devices attaching to uhidev want this
> behavior?
I don't know since I was not involved in that code yet.
Correct, that's what I did read in the HID specs as well.
uhidev(4) does call set idle statically for all devices during attach.
I was also questioning myself if that is the right thing, since for the
uaudio(4) devices I own, it seems to make no difference when skipping
the set idle request.
> > The good news is that when we skip the UR_SET_IDLE request on
> > xhci(4), the following UR_GET_REPORT request works, and isoc
> > transfers also work perfectly fine. You can use the device for
> > audio streaming.
> >
> > Therefore the only thing I can offer is a quirk to skip the
> > UR_SET_IDLE request when attaching this device. On ehci(4) the
> > device continues to work as before with this quirk. Therefore I
> > didn't include any code to only apply the quirk on non-ehci
> > controllers.
> >
> > I know it's not a nice solution, but at least it makes this device
> > usable on xhci(4) while not impacting other things.
>
> Maybe it is a step towards a real solution. Should usbd_set_idle()
> stay in uhidev(4) or, if it doesn't make sense for all devices, should
> we move it in child drivers like ukbd(4), etc?
I would also think that the set idle request is only required for
keyboard, mouse, etc., but honestly I'm too less familiar with the HID
specs.
> > If anyone is OK with that and has no better idea how to fix it, I'm
> > happy to commit.
> >
> > Cheers,
> > Marcus
> >
> >
> > Index: uhidev.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/uhidev.c,v
> > retrieving revision 1.80
> > diff -u -p -u -p -r1.80 uhidev.c
> > --- uhidev.c 31 Jul 2020 10:49:33 -0000 1.80
> > +++ uhidev.c 18 Aug 2020 13:36:13 -0000
> > @@ -151,7 +151,8 @@ uhidev_attach(struct device *parent, str
> > sc->sc_ifaceno = uaa->ifaceno;
> > id = usbd_get_interface_descriptor(sc->sc_iface);
> >
> > - usbd_set_idle(sc->sc_udev, sc->sc_ifaceno, 0, 0);
> > + if (!(usbd_get_quirks(uaa->device)->uq_flags &
> > UQ_NO_SET_IDLE))
> > + usbd_set_idle(sc->sc_udev, sc->sc_ifaceno, 0, 0);
> >
> > sc->sc_iep_addr = sc->sc_oep_addr = -1;
> > for (i = 0; i < id->bNumEndpoints; i++) {
> > Index: usb_quirks.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/usb_quirks.c,v
> > retrieving revision 1.76
> > diff -u -p -u -p -r1.76 usb_quirks.c
> > --- usb_quirks.c 5 Jan 2020 00:54:13 -0000 1.76
> > +++ usb_quirks.c 18 Aug 2020 13:36:13 -0000
> > @@ -52,6 +52,7 @@ const struct usbd_quirk_entry {
> > u_int16_t bcdDevice;
> > struct usbd_quirks quirks;
> > } usb_quirks[] = {
> > + { USB_VENDOR_MICROCHIP, USB_PRODUCT_MICROCHIP_SOUNDKEY, ANY, {
> > UQ_NO_SET_IDLE }}, { USB_VENDOR_KYE, USB_PRODUCT_KYE_NICHE,
> > 0x100, { UQ_NO_SET_PROTO}}, { USB_VENDOR_INSIDEOUT,
> > USB_PRODUCT_INSIDEOUT_EDGEPORT4, 0x094, { UQ_SWAP_UNICODE}},
> > Index: usb_quirks.h
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/usb_quirks.h,v
> > retrieving revision 1.16
> > diff -u -p -u -p -r1.16 usb_quirks.h
> > --- usb_quirks.h 19 Jul 2010 05:08:37 -0000 1.16
> > +++ usb_quirks.h 18 Aug 2020 13:36:13 -0000
> > @@ -49,6 +49,7 @@ struct usbd_quirks {
> > #define UQ_MS_LEADING_BYTE 0x00010000 /* mouse sends unknown
> > leading byte */ #define UQ_EHCI_NEEDTO_DISOWN 0x00020000 /*
> > must hand device over to USB 1.1 if attached to EHCI */
> > +#define UQ_NO_SET_IDLE 0x00040000 /* cannot handle
> > set idle request */ };
> >
> > extern const struct usbd_quirks usbd_no_quirk;
> > Index: usbdevs
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/usbdevs,v
> > retrieving revision 1.720
> > diff -u -p -u -p -r1.720 usbdevs
> > --- usbdevs 3 Aug 2020 14:25:44 -0000 1.720
> > +++ usbdevs 18 Aug 2020 13:36:14 -0000
> > @@ -3009,6 +3009,7 @@ product MGE UPS2 0xffff
> > UPS /* Microchip Technology, Inc. products */
> > product MICROCHIP USBLCD20X2 0x0002 USB-LCD-20x2
> > product MICROCHIP USBLCD256X64 0xc002 USB-LCD-256x64
> > +product MICROCHIP SOUNDKEY 0xf0bf Cyrus soundKey
> >
> > /* Microdia / Sonix Techonology Co., Ltd. products */
> > product MICRODIA YUREX 0x1010 YUREX
> >
