Messages in this thread | | | From | "Rafael J. Wysocki" <> | Subject | Re: PCI PM: Restore standard config registers of all devices early | Date | Wed, 4 Feb 2009 00:23:02 +0100 |
| |
On Tuesday 03 February 2009, Benjamin Herrenschmidt wrote: > > > You've found a bug somewhere. > > Yup :-) > > > We _should_ be saving things, the legacy code does something like this: > > > > if (drv && drv->suspend) { > > pci_dev->state_saved = false; > > > > i = drv->suspend(pci_dev, state); > > suspend_report_result(drv->suspend, i); > > if (i) > > return i; > > > > if (pci_dev->state_saved) > > goto Fixup; > > > > if (WARN_ON_ONCE(pci_dev->current_state != PCI_D0)) > > goto Fixup; > > It looks like the above is what breaks. Looks like current_state is > UNKNOWN. The device is an old mach64 that has no PCI PM capability, thus > the driver doesn't call any PCI PM stuff, the state basically stays set > to what the core set it to at probe time which appears to be > PCI_UNKNOWN.
Overlooked, sorry.
> Thus we don't call pci_save_state(). > > Then ... > > > } > > > > pci_save_state(pci_dev); > > > > ie if your ->suspend function doesn't use pci_save_state() itself (which > > sets that "state_saved" flag to true), then the generic code will do it > > for you. > > > > Also, on the resume path, we actually have > > > > if (pci_dev->state_saved) > > pci_restore_standard_config(pci_dev); > > > > so I wonder how the heck you got that blast of all zeroes - because we > > clearly shouldn't be trying to restore any unsaved state! > > Well, that's it ... we don't actually test pci_dev->state_saved in > whatever is currently upstream. The code is: > > static void pci_pm_default_resume_noirq(struct pci_dev *pci_dev) > { > pci_restore_standard_config(pci_dev); > pci_fixup_device(pci_fixup_resume_early, pci_dev); > } > > Oops...
The assumption here is that the state will be saved either by the driver or by the core, so the bug here is a consequence of the previous one.
I'll add a check.
> Rafael, the second one is trivial to fix, but what about the first one ? > Should we not count UNKNOWN in that goto or should we set legacy stuff > that don't do PCI PM to PCI_D0 somewhere ? Or both ? :-)
Well, if the state is UNKNOWN, I think it's safe to save the config, so the WARN_ON should really catch the low power states only. I'll fix that.
Thanks for debugging it!
Rafael
| |