From patchwork Tue Oct 13 11:40:08 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: usb-linux: return USB_RET_STALL on -EPIPE From: Paul Bolle X-Patchwork-Id: 35845 Message-Id: <1255434008.1817.40.camel@localhost.localdomain> To: qemu-devel@nongnu.org Cc: Anthony Liguori , Mark Burkley , Max Krasnyansky Date: Tue, 13 Oct 2009 13:40:08 +0200 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 Tested-by: Paul Bolle --- 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;