On 21/08/20(Fri) 11:46, Marcus Glocker wrote:
> 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?
Makes sense to me, ok mpi@
> 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;
