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

[Rafael J. Wysocki]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/