> -----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! Thanks, Andiry -- 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
