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

[Jesse Barnes <jesse.barnes@intel.com>]

On Monday, February 2, 2009 10:07 pm Benjamin Herrenschmidt wrote:
> On Mon, 2009-02-02 at 16:28 -0800, Linus Torvalds wrote:
> > On Tue, 3 Feb 2009, Rafael J. Wysocki wrote:
> > > BTW, on the PMAC the problematic driver appears to be the radeon
> > > driver, according to Ben, and the breakage is not related to USB.
> >
> > Hmm. atyfb_base.c has the same kind of things with magic PMAC code, but
> > it doesn't follow the USB pattern - it just replaces
> > "pci_set_power_state()" _entirely_.
> >
> > It seems a very interesting suspend routine, btw. It doesn't seem to do
> > any of the pci_save_state() at all, just re-initializes from scratch.
> > Maybe it is unhappy with the PM layer deciding to try to restore state
> > since it clearly didn't..
>
> In fact, atyfb is also busted in -rc3 for different reasons though
> probably by the same patch. I just dug an old powerbook with a mach64
> and it crashes on resume with a machine check in there. (Among 2 or 3
> other problems introduced by recent kernels, such as pci_get_* now
> kmallocs with GFP_KERNEL internally which makes it WARN when I use it in
> my low level suspend/resume code to whack the memory controller, etc...
> this one is going to bite others I reckon, or IDE having some interrupt
> problems on resume).
>
> Adding a pci_save_state() to atyfb_pci_suspend() and a
> pci_set_power_state() + pci_restore_state() at the beginning of
> atyfb_pci_resume() fixes the machine check here.
>
> Now where it gets funny is that I've added code to read the BAR and
> command register content before, between, and after those calls and
> print it and .. they are sane... Until i discovered that what happens is
> that the new generic code seems to actually blast 0 all over my config
> space if I don't call pci_save_state() in suspend(). I suppose I was
> missing a "mandatory" call here... but the core should be more robust,
> ie it shouldn't erase the config space of something because a driver
> "forgot" to call pci_save_state() !

Whoa, I don't think we actually zero the contents in the suspend/resume core, 
but if the device goes into D3 the config space contents may be lost, maybe 
that's what happening here?

-- 
Jesse Barnes, Intel Open Source Technology Center