Patchwork usb-linux: return USB_RET_STALL on -EPIPE

login
register
mail settings
Submitter Paul Bolle
Date Oct. 13, 2009, 11:40 a.m.
Message ID <1255434008.1817.40.camel@localhost.localdomain>
Download mbox | patch
Permalink /patch/35845/
State New
Headers show

Comments

Paul Bolle - Oct. 13, 2009, 11:40 a.m.
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(-)
Anthony Liguori - Oct. 13, 2009, 1:46 p.m.
Paul Bolle wrote:
> 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.
>   

Yeah, it's ashame that noone's followed up with this patch.

> 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.
>   

Yes, separate fixes should always be separate patches.

> 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>
>   

Someone needs to  provide a Signed-off-by.

> ---
>  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;
>
Paul Bolle - Oct. 13, 2009, 3:52 p.m.
On Tue, 2009-10-13 at 08:46 -0500, Anthony Liguori wrote:
> Paul Bolle wrote:
> > Acked-by: Max Krasnyansky <maxk@qualcomm.com>
> > Tested-by: Paul Bolle <pebolle@tiscali.nl>
> >   
> Someone needs to  provide a Signed-off-by.

Does it matter who actually signs off this patch?


Paul Bolle
Anthony Liguori - Oct. 13, 2009, 4:04 p.m.
Paul Bolle wrote:
> On Tue, 2009-10-13 at 08:46 -0500, Anthony Liguori wrote:
>   
>> Paul Bolle wrote:
>>     
>>> Acked-by: Max Krasnyansky <maxk@qualcomm.com>
>>> Tested-by: Paul Bolle <pebolle@tiscali.nl>
>>>   
>>>       
>> Someone needs to  provide a Signed-off-by.
>>     
>
> Does it matter who actually signs off this patch?
>   

Yes.
Paul Bolle - Oct. 13, 2009, 4:12 p.m.
On Tue, 2009-10-13 at 11:04 -0500, Anthony Liguori wrote:
> Paul Bolle wrote:
> > Does it matter who actually signs off this patch?
> >
> Yes.

So a Signed-off-by tag provided by me won't do?


Paul
Anthony Liguori - Oct. 13, 2009, 4:22 p.m.
Paul Bolle wrote:
> On Tue, 2009-10-13 at 11:04 -0500, Anthony Liguori wrote:
>   
>> Paul Bolle wrote:
>>     
>>> Does it matter who actually signs off this patch?
>>>
>>>       
>> Yes.
>>     
>
> So a Signed-off-by tag provided by me won't do?
>   

A SoB is a statement of intent so if you feel you can contribute a SoB 
is a decision for you to make.  See Documentation/SubmittingPatches in 
Linux for more details about the ramification of DCO.

Regards,

Anthony Liguori
Max Krasnyansky - Oct. 13, 2009, 5:23 p.m.
On 10/13/2009 09:22 AM, Anthony Liguori wrote:
> Paul Bolle wrote:
>> On Tue, 2009-10-13 at 11:04 -0500, Anthony Liguori wrote:
>>
>>> Paul Bolle wrote:
>>>
>>>> Does it matter who actually signs off this patch?
>>>>
>>>>
>>> Yes.
>>>
>> So a Signed-off-by tag provided by me won't do?
>>
>
> A SoB is a statement of intent so if you feel you can contribute a SoB
> is a decision for you to make.  See Documentation/SubmittingPatches in
> Linux for more details about the ramification of DCO.

I guess I could've provided the Signed-off-by instead of Acked-by.
In other words if you're ok with converting my Acked-By to
Signed-off-by in the aforementioned patch then lets go ahead an do it.

I could also resend the full patch but I'm totally swamped right now and 
might not get to doing it this week.

btw Paul you can also resend and sign off yourself and keep my ack.

Max

Patch

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;