On Tue, 9 Dec 2008, Rafael J. Wysocki wrote:
> Hi,
>
> > Here's the current version of my patch. All of the PCI stuff has been
> > moved to the suspend_late and resume_early routines.
> >
> > Now maybe that's not the right thing to do, or at least not until some
> > further fixes have been added. All I can tell you is that this looks
> > right to me, but it doesn't work. When I tried it, all the I/O and
> > MMIO accesses after resume_early returned nothing but 0xffffffff.
>
> Well, it would be good to understand why this happens.
Probably because the state being restored is totally bogus, as you
noticed.
> > Here's a sample extracted from the log:
> >
> > [ 472.248029] ehci_hcd 0000:00:1d.7: resume from previous PCI D3: 0
> > [ 472.248139] ehci_hcd 0000:00:1d.7: restoring config space at offset 0xf (was 0x400, writing 0x0)
> > [ 472.248283] ehci_hcd 0000:00:1d.7: restoring config space at offset 0xd (was 0x50, writing 0x0)
> > [ 472.248430] ehci_hcd 0000:00:1d.7: restoring config space at offset 0xb (was 0x52478086, writing 0x0)
> > [ 472.248591] ehci_hcd 0000:00:1d.7: restoring config space at offset 0x2 (was 0xc032002, writing 0x0)
> > [ 472.248735] ehci_hcd 0000:00:1d.7: restoring config space at offset 0x1 (was 0x2900000, writing 0x0)
> > [ 472.248878] ehci_hcd 0000:00:1d.7: restoring config space at offset 0x0 (was 0x24cd8086, writing 0x0)
> > [ 472.249037] ehci_hcd 0000:00:1d.7: enabling device (0000 -> 0002)
> > [ 472.249136] ehci_hcd 0000:00:1d.7: PCI INT D -> GSI 23 (level, low) -> IRQ 23
> > [ 472.249237] ehci_hcd 0000:00:1d.7: setting latency timer to 64
> > [ 472.264026] ohci_hcd 0000:01:00.0: resume from previous PCI D3: 0
> > [ 472.264130] ohci_hcd 0000:01:00.0: restoring config space at offset 0xf (was 0x2a01010b, writing 0x0)
> > [ 472.264275] ohci_hcd 0000:01:00.0: restoring config space at offset 0xd (was 0x40, writing 0x0)
> > [ 472.264418] ohci_hcd 0000:01:00.0: restoring config space at offset 0xb (was 0x351033, writing 0x0)
> > [ 472.264571] ohci_hcd 0000:01:00.0: restoring config space at offset 0x4 (was 0xff8fd000, writing 0x0)
> > [ 472.264713] ohci_hcd 0000:01:00.0: restoring config space at offset 0x3 (was 0x802008, writing 0x0)
> > [ 472.264855] ohci_hcd 0000:01:00.0: restoring config space at offset 0x2 (was 0xc031043, writing 0x0)
> > [ 472.264997] ohci_hcd 0000:01:00.0: restoring config space at offset 0x1 (was 0x2100000, writing 0x0)
> > [ 472.265139] ohci_hcd 0000:01:00.0: restoring config space at offset 0x0 (was 0x351033, writing 0x0)
>
> All of the above writes don't look good. Only zeros were written, looks like
> the saved state was bogus.
Just so.
> > +int usb_hcd_pci_suspend_late(struct pci_dev *dev, pm_message_t message)
> > +{
> > + int retval = 0;
> > + int has_pci_pm;
> > + struct usb_hcd *hcd = pci_get_drvdata(dev);
> > + int wake;
> > +
> > + /* We might already be suspended (runtime PM -- not yet written) */
> > + if (dev->current_state != PCI_D0)
> > + goto done;
> > +
> > + /* Don't change state if we don't need to */
> > + if (message.event == PM_EVENT_FREEZE ||
> > + message.event == PM_EVENT_PRETHAW) {
> > + dev_dbg(&dev->dev, "--> no state change\n");
> > + goto done;
> > + }
> >
> > - /* even when the PCI layer rejects some of the PCI calls
> > - * below, HCs can try global suspend and reduce DMA traffic.
> > - * PM-sensitive HCDs may already have done this.
> > - */
> > has_pci_pm = pci_find_capability(dev, PCI_CAP_ID_PM);
> > + if (!has_pci_pm) {
> > + dev_dbg(&dev->dev, "--> PCI D0/legacy\n");
> > + goto done;
> > + }
> > +
> > + /* Ignore these return values. We rely on pci code to
> > + * reject requests the hardware can't implement, rather
> > + * than coding the same thing.
> > + */
> > + wake = (hcd->state == HC_STATE_SUSPENDED &&
> > + device_may_wakeup(&dev->dev));
> > + (void) pci_enable_wake(dev, PCI_D3hot, wake);
> > + (void) pci_enable_wake(dev, PCI_D3cold, wake);
>
> I'm not sure if this is safe to call these with interrupts disabled.
Then which calls should be made in the suspend method and which in the
suspend_late method?
> Also,
> there is pci_wake_from_d3() to call in such a context.
Okay, good.
> > /* Downstream ports from this root hub should already be quiesced, so
> > * there will be no DMA activity. Now we can shut down the upstream
> > * link (except maybe for PME# resume signaling) and enter some PCI
> > * low power state, if the hardware allows.
> > */
> > - if (hcd->state == HC_STATE_SUSPENDED) {
> > -
> > - /* no DMA or IRQs except when HC is active */
> > - if (dev->current_state == PCI_D0) {
> > - pci_save_state(dev);
> > - pci_disable_device(dev);
> > - }
> > -
> > - if (message.event == PM_EVENT_FREEZE ||
> > - message.event == PM_EVENT_PRETHAW) {
> > - dev_dbg(hcd->self.controller, "--> no state change\n");
> > - goto done;
> > - }
> > -
> > - if (!has_pci_pm) {
> > - dev_dbg(hcd->self.controller, "--> PCI D0/legacy\n");
> > - goto done;
> > - }
> > + pci_disable_device(dev);
> >
> > - /* NOTE: dev->current_state becomes nonzero only here, and
> > - * only for devices that support PCI PM. Also, exiting
> > - * PCI_D3 (but not PCI_D1 or PCI_D2) is allowed to reset
> > - * some device state (e.g. as part of clock reinit).
> > - */
> > - retval = pci_set_power_state(dev, PCI_D3hot);
> > - suspend_report_result(pci_set_power_state, retval);
> > - if (retval == 0) {
> > - int wake = device_can_wakeup(&hcd->self.root_hub->dev);
> > -
> > - wake = wake && device_may_wakeup(hcd->self.controller);
> > -
> > - dev_dbg(hcd->self.controller, "--> PCI D3%s\n",
> > - wake ? "/wakeup" : "");
> > -
> > - /* Ignore these return values. We rely on pci code to
> > - * reject requests the hardware can't implement, rather
> > - * than coding the same thing.
> > - */
> > - (void) pci_enable_wake(dev, PCI_D3hot, wake);
> > - (void) pci_enable_wake(dev, PCI_D3cold, wake);
> > - } else {
> > - dev_dbg(&dev->dev, "PCI D3 suspend fail, %d\n",
> > - retval);
> > - (void) usb_hcd_pci_resume(dev);
> > - }
> > -
> > - } else if (hcd->state != HC_STATE_HALT) {
> > - dev_dbg(hcd->self.controller, "hcd state %d; not suspended\n",
> > - hcd->state);
> > - WARN_ON(1);
> > - retval = -EINVAL;
> > + /* NOTE: dev->current_state becomes nonzero only here, and
> > + * only for devices that support PCI PM. Also, exiting
> > + * PCI_D3 (but not PCI_D1 or PCI_D2) is allowed to reset
> > + * some device state (e.g. as part of clock reinit).
> > + */
> > + retval = pci_set_power_state(dev, PCI_D3hot);
>
> I'm not sure if it's safe to call this with interrupts disabled too. It
> invokes some ACPI callbacks that in turn use the AML interpreter, so I guess
> it is not, in general.
You and Linus agreed in
http://marc.info/?l=linux-kernel&m=122851530232679&w=2
that pci_{save|restore}_state should be called with interrupts off (and
presumably everything else with interrupts enabled). But the example
code shows pci_set_power_state called _after_ pci_save_state during
suspend and _before_ pci_restore_state during resume.
So then how can pci_set_power_state be called with interrupts enabled?
Is the order in the code examples wrong?
> > + suspend_report_result(pci_set_power_state, retval);
> > + if (retval == 0) {
> > + dev_dbg(hcd->self.controller, "--> PCI D3%s\n",
> > + wake ? "/wakeup" : "");
> > + } else {
> > + dev_dbg(&dev->dev, "PCI D3 suspend fail, %d\n", retval);
> > + (void) usb_hcd_pci_resume(dev);
> > }
> >
> > -done:
> > - if (retval == 0) {
> > + done:
> > #ifdef CONFIG_PPC_PMAC
> > + if (retval == 0) {
> > /* Disable ASIC clocks for USB */
> > if (machine_is(powermac)) {
> > struct device_node *of_node;
> > @@ -311,30 +299,23 @@ done:
> > pmac_call_feature(PMAC_FTR_USB_ENABLE,
> > of_node, 0, 0);
> > }
> > -#endif
> > }
> > +#endif
> >
> > return retval;
> > }
> > -EXPORT_SYMBOL_GPL(usb_hcd_pci_suspend);
> > +EXPORT_SYMBOL_GPL(usb_hcd_pci_suspend_late);
>
> Well, it looks like you forgot to actually call pci_save_power_state(). :-)
I didn't forget it -- in my tree there is no such function! :-)
Alan Stern
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
