On Fri, Dec 24, 2010 at 04:43:22PM +0800, Xu, Andiry wrote: > > -----Original Message----- > > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > > Sent: Friday, December 24, 2010 7:14 AM > > To: Xu, Andiry > > Cc: linux-usb@xxxxxxxxxxxxxxx > > Subject: Possible bug in xhci_resume() > > > > Hi Andiry, > > > > I'm seeing an unreachable code path in xhci_resume(). Can you please > > explain what you meant to do here? > > > > In xhci_resume, you save the host controller's state in the variable > > old_state. But then you only use it on an unreachable code path: > > > > int xhci_resume(struct xhci_hcd *xhci, bool hibernated) > > { > > u32 command, temp = 0; > > struct usb_hcd *hcd = xhci_to_hcd(xhci); > > int old_state, retval; > > > > old_state = hcd->state; > > ... > > > > if (!hibernated) { > > /* restore registers */ > > } > > > > if ((temp & STS_SRE) || hibernated) { > > /* restart the host controller */ > > > > if (!retval) > > set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > > hcd->state = HC_STATE_SUSPENDED; > > return retval; > > > > /* Note the return here - this means if hibernated is > > * true, we will always return out of this function > > * here. > > */ > > } > > > > ... > > set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > > if (!hibernated) > > hcd->state = old_state; > > else > > hcd->state = HC_STATE_SUSPENDED; > > ... > > } > > > > Did you mean to set the hcd->state back to the old value when the host > > was resumed after a power loss? If so, perhaps the if statement that > > tests for hibernation or a power loss could be changed to: > > > > if ((temp & STS_SRE) || hibernated) { > > /* restart the host controller */ > > if (!retval) > > set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > > if (temp & STS_SRE) > > hcd->state = old_state; > > else > > hcd->state = HC_STATE_SUSPENDED; > > return retval; > > } > > > > Oh, thanks for catching this. I think it's another legacy design issue. > > The right logic should be: set hcd->state = HC_STATE_SUSPENDED if there > is a power loss in resume or the system is hibernated, otherwise leave > it be. I referred to ehci_pci_resume(). > > So the old_state variable is redundant. The solution would be like this: > > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -705,9 +705,8 @@ int xhci_resume(struct xhci_hcd *xhci, bool > hibernated) > { > u32 command, temp = 0; > struct usb_hcd *hcd = xhci_to_hcd(xhci); > - int old_state, retval; > + int retval; > > - old_state = hcd->state; > if (time_before(jiffies, xhci->next_statechange)) > msleep(100); > > @@ -791,10 +790,6 @@ int xhci_resume(struct xhci_hcd *xhci, bool > hibernated) > */ > > set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > - if (!hibernated) > - hcd->state = old_state; > - else > - hcd->state = HC_STATE_SUSPENDED; > > spin_unlock_irq(&xhci->lock); > return 0; > > If you are OK with this, I'll send a formal patch later. And Merry > Christmas! Yes, that looks fine, please send a formal patch. Merry Christmas and a happy new year! Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
