[Qemu-devel] [PATCH] usb-linux: return USB_RET_STALL on -EPIPE

[Paul Bolle <pebolle@tiscali.nl>]

0) This is an attempt to get an issue in usb-linux.c, for which a patch
was posted about a year ago, finally fixed.

1) Mark Burkley submitted a "EHCI emulation module" for review in in
October 2008 (see:
http://lists.gnu.org/archive/html/qemu-devel/2008-10/msg01326.html). No
EHCI emulation module was ever committed to qemu.

2) Part of that (large) patch was a fix for a separate issue in
usb-linux.c. Max Krasnyansky has ACK'ed that fix (see:
http://lists.gnu.org/archive/html/qemu-devel/2008-11/msg00032.html).

3) I already asked whether this fix was ready to be committed in last
April (see:
http://lists.gnu.org/archive/html/qemu-devel/2009-04/msg01763.html)

4) Maybe submitting this fix as a separate patch (with a really long
commit message but without a Signed-off-by) and cc-ing everbody involved
will help if actually getting this issue fixed.


Paul Bolle
---
usb-linux: return USB_RET_STALL on -EPIPE

Max Krasnyansky wrote:
> Anthony Liguori wrote:
>> Mark Burkley wrote:
>>> Hi Anthony,
>>> [...]
>>> I am seeing an issue with a
>>> memory key I am using for testing.  ioctl returns EPIPE (which I
>>> would have thought was a STALL) to an asynchronous IN completion in
>>> usb-linux.c but then this is returned as USB_RET_NAK to EHCI which
>>> confuses my WinXP target because the transfer is then never
>>> completed.
>>>
>>> Can I just check that it was intentional to return NAK for EPIPE
>>> returns in asynchronous completions?  If so, then I will try to
>>> detect the stall in my implementation and treat differently to a
>>> NAK.  It's just that if I modify usb-linux.c to return
>>> USB_RET_STALL on -EPIPE then it works fine.
>
> I just looked at the usb-linuc.c:async_complete() code and it looks like
> it was intentional but I cannot remember why I wrote that way. And what
> you're saying makes sense. ie It should be a STALL. In fact I think that
> might fix the regression with USB storage devices that some people have
> reported.
> I'll play some more with this later today. I want to make sure that the
> change we're talking about does not break existing devices that I
> thoroughly tested as part of the usb async re-write. If everything works
> as expected then we'll change it.

Ok. I just tested that change (ie returning STALL instead of NAK on EPIPE)
with a bunch of devices: USB serial adapter, CF card reader, USB webcam (MS
VX-3000) and MS USB mouse. All that stuff was hooked up to XP-SP3 and all of
them are perfectly usable at the same time.

In other words here is my ACK :)

Acked-by: Max Krasnyansky <maxk@qualcomm.com>
Tested-by: Paul Bolle <pebolle@tiscali.nl>
---
 usb-linux.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 9e5d9c4..d712134 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -275,7 +275,9 @@ static void async_complete(void *opaque)
 
             case -EPIPE:
                 set_halt(s, p->devep);
-                /* fall through */
+		p->len = USB_RET_STALL;
+		break;
+
             default:
                 p->len = USB_RET_NAK;
                 break;
-- 
1.6.5.rc2