[PATCH 9/9] PCI PM: generic suspend/resume fixes

[PATCH 9/9] PCI PM: generic suspend/resume fixes

[Adam Belay]

Some drivers never call pci_enable_device() or pci_enable_master() and
instead assume the bios will have initialized the device.  As a
workaround, this patch modifies the generic resume function to verify
which device features need to be enabled by checking the previous state
of the COMMAND register.  Also, pci_disable_device() is now called
during generic suspend.

Signed-off-by: Adam Belay <abelay@novell.com>

---
 pci-driver.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff -urN a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
--- a/drivers/pci/pci-driver.c	2006-05-30 21:41:14.000000000 -0400
+++ b/drivers/pci/pci-driver.c	2006-06-05 00:29:24.000000000 -0400
@@ -265,6 +265,16 @@
 	return 0;
 }
 
+/*
+ * Default suspend method for devices that have no driver provided suspend,
+ * or not even a driver at all.
+ */
+static void pci_default_suspend(struct pci_dev *pci_dev)
+{
+	pci_save_state(pci_dev);
+	pci_disable_device(pci_dev);
+}
+
 static int pci_device_suspend(struct device * dev, pm_message_t state)
 {
 	struct pci_dev * pci_dev = to_pci_dev(dev);
@@ -274,13 +284,11 @@
 	if (drv && drv->suspend) {
 		i = drv->suspend(pci_dev, state);
 		suspend_report_result(drv->suspend, i);
-	} else {
-		pci_save_state(pci_dev);
-	}
+	} else
+		pci_default_suspend(pci_dev);
 	return i;
 }
 
-
 /*
  * Default resume method for devices that have no driver provided resume,
  * or not even a driver at all.
@@ -288,14 +296,15 @@
 static void pci_default_resume(struct pci_dev *pci_dev)
 {
 	int retval;
+	u16 cmd = pci_dev->saved_config.command;
 
 	/* restore the PCI config space */
 	pci_restore_state(pci_dev);
 	/* if the device was enabled before suspend, reenable */
-	if (pci_dev->is_enabled)
+	if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY))
 		retval = pci_enable_device(pci_dev);
 	/* if the device was busmaster before the suspend, make it busmaster again */
-	if (pci_dev->is_busmaster)
+	if (cmd & PCI_COMMAND_MASTER)
 		pci_set_master(pci_dev);
 }
 

Re: [PATCH 9/9] PCI PM: generic suspend/resume fixes

[Alan Cox]

Ar Llu, 2006-06-05 am 04:46 -0400, ysgrifennodd Adam Belay:
> + * Default suspend method for devices that have no driver provided suspend,
> + * or not even a driver at all.
> + */
> +static void pci_default_suspend(struct pci_dev *pci_dev)
> +{
> +	pci_save_state(pci_dev);
> +	pci_disable_device(pci_dev);
> +}

How much testing has this had ? When people starting doing
disable_device on arbitary hardware various platforms broke horribly
as a result.

Re: [PATCH 9/9] PCI PM: generic suspend/resume fixes

[Adam Belay]

On Mon, 2006-06-05 at 11:21 +0100, Alan Cox wrote:
> Ar Llu, 2006-06-05 am 04:46 -0400, ysgrifennodd Adam Belay:
> > + * Default suspend method for devices that have no driver provided suspend,
> > + * or not even a driver at all.
> > + */
> > +static void pci_default_suspend(struct pci_dev *pci_dev)
> > +{
> > +	pci_save_state(pci_dev);
> > +	pci_disable_device(pci_dev);
> > +}
> 
> How much testing has this had ? When people starting doing
> disable_device on arbitary hardware various platforms broke horribly
> as a result.

Hi Alan,

I've only tested this on a few x86 boxes.  However, I think it's moving
in the right direction for correctly suspending devices.  It's worth
mentioning that the PCI PM specification requires the device to be
disabled before entering D3 (something that we fail to do before this
patchset), and the vast majority of devices would end up in this state
if we were using pci_set_power_state() in this function.

Unfortunately, far too many drivers still depend on this generic suspend
call, when they should all implement their own suspend function.  I
would except pci_disable_device() issues to the the exception, and as
such, device drivers should provide a ->suspend function that doesn't
call pci_disable_device() when they know their hardware can be
problematic.

With that in mind, any thoughts on giving this a little time in -mm and
seeing how it fares?  If any problems come up, we could revert to a more
conservative approach.

Thanks,
Adam

Re: [PATCH 9/9] PCI PM: generic suspend/resume fixes

[Alan Cox]

Ar Llu, 2006-06-05 am 13:48 -0400, ysgrifennodd Adam Belay:
> disabled before entering D3 (something that we fail to do before this
> patchset), and the vast majority of devices would end up in this state
> if we were using pci_set_power_state() in this function.

Only if that hardware supports D3 in the first place. That may be the
thing that is critical. 

> With that in mind, any thoughts on giving this a little time in -mm and
> seeing how it fares?  If any problems come up, we could revert to a more
> conservative approach.

It was a question not an objection. If the spec says it is right then it
has to be worth trying

Alan

Re: [PATCH 9/9] PCI PM: generic suspend/resume fixes

[Benjamin Herrenschmidt]

On Mon, 2006-06-05 at 13:48 -0400, Adam Belay wrote:

> I've only tested this on a few x86 boxes.  However, I think it's moving
> in the right direction for correctly suspending devices.  It's worth
> mentioning that the PCI PM specification requires the device to be
> disabled before entering D3 (something that we fail to do before this
> patchset), and the vast majority of devices would end up in this state
> if we were using pci_set_power_state() in this function.

That should be done only for D3 though. Not other states.

> Unfortunately, far too many drivers still depend on this generic suspend
> call, when they should all implement their own suspend function.  I
> would except pci_disable_device() issues to the the exception, and as
> such, device drivers should provide a ->suspend function that doesn't
> call pci_disable_device() when they know their hardware can be
> problematic.
> 
> With that in mind, any thoughts on giving this a little time in -mm and
> seeing how it fares?  If any problems come up, we could revert to a more
> conservative approach.

Problem is that you may end up disabling something like ... the ACPI
controller, and that will cause VERY BAD things with your BIOS when
actually trying to enter S3 or S4

Ben.

Re: [PATCH 9/9] PCI PM: generic suspend/resume fixes

[Adam Belay]

On Tue, 2006-06-06 at 10:33 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2006-06-05 at 13:48 -0400, Adam Belay wrote:
> 
> > I've only tested this on a few x86 boxes.  However, I think it's moving
> > in the right direction for correctly suspending devices.  It's worth
> > mentioning that the PCI PM specification requires the device to be
> > disabled before entering D3 (something that we fail to do before this
> > patchset), and the vast majority of devices would end up in this state
> > if we were using pci_set_power_state() in this function.
> 
> That should be done only for D3 though. Not other states.

I still think it's important that we actually do disable devices before
entering D3.  Currently there isn't a driver that does.

> 
> > Unfortunately, far too many drivers still depend on this generic suspend
> > call, when they should all implement their own suspend function.  I
> > would except pci_disable_device() issues to the the exception, and as
> > such, device drivers should provide a ->suspend function that doesn't
> > call pci_disable_device() when they know their hardware can be
> > problematic.
> > 
> > With that in mind, any thoughts on giving this a little time in -mm and
> > seeing how it fares?  If any problems come up, we could revert to a more
> > conservative approach.
> 
> Problem is that you may end up disabling something like ... the ACPI
> controller, and that will cause VERY BAD things with your BIOS when
> actually trying to enter S3 or S4

Alright, trying again then. :)


PCI PM: generic suspend/resume fixes

Some drivers never call pci_enable_device() or pci_enable_master() and
instead assume the bios will have initialized the device.  As a
workaround, this patch modifies the generic resume function to verify
which device features need to be enabled by checking the previous state
of the COMMAND register.

Signed-off-by: Adam Belay <abelay@novell.com>
---
 pci-driver.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff -urN a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
--- a/drivers/pci/pci-driver.c	2006-06-06 22:54:47.000000000 -0400
+++ b/drivers/pci/pci-driver.c	2006-06-06 23:05:32.000000000 -0400
@@ -280,7 +280,6 @@
 	return i;
 }
 
-
 /*
  * Default resume method for devices that have no driver provided resume,
  * or not even a driver at all.
@@ -288,14 +287,15 @@
 static void pci_default_resume(struct pci_dev *pci_dev)
 {
 	int retval;
+	u16 cmd = pci_dev->saved_config.command;
 
 	/* restore the PCI config space */
 	pci_restore_state(pci_dev);
 	/* if the device was enabled before suspend, reenable */
-	if (pci_dev->is_enabled)
+	if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY))
 		retval = pci_enable_device(pci_dev);
 	/* if the device was busmaster before the suspend, make it busmaster again */
-	if (pci_dev->is_busmaster)
+	if (cmd & PCI_COMMAND_MASTER)
 		pci_set_master(pci_dev);
 }