On Sun, 22 Sep 2013, Kurt Garloff wrote: > Hi, > > USB devio rejects control messages when the index does not have the > direction bit set correctly. I wouldn't describe it that way. It would be more accurate to say that the message is rejected when wIndex contains an invalid endpoint address. > This breaks windows apps in KVM -- and might be overly strict according > to my reading of USB HID spec. It is not overly strict. > Attached patch makes the kernel tolerant against it and makes the app > work for me. > > More details in the patch header. > > USB experts: Please review this and judge whether this is correct, > applies more generically, > or maybe needs to be special cased (only for USB HID devices?) or > implemented as quirk > or module/kernel parameter. The patch seems reasonable enough, although the description needs improvement and a couple of minor things should be fixed. More comments below. In the future, please put patches inline with the rest of the email message. Don't make them attachments unless you have to; that way they become harder to read and comment on. > Once in the final form, this *might* be stable material. Yes, it should be. > commit bc1e4e1ae1d5a4f9b2d263f22c651dd5ba4f8ff9 > Author: Kurt Garloff <kurt@xxxxxxxxxx> > Date: Sun Sep 22 11:54:59 2013 +0200 > > From: Kurt Garloff <kurt@xxxxxxxxxx> > Subject: Tolerate wrong direction bit endpoint index for control messages > Signed-off-by: Kurt Garloff <kurt@xxxxxxxxxx> > > Trying to read data from the Pegasus Technologies NoteTaker (0e20:0101) > [1] with the Windows App (EasyNote) works natively but fails when > WIndows is running under KVM (and the USB device handed to KVM). > > The reason is a USB control message > usb 4-2.2: control urb: bRequestType=22 bRequest=09 wValue=0200 wIndex=0001 wLength=0008 > This goes to endpoint 1 (wIndex), however, the endpoint is an input > endpoint and thus enumerated 0x81. > > The kernel thus rejects the IO and thus we see the failure. > > Apparently, Linux is more strict here than Windows ... we can't change > the Win app easily, so that's a problem. Indeed, this indicates that the device itself is also buggy. If it worked correctly, it would reject the incorrect control message. > However, the app might not even be buggy here. Browsing the USB HID > spec, there's a note that the bit for direction is ignored for control > endpoints ... so Linux might be overly strict? No, Linux is correct. While it is true that the direction bit is ignored for control endpoints, in this case we are talking about endpoint 1, which is not a control endpoint. In an HID device, it is almost certainly an interrupt endpoint. > With attached patch, everything works. > usb 4-2.2: check_ctrlrecip: process 13073 (qemu-kvm) requesting ep 01 but needs 81 (or 00) > > Notes: > - I have not checked whether the ignorance of the direction bit for > control endpoints only applies to USB HID devices, thus I have not > special-cased depending on the device type. It applies to all devices, but it isn't relevant here. > - We do output a warning into the kernel log, as this might still be > caused by an application error. > > - We don't risk sending to a wrong endpoint, as there can't be two > endpoints with same index and just different direction. Of course there can be. It is entirely possible to have one endpoint with address 0x01 and another with address 0x81. However, your patch doesn't run into this problem. If both endpoints exist, the routine will use the one with the address given by wIndex and ignore the other -- your new code won't run at all. > - I suspect this will mostly affect apps in virtual environments; as on > Linux the apps would have been adapted to the stricter handling of the > kernel. I have done that for mine[2], although delaying the release by > two years .... > > [1} http://www.pegatech.com/ > [2] https://sourceforge.net/projects/notetakerpen/ > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > index c2f62a3..8acbc2f 100644 > --- a/drivers/usb/core/devio.c > +++ b/drivers/usb/core/devio.c > @@ -623,6 +623,19 @@ static int check_ctrlrecip(struct dev_state *ps, unsigned int requesttype, > switch (requesttype & USB_RECIP_MASK) { > case USB_RECIP_ENDPOINT: > ret = findintfep(ps->dev, index); > + if (ret < 0) { > + /* OK, give it a second try -- user might have confused > + * direction -- this happens from virtualized win apps > + * e.g. -- warn, but be graceful */ /* * The accepted format for multiline comments * looks like this. */ > + ret = findintfep(ps->dev, index ^ 0x80); > + if (ret >= 0) > + dev_info(&ps->dev->dev , > + "%s: process %i (%s) requesting ep %02x but needs %02x (or 00)\n", Why do you have the "(or 00)" part? It doesn't seem to make any sense. > + __func__, > + task_pid_nr(current), > + current->comm, > + index, index ^ 0x80); > + } > if (ret >= 0) > ret = checkintf(ps, ret); > break; Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
