On Tue, Nov 01, 2011 at 09:03:00AM -0700, Alan Stern wrote:
> With xhci-hcd it's a little more complicated because there are two root
> hubs to consider. xhci-resume could check the status of both and
> resume whichever got a status change. Or it could simply resume both
> of them every time. Sarah, which do you think is better?
With the NEC xHCI host on my x220, I did verify that the port status
change bits were set at the end of xhci_resume. So it's possible to
check those and only resume the roothub (either USB3 or USB2) with a
change bit set. That said, I think it's probably safer to just resume
both host controllers because we don't know how xHCI hosts from other
manufacturers (like the AsMedia host) behave.
Your patch looks fine, although I'm a little confused as to why you
needed to move setting the HCD_FLAG_HW_ACCESSIBLE bits. Now they'll be
set unconditionally, instead of not being set if restore context state
command times out or if the xHCI re-initialization fails after a power
loss. You know those host controller flags better than I do, but is
that what you wanted to do?
Sarah Sharp
On Tue, Nov 01, 2011 at 11:41:14AM -0700, Alan Stern wrote:
> Here is the promised test patch. It doesn't involve adding any funny
> delays. You might be able to improve this a little, but at least it
> should get things to work properly.
>
> Alan Stern
>
>
>
> Index: usb-3.2/drivers/usb/host/xhci.c
> ===================================================================
> --- usb-3.2.orig/drivers/usb/host/xhci.c
> +++ usb-3.2/drivers/usb/host/xhci.c
> @@ -799,7 +799,7 @@ int xhci_resume(struct xhci_hcd *xhci, b
> u32 command, temp = 0;
> struct usb_hcd *hcd = xhci_to_hcd(xhci);
> struct usb_hcd *secondary_hcd;
> - int retval;
> + int retval = 0;
>
> /* Wait a bit if either of the roothubs need to settle from the
> * transition into bus suspend.
> @@ -809,6 +809,9 @@ int xhci_resume(struct xhci_hcd *xhci, b
> xhci->bus_state[1].next_statechange))
> msleep(100);
>
> + set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> + set_bit(HCD_FLAG_HW_ACCESSIBLE, &xhci->shared_hcd->flags);
> +
> spin_lock_irq(&xhci->lock);
> if (xhci->quirks & XHCI_RESET_ON_RESUME)
> hibernated = true;
> @@ -878,20 +881,13 @@ int xhci_resume(struct xhci_hcd *xhci, b
> return retval;
> xhci_dbg(xhci, "Start the primary HCD\n");
> retval = xhci_run(hcd->primary_hcd);
> - if (retval)
> - goto failed_restart;
> -
> - xhci_dbg(xhci, "Start the secondary HCD\n");
> - retval = xhci_run(secondary_hcd);
> if (!retval) {
> - set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> - set_bit(HCD_FLAG_HW_ACCESSIBLE,
> - &xhci->shared_hcd->flags);
> + xhci_dbg(xhci, "Start the secondary HCD\n");
> + retval = xhci_run(secondary_hcd);
> }
> -failed_restart:
> hcd->state = HC_STATE_SUSPENDED;
> xhci->shared_hcd->state = HC_STATE_SUSPENDED;
> - return retval;
> + goto done;
> }
>
> /* step 4: set Run/Stop bit */
> @@ -910,11 +906,14 @@ failed_restart:
> * Running endpoints by ringing their doorbells
> */
>
> - set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> - set_bit(HCD_FLAG_HW_ACCESSIBLE, &xhci->shared_hcd->flags);
> -
> spin_unlock_irq(&xhci->lock);
> - return 0;
> +
> + done:
> + if (retval == 0) {
> + usb_hcd_resume_root_hub(hcd);
> + usb_hcd_resume_root_hub(xhci->shared_hcd);
> + }
> + return retval;
> }
> #endif /* CONFIG_PM */
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
