Re: [linux-usb-devel] OHCI and suspend/resume problems

[David Brownell]

On Saturday 07 May 2005 2:50 pm, Alan Stern wrote:
> On Wed, 4 May 2005, David Brownell wrote:
> 
> > On Sunday 01 May 2005 7:14 pm, Alan Stern wrote:
> > > David:
> > > 
> > > I found part of the source for the trouble I've had with root-hub 
> > > suspend/resume on OHCI.  It's these two lines near the end of 
> > > ohci-hub.c:ohci_hub_suspend():
> > > 
> > >         if (status == 0)
> > >                 hcd->state = HC_STATE_SUSPENDED;
> > 
> > I think I remember why that's there.   It pairs the earlier line
> > in the same function to set hcd->state to QUIESCING.  And the reason
> > for that is because hcd->state is the only hook we have for, erm,
> > quiescing the traffic going into the HCD.
> 
> More testing showed that this causes a problem at the end of ohci_irq(), 
> where the code says:
> 
>       if (HC_IS_RUNNING(hcd->state)) {
>                 ohci_writel (ohci, ints, &regs->intrstatus);
>                 ohci_writel (ohci, OHCI_INTR_MIE, &regs->intrenable);   
>                 // flush those writes
>                 (void) ohci_readl (ohci, &ohci->regs->control);
>         }
> 
> The HC_IS_RUNNING test fails after the state has been set to 
> HC_STATE_SUSPENDED, so the interrupt handling isn't completed. 

That's odd.  The MIE update should be harmless, though pointless
given that it's not disabled ... that may be leftover from some
ancient stuff, or maybe even a workaround for quirks on some
particular controller.

What's odd is getting an IRQ there when it's marked as SUSPENDED!

 - INTR_RD would make sense; that should get acked.  Likely the
   previous branch should ack that, though normally that'd fire
   when the controller is in the OHCI suspend state and both the
   root hub and the HC are otherwise fully operational.

 - INTR_WDH should not be happening there at all; if it's suspended
   the HC shouldn't be writing anything.

No other IRQ makes sense.   Try the attached patch, on the theory
that the problem is INTR_RD.  (If this is really the issue, then
I'm surprised it's not appeared before.)


> If I  
> replace that test with "if (1) {" then my system no longer hangs.  It 
> still doesn't work correctly, in that the root hub doesn't get resumed as 
> one would expect, but that's a separate issue...
> 
> So one of the two places appears to be wrong.  Either ohci_hub_suspend()  
> shouldn't set hcd->state to HC_STATE_SUSPENDED or ohci_irq() shouldn't 
> contain this test for HC_IS_RUNNING(hcd->state).  Could this be an example 
> of trying to use hcd->state to mean too many different things?

Maybe, but it feels just like old code that didn't evolve right.

Originally that test was to prevent writing to an HC that wasn't
in PCI D0 state.  That was in late 2003, and even that test should
now be caught by the earlier "device removed" code.

- Dave

--- 1.102/drivers/usb/host/ohci-hcd.c	2005-04-23 12:54:58 -07:00
+++ edited/drivers/usb/host/ohci-hcd.c	2005-05-08 14:06:07 -07:00
@@ -748,6 +748,8 @@
 		ohci_vdbg (ohci, "resume detect\n");
 		if (hcd->state != HC_STATE_QUIESCING)
 			schedule_work(&ohci->rh_resume);
+		ohci_writel (ohci, OHCI_INTR_RD, &regs->intrstatus);
+		ints &= ~OHCI_INTR_RD;
 	}
 
 	if (ints & OHCI_INTR_WDH) {
@@ -773,7 +775,7 @@
 		ohci_writel (ohci, OHCI_INTR_SF, &regs->intrdisable);	
 	spin_unlock (&ohci->lock);
 
-	if (HC_IS_RUNNING(hcd->state)) {
+	if (ints != 0 && HC_IS_RUNNING(hcd->state)) {
 		ohci_writel (ohci, ints, &regs->intrstatus);
 		ohci_writel (ohci, OHCI_INTR_MIE, &regs->intrenable);	
 		// flush those writes