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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

> 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.

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... 

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 ? :-)

Cheers,
Ben.