[PATCH] USB: consolidate error values from EHCI,UHCI and OHCI _suspend()

[PATCH] USB: consolidate error values from EHCI,UHCI and OHCI _suspend()

[Jiri Kosina]

Hi,

EHCI, UHCI and OHCI USB host drivers are not consistent when returining 
error values from their _suspend() functions, in case that the device is 
not in suspended state. This could confuse users, so let all three of them 
return -EBUSY.

Patch against 2.6.18-rc6-mm2.

Signed-off-by: Jiri Kosina <jikos@jikos.cz>

--- linux-2.6.18-rc6-mm2.orig/drivers/usb/host/ehci-pci.c	2006-09-14 16:20:48.000000000 +0200
+++ linux-2.6.18-rc6-mm2/drivers/usb/host/ehci-pci.c	2006-09-19 03:20:22.000000000 +0200
@@ -232,7 +232,7 @@ static int ehci_pci_suspend(struct usb_h
 	 */
 	spin_lock_irqsave (&ehci->lock, flags);
 	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
+		rc = -EBUSY;
 		goto bail;
 	}
 	writel (0, &ehci->regs->intr_enable);
--- linux-2.6.18-rc6-mm2.orig/drivers/usb/host/ohci-pci.c	2006-09-14 16:20:48.000000000 +0200
+++ linux-2.6.18-rc6-mm2/drivers/usb/host/ohci-pci.c	2006-09-19 03:36:35.000000000 +0200
@@ -130,7 +130,7 @@ static int ohci_pci_suspend (struct usb_
 	 */
 	spin_lock_irqsave (&ohci->lock, flags);
 	if (hcd->state != HC_STATE_SUSPENDED) {
-		rc = -EINVAL;
+		rc = -EBUSY;
 		goto bail;
 	}
 	ohci_writel(ohci, OHCI_INTR_MIE, &ohci->regs->intrdisable);

-- 
Jiri Kosina

Re: [linux-usb-devel] [PATCH] USB: consolidate error values from EHCI, UHCI and OHCI _suspend()

[David Brownell]

On Monday 18 September 2006 6:44 pm, Jiri Kosina wrote:
> Hi,
> 
> EHCI, UHCI and OHCI USB host drivers are not consistent when returining 
> error values from their _suspend() functions, in case that the device is 
> not in suspended state. This could confuse users, so let all three of them 
> return -EBUSY.

Shouldn't you also update uhci_suspend()?  Currently it
just ignores hcd->state ...


> Patch against 2.6.18-rc6-mm2.
> 
> Signed-off-by: Jiri Kosina <jikos@jikos.cz>
> 
> --- linux-2.6.18-rc6-mm2.orig/drivers/usb/host/ehci-pci.c	2006-09-14 16:20:48.000000000 +0200
> +++ linux-2.6.18-rc6-mm2/drivers/usb/host/ehci-pci.c	2006-09-19 03:20:22.000000000 +0200
> @@ -232,7 +232,7 @@ static int ehci_pci_suspend(struct usb_h
>  	 */
>  	spin_lock_irqsave (&ehci->lock, flags);
>  	if (hcd->state != HC_STATE_SUSPENDED) {
> -		rc = -EINVAL;
> +		rc = -EBUSY;
>  		goto bail;
>  	}
>  	writel (0, &ehci->regs->intr_enable);
> --- linux-2.6.18-rc6-mm2.orig/drivers/usb/host/ohci-pci.c	2006-09-14 16:20:48.000000000 +0200
> +++ linux-2.6.18-rc6-mm2/drivers/usb/host/ohci-pci.c	2006-09-19 03:36:35.000000000 +0200
> @@ -130,7 +130,7 @@ static int ohci_pci_suspend (struct usb_
>  	 */
>  	spin_lock_irqsave (&ohci->lock, flags);
>  	if (hcd->state != HC_STATE_SUSPENDED) {
> -		rc = -EINVAL;
> +		rc = -EBUSY;
>  		goto bail;
>  	}
>  	ohci_writel(ohci, OHCI_INTR_MIE, &ohci->regs->intrdisable);
> 
> -- 
> Jiri Kosina
> 
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys -- and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> _______________________________________________
> linux-usb-devel@lists.sourceforge.net
> To unsubscribe, use the last form field at:
> https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
> 

Re: [linux-usb-devel] [PATCH] USB: consolidate error values from EHCI, UHCI and OHCI _suspend()

[Jiri Kosina]

On Mon, 18 Sep 2006, David Brownell wrote:

> > EHCI, UHCI and OHCI USB host drivers are not consistent when returining 
> > error values from their _suspend() functions, in case that the device is 
> > not in suspended state. This could confuse users, so let all three of them 
> > return -EBUSY.
> Shouldn't you also update uhci_suspend()?  Currently it just ignores 
> hcd->state ...

You are right that the patch is possibly not fully correct. I was trying 
to fix the situation I was getting into with the bug in usb_resume_both() 
(see my "[PATCH] 2.6.18-rc6-mm2 - usb_resume_both() - fix suspend/resume" 
mail from yesterday), but now it is obvious that the EINVAL from UHCI is 
of a "different kind" than EBUSY from OHCI and UHCI (though they are 
triggered in the same situations -- when the previous resume was not done 
correctly).

As far as I can see, the UHCI driver is, strangely enough, not using 
hcd->state at all. 

(by the way, EHCI and OHCI seem to have broken (read: missing) locking 
when accessing the hcd->state. Should I fix it by per-hcd spinlock, or 
does the patch already exist somewhere?)

Thanks,

-- 
Jiri Kosina

Re: [linux-usb-devel] [PATCH] USB: consolidate error values from EHCI, UHCI and OHCI _suspend()

[Alan Stern]

On Tue, 19 Sep 2006, Jiri Kosina wrote:

> On Mon, 18 Sep 2006, David Brownell wrote:
> 
> > > EHCI, UHCI and OHCI USB host drivers are not consistent when returining 
> > > error values from their _suspend() functions, in case that the device is 
> > > not in suspended state. This could confuse users, so let all three of them 
> > > return -EBUSY.
> > Shouldn't you also update uhci_suspend()?  Currently it just ignores 
> > hcd->state ...
> 
> You are right that the patch is possibly not fully correct. I was trying 
> to fix the situation I was getting into with the bug in usb_resume_both() 
> (see my "[PATCH] 2.6.18-rc6-mm2 - usb_resume_both() - fix suspend/resume" 
> mail from yesterday), but now it is obvious that the EINVAL from UHCI is 
> of a "different kind" than EBUSY from OHCI and UHCI (though they are 
> triggered in the same situations -- when the previous resume was not done 
> correctly).

Sort of.  The real trigger is that the PM core tries to suspend the 
upper-level PCI device but the lower-level root-hub device has not been 
suspended.  You saw this occurring because during the previous resume, the 
root-hub device's power.power_state.event was not updated, causing the PM 
core to think the root hub was still suspended when in fact it wasn't.

> As far as I can see, the UHCI driver is, strangely enough, not using 
> hcd->state at all. 
> 
> (by the way, EHCI and OHCI seem to have broken (read: missing) locking 
> when accessing the hcd->state. Should I fix it by per-hcd spinlock, or 
> does the patch already exist somewhere?)

Believe it or not, these two paragraphs are closely connected.  The reason 
uhci-hcd doesn't use hcd->state at all is because there is no locking to 
protect it!

My feeling has long been that hcd->state deserves to disappear.  It tries 
to serve too many functions.  Please don't add any code in a futile 
attempt to preserve it.

Alan Stern

Re: [linux-usb-devel] [PATCH] USB: consolidate error values from EHCI, UHCI and OHCI _suspend()

[David Brownell]

On Tuesday 19 September 2006 3:43 am, Jiri Kosina wrote:

> (by the way, EHCI and OHCI seem to have broken (read: missing) locking 
> when accessing the hcd->state. Should I fix it by per-hcd spinlock, or 
> does the patch already exist somewhere?)

They should only ever access it while holding their internal spinlocks;
which are held during most driver operations, easy to miss.  And except
for hardware faults, the HCD state changes only when usbcore pushes an
HCD through driver model state transitions like probe(), suspend(), and
their inverses.  I see some dodgey code in the OHCI IRQ handler, but
even that shouldn't make trouble.

Admittedly the usbcore access to that field is a bit problematic, since
it doesn't handle the hardware faulting cases very cleanly.  For those
cases, other problems are more severe ... like basic cleanup of all the
pending transactions, and removal of the usb devices, didn't work the
last time I tripped over such cases.


Eventually we want hcd->state to vanish, but until it does it sure seems
like a problem if usbcore can't rely on all HCDs to treat it the same.

- Dave

Re: [linux-usb-devel] [PATCH] USB: consolidate error values from EHCI, UHCI and OHCI _suspend()

[Alan Stern]

On Tue, 19 Sep 2006, David Brownell wrote:

> Eventually we want hcd->state to vanish, but until it does it sure seems
> like a problem if usbcore can't rely on all HCDs to treat it the same.

uhci-hcd sets hcd->state wherever needed by usbcore, just as the other 
HCDs do.  (At least that was my intention -- if I missed setting it 
somewhere then the driver should be fixed.)

But uhci-hcd never reads hcd->state or tests its value.  I think that's 
what Jiri meant.

Alan Stern