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@xxxxxxxxxx>
---
 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);
 }
 


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/