[PATCH 2.4.32] usb-uhci.c failing "-"

[PATCH 2.4.32] usb-uhci.c failing "-"

[Guennadi Liakhovetski]

Hi

Looks like a bug?

Thanks
Guennadi
---------------------------------
Guennadi Liakhovetski, Ph.D.
DSA Daten- und Systemtechnik GmbH
Pascalstr. 28
D-52076 Aachen
Germany

Signed-off-by Guennadi Liakhovetski <g.liakhovetski@gmx.de>

--- a/drivers/usb/host/usb-uhci.c	Fri Jan 20 09:27:50 2006
+++ b/drivers/usb/host/usb-uhci.c	Fri Jan 20 09:28:05 2006
@@ -2505,7 +2505,7 @@
  			((urb_priv_t*)urb->hcpriv)->flags=0;
  		}

-		if ((urb->status != -ECONNABORTED) && (urb->status != ECONNRESET) &&
+		if ((urb->status != -ECONNABORTED) && (urb->status != -ECONNRESET) &&
  			    (urb->status != -ENOENT)) {

  			urb->status = -EINPROGRESS;

Re: [PATCH 2.4.32] usb-uhci.c failing "-"

[Willy Tarreau]

On Fri, Jan 20, 2006 at 09:33:26AM +0100, Guennadi Liakhovetski wrote:
> Hi
> 
> Looks like a bug?

Looks like you're right.

Marcelo, I've merged it into -upstream.

> Thanks
> Guennadi

Thanks,
Willy

> ---------------------------------
> Guennadi Liakhovetski, Ph.D.
> DSA Daten- und Systemtechnik GmbH
> Pascalstr. 28
> D-52076 Aachen
> Germany
> 
> Signed-off-by Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> --- a/drivers/usb/host/usb-uhci.c	Fri Jan 20 09:27:50 2006
> +++ b/drivers/usb/host/usb-uhci.c	Fri Jan 20 09:28:05 2006
> @@ -2505,7 +2505,7 @@
>  			((urb_priv_t*)urb->hcpriv)->flags=0;
>  		}
> 
> -		if ((urb->status != -ECONNABORTED) && (urb->status != 
> ECONNRESET) &&
> +		if ((urb->status != -ECONNABORTED) && (urb->status != 
> -ECONNRESET) &&
>  			    (urb->status != -ENOENT)) {
> 
>  			urb->status = -EINPROGRESS;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2.4.32] usb-uhci.c failing "-"

[Pete Zaitcev]

On Fri, 20 Jan 2006 09:33:26 +0100 (CET), Guennadi Liakhovetski <gl@dsa-ac.de> wrote:

> Looks like a bug?

> --- a/drivers/usb/host/usb-uhci.c	Fri Jan 20 09:27:50 2006
> +++ b/drivers/usb/host/usb-uhci.c	Fri Jan 20 09:28:05 2006
> @@ -2505,7 +2505,7 @@
>   			((urb_priv_t*)urb->hcpriv)->flags=0;
>   		}
> 
> -		if ((urb->status != -ECONNABORTED) && (urb->status != ECONNRESET) &&
> +		if ((urb->status != -ECONNABORTED) && (urb->status != -ECONNRESET) &&
>   			    (urb->status != -ENOENT)) {

This is not what the author intended, obviously. But I am not quite sure
what happens because of it. Seems like we unlink some things which are
about to return anyway... and then return -104 instead of -84. This
may be relatively harmless. At worst, the driver resubmits and gets
its -84 that way.

I vote to apply this and see what happens. We are early in 2.4.33 cycle,
so it should be safe.

-- Pete

Re: [PATCH 2.4.32] usb-uhci.c failing "-"

[Guennadi Liakhovetski]

On Fri, 20 Jan 2006, Pete Zaitcev wrote:

> On Fri, 20 Jan 2006 09:33:26 +0100 (CET), Guennadi Liakhovetski <gl@dsa-ac.de> wrote:
> 
> > Looks like a bug?
> 
> > --- a/drivers/usb/host/usb-uhci.c	Fri Jan 20 09:27:50 2006
> > +++ b/drivers/usb/host/usb-uhci.c	Fri Jan 20 09:28:05 2006
> > @@ -2505,7 +2505,7 @@
> >   			((urb_priv_t*)urb->hcpriv)->flags=0;
> >   		}
> > 
> > -		if ((urb->status != -ECONNABORTED) && (urb->status != ECONNRESET) &&
> > +		if ((urb->status != -ECONNABORTED) && (urb->status != -ECONNRESET) &&
> >   			    (urb->status != -ENOENT)) {
> 
> This is not what the author intended, obviously. But I am not quite sure
> what happens because of it. Seems like we unlink some things which are

This is my concirn too. The current behaviour is in fact just

> > -		if ((urb->status != -ECONNABORTED) && (urb->status != ECONNRESET) &&
> > +           if ((urb->status != -ECONNABORTED) &&
> >                         (urb->status != -ENOENT)) {

and nobody complains...:-) So, maybe this would be the right fix? At least 
safe in that it cannot break anything:-) But I don't understand that code 
very well. E.g., I don't understand why about 15 lines above the code in 
question

		if (urb->complete) {
			//dbg("process_interrupt: calling completion, status %i",status);
			urb->status = status;

i.e., if (!urb->completion) urb->status is not set, so, depending on 
whether the urb has ->completion either the old or the new status will be 
tested. Is this really correct? And a couple lines above that "goto 
recycle;" is superfluous...

Thanks
Guennadi

> about to return anyway... and then return -104 instead of -84. This
> may be relatively harmless. At worst, the driver resubmits and gets
> its -84 that way.
> 
> I vote to apply this and see what happens. We are early in 2.4.33 cycle,
> so it should be safe.
> 
> -- Pete
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

---
Guennadi Liakhovetski

Re: [PATCH 2.4.32] usb-uhci.c failing "-"

[Pete Zaitcev]

On Sun, 22 Jan 2006 01:15:23 +0100 (CET), Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> i.e., if (!urb->completion) urb->status is not set, so, depending on 
> whether the urb has ->completion either the old or the new status will be 
> tested. Is this really correct? And a couple lines above that "goto 
> recycle;" is superfluous...

I would not recommend to get too excercised over uhci-hcd in 2.4.32.
I do not with to touch it, because it mostly works, and the risk of
regressions is too great.

-- Pete