Re: [RFC][PATCH][0/8] PM: Rework suspend-resume ordering to avoid problems with shared interrupts

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Saturday 14 March 2009, Frans Pop wrote:
> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
> > > # These don't need restoring anymore?
> >
> > I think they generally do, but the restored values may (and often are)
> > identical to the current ones.
> >
> > >    -pci 0000:00:02.1: restoring config space at offset 0x4 (was 0x4, writing 0xe0500004)
> > >    -pci 0000:00:02.1: restoring config space at offset 0x1 (was 0x900000, writing 0x900007)
> > >    -pci 0000:00:03.0: restoring config space at offset 0xf (was 0x100, writing 0x1ff)
> > >    -pci 0000:00:03.0: restoring config space at offset 0x4 (was 0xfed12004, writing 0xe0600004)
> > >    -pci 0000:00:03.2: restoring config space at offset 0xf (was 0x300, writing 0x30b)
> > >    -pci 0000:00:03.2: restoring config space at offset 0x8 (was 0x1, writing 0x2031)
> > >    -pci 0000:00:03.2: restoring config space at offset 0x7 (was 0x1, writing 0x2021)
> > >    -pci 0000:00:03.2: restoring config space at offset 0x6 (was 0x1, writing 0x2019)
> > >    -pci 0000:00:03.2: restoring config space at offset 0x5 (was 0x1, writing 0x2011)
> > >    -pci 0000:00:03.2: restoring config space at offset 0x4 (was 0x1, writing 0x2009)
> > >    -pci 0000:00:03.2: restoring config space at offset 0x1 (was 0xb00000, writing 0xb00001)
> [...]
> > > # These have moved down to late resume.
> >
> > That's a bit strange.  It looks like the registers changed after we had
> > restored them during "early" resume.  So either we hadn't actually
> > restored them (it would be interesting to find out why), or they really
> > changed (again, it would be interesting to see why).
> >
> > >    -uhci_hcd 0000:00:1a.0: restoring config space at offset 0xf (was 0x100, writing 0x10a)
> > >    -uhci_hcd 0000:00:1a.0: restoring config space at offset 0x8 (was 0x1, writing 0x2081)
> > >    -uhci_hcd 0000:00:1a.0: restoring config space at offset 0x1 (was 0x2800000, writing 0x2800001)
> > >    -uhci_hcd 0000:00:1a.1: restoring config space at offset 0xf (was 0x200, writing 0x20a)
> > >    -uhci_hcd 0000:00:1a.1: restoring config space at offset 0x8 (was 0x1, writing 0x20a1)
> > >    -uhci_hcd 0000:00:1a.1: restoring config space at offset 0x1 (was 0x2800000, writing 0x2800001)
> 
> These changes look to have been reverted somehow with rc8 + your latest
> patch set. Not sure if it's due to changes in the patches, or just an
> effect of local circumstances (such as (un)suspending while the system
> is docked). Or sun spots of course.
> 
> The "restoring config space" messages now look virtually the same
> as for rc5, only some messages for the ricoh-mmc module are still
> "missing", but I'm not worried about that.

Thanks for testing!

Could you please also test the last iteration of the $subject patch series
(just sent) with the appended patch applied on top and post dmesg output?

Rafael

---
 drivers/pci/pci-driver.c |   23 +++++++++++++++++++++--
 drivers/pci/pci.c        |    5 +++++
 2 files changed, 26 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -732,6 +732,9 @@ int
 pci_save_state(struct pci_dev *dev)
 {
 	int i;
+
+	dev_info(&dev->dev, "saving PCI config space\n");
+
 	/* XXX: 100% dword access ok here? */
 	for (i = 0; i < 16; i++)
 		pci_read_config_dword(dev, i * 4,&dev->saved_config_space[i]);
@@ -753,6 +756,8 @@ pci_restore_state(struct pci_dev *dev)
 	int i;
 	u32 val;
 
+	dev_info(&dev->dev, "restoring PCI config space\n");
+
 	/* PCI Express register must be restored first */
 	pci_restore_pcie_state(dev);
 
Index: linux-2.6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-driver.c
+++ linux-2.6/drivers/pci/pci-driver.c
@@ -438,10 +438,24 @@ static int pci_restore_standard_config(s
 {
 	pci_update_current_state(pci_dev, PCI_UNKNOWN);
 
+	switch (pci_dev->current_state) {
+	case PCI_UNKNOWN:
+	case PCI_POWER_ERROR:
+		dev_info(&pci_dev->dev, "%s: unknown power state\n",
+				__FUNCTION__);
+		break;
+	default:
+		dev_info(&pci_dev->dev, "%s: power state D%d\n",
+				__FUNCTION__, pci_dev->current_state);
+	}
+
 	if (pci_dev->current_state != PCI_D0) {
 		int error = pci_set_power_state(pci_dev, PCI_D0);
-		if (error)
+		if (error) {
+			dev_err(&pci_dev->dev,
+				"error %d while changing power state\n", error);
 			return error;
+		}
 	}
 
 	return pci_dev->state_saved ? pci_restore_state(pci_dev) : 0;
@@ -449,6 +463,8 @@ static int pci_restore_standard_config(s
 
 static void pci_pm_default_resume_noirq(struct pci_dev *pci_dev)
 {
+	dev_info(&pci_dev->dev, "%s: calling pci_restore_standard_config()\n",
+			__FUNCTION__);
 	pci_restore_standard_config(pci_dev);
 	pci_dev->state_saved = false;
 	pci_fixup_device(pci_fixup_resume_early, pci_dev);
@@ -615,8 +631,11 @@ static int pci_pm_resume(struct device *
 	 * This is necessary for the suspend error path in which resume is
 	 * called without restoring the standard config registers of the device.
 	 */
-	if (pci_dev->state_saved)
+	if (pci_dev->state_saved) {
+		dev_info(dev, "%s: restoring standard PCI config registers\n",
+				__FUNCTION__);
 		pci_restore_standard_config(pci_dev);
+	}
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);