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?

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.

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.

Hi!

Quoted 6 lines

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.

...

drivers/pci/pci-driver.c static int pci_device_suspend(struct dev · line 317

Quoted 6 lines

drivers/pci/pci-driver.c +6 -0

+/* 
+ * 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?

Quoted 6 lines

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.

[1 <text/plain; us-ascii (7bit)>]

Quoted 9 lines · Pavel Machek

Hi!

Quoted 6 lines

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.

...

Quoted 9 lines

drivers/pci/pci-driver.c static int pci_device_suspend(struct dev · line 317

Quoted 6 lines

drivers/pci/pci-driver.c +6 -0

+/* 
+ * 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...

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.

[1 <text/plain (quoted-printable)>]

Quoted 7 lines

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

Quoted 3 lines

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,

Quoted 11 lines · Arjan van de Ven

[1 <text/plain (quoted-printable)>]

Quoted 7 lines

int xxx_resume(struct pci_dev *dev)

{
	int err;
	if ((err = pci_default_resume(dev)) < 0)
		return err;
	// ... do h/w specific
}

Quoted 3 lines

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.

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

[1 <text/plain; us-ascii (7bit)>]

Quoted 17 lines · Takashi Iwai

At Tue, 01 Jun 2004 17:06:38 +0200,

Quoted 14 lines · Arjan van de Ven

[1 <text/plain (quoted-printable)>]

Quoted 7 lines

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

Quoted 2 lines

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 ?

Quoted 3 lines

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.

Quoted 14 lines

Quoted 13 lines

[1 <text/plain (quoted-printable)>]

Quoted 7 lines

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

Quoted 2 lines

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 ?

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