On Wed, 19 Aug 2020 20:31:05 +0200 Marcus Glocker <[email protected]> wrote:
> 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. I did read the SET_IDLE HID specs in the meantime, and I also think that SET_IDLE only makes sense to be applied on HID input devices like keyboards and mouse-like devices. I went through our drivers which attach to uhidev(4) and I would apply SET_IDLE to: ukbd(4) ums(4) umstc(4) umt(4) utpms(4) uwacom(4) I wouldn't apply it to the generic HID driver uhid(4), since this would re-introduce the issue with audio devices, since their HID usually attaches to uhid(4). I also didn't apply it to ubcmtp(4) since I don't believe it attaches to uhidev(4), see my other mail from yesterday to tech@. Also we could use the reportid going forward in usbd_set_idle() instead of always passing 0. According to the HID specs there can be multiple reportids, all having their individual idle settings. Would that be an approach to start with? Index: sys/dev/usb/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 --- sys/dev/usb/uhidev.c 31 Jul 2020 10:49:33 -0000 1.80 +++ sys/dev/usb/uhidev.c 21 Aug 2020 09:21:58 -0000 @@ -151,8 +151,6 @@ 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); - sc->sc_iep_addr = sc->sc_oep_addr = -1; for (i = 0; i < id->bNumEndpoints; i++) { ed = usbd_interface2endpoint_descriptor(sc->sc_iface, i); Index: sys/dev/usb/ukbd.c =================================================================== RCS file: /cvs/src/sys/dev/usb/ukbd.c,v retrieving revision 1.78 diff -u -p -u -p -r1.78 ukbd.c --- sys/dev/usb/ukbd.c 12 May 2017 09:16:55 -0000 1.78 +++ sys/dev/usb/ukbd.c 21 Aug 2020 09:21:58 -0000 @@ -227,6 +227,8 @@ ukbd_attach(struct device *parent, struc sc->sc_hdev.sc_udev = uha->uaa->device; sc->sc_hdev.sc_report_id = uha->reportid; + usbd_set_idle(uha->parent->sc_udev, uha->parent->sc_ifaceno, 0, 0); + uhidev_get_report_desc(uha->parent, &desc, &dlen); repid = uha->reportid; sc->sc_hdev.sc_isize = hid_report_size(desc, dlen, hid_input, repid); Index: sys/dev/usb/ums.c =================================================================== RCS file: /cvs/src/sys/dev/usb/ums.c,v retrieving revision 1.44 diff -u -p -u -p -r1.44 ums.c --- sys/dev/usb/ums.c 17 Jun 2020 23:43:08 -0000 1.44 +++ sys/dev/usb/ums.c 21 Aug 2020 09:21:58 -0000 @@ -129,6 +129,8 @@ ums_attach(struct device *parent, struct sc->sc_hdev.sc_udev = uaa->device; sc->sc_hdev.sc_report_id = uha->reportid; + usbd_set_idle(uha->parent->sc_udev, uha->parent->sc_ifaceno, 0, 0); + quirks = usbd_get_quirks(sc->sc_hdev.sc_udev)->uq_flags; uhidev_get_report_desc(uha->parent, &desc, &size); Index: sys/dev/usb/umstc.c =================================================================== RCS file: /cvs/src/sys/dev/usb/umstc.c,v retrieving revision 1.1 diff -u -p -u -p -r1.1 umstc.c --- sys/dev/usb/umstc.c 31 May 2020 18:15:37 -0000 1.1 +++ sys/dev/usb/umstc.c 21 Aug 2020 09:21:58 -0000 @@ -30,6 +30,7 @@ #include <dev/usb/usb.h> #include <dev/usb/usbhid.h> #include <dev/usb/usbdi.h> +#include <dev/usb/usbdi_util.h> #include <dev/usb/usbdevs.h> #include <dev/usb/uhidev.h> @@ -98,6 +99,8 @@ umstc_attach(struct device *parent, stru sc->sc_hdev.sc_parent = uha->parent; sc->sc_hdev.sc_udev = uaa->device; sc->sc_hdev.sc_report_id = uha->reportid; + + usbd_set_idle(uha->parent->sc_udev, uha->parent->sc_ifaceno, 0, 0); uhidev_get_report_desc(uha->parent, &desc, &size); repid = uha->reportid; Index: sys/dev/usb/umt.c =================================================================== RCS file: /cvs/src/sys/dev/usb/umt.c,v retrieving revision 1.1 diff -u -p -u -p -r1.1 umt.c --- sys/dev/usb/umt.c 25 Aug 2018 20:31:31 -0000 1.1 +++ sys/dev/usb/umt.c 21 Aug 2020 09:21:58 -0000 @@ -29,6 +29,7 @@ #include <dev/usb/usb.h> #include <dev/usb/usbhid.h> #include <dev/usb/usbdi.h> +#include <dev/usb/usbdi_util.h> #include <dev/usb/usbdevs.h> #include <dev/usb/uhidev.h> @@ -148,6 +149,8 @@ umt_attach(struct device *parent, struct sc->sc_hdev.sc_intr = umt_intr; sc->sc_hdev.sc_parent = uha->parent; + + usbd_set_idle(uha->parent->sc_udev, uha->parent->sc_ifaceno, 0, 0); uhidev_get_report_desc(uha->parent, &desc, &size); umt_find_winptp_reports(uha->parent, desc, size, sc); Index: sys/dev/usb/utpms.c =================================================================== RCS file: /cvs/src/sys/dev/usb/utpms.c,v retrieving revision 1.7 diff -u -p -u -p -r1.7 utpms.c --- sys/dev/usb/utpms.c 5 Jun 2016 20:02:36 -0000 1.7 +++ sys/dev/usb/utpms.c 21 Aug 2020 09:21:58 -0000 @@ -301,6 +301,8 @@ utpms_attach(struct device *parent, stru sc->sc_datalen = UTPMS_DATA_LEN; sc->sc_hdev.sc_udev = uha->uaa->device; + usbd_set_idle(uha->parent->sc_udev, uha->parent->sc_ifaceno, 0, 0); + /* Fill in device-specific parameters. */ if ((udd = usbd_get_device_descriptor(uha->parent->sc_udev)) != NULL) { product = UGETW(udd->idProduct); Index: sys/dev/usb/uwacom.c =================================================================== RCS file: /cvs/src/sys/dev/usb/uwacom.c,v retrieving revision 1.1 diff -u -p -u -p -r1.1 uwacom.c --- sys/dev/usb/uwacom.c 12 Sep 2016 08:12:06 -0000 1.1 +++ sys/dev/usb/uwacom.c 21 Aug 2020 09:21:58 -0000 @@ -26,6 +26,7 @@ #include <dev/usb/usbhid.h> #include <dev/usb/usbdi.h> +#include <dev/usb/usbdi_util.h> #include <dev/usb/usbdevs.h> #include <dev/usb/uhidev.h> @@ -101,6 +102,8 @@ uwacom_attach(struct device *parent, str sc->sc_hdev.sc_parent = uha->parent; sc->sc_hdev.sc_udev = uaa->device; sc->sc_hdev.sc_report_id = uha->reportid; + + usbd_set_idle(uha->parent->sc_udev, uha->parent->sc_ifaceno, 0, 0); uhidev_get_report_desc(uha->parent, &desc, &size); repid = uha->reportid;
