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

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

[Paul Bolle]

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

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

[Anthony Liguori]

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


-- 
Regards,

Anthony Liguori

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

[Paul Bolle]

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

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

[Anthony Liguori]

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.

-- 
Regards,

Anthony Liguori

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

[Paul Bolle]

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

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

[Anthony Liguori]

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

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

[Max Krasnyansky]

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

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

[Paul Bolle]

On Tue, 2009-10-13 at 10:23 -0700, Max Krasnyansky wrote: 
> On 10/13/2009 09:22 AM, Anthony Liguori wrote:
> > 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.

I guess this patch should make everybody happy.

I've replaced the verbose commit message with (basically) a longer
version of the commit summary. A search for the summary should turn up
this thread, which will in turn point to previous steps for those
readers really interested.
--- 
usb-linux: return USB_RET_STALL on -EPIPE

If -EPIPE is returned to an asynchronous IN completion async_complete()
should return USB_RET_STALL.

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
Cc: Mark Burkley <qemu@emutex.com>
Signed-off-by: Max Krasnyansky <maxk@qualcomm.com>
---
 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