Re: Resume enhancement: restore pci config space

[Takashi Iwai <tiwai@suse.de>]

At Tue, 1 Jun 2004 17:38:00 +0200,
Arjan van de Ven wrote:
> 
> [1  <text/plain; us-ascii (7bit)>]
> On Tue, Jun 01, 2004 at 05:26:51PM +0200, Takashi Iwai wrote:
> > At Tue, 01 Jun 2004 17:06:38 +0200,
> > Arjan van de Ven wrote:
> > > 
> > > [1  <text/plain (quoted-printable)>]
> > > 
> > > > int xxx_resume(struct pci_dev *dev)
> > > > {
> > > > 	int err;
> > > > 	if ((err = pci_default_resume(dev)) < 0)
> > > > 		return err;
> > > > 	// ... do h/w specific
> > > > }
> > > 
> > > well define "h/w specific", just give me an example of a real (alsa?)
> > > driver that would use it (or point me to one) so that I can see if this
> > > is the best API, what the return value should be etc etc 
> > 
> > I'm afraid the ALSA drivers aren't be the best examples :)
> > It doesn't handle the error in suspend/resume at all.
> 
> hm it looks like all this would gain is that instead of 2 or 3 function calls
> you need to do one which then calls those 3. The *driver* already knows if
> it needs busmaster or not etc, so when I wrote this code I felt that the
> driver could do a better job really. But well if you think it's worth it to
> save those 3 lines into 1 ?

Well, if it's all over hundreds of drivers, we'll gain a couple of
hundreds of lines ;)
Another good reason would be that it will prevent to forget the proper
calls, e.g. in the case you changed the busmastering in the main probe
code.

Anyway, it's just a little gain.  I don't mind even if such a function
is not exported.

> > Hmm, looking at them right now, and i found most of them don't have
> > pci_suspend_state() because it worked without saving/restoring the pci
> > state _casually_, and missing pci_set_power_state(), etc...
> 
> I made the PCI layer save PCI config space always, and the generic resume callback
> conditional, so saving PCI config state is not something that explicitly
> needs to be done in the suspend hook.

That's nice.

>  I don't know what else a suspend
> standard function needs to do.

I don't see others for suspends, too.
pci_disable_device() and pci_set_power_state() should be
driver-specific. 
(for example, ymfpci device doesn't like pci_set_power_state(3) - it
 won't wake up at the next time.)


Takashi