On Wed, Feb 20, 2013 at 12:35:11PM -0500, Alan Stern wrote:
> On Wed, 20 Feb 2013, Don Zickus wrote:
> > Hi Alan,
> >
> > I worked with Tony on this issue. The instrumentation we did was to add
> > printks in the xhci_suspend, xhci_resume and
> > compliance_mode_recovery_timer_init.
> >
> > What we noticed was that the xhci_suspend was not called during hibernate.
>
> I bet it really was called, but you didn't realize it because of the
> way you monitored the output from the printks. You looked at the log
> buffer or the kernel log file after resuming, right? You didn't have a
> serial console attached to another machine, to capture the output as
> it occurred.
>
> This makes a difference, because output that occurs after the memory
> image is stored on disk will not be present in the image and hence will
> not be visible after you resume from hibernation.
>
> Of course, in your case this doesn't matter. In the memory image, the
> timer is active. Hence it is still active when the system resumes,
> even though xhci_suspend _was_ called.
Ah, I see now. So basically any memory changes in xhci_suspend will be
wiped on resume, but any hardware state should be saved. I was initially
concerned about the large number of memory writes in xhci_suspend (e.g.
zeroing the command ring), but on resume from hibernate, xhci_resume
basically ends up wiping the entire slate and re-initializing the host.
It would be nice if xhci_suspend was passed a boolean to indicate it was
being called as part of the hibernate process. That way we could skip a
lot of unnecessary state saving.
One thing that isn't clear to me is how hibernate and host reset effect
the Synopsys part. Say that all ports have seen U0 before hibernate
(and the timer wasn't running). If we resume from hibernate and reset
the host, will it be possible to run into the re-driver issue with dead
ports?
Alex, do you know?
I think the safe bet would be to assume that we can hit the bug after
hibernate. That would mean we would have to delete a running timer on
resume from hibernate, and let xhci_init re-initialize the U0 port state
bitmask. Which is exactly what your first patch does:
> /* If restore operation fails, re-initialize the HC during resume */
> if ((temp & STS_SRE) || hibernated) {
> +
> + if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
> + (!(xhci_all_ports_seen_u0(xhci)))) {
> + del_timer_sync(&xhci->comp_mode_recovery_timer);
> + xhci_dbg(xhci, "Compliance Mode Recovery Timer Deleted!\n");
> + }
> +
> /* Let the USB core know _both_ roothubs lost power. */
> usb_root_hub_lost_power(xhci->main_hcd->self.root_hub);
> usb_root_hub_lost_power(xhci->shared_hcd->self.root_hub);
> @@ -1058,7 +1065,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> * to suffer the Compliance Mode issue again. It doesn't matter if
> * ports have entered previously to U0 before system's suspension.
> */
> - if (xhci->quirks & XHCI_COMP_MODE_QUIRK)
> + if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) && (!hibernated))
> compliance_mode_recovery_timer_init(xhci);
>
> return retval;
I think you should just clean up the patch description with Alan's
explanation and resubmit it.
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
