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.
> 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?
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?
Did you try passing a non-0 duration parameter to the SET_IDLE command?
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?
> 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?
> 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
>
