LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* RE: Resume enhancement: restore pci config space 
@ 2004-05-26 21:44 Nakajima, Jun
  0 siblings, 0 replies; 14+ messages in thread
From: Nakajima, Jun @ 2004-05-26 21:44 UTC (permalink / raw)
  To: Arjan van de Ven, greg, linux-kernel; +Cc: linux-acpi

>From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>owner@vger.kernel.org] On Behalf Of Arjan van de Ven
>Sent: Wednesday, May 26, 2004 1:35 PM
>To: greg@kroah.com; linux-kernel@vger.kernel.org
>Subject: Resume enhancement: restore pci config space
>
>Hi,
>
>The patch below enhances the PCI layer with 2 things
>1) enable and busmaster state are stored in the pci device struct
>2) pci config space is stored to the pci device struct
>
>with that, it is possible to make a generic pci resume method that
restores
>config space and reenables the device, including busmaster when
appropriate.
>
>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.
>
>Comments?

Arjan, Hi

We need this kind of functionality in the kernel to get suspend/resume
work, because the BIOS resets the PCI config spaces as they were at the
initial boot upon resume. Actually we've been working on issue too, and
glad to see the code ;-). Whatever changes (e.g. BARs, bridge settings)
the kernel (or driver) made after the boot, need to be restored by the
kernel. 

I'll ask the ACPI team if this fixes, for example,
http://bugme.osdl.org/show_bug.cgi?id=2467

Jun
>
>
>Greetings,
>   Arjan van de Ven
>
>
>diff -urNp linux-1100/drivers/pci/pci.c linux-1110/drivers/pci/pci.c
>--- linux-1100/drivers/pci/pci.c
>+++ linux-1110/drivers/pci/pci.c
>@@ -385,6 +385,7 @@ pci_enable_device_bars(struct pci_dev *d
> int
> pci_enable_device(struct pci_dev *dev)
> {
>+	dev->is_enabled = 1;
> 	return pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) -
1);
> }
>
>@@ -399,6 +400,9 @@ void
> pci_disable_device(struct pci_dev *dev)
> {
> 	u16 pci_command;
>+
>+	dev->is_enabled = 0;
>+	dev->is_busmaster = 0;
>
> 	pci_read_config_word(dev, PCI_COMMAND, &pci_command);
> 	if (pci_command & PCI_COMMAND_MASTER) {
>@@ -601,6 +605,7 @@ pci_set_master(struct pci_dev *dev)
> 		cmd |= PCI_COMMAND_MASTER;
> 		pci_write_config_word(dev, PCI_COMMAND, cmd);
> 	}
>+	dev->is_busmaster = 1;
> 	pcibios_set_master(dev);
> }
>
>diff -urNp linux-1100/drivers/pci/pci-driver.c
linux-1110/drivers/pci/pci-
>driver.c
>--- linux-1100/drivers/pci/pci-driver.c
>+++ linux-1110/drivers/pci/pci-driver.c
>@@ -299,10 +299,30 @@ static int pci_device_suspend(struct dev
> {
> 	struct pci_dev * pci_dev = to_pci_dev(dev);
> 	struct pci_driver * drv = pci_dev->driver;
>+	int i = 0;
>
> 	if (drv && drv->suspend)
>-		return drv->suspend(pci_dev,state);
>-	return 0;
>+		i = drv->suspend(pci_dev,state);
>+
>+	pci_save_state(pci_dev, pci_dev->saved_config_space);
>+	return i;
>+}
>+
>+
>+/*
>+ * 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)
>+{
>+	/* restore the PCI config space */
>+	pci_restore_state(pci_dev, pci_dev->saved_config_space);
>+	/* if the device was enabled before suspend, reenable */
>+	if (pci_dev->is_enabled)
>+		pci_enable_device(pci_dev);
>+	/* if the device was busmaster before the suspend, make it
busmaster
>again */
>+	if (pci_dev->is_busmaster)
>+		pci_set_master(pci_dev);
> }
>
> static int pci_device_resume(struct device * dev)
>@@ -312,6 +332,8 @@ static int pci_device_resume(struct devi
>
> 	if (drv && drv->resume)
> 		drv->resume(pci_dev);
>+	else
>+		pci_default_resume(pci_dev);
> 	return 0;
> }
>
>diff -urNp linux-1100/include/linux/pci.h
linux-1110/include/linux/pci.h
>--- linux-1100/include/linux/pci.h
>+++ linux-1110/include/linux/pci.h
>@@ -488,6 +488,11 @@ struct pci_dev {
> 	/* These fields are used by common fixups */
> 	unsigned int	transparent:1;	/* Transparent PCI bridge */
> 	unsigned int	multifunction:1;/* Part of multi-function device
*/
>+	/* keep track of device state */
>+	unsigned int	is_enabled:1;	/* pci_enable_device has been
>called */
>+	unsigned int	is_busmaster:1; /* device is busmaster */
>+
>+	unsigned int 	saved_config_space[16]; /* config space saved at
>suspend time */
> #ifdef CONFIG_PCI_NAMES
> #define PCI_NAME_SIZE	96
> #define PCI_NAME_HALF	__stringify(43)	/* less than half to handle slop
>*/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Resume enhancement: restore pci config space
  2004-06-01 16:02             ` Pavel Machek
@ 2004-06-01 16:18               ` Takashi Iwai
  0 siblings, 0 replies; 14+ messages in thread
From: Takashi Iwai @ 2004-06-01 16:18 UTC (permalink / raw)
  To: Pavel Machek; +Cc: Arjan van de Ven, greg, linux-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Resume enhancement: restore pci config space
  2004-06-01 15:38           ` Arjan van de Ven
  2004-06-01 15:58             ` Takashi Iwai
@ 2004-06-01 16:02             ` Pavel Machek
  2004-06-01 16:18               ` Takashi Iwai
  1 sibling, 1 reply; 14+ messages in thread
From: Pavel Machek @ 2004-06-01 16:02 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: Takashi Iwai, greg, linux-kernel

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Resume enhancement: restore pci config space
  2004-06-01 15:38           ` Arjan van de Ven
@ 2004-06-01 15:58             ` Takashi Iwai
  2004-06-01 16:02             ` Pavel Machek
  1 sibling, 0 replies; 14+ messages in thread
From: Takashi Iwai @ 2004-06-01 15:58 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: Pavel Machek, greg, linux-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Resume enhancement: restore pci config space
  2004-06-01 15:26         ` Takashi Iwai
@ 2004-06-01 15:38           ` Arjan van de Ven
  2004-06-01 15:58             ` Takashi Iwai
  2004-06-01 16:02             ` Pavel Machek
  0 siblings, 2 replies; 14+ messages in thread
From: Arjan van de Ven @ 2004-06-01 15:38 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: Pavel Machek, greg, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1515 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Resume enhancement: restore pci config space
  2004-06-01 15:06       ` Arjan van de Ven
@ 2004-06-01 15:26         ` Takashi Iwai
  2004-06-01 15:38           ` Arjan van de Ven
  0 siblings, 1 reply; 14+ messages in thread
From: Takashi Iwai @ 2004-06-01 15:26 UTC (permalink / raw)
  To: arjanv; +Cc: Pavel Machek, greg, linux-kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Resume enhancement: restore pci config space
  2004-06-01 13:54     ` Takashi Iwai
@ 2004-06-01 15:06       ` Arjan van de Ven
  2004-06-01 15:26         ` Takashi Iwai
  0 siblings, 1 reply; 14+ messages in thread
From: Arjan van de Ven @ 2004-06-01 15:06 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: Pavel Machek, greg, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 741 bytes --]


> 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.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Resume enhancement: restore pci config space
  2004-05-31 13:38   ` Arjan van de Ven
@ 2004-06-01 13:54     ` Takashi Iwai
  2004-06-01 15:06       ` Arjan van de Ven
  0 siblings, 1 reply; 14+ messages in thread
From: Takashi Iwai @ 2004-06-01 13:54 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: Pavel Machek, greg, linux-kernel

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 <tiwai@suse.de>		ALSA Developer - www.alsa-project.org

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Resume enhancement: restore pci config space
  2004-05-30 18:40 ` Pavel Machek
  2004-05-31 13:38   ` Arjan van de Ven
@ 2004-05-31 16:38   ` Jan-Benedict Glaw
  1 sibling, 0 replies; 14+ messages in thread
From: Jan-Benedict Glaw @ 2004-05-31 16:38 UTC (permalink / raw)
  To: linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]

On Sun, 2004-05-30 20:40:31 +0200, Pavel Machek <pavel@ucw.cz>
wrote in message <20040530184031.GF997@openzaurus.ucw.cz>:
> > 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.

All that reminds me... The PCI subsystem should probably record any PCI
device's initial configuration state after boot-up and restore those
settings before system reboot. Code like that was already in place
(during 2.2.x cycle IIRC) for Alphas, but got lost somewhen (and was
never added again). Upon reboot, Alphas (with current 2.6.x) running SRM
firmware might crash horribly because of "misconfigured" PCI devices.

MfG, JBG

-- 
   Jan-Benedict Glaw       jbglaw@lug-owl.de    . +49-172-7608481
   "Eine Freie Meinung in  einem Freien Kopf    | Gegen Zensur | Gegen Krieg
    fuer einen Freien Staat voll Freier Bürger" | im Internet! |   im Irak!
   ret = do_actions((curr | FREE_SPEECH) & ~(NEW_COPYRIGHT_LAW | DRM | TCPA));

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Resume enhancement: restore pci config space
  2004-05-30 18:40 ` Pavel Machek
@ 2004-05-31 13:38   ` Arjan van de Ven
  2004-06-01 13:54     ` Takashi Iwai
  2004-05-31 16:38   ` Jan-Benedict Glaw
  1 sibling, 1 reply; 14+ messages in thread
From: Arjan van de Ven @ 2004-05-31 13:38 UTC (permalink / raw)
  To: Pavel Machek; +Cc: greg, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1269 bytes --]

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...

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Resume enhancement: restore pci config space
  2004-05-26 20:35 Arjan van de Ven
  2004-05-26 22:39 ` Greg KH
  2004-05-27  9:44 ` Takashi Iwai
@ 2004-05-30 18:40 ` Pavel Machek
  2004-05-31 13:38   ` Arjan van de Ven
  2004-05-31 16:38   ` Jan-Benedict Glaw
  2 siblings, 2 replies; 14+ messages in thread
From: Pavel Machek @ 2004-05-30 18:40 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: greg, linux-kernel

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?
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Resume enhancement: restore pci config space 
  2004-05-26 20:35 Arjan van de Ven
  2004-05-26 22:39 ` Greg KH
@ 2004-05-27  9:44 ` Takashi Iwai
  2004-05-30 18:40 ` Pavel Machek
  2 siblings, 0 replies; 14+ messages in thread
From: Takashi Iwai @ 2004-05-27  9:44 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: greg, linux-kernel

Hi Arjan,

At Wed, 26 May 2004 22:35:26 +0200,
Arjan van de Ven wrote:
> 
> Hi,
> 
> The patch below enhances the PCI layer with 2 things
> 1) enable and busmaster state are stored in the pci device struct
> 2) pci config space is stored to the pci device struct
> 
> with that, it is possible to make a generic pci resume method that restores
> config space and reenables the device, including busmaster when appropriate.
> 
> 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.

Nice to have save_config_space[] in pci_dev even for the driver with
resume/suspend callbacks.
I already added bunch of such an array in the ALSA driver codes with
ifdef CONFIG_PM, but now they all can be removed again :)


--
Takashi Iwai <tiwai@suse.de>		ALSA Developer - www.alsa-project.org

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Resume enhancement: restore pci config space
  2004-05-26 20:35 Arjan van de Ven
@ 2004-05-26 22:39 ` Greg KH
  2004-05-27  9:44 ` Takashi Iwai
  2004-05-30 18:40 ` Pavel Machek
  2 siblings, 0 replies; 14+ messages in thread
From: Greg KH @ 2004-05-26 22:39 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: linux-kernel

On Wed, May 26, 2004 at 10:35:26PM +0200, Arjan van de Ven wrote:
> Hi,
> 
> The patch below enhances the PCI layer with 2 things
> 1) enable and busmaster state are stored in the pci device struct
> 2) pci config space is stored to the pci device struct
> 
> with that, it is possible to make a generic pci resume method that restores
> config space and reenables the device, including busmaster when appropriate.
> 
> 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.
> 
> Comments?

This looks good to me, I've applied it to my trees, thanks.

Hm, it still doesn't let me resume properly on my laptop when I suspend
to ram, but I'm trying to track that down...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Resume enhancement: restore pci config space 
@ 2004-05-26 20:35 Arjan van de Ven
  2004-05-26 22:39 ` Greg KH
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Arjan van de Ven @ 2004-05-26 20:35 UTC (permalink / raw)
  To: greg, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3581 bytes --]

Hi,

The patch below enhances the PCI layer with 2 things
1) enable and busmaster state are stored in the pci device struct
2) pci config space is stored to the pci device struct

with that, it is possible to make a generic pci resume method that restores
config space and reenables the device, including busmaster when appropriate.

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.

Comments?


Greetings,
   Arjan van de Ven


diff -urNp linux-1100/drivers/pci/pci.c linux-1110/drivers/pci/pci.c
--- linux-1100/drivers/pci/pci.c
+++ linux-1110/drivers/pci/pci.c
@@ -385,6 +385,7 @@ pci_enable_device_bars(struct pci_dev *d
 int
 pci_enable_device(struct pci_dev *dev)
 {
+	dev->is_enabled = 1;
 	return pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1);
 }
 
@@ -399,6 +400,9 @@ void
 pci_disable_device(struct pci_dev *dev)
 {
 	u16 pci_command;
+	
+	dev->is_enabled = 0;
+	dev->is_busmaster = 0;
 
 	pci_read_config_word(dev, PCI_COMMAND, &pci_command);
 	if (pci_command & PCI_COMMAND_MASTER) {
@@ -601,6 +605,7 @@ pci_set_master(struct pci_dev *dev)
 		cmd |= PCI_COMMAND_MASTER;
 		pci_write_config_word(dev, PCI_COMMAND, cmd);
 	}
+	dev->is_busmaster = 1;
 	pcibios_set_master(dev);
 }
 
diff -urNp linux-1100/drivers/pci/pci-driver.c linux-1110/drivers/pci/pci-driver.c
--- linux-1100/drivers/pci/pci-driver.c
+++ linux-1110/drivers/pci/pci-driver.c
@@ -299,10 +299,30 @@ static int pci_device_suspend(struct dev
 {
 	struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * drv = pci_dev->driver;
+	int i = 0;
 
 	if (drv && drv->suspend)
-		return drv->suspend(pci_dev,state);
-	return 0;
+		i = drv->suspend(pci_dev,state);
+		
+	pci_save_state(pci_dev, pci_dev->saved_config_space);
+	return i;
+}
+
+
+/* 
+ * 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)
+{
+	/* restore the PCI config space */
+	pci_restore_state(pci_dev, pci_dev->saved_config_space);
+	/* if the device was enabled before suspend, reenable */
+	if (pci_dev->is_enabled)
+		pci_enable_device(pci_dev);
+	/* if the device was busmaster before the suspend, make it busmaster again */
+	if (pci_dev->is_busmaster)
+		pci_set_master(pci_dev);
 }
 
 static int pci_device_resume(struct device * dev)
@@ -312,6 +332,8 @@ static int pci_device_resume(struct devi
 
 	if (drv && drv->resume)
 		drv->resume(pci_dev);
+	else
+		pci_default_resume(pci_dev);
 	return 0;
 }
 
diff -urNp linux-1100/include/linux/pci.h linux-1110/include/linux/pci.h
--- linux-1100/include/linux/pci.h
+++ linux-1110/include/linux/pci.h
@@ -488,6 +488,11 @@ struct pci_dev {
 	/* These fields are used by common fixups */
 	unsigned int	transparent:1;	/* Transparent PCI bridge */
 	unsigned int	multifunction:1;/* Part of multi-function device */
+	/* keep track of device state */
+	unsigned int	is_enabled:1;	/* pci_enable_device has been called */
+	unsigned int	is_busmaster:1; /* device is busmaster */
+	
+	unsigned int 	saved_config_space[16]; /* config space saved at suspend time */
 #ifdef CONFIG_PCI_NAMES
 #define PCI_NAME_SIZE	96
 #define PCI_NAME_HALF	__stringify(43)	/* less than half to handle slop */

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2004-06-01 16:26 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-05-26 21:44 Resume enhancement: restore pci config space Nakajima, Jun
  -- strict thread matches above, loose matches on Subject: below --
2004-05-26 20:35 Arjan van de Ven
2004-05-26 22:39 ` Greg KH
2004-05-27  9:44 ` Takashi Iwai
2004-05-30 18:40 ` Pavel Machek
2004-05-31 13:38   ` Arjan van de Ven
2004-06-01 13:54     ` Takashi Iwai
2004-06-01 15:06       ` Arjan van de Ven
2004-06-01 15:26         ` Takashi Iwai
2004-06-01 15:38           ` Arjan van de Ven
2004-06-01 15:58             ` Takashi Iwai
2004-06-01 16:02             ` Pavel Machek
2004-06-01 16:18               ` Takashi Iwai
2004-05-31 16:38   ` Jan-Benedict Glaw

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).