At Mon, 31 May 2004 15:38:34 +0200,
Arjan van de Ven wrote:
>
> [1 <text/plain; us-ascii (7bit)>]
> On Sun, May 30, 2004 at 08:40:31PM +0200, Pavel Machek wrote:
> > Hi!
> >
> > > One can rightfully argue that the driver resume method should do this, and
> > > yes that is right. So the patch only does it for devices that don't have a
> > > resume method. Like the main PCI bridge on my testbox of which the bios so
> > > nicely forgets to restore the bus master bit during resume.. With this patch
> > > my testbox resumes just fine while it, well, wasn't all too happy as you can
> > > imagine without a busmaster pci bridge.
> > ...
> > > +/*
> > > + * Default resume method for devices that have no driver provided resume,
> > > + * or not even a driver at all.
> > > + */
> > > +static void pci_default_resume(struct pci_dev *pci_dev)
> > > +{
> >
> > Perhaps this should not be static so that drivers don't
> > need to duplicate this?
>
> I wonder if that is useful, can you see cases where it would be?
> I mean, all it does is provide a default handler for places that don't have
> one. All this is info drivers already have, if a driver chooses to implement
> it's resume handler I think they can do better than this (and thus don't
> need this helper). But... if you can come up with a reasonable use I don't
> oppose it. I do like to see a sane user first though before adding this to
> the driver API...
well, most drivers need more or less the similar procedure like
pci_default_suspend/resume(): enable/disable the pci device, toggle
busmastering, and store/restore the pci status.
if default callbacks are exported, the driver callbacks can be
simplified, such as
int xxx_suspend(struct pci_dev *dev, u32 state)
{
// ... do h/w specific things
return pci_default_suspend(dev, state);
}
int xxx_resume(struct pci_dev *dev)
{
int err;
if ((err = pci_default_resume(dev)) < 0)
return err;
// ... do h/w specific
}
but IMO, the jobs of pci_default_suspend/resume() should be applied
always after/before calling driver's suspend/resume callbacks.
can they break anything potentially?
--
Takashi Iwai <[email protected]> ALSA Developer - http://www.alsa-project.org
> 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
>
>
> but IMO, the jobs of pci_default_suspend/resume() should be applied
> always after/before calling driver's suspend/resume callbacks.
I would be very wary of unconditionally doing the resume thing without
the driver having had a chance to do it's thing. Imo the driver HAS to
be able to override stuff or at least talk to the hw before the generic
resume happens.
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.
Anyway, you can find suspend/resume callbacks in linux/sound/pci and
subdirectories (e.g. atiixp.c, es1968.c, intel8x0.c...)
Note that suspend/resume prototype is different from pci callbacks to
be interated with the power handler on ALSA control API.
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...
Using "standard" functions would be easier to fix such things.
> >
> > but IMO, the jobs of pci_default_suspend/resume() should be applied
> > always after/before calling driver's suspend/resume callbacks.
>
> I would be very wary of unconditionally doing the resume thing without
> the driver having had a chance to do it's thing. Imo the driver HAS to
> be able to override stuff or at least talk to the hw before the generic
> resume happens.
Ok, agreed.
Providing functions to do the standard jobs would suffice.
Takashi
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 ?
> 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. I don't know what else a suspend
standard function needs to do.
Gretings,
Arjan van de Ven
Ahoj!
> > > [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 ?
I'd prefer one line, same in all alsa drivers, than each driver trying
to be clever.
[It seems to me like ALSA can't easily put NULL into suspend/resume
fields, because they have additional layer of abstraction between them
and kernel.]
Pavel
--
Horseback riding is like software...
...vgf orggre jura vgf serr.
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
At Tue, 1 Jun 2004 18:02:34 +0200,
Pavel Machek wrote:
>
> [It seems to me like ALSA can't easily put NULL into suspend/resume
> fields, because they have additional layer of abstraction between them
> and kernel.]
It does indeed. ALSA provides the "common" PCI suspend/resume
callbacks, which call the internal suspend/resume callbacks.
It's for handling the power status from the external program, too.
These are defined as SND_PCI_PM_CALLBACKS. If CONFIG_PM is not set,
it's expanded to empty. So, typically the code looks like the
following:
static struct pci_driver driver = {
.name = "Intel ICH",
.id_table = snd_intel8x0_ids,
.probe = snd_intel8x0_probe,
.remove = __devexit_p(snd_intel8x0_remove),
SND_PCI_PM_CALLBACKS
};
Takashi