On Tue, 21 Feb 2006, Shaohua Li wrote:
> On Mon, 2006-02-20 at 17:31 -0800, Patrick Mochel wrote:
> > With the patch below, we don't touch the pci_disable_device() path, since
> > the device may need to enter a low-power state afterwards (whether it's
> > because of a system suspend or a module unload). Instead, we set the power
> > state of an unbound device to PCI_UNKNOWN on resume and do not try to
> > power it on or restore any of its config space.
> >
> > This is different than current behavior, where the config space is always
> > restored, even if the device is not bound to a driver. The fact that we
> > were doing this without putting the device into D0 is bad in itself - the
> > restored config space may not hold if the device is in D3.
> This might not work. We haven't driver for pci bridges, and we need
> restore bridges's config space.
Ugh, what a mess. We could really use a PCI bridge driver since that seems
to be the only thing we need that code for.
> Or another solution is we could simply set device's current_state to
> unknown in pci_device_remove.
Yes, that would work, as long we only did it when the device was in D0
when the driver was unloaded.
There is also another problem that may or may not have latent effects. If
the BIOS does what you describe -- leaves the device in D3 after a suspend
transition -- and the driver does not have a suspend method, then we might
have a problem in pci_default_resume().
The device might still be in D3 when we try to restore the config space,
which might get lost on the transition from D3 to D0: the PCI spec says
that a device will perform the equivalent of a soft reset on the D3->D0
transition. Whether or not this means the config space will be lost
depends on the device and would be impossible to tell.
The solution is to put the device in D0 before we restore the config
space. The patch below does that, with the caveat that it changes the
current behavior to only save/restore the config space for devices that
are enabled and in D0 on suspend.
Devices that are disabled and don't have a driver bound to them and are
not PCI bridges, so we shouldn't have to save their state. Devices that
are in a low-power state already should have accounted for the possible
lost state. And, we don't want to unnecessarily power up any devices on
resume (devices that are already in a low-power state or are not bound to
a driver).
The patch is currently untested, but should be straightforward.. Any
thoughts?
Thanks,
Pat
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 0aa14c9..6f5d0a9 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -254,6 +254,13 @@ static int pci_device_remove(struct devi
}
/*
+ * If the device is still on, set the power state as "unknown",
+ * since it might change by the next time we load the driver.
+ */
+ if (pci_dev->current_state == PCI_D0)
+ pci_dev->current_state = PCI_UNKNOWN;
+
+ /*
* We would love to complain here if pci_dev->is_enabled is set, that
* the driver should have called pci_disable_device(), but the
* unfortunate fact is there are too many odd BIOS and bridge setups
@@ -274,8 +281,18 @@ static int pci_device_suspend(struct dev
if (drv && drv->suspend)
i = drv->suspend(pci_dev, state);
- else
- pci_save_state(pci_dev);
+ else {
+ /*
+ * If device is enabled and in D0, then save its config
+ * space and mark its power state as "unknown", since we don't
+ * know if e.g. the BIOS will change its device state when we
+ * suspend.
+ */
+ if (pci_dev->is_enabled && pci_dev->current_state == PCI_D0) {
+ pci_save_state(pci_dev);
+ pci_dev->current_state = PCI_UNKNOWN;
+ }
+ }
return i;
}
@@ -286,16 +303,24 @@ static int pci_device_suspend(struct dev
*/
static void pci_default_resume(struct pci_dev *pci_dev)
{
- int retval;
-
- /* restore the PCI config space */
- pci_restore_state(pci_dev);
- /* if the device was enabled before suspend, reenable */
- if (pci_dev->is_enabled)
- retval = 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);
+ /*
+ * If the device is enabled but the device state is unknown,
+ * then bring the device into D0, restore the config space and
+ * re-enable the device.
+ *
+ * This will restore the config space for things like PCI bridges
+ * that don't have a driver and definitely need to be up and running.
+ * But, it will not touch devices that are in a specific low-power
+ * state, or are disabled for whatever reason.
+ */
+ if (pci_dev->is_enabled && pci_dev->current_state == PCI_UNKNOWN) {
+ pci_set_power_state(pci_dev, PCI_D0);
+ pci_restore_state(pci_dev);
+ 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)
-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems? Stop! Download the new AJAX search engine that makes
searching your log files as easy as surfing the web. DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
