On Wed, Aug 19, 2020 at 01:21:35PM +0200, Marcus Glocker wrote:
> On Wed, 19 Aug 2020 12:02:23 +0200
> Martin Pieuchot <[email protected]> wrote:
>
> > On 18/08/20(Tue) 18:53, Marcus Glocker wrote:
> > > On Wed, 12 Aug 2020 21:39:15 +0200
> > > Marcus Glocker <[email protected]> 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.
OK, I've tracked the two lsusb requests down finally which also stall this
device beside our set idle call in the kernel.
UR_GET_DESCRIPTOR, UDESC_DEVICE_QUALIFIER:
ret = usb_control_msg(fd, LIBUSB_ENDPOINT_IN |
LIBUSB_REQUEST_TYPE_STANDARD | LIBUSB_RECIPIENT_DEVICE,
LIBUSB_REQUEST_GET_DESCRIPTOR,
USB_DT_DEBUG << 8, 0,
buf, sizeof buf, CTRL_TIMEOUT);
UR_GET_DESCRIPTOR, UDESC_DEBUG:
ret = usb_control_msg(fd, LIBUSB_ENDPOINT_IN |
LIBUSB_REQUEST_TYPE_STANDARD | LIBUSB_RECIPIENT_DEVICE,
LIBUSB_REQUEST_GET_DESCRIPTOR,
USB_DT_DEBUG << 8, 0,
buf, sizeof buf, CTRL_TIMEOUT);
When you comment those two control requests out, lsusb -v runs through.
If I wouldn't know better, I would say that this device isn't able to
handle UR_SET_IDLE, UDESC_DEVICE_QUALIFIER, and UDESC_DEBUG requests.
But what still puzzles me is why the UR_SET_IDLE request works on
ehci(4). Would you have any explanation for that?
> > > 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
> > >
>
--
[ Marcus Glocker, [email protected], http://nazgul.ch ]
