Re: PCI PM: Restore standard config registers of all devices early

Rafael J. Wysocki
2009-02-03T23:23:55Z

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