Concerns about our pci_{save,restore}_state()

Concerns about our pci_{save,restore}_state()

[Benjamin Herrenschmidt]

Hi Greg !

I was looking at our "generic" pci_save_state() and pci_restore_state()
and I have various concerns with them, I was wondering what you though
about them...

 - We should always write the command register after all the BARs,
typically that mean write it back _last_
 - We shouldn't write to the BIST register, it is defined as having
side effects and writing to it any value may trigger a BIST on the
card, with all the possible bad consequences that has
 - What about saving/restoring more registers ? I'm not sure wether it
should be the responsibility of the driver to save and restore things
above dword 15, but we should at least deal with the case of P2P bridges
who have more "standard" registers

In addition, we currently have no mecanism to save/restore the state of
P2P bridges. Shouldn't we do that in pci_device_suspend() if there is no
driver attached ?

Ben.

Re: Concerns about our pci_{save,restore}_state()

[Jeff Garzik]

Benjamin Herrenschmidt wrote:
>  - What about saving/restoring more registers ? I'm not sure wether it
> should be the responsibility of the driver to save and restore things
> above dword 15, but we should at least deal with the case of P2P bridges
> who have more "standard" registers


This is a key concern of mine.

The _driver_ is the only entity that knows really how much space to 
save/restore, and the generic versions are obviously _not_ sufficient to 
support:

* hardware errata such as S3 Trio, where _reading_ or writing certain 
registers in the standard range cause a system lockup

* saving/restoring the standard-defined capability lists, which 
certainly could extend way beyond what's stored now

* saving/restoring the new PCI-Express 4K config area

This is _clearly_ something that should be decided upon in the driver. 
The PCI layer should _only_ present standard helper functions, and maybe 
a standard storage space that works for most drivers; not force all 
drivers through a narrow funnel.

	Jeff

Re: Concerns about our pci_{save,restore}_state()

[Benjamin Herrenschmidt]

On Mon, 2004-10-25 at 02:11 -0400, Jeff Garzik wrote:
> Benjamin Herrenschmidt wrote:
> >  - What about saving/restoring more registers ? I'm not sure wether it
> > should be the responsibility of the driver to save and restore things
> > above dword 15, but we should at least deal with the case of P2P bridges
> > who have more "standard" registers
> 
> 
> This is a key concern of mine.
> 
> The _driver_ is the only entity that knows really how much space to 
> save/restore, and the generic versions are obviously _not_ sufficient to 
> support:
> 
> * hardware errata such as S3 Trio, where _reading_ or writing certain 
> registers in the standard range cause a system lockup

Currently, the default does nothing (doesn't even save/restore). It
would be nice if it did though, in absence of a driver, eventually with
a quirk call, so we can add "workarounds" for things like the S3 without
having a full pci_driver for it (it may well be using vgacon).

> * saving/restoring the standard-defined capability lists, which 
> certainly could extend way beyond what's stored now

Apple in Darwin doesn't quite bother apparently... If there is more to
save/restore, it's expected that you have a driver doing it, however,
they definitely do have the default save/restore P2P bridges. In fact,
they even (and I'll need that for pmac too), have a separate "hook" that
allow restoring all P2P bridges outside of the normal loop which is to
be called by the arch code early during resume. Can be necessary to
access some on-board things like the ACPI controller I suppose, on
pmacs, it's for accessing the "mac-io" IO chip early during resume to
re-enable various clocks.

> * saving/restoring the new PCI-Express 4K config area

Ok, I don't know about that one.

> This is _clearly_ something that should be decided upon in the driver. 
> The PCI layer should _only_ present standard helper functions, and maybe 
> a standard storage space that works for most drivers; not force all 
> drivers through a narrow funnel.

Agreed. However, my concern is to have some "default" stuff that will
take over in absence of a driver. This is, I think, important for things
like P2P bridges which are rather standard and will usually survive well
with a simple save/retore of whatever is there. I suppose it would be
interesting to define a pair of quirk types to hook on the "default"
implementation, unless we actually want to have a bunch of pci_driver's
just for things that don't have normally a driver but need some specific
save/restore procedure ...

Ben.

Re: Concerns about our pci_{save,restore}_state()

[Arjan van de Ven]

On Mon, 2004-10-25 at 16:24 +1000, Benjamin Herrenschmidt wrote:

> Currently, the default does nothing (doesn't even save/restore).

Are you sure? I could have sworn I made the default action to
save/restore some time ago

-- 

Re: Concerns about our pci_{save,restore}_state()

[Benjamin Herrenschmidt]

On Mon, 2004-10-25 at 10:32 +0200, Arjan van de Ven wrote:
> On Mon, 2004-10-25 at 16:24 +1000, Benjamin Herrenschmidt wrote:
> 
> > Currently, the default does nothing (doesn't even save/restore).
> 
> Are you sure? I could have sworn I made the default action to
> save/restore some time ago

You are right, I was watching an outdated file for some strange reason I
won't get into now :)

However, it doesn't seem to save/restore enough for a P2P bridge..

Ben.

Re: [linux-pm] Re: Concerns about our pci_{save,restore}_state()

[Patrick Mochel]

On Mon, 25 Oct 2004, Benjamin Herrenschmidt wrote:

> On Mon, 2004-10-25 at 02:11 -0400, Jeff Garzik wrote:

> > This is _clearly_ something that should be decided upon in the driver.
> > The PCI layer should _only_ present standard helper functions, and maybe
> > a standard storage space that works for most drivers; not force all
> > drivers through a narrow funnel.
>
> Agreed. However, my concern is to have some "default" stuff that will
> take over in absence of a driver. This is, I think, important for things
> like P2P bridges which are rather standard and will usually survive well
> with a simple save/retore of whatever is there. I suppose it would be
> interesting to define a pair of quirk types to hook on the "default"
> implementation, unless we actually want to have a bunch of pci_driver's
> just for things that don't have normally a driver but need some specific
> save/restore procedure ...

What's wrong with that? They would be simple and straightforward, and
could probably work to remove many of the quirks, too..


	Pat

Re: [linux-pm] Re: Concerns about our pci_{save,restore}_state()

[Benjamin Herrenschmidt]

On Thu, 2004-10-28 at 01:50 -0700, Patrick Mochel wrote:

> What's wrong with that? They would be simple and straightforward, and
> could probably work to remove many of the quirks, too..

Right.

ben.

Re: Concerns about our pci_{save,restore}_state()

[Greg KH]

On Mon, Oct 25, 2004 at 02:06:22PM +1000, Benjamin Herrenschmidt wrote:
> Hi Greg !
> 
> I was looking at our "generic" pci_save_state() and pci_restore_state()
> and I have various concerns with them, I was wondering what you though
> about them...

Note, these concerns are the same before the last pci_save_state()
changes, right?  I didn't break anything new did I?  :)

>  - We should always write the command register after all the BARs,
> typically that mean write it back _last_

Hm, probably.  I'm away from my PCI book, so I don't really know about
that one.  For some reason we've been ok so far...

>  - We shouldn't write to the BIST register, it is defined as having
> side effects and writing to it any value may trigger a BIST on the
> card, with all the possible bad consequences that has

yeah, good point.  I guess most of these cards don't have BIST stuff in
them.  Or just writing back the read value is sane.  I'll dig through
the PCI book next week.

>  - What about saving/restoring more registers ? I'm not sure wether it
> should be the responsibility of the driver to save and restore things
> above dword 15, but we should at least deal with the case of P2P bridges
> who have more "standard" registers

We need to have a "bridge" driver for something like that.  I think lots
of things would benifit if we had that.  But if we had that, we need a
way to overload (or weight) different drivers that might bind to the
same device.  This is something that we've talked about for a while now,
and it's on my list of things to do in the near future.  I think once we
get that, a "generic" bridge driver will be ok to have.  Any hardware
specific quirks needed can also just be their own driver (I think Red
Hat ran into odd issues when they tried to add a pci bridge driver to
one of their older kernel trees.)

Oh, and yeah, we should probably save and restore pci express config
space too.  I need to go look to see if pci x 2.0 also has a expanded
config or not...

thanks,

greg k-h

Re: Concerns about our pci_{save,restore}_state()

[Benjamin Herrenschmidt]

On Thu, 2004-10-28 at 16:31 -0500, Greg KH wrote:
> On Mon, Oct 25, 2004 at 02:06:22PM +1000, Benjamin Herrenschmidt wrote:
> > Hi Greg !
> > 
> > I was looking at our "generic" pci_save_state() and pci_restore_state()
> > and I have various concerns with them, I was wondering what you though
> > about them...
> 
> Note, these concerns are the same before the last pci_save_state()
> changes, right?  I didn't break anything new did I?  :)

Yes, those are generic concerns I had for a while :)

> >  - We should always write the command register after all the BARs,
> > typically that mean write it back _last_
> 
> Hm, probably.  I'm away from my PCI book, so I don't really know about
> that one.  For some reason we've been ok so far...

Proably not a problem in 99% of the cases, but sounds saner to do (oh,
and did I tell you that Darwin does this ? :) I think it may even be
better to 1) turn IO & MEM off in the command reg, 2) restore the stuff,
3) restore the command reg.

> >  - We shouldn't write to the BIST register, it is defined as having
> > side effects and writing to it any value may trigger a BIST on the
> > card, with all the possible bad consequences that has
> 
> yeah, good point.  I guess most of these cards don't have BIST stuff in
> them.  Or just writing back the read value is sane.  I'll dig through
> the PCI book next week.

Well, some cards will have side effects, whatever you write there (like
going offline for a while, remember that thread about those nasty IBM
scsi controllers needing special workaround for this ? :)

> We need to have a "bridge" driver for something like that.  I think lots
> of things would benifit if we had that.  But if we had that, we need a
> way to overload (or weight) different drivers that might bind to the
> same device.

Yes, which is why, in the meantime, just knowing about them in
save/restore and just saving/restoring a bit more is an acceptable
solution I think ....
 
>   This is something that we've talked about for a while now,
> and it's on my list of things to do in the near future.  I think once we
> get that, a "generic" bridge driver will be ok to have.  Any hardware
> specific quirks needed can also just be their own driver (I think Red
> Hat ran into odd issues when they tried to add a pci bridge driver to
> one of their older kernel trees.)
> 
> Oh, and yeah, we should probably save and restore pci express config
> space too.  I need to go look to see if pci x 2.0 also has a expanded
> config or not...
> 
> thanks,
> 
> greg k-h
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>