[RFC][PATCH] PCI PM: Make new suspend-resume callbacks carry out core operations (rev. 2)

[RFC][PATCH] PCI PM: Make new suspend-resume callbacks carry out core operations (rev. 2)

[Rafael J. Wysocki]

On Friday 19 December 2008, Rafael J. Wysocki wrote:> Hi Len,> > The patch below adds the callbacks that save/restore the standard PCI config> registers, change the power state of the device etc. to the new suspend-resume> callbacks (which are not yet used by device drivers), so that the drivers don't> have to worry about these operations.> > The saving and restoring of the standard PCI config registers is done with> interrupts off, the other things are done with interrupts enabled.> > The patch should apply to the current linux-next tree.
Below is a new version of the patch.
Now, the standard config spaces of devices are restored with interruptsdisabled, but only if the bridges the devices are behind are in D0.  Otherwise,the operation is attempted again with interrupts enabled (presumably thebridge would be put into D0 before that happens).
Also, pci_disable_device() is not called during resume andpci_reenable_device() is used to enable the devices that were enabled duringsuspend.  This allows us to automatically call pci_set_master() for devicesthat were bus masters before suspend.  Additionally, the pci_reenable_device()may be overriden by the driver, by disabling the device during suspend.
Finally, bridges are handled a bit differently from the regular devices (theyare not put into low power states during suspend and pci_enable_wake() isnot called for them.
The patch should apply on top of linux-next with the patch fixingpci_update_current_state() I sent yesterday(http://marc.info/?l=linux-kernel&m=123039199607159&w=4).
Please review.
Thanks,Rafael
---There are a few standard PCI callbacks that should be used by anyPCI driver implementing suspend-resume handlers.  The drivers,however, tend to use these callbacks in different ways, in differentorder or even not at all.  This sometimes causes problems to appearand makes PCI suspend-resume diagnostics troublesome.  Thus it seemsreasonable to make the PCI core execute the standard callbacks sothat the drivers don't have to do it.
Modify the power management framework for PCI devices so that thestandard PCI PM callbacks are executed by the core in the followingway:
* Right after executing the ->suspend() callback from the device  driver's pm object (ie. with interrupts enabled) the core will  execute pci_save_state() and, if the device is not a PCI bridge,  the core will execute pci_prepare_to_sleep().
* Right prior to executing the ->resume_noirq() callback from the  device driver's pm object (ie. with interrupts disabled), the core  will execute pci_restore_state() and will update the device's  pci_dev structure to reflect the actual current power state of the  device.  However, this is only going to succeed if the device's bus  is operational (ie. its parent bridge in is the D0 power state) at  that time, so the last operation has to be repeated later.
* Prior to executing the ->resume() callback from the device driver's  pm object (ie. with interrupts enabled) the core will check if the  device's standard config registers need to be restored and it will  call pci_restore_state() if necessary.  Next, if the device is not  a PCI bridge, the core will disable the device's wake-up capability  using pci_enable_wake().  Finally, it will execute  pci_reenable_device() and, if necessary, pci_set_master(), in this  order (all of this happens before the ->resume() callback is  invoked).
The standard callbacks mentioned above will always be executed forall devices without drivers providing the legacy PCI PM support(ie. the ->suspend(), ->suspend_late(), ->resume_early()or ->resume() hooks in the pci_driver structure).  In particular,they will be executed for devices that don't have drivers at all.
Analogous changes are made in the hibernation-related PCI PMroutines.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>--- drivers/pci/pci-driver.c |  356 +++++++++++++++++++++++++++-------------------- drivers/pci/pci.c        |    2  drivers/pci/pci.h        |    1  3 files changed, 212 insertions(+), 147 deletions(-)
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@@ -300,98 +300,83 @@ static void pci_device_shutdown(struct d  #ifdef CONFIG_PM_SLEEP -static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)-{-	struct pci_driver *drv = pci_dev->driver;--	return drv && (drv->suspend || drv->suspend_late || drv->resume-		|| drv->resume_early);-}+/* Routines used in both the legacy and new power management callbacks */ -/*- * Default "suspend" method for devices that have no driver provided suspend,- * or not even a driver at all.- */-static void pci_default_pm_suspend(struct pci_dev *pci_dev)+static int pci_restore_standard_config(struct pci_dev *pci_dev) {-	pci_save_state(pci_dev);-	/*-	 * 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->current_state == PCI_D0)+	struct pci_dev *parent = pci_dev->bus->self;+	int error = 0;++	/* Check if the device's bus is operational */+	if (!parent || parent->current_state == PCI_D0) {+		pci_restore_state(pci_dev);+		pci_update_current_state(pci_dev, PCI_D0);+	} else {+		dev_warn(&pci_dev->dev, "unable to restore config, "+			"bridge %s in low power state D%d\n", pci_name(parent),+			parent->current_state); 		pci_dev->current_state = PCI_UNKNOWN;-}+		error = -EAGAIN;+	} -/*- * Default "resume" method for devices that have no driver provided resume,- * or not even a driver at all (first part).- */-static void pci_default_pm_resume_early(struct pci_dev *pci_dev)-{-	/* restore the PCI config space */-	pci_restore_state(pci_dev);+	return error; } -/*- * Default "resume" method for devices that have no driver provided resume,- * or not even a driver at all (second part).- */-static int pci_default_pm_resume_late(struct pci_dev *pci_dev)-{-	int retval;--	/* if the device was enabled before suspend, reenable */-	retval = pci_reenable_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);--	return retval;-}+/* Legacy power management callbacks */  static int pci_legacy_suspend(struct device *dev, pm_message_t state) { 	struct pci_dev * pci_dev = to_pci_dev(dev); 	struct pci_driver * drv = pci_dev->driver;-	int i = 0;+	int error = 0;  	if (drv && drv->suspend) {-		i = drv->suspend(pci_dev, state);-		suspend_report_result(drv->suspend, i);+		error = drv->suspend(pci_dev, state);+		suspend_report_result(drv->suspend, error); 	} else {-		pci_default_pm_suspend(pci_dev);+		error = pci_save_state(pci_dev);+		/*+		 * 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 (!error && pci_dev->current_state == PCI_D0)+			pci_dev->current_state = PCI_UNKNOWN; 	}-	return i;+	return error; }  static int pci_legacy_suspend_late(struct device *dev, pm_message_t state) { 	struct pci_dev * pci_dev = to_pci_dev(dev); 	struct pci_driver * drv = pci_dev->driver;-	int i = 0;+	int error = 0;  	if (drv && drv->suspend_late) {-		i = drv->suspend_late(pci_dev, state);-		suspend_report_result(drv->suspend_late, i);+		error = drv->suspend_late(pci_dev, state);+		suspend_report_result(drv->suspend_late, error); 	}-	return i;+	return error; }  static int pci_legacy_resume(struct device *dev) {-	int error; 	struct pci_dev * pci_dev = to_pci_dev(dev); 	struct pci_driver * drv = pci_dev->driver;+	int error;  	if (drv && drv->resume) { 		error = drv->resume(pci_dev); 	} else {-		pci_default_pm_resume_early(pci_dev);-		error = pci_default_pm_resume_late(pci_dev);+		/* restore the PCI config space */+		pci_restore_state(pci_dev);+		/* if the device was enabled before suspend, reenable */+		error = pci_reenable_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); 	} 	return error; }@@ -407,6 +392,62 @@ static int pci_legacy_resume_early(struc 	return error; } +/* Routines used in the new power management callbacks */++static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)+{+	struct pci_driver *drv = pci_dev->driver;++	return drv && (drv->suspend || drv->suspend_late || drv->resume+		|| drv->resume_early);+}++static bool pci_is_bridge(struct pci_dev *pci_dev)+{+	return !!(pci_dev->subordinate);+}++static void pci_pm_default_suspend_noirq(struct pci_dev *pci_dev)+{+	/* The device's power state may be changed by the BIOS before resume */+	if( pci_dev->current_state == PCI_D0)+		pci_dev->current_state = PCI_UNKNOWN;+}++static int pci_pm_default_resume(struct pci_dev *pci_dev)+{+	int error = 0;++	/*+	 * pci_restore_standard_config() should have been called before, but it+	 * would have failed if the device's bus had not been operational at+	 * that time.  Check it and try again if necessary.+	 */+	if (pci_dev->current_state == PCI_UNKNOWN) {+		error = pci_restore_standard_config(pci_dev);+		if (error)+			return error;+	}++	pci_fixup_device(pci_fixup_resume, pci_dev);++	/* Disable wake-up capability of regular devices */+	if (!pci_is_bridge(pci_dev))+		pci_enable_wake(pci_dev, PCI_D0, false);+	/* If device was enabled before suspend, reenable it. */+	error = pci_reenable_device(pci_dev);+	/*+	 * If device was a bus master before the suspend, make it a bus master+	 * again.+	 */+	if (pci_dev->is_busmaster)+		pci_set_master(pci_dev);++	return error;+}++/* New power management callbacks */+ static int pci_pm_prepare(struct device *dev) { 	struct device_driver *drv = dev->driver;@@ -434,14 +475,23 @@ static int pci_pm_suspend(struct device  	struct device_driver *drv = dev->driver; 	int error = 0; -	if (drv && drv->pm) {-		if (drv->pm->suspend) {-			error = drv->pm->suspend(dev);-			suspend_report_result(drv->pm->suspend, error);-		}-	} else if (pci_has_legacy_pm_support(pci_dev)) {+	if (pci_has_legacy_pm_support(pci_dev)) { 		error = pci_legacy_suspend(dev, PMSG_SUSPEND);+		goto Exit;+	}++	if (drv && drv->pm && drv->pm->suspend) {+		error = drv->pm->suspend(dev);+		suspend_report_result(drv->pm->suspend, error);+	}++	if (!error) {+		error = pci_save_state(pci_dev);+		if (!error && !pci_is_bridge(pci_dev))+			pci_prepare_to_sleep(pci_dev); 	}++ Exit: 	pci_fixup_device(pci_fixup_suspend, pci_dev);  	return error;@@ -453,17 +503,17 @@ static int pci_pm_suspend_noirq(struct d 	struct device_driver *drv = dev->driver; 	int error = 0; -	if (drv && drv->pm) {-		if (drv->pm->suspend_noirq) {-			error = drv->pm->suspend_noirq(dev);-			suspend_report_result(drv->pm->suspend_noirq, error);-		}-	} else if (pci_has_legacy_pm_support(pci_dev)) {-		error = pci_legacy_suspend_late(dev, PMSG_SUSPEND);-	} else {-		pci_default_pm_suspend(pci_dev);+	if (pci_has_legacy_pm_support(pci_dev))+		return pci_legacy_suspend_late(dev, PMSG_SUSPEND);++	if (drv && drv->pm && drv->pm->suspend_noirq) {+		error = drv->pm->suspend_noirq(dev);+		suspend_report_result(drv->pm->suspend_noirq, error); 	} +	if (!error)+		pci_pm_default_suspend_noirq(pci_dev);+ 	return error; } @@ -473,17 +523,16 @@ static int pci_pm_resume(struct device * 	struct device_driver *drv = dev->driver; 	int error = 0; -	pci_fixup_device(pci_fixup_resume, pci_dev);--	if (drv && drv->pm) {-		if (drv->pm->resume)-			error = drv->pm->resume(dev);-	} else if (pci_has_legacy_pm_support(pci_dev)) {-		error = pci_legacy_resume(dev);-	} else {-		error = pci_default_pm_resume_late(pci_dev);+	if (pci_has_legacy_pm_support(pci_dev)) {+		pci_fixup_device(pci_fixup_resume, pci_dev);+		return pci_legacy_resume(dev); 	} +	error = pci_pm_default_resume(pci_dev);++	if (!error && drv && drv->pm && drv->pm->resume)+		error = drv->pm->resume(dev);+ 	return error; } @@ -493,15 +542,16 @@ static int pci_pm_resume_noirq(struct de 	struct device_driver *drv = dev->driver; 	int error = 0; -	pci_fixup_device(pci_fixup_resume_early, to_pci_dev(dev));+	if (pci_has_legacy_pm_support(pci_dev)) {+		pci_fixup_device(pci_fixup_resume_early, pci_dev);+		return pci_legacy_resume_early(dev);+	} -	if (drv && drv->pm) {-		if (drv->pm->resume_noirq)+	if (!pci_restore_standard_config(pci_dev)) {+		pci_fixup_device(pci_fixup_resume_early, pci_dev);++		if (drv && drv->pm && drv->pm->resume_noirq) 			error = drv->pm->resume_noirq(dev);-	} else if (pci_has_legacy_pm_support(pci_dev)) {-		error = pci_legacy_resume_early(dev);-	} else {-		pci_default_pm_resume_early(pci_dev); 	}  	return error;@@ -524,16 +574,20 @@ static int pci_pm_freeze(struct device * 	struct device_driver *drv = dev->driver; 	int error = 0; -	if (drv && drv->pm) {-		if (drv->pm->freeze) {-			error = drv->pm->freeze(dev);-			suspend_report_result(drv->pm->freeze, error);-		}-	} else if (pci_has_legacy_pm_support(pci_dev)) {+	if (pci_has_legacy_pm_support(pci_dev)) { 		error = pci_legacy_suspend(dev, PMSG_FREEZE); 		pci_fixup_device(pci_fixup_suspend, pci_dev);+		return error; 	} +	if (drv && drv->pm && drv->pm->freeze) {+		error = drv->pm->freeze(dev);+		suspend_report_result(drv->pm->freeze, error);+	}++	if (!error)+		error = pci_save_state(pci_dev);+ 	return error; } @@ -543,17 +597,17 @@ static int pci_pm_freeze_noirq(struct de 	struct device_driver *drv = dev->driver; 	int error = 0; -	if (drv && drv->pm) {-		if (drv->pm->freeze_noirq) {-			error = drv->pm->freeze_noirq(dev);-			suspend_report_result(drv->pm->freeze_noirq, error);-		}-	} else if (pci_has_legacy_pm_support(pci_dev)) {-		error = pci_legacy_suspend_late(dev, PMSG_FREEZE);-	} else {-		pci_default_pm_suspend(pci_dev);+	if (pci_has_legacy_pm_support(pci_dev))+		return pci_legacy_suspend_late(dev, PMSG_FREEZE);++	if (drv && drv->pm && drv->pm->freeze_noirq) {+		error = drv->pm->freeze_noirq(dev);+		suspend_report_result(drv->pm->freeze_noirq, error); 	} +	if (!error)+		pci_pm_default_suspend_noirq(pci_dev);+ 	return error; } @@ -563,14 +617,14 @@ static int pci_pm_thaw(struct device *de 	struct device_driver *drv = dev->driver; 	int error = 0; -	if (drv && drv->pm) {-		if (drv->pm->thaw)-			error =  drv->pm->thaw(dev);-	} else if (pci_has_legacy_pm_support(pci_dev)) {+	if (pci_has_legacy_pm_support(pci_dev)) { 		pci_fixup_device(pci_fixup_resume, pci_dev);-		error = pci_legacy_resume(dev);+		return pci_legacy_resume(dev); 	} +	if (drv && drv->pm && drv->pm->thaw)+		error = drv->pm->thaw(dev);+ 	return error; } @@ -580,14 +634,17 @@ static int pci_pm_thaw_noirq(struct devi 	struct device_driver *drv = dev->driver; 	int error = 0; -	if (drv && drv->pm) {-		if (drv->pm->thaw_noirq)-			error = drv->pm->thaw_noirq(dev);-	} else if (pci_has_legacy_pm_support(pci_dev)) {-		pci_fixup_device(pci_fixup_resume_early, to_pci_dev(dev));-		error = pci_legacy_resume_early(dev);+	if (pci_has_legacy_pm_support(pci_dev)) {+		pci_fixup_device(pci_fixup_resume_early, pci_dev);+		return pci_legacy_resume_early(dev); 	} +	if (pci_dev->current_state == PCI_UNKNOWN)+		pci_update_current_state(pci_dev, PCI_D0);++	if (drv && drv->pm && drv->pm->thaw_noirq)+		error = drv->pm->thaw_noirq(dev);+ 	return error; } @@ -597,32 +654,39 @@ static int pci_pm_poweroff(struct device 	struct device_driver *drv = dev->driver; 	int error = 0; -	pci_fixup_device(pci_fixup_suspend, pci_dev);--	if (drv && drv->pm) {-		if (drv->pm->poweroff) {-			error = drv->pm->poweroff(dev);-			suspend_report_result(drv->pm->poweroff, error);-		}-	} else if (pci_has_legacy_pm_support(pci_dev)) {+	if (pci_has_legacy_pm_support(pci_dev)) { 		error = pci_legacy_suspend(dev, PMSG_HIBERNATE);+		goto Exit;+	}++	if (drv && drv->pm && drv->pm->poweroff) {+		error = drv->pm->poweroff(dev);+		suspend_report_result(drv->pm->poweroff, error);+	}++	if (!error && !pci_is_bridge(pci_dev)) {+		pci_disable_device(pci_dev);+		pci_prepare_to_sleep(pci_dev); 	} + Exit:+	pci_fixup_device(pci_fixup_suspend, pci_dev);+ 	return error; }  static int pci_pm_poweroff_noirq(struct device *dev) {+	struct pci_dev *pci_dev = to_pci_dev(dev); 	struct device_driver *drv = dev->driver; 	int error = 0; -	if (drv && drv->pm) {-		if (drv->pm->poweroff_noirq) {-			error = drv->pm->poweroff_noirq(dev);-			suspend_report_result(drv->pm->poweroff_noirq, error);-		}-	} else if (pci_has_legacy_pm_support(to_pci_dev(dev))) {-		error = pci_legacy_suspend_late(dev, PMSG_HIBERNATE);+	if (pci_has_legacy_pm_support(pci_dev))+		return pci_legacy_suspend_late(dev, PMSG_HIBERNATE);++	if (drv && drv->pm && drv->pm->poweroff_noirq) {+		error = drv->pm->poweroff_noirq(dev);+		suspend_report_result(drv->pm->poweroff_noirq, error); 	}  	return error;@@ -634,15 +698,15 @@ static int pci_pm_restore(struct device  	struct device_driver *drv = dev->driver; 	int error = 0; -	if (drv && drv->pm) {-		if (drv->pm->restore)-			error = drv->pm->restore(dev);-	} else if (pci_has_legacy_pm_support(pci_dev)) {-		error = pci_legacy_resume(dev);-	} else {-		error = pci_default_pm_resume_late(pci_dev);+	if (pci_has_legacy_pm_support(pci_dev)) {+		pci_fixup_device(pci_fixup_resume, pci_dev);+		return pci_legacy_resume(dev); 	}-	pci_fixup_device(pci_fixup_resume, pci_dev);++	error = pci_pm_default_resume(pci_dev);++	if (!error && drv && drv->pm && drv->pm->restore)+		error = drv->pm->restore(dev);  	return error; }@@ -653,17 +717,17 @@ static int pci_pm_restore_noirq(struct d 	struct device_driver *drv = dev->driver; 	int error = 0; -	pci_fixup_device(pci_fixup_resume, pci_dev);+	if (pci_has_legacy_pm_support(pci_dev)) {+		pci_fixup_device(pci_fixup_resume_early, pci_dev);+		return pci_legacy_resume_early(dev);+	}++	if (!pci_restore_standard_config(pci_dev)) {+		pci_fixup_device(pci_fixup_resume_early, pci_dev); -	if (drv && drv->pm) {-		if (drv->pm->restore_noirq)+		if (drv && drv->pm && drv->pm->restore_noirq) 			error = drv->pm->restore_noirq(dev);-	} else if (pci_has_legacy_pm_support(pci_dev)) {-		error = pci_legacy_resume_early(dev);-	} else {-		pci_default_pm_resume_early(pci_dev); 	}-	pci_fixup_device(pci_fixup_resume_early, pci_dev);  	return error; }Index: linux-2.6/drivers/pci/pci.c===================================================================--- linux-2.6.orig/drivers/pci/pci.c+++ linux-2.6/drivers/pci/pci.c@@ -526,7 +526,7 @@ pci_raw_set_power_state(struct pci_dev *  * @dev: PCI device to handle.  * @state: State to cache in case the device doesn't have the PM capability  */-static void pci_update_current_state(struct pci_dev *dev, pci_power_t state)+void pci_update_current_state(struct pci_dev *dev, pci_power_t state) { 	if (dev->pm_cap) { 		u16 pmcsr;Index: linux-2.6/drivers/pci/pci.h===================================================================--- linux-2.6.orig/drivers/pci/pci.h+++ linux-2.6/drivers/pci/pci.h@@ -121,6 +121,7 @@ static inline int pci_no_d1d2(struct pci 	return (dev->no_d1d2 || parent_dstates);  }+extern void pci_update_current_state(struct pci_dev *dev, pci_power_t state); extern int pcie_mch_quirk; extern struct device_attribute pci_dev_attrs[]; extern struct device_attribute dev_attr_cpuaffinity;\0˙ôčş{.nÇ+‰ˇŸŽ‰­†+%ŠË˙ąéÝś\x17ĽŠw˙ş{.nÇ+‰ˇĽŠ{ąţGŤé˙Š{ayş\x1dʇڙë,j\a­˘fŁ˘ˇhšďę˙‘ęçz_čŽ\x03(­éšŽŠÝ˘j"ú\x1aś^[m§˙˙ž\aŤţGŤé˙˘¸?™¨č­Ú&Łř§~áśiO•ćŹzˇšvŘ^\x14\x04\x1aś^[m§˙˙Ă\f˙śě˙˘¸?–IĽ

[RFC][PATCH 0/10] PCI PM: Make new suspend-resume callbacks carry out core operations

[Rafael J. Wysocki]

On Sunday 28 December 2008, Rafael J. Wysocki wrote:
> On Friday 19 December 2008, Rafael J. Wysocki wrote:
> > Hi Len,
> > 
> > The patch below adds the callbacks that save/restore the standard PCI config
> > registers, change the power state of the device etc. to the new suspend-resume
> > callbacks (which are not yet used by device drivers), so that the drivers don't
> > have to worry about these operations.
> > 
> > The saving and restoring of the standard PCI config registers is done with
> > interrupts off, the other things are done with interrupts enabled.
> > 
> > The patch should apply to the current linux-next tree.
> 
> Below is a new version of the patch.
> 
> Now, the standard config spaces of devices are restored with interrupts
> disabled, but only if the bridges the devices are behind are in D0.  Otherwise,
> the operation is attempted again with interrupts enabled (presumably the
> bridge would be put into D0 before that happens).
> 
> Also, pci_disable_device() is not called during resume and
> pci_reenable_device() is used to enable the devices that were enabled during
> suspend.  This allows us to automatically call pci_set_master() for devices
> that were bus masters before suspend.  Additionally, the pci_reenable_device()
> may be overriden by the driver, by disabling the device during suspend.
> 
> Finally, bridges are handled a bit differently from the regular devices (they
> are not put into low power states during suspend and pci_enable_wake() is
> not called for them.
> 
> The patch should apply on top of linux-next with the patch fixing
> pci_update_current_state() I sent yesterday
> (http://marc.info/?l=linux-kernel&m=123039199607159&w=4).

In the meantime I realized that the pcibios_enable_device() called from
pci_reenable_device() in the resume code path should be balanced by a "suspend"
call of the same kind.

Also, I decided to split the patch into a series of smaller patches with their
own changelogs to make it easier to follow the changes.  Perhaps this way
it will be more clear what I'm after.

The patches should apply to linux-next with the patch from
http://marc.info/?l=linux-kernel&m=123039199607159&w=4 on top.

Comments welcome.

Happy New Year to everyone,
Rafael

[RFC][PATCH 1/10] PCI PM: Fix poweroff and restore callbacks

[Rafael J. Wysocki]

pci_fixup_device() is called too early in pci_pm_poweroff() and too
late in pci_pm_restore().  Moreover, pci_pm_restore_noirq() calls
pci_fixup_device() twice and in a wrong way.  Fix that.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

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
@@ -597,8 +597,6 @@ static int pci_pm_poweroff(struct device
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	pci_fixup_device(pci_fixup_suspend, pci_dev);
-
 	if (drv && drv->pm) {
 		if (drv->pm->poweroff) {
 			error = drv->pm->poweroff(dev);
@@ -608,6 +606,8 @@ static int pci_pm_poweroff(struct device
 		error = pci_legacy_suspend(dev, PMSG_HIBERNATE);
 	}
 
+	pci_fixup_device(pci_fixup_suspend, pci_dev);
+
 	return error;
 }
 
@@ -634,6 +634,8 @@ static int pci_pm_restore(struct device 
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	pci_fixup_device(pci_fixup_resume, pci_dev);
+
 	if (drv && drv->pm) {
 		if (drv->pm->restore)
 			error = drv->pm->restore(dev);
@@ -642,7 +644,6 @@ static int pci_pm_restore(struct device 
 	} else {
 		error = pci_default_pm_resume_late(pci_dev);
 	}
-	pci_fixup_device(pci_fixup_resume, pci_dev);
 
 	return error;
 }
@@ -653,7 +654,7 @@ static int pci_pm_restore_noirq(struct d
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	pci_fixup_device(pci_fixup_resume, pci_dev);
+	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 
 	if (drv && drv->pm) {
 		if (drv->pm->restore_noirq)
@@ -663,7 +664,6 @@ static int pci_pm_restore_noirq(struct d
 	} else {
 		pci_default_pm_resume_early(pci_dev);
 	}
-	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 
 	return error;
 }

Re: [RFC][PATCH 1/10] PCI PM: Fix poweroff and restore callbacks

[Pavel Machek]

Hi!

> pci_fixup_device() is called too early in pci_pm_poweroff() and too
> late in pci_pm_restore().  Moreover, pci_pm_restore_noirq() calls
> pci_fixup_device() twice and in a wrong way.  Fix that.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Acked-by: Pavel Machek <pavel@suse.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC][PATCH 2/10] PCI PM: Add suspend counterpart of pci_reenable_device

[Rafael J. Wysocki]

PCI devices without drivers are not disabled during suspend and
hibernation, but they are enabled during resume, with the help of
pci_reenable_device(), so there is an unbalanced execution of
pcibios_enable_device() in the resume code path.

To correct this introduce function pci_disable_enabled_device()
that will disable the argument device, if it is enabled when the
function is being run, without updating the device's pci_dev
structure and use it in the suspend code path to balance the
pci_reenable_device() executed during resume.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |   35 ++++++++++++++++++++++++++++++-----
 drivers/pci/pci.c        |   36 ++++++++++++++++++++++++++++--------
 drivers/pci/pci.h        |    1 +
 3 files changed, 59 insertions(+), 13 deletions(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -969,6 +969,32 @@ void pcim_pin_device(struct pci_dev *pde
  */
 void __attribute__ ((weak)) pcibios_disable_device (struct pci_dev *dev) {}
 
+static void do_pci_disable_device(struct pci_dev *dev)
+{
+	u16 pci_command;
+
+	pci_read_config_word(dev, PCI_COMMAND, &pci_command);
+	if (pci_command & PCI_COMMAND_MASTER) {
+		pci_command &= ~PCI_COMMAND_MASTER;
+		pci_write_config_word(dev, PCI_COMMAND, pci_command);
+	}
+
+	pcibios_disable_device(dev);
+}
+
+/**
+ * pci_disable_enabled_device - Disable device without updating enable_cnt
+ * @dev: PCI device to disable
+ *
+ * NOTE: This function is a backend of PCI power management routines and is
+ * not supposed to be called drivers.
+ */
+void pci_disable_enabled_device(struct pci_dev *dev)
+{
+	if (atomic_read(&dev->enable_cnt))
+		do_pci_disable_device(dev);
+}
+
 /**
  * pci_disable_device - Disable PCI device after use
  * @dev: PCI device to be disabled
@@ -983,7 +1009,6 @@ void
 pci_disable_device(struct pci_dev *dev)
 {
 	struct pci_devres *dr;
-	u16 pci_command;
 
 	dr = find_pci_dr(dev);
 	if (dr)
@@ -992,14 +1017,9 @@ pci_disable_device(struct pci_dev *dev)
 	if (atomic_sub_return(1, &dev->enable_cnt) != 0)
 		return;
 
-	pci_read_config_word(dev, PCI_COMMAND, &pci_command);
-	if (pci_command & PCI_COMMAND_MASTER) {
-		pci_command &= ~PCI_COMMAND_MASTER;
-		pci_write_config_word(dev, PCI_COMMAND, pci_command);
-	}
-	dev->is_busmaster = 0;
+	do_pci_disable_device(dev);
 
-	pcibios_disable_device(dev);
+	dev->is_busmaster = 0;
 }
 
 /**
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -40,6 +40,7 @@ struct pci_platform_pm_ops {
 };
 
 extern int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
+extern void pci_disable_enabled_device(struct pci_dev *dev);
 extern void pci_pm_init(struct pci_dev *dev);
 extern void pci_allocate_cap_save_buffers(struct pci_dev *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
@@ -310,9 +310,19 @@ static bool pci_has_legacy_pm_support(st
 
 /*
  * Default "suspend" method for devices that have no driver provided suspend,
- * or not even a driver at all.
+ * or not even a driver at all (first part).
+ */
+static void pci_default_pm_suspend_early(struct pci_dev *pci_dev)
+{
+	/* If device is enabled at this point, disable it */
+	pci_disable_enabled_device(pci_dev);
+}
+
+/*
+ * Default "suspend" method for devices that have no driver provided suspend,
+ * or not even a driver at all (second part).
  */
-static void pci_default_pm_suspend(struct pci_dev *pci_dev)
+static void pci_default_pm_suspend_late(struct pci_dev *pci_dev)
 {
 	pci_save_state(pci_dev);
 	/*
@@ -363,7 +373,11 @@ static int pci_legacy_suspend(struct dev
 		i = drv->suspend(pci_dev, state);
 		suspend_report_result(drv->suspend, i);
 	} else {
-		pci_default_pm_suspend(pci_dev);
+		/*
+		 * For compatibility with existing code with legacy PM support
+		 * don't call pci_default_pm_suspend_early() here.
+		 */
+		pci_default_pm_suspend_late(pci_dev);
 	}
 	return i;
 }
@@ -441,7 +455,10 @@ static int pci_pm_suspend(struct device 
 		}
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
+	} else {
+		pci_default_pm_suspend_early(pci_dev);
 	}
+
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
 
 	return error;
@@ -461,7 +478,7 @@ static int pci_pm_suspend_noirq(struct d
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend_late(dev, PMSG_SUSPEND);
 	} else {
-		pci_default_pm_suspend(pci_dev);
+		pci_default_pm_suspend_late(pci_dev);
 	}
 
 	return error;
@@ -532,6 +549,8 @@ static int pci_pm_freeze(struct device *
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend(dev, PMSG_FREEZE);
 		pci_fixup_device(pci_fixup_suspend, pci_dev);
+	} else {
+		pci_default_pm_suspend_early(pci_dev);
 	}
 
 	return error;
@@ -551,7 +570,7 @@ static int pci_pm_freeze_noirq(struct de
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend_late(dev, PMSG_FREEZE);
 	} else {
-		pci_default_pm_suspend(pci_dev);
+		pci_default_pm_suspend_late(pci_dev);
 	}
 
 	return error;
@@ -569,6 +588,8 @@ static int pci_pm_thaw(struct device *de
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		pci_fixup_device(pci_fixup_resume, pci_dev);
 		error = pci_legacy_resume(dev);
+	} else {
+		pci_default_pm_resume_late(pci_dev);
 	}
 
 	return error;
@@ -586,6 +607,8 @@ static int pci_pm_thaw_noirq(struct devi
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		pci_fixup_device(pci_fixup_resume_early, to_pci_dev(dev));
 		error = pci_legacy_resume_early(dev);
+	} else {
+		pci_default_pm_resume_early(pci_dev);
 	}
 
 	return error;
@@ -604,6 +627,8 @@ static int pci_pm_poweroff(struct device
 		}
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend(dev, PMSG_HIBERNATE);
+	} else {
+		pci_default_pm_suspend_early(pci_dev);
 	}
 
 	pci_fixup_device(pci_fixup_suspend, pci_dev);

Re: [RFC][PATCH 2/10] PCI PM: Add suspend counterpart of pci_reenable_device

[Pavel Machek]

Hi!

> PCI devices without drivers are not disabled during suspend and
> hibernation, but they are enabled during resume, with the help of
> pci_reenable_device(), so there is an unbalanced execution of
> pcibios_enable_device() in the resume code path.
> 
> To correct this introduce function pci_disable_enabled_device()
> that will disable the argument device, if it is enabled when the
> function is being run, without updating the device's pci_dev
> structure and use it in the suspend code path to balance the
> pci_reenable_device() executed during resume.

> +/**
> + * pci_disable_enabled_device - Disable device without updating enable_cnt
> + * @dev: PCI device to disable
> + *
> + * NOTE: This function is a backend of PCI power management routines and is
> + * not supposed to be called drivers.

"by drivers"?

> @@ -441,7 +455,10 @@ static int pci_pm_suspend(struct device 
>  		}
>  	} else if (pci_has_legacy_pm_support(pci_dev)) {
>  		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
> +	} else {
> +		pci_default_pm_suspend_early(pci_dev);
>  	}
> +
>  	pci_fixup_device(pci_fixup_suspend, pci_dev);
>  
>  	return error;

So tre fixup runs on the disabled device?

Acked-by: Pavel Machek <Pavel@suse.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH 2/10] PCI PM: Add suspend counterpart of pci_reenable_device

[Rafael J. Wysocki]

On Monday 05 January 2009, Pavel Machek wrote:
> Hi!
> 
> > PCI devices without drivers are not disabled during suspend and
> > hibernation, but they are enabled during resume, with the help of
> > pci_reenable_device(), so there is an unbalanced execution of
> > pcibios_enable_device() in the resume code path.
> > 
> > To correct this introduce function pci_disable_enabled_device()
> > that will disable the argument device, if it is enabled when the
> > function is being run, without updating the device's pci_dev
> > structure and use it in the suspend code path to balance the
> > pci_reenable_device() executed during resume.
> 
> > +/**
> > + * pci_disable_enabled_device - Disable device without updating enable_cnt
> > + * @dev: PCI device to disable
> > + *
> > + * NOTE: This function is a backend of PCI power management routines and is
> > + * not supposed to be called drivers.
> 
> "by drivers"?

Yes, thanks.

> > @@ -441,7 +455,10 @@ static int pci_pm_suspend(struct device 
> >  		}
> >  	} else if (pci_has_legacy_pm_support(pci_dev)) {
> >  		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
> > +	} else {
> > +		pci_default_pm_suspend_early(pci_dev);
> >  	}
> > +
> >  	pci_fixup_device(pci_fixup_suspend, pci_dev);
> >  
> >  	return error;
> 
> So tre fixup runs on the disabled device?

Yes, but the device's config space is still available at this point.
 
> Acked-by: Pavel Machek <Pavel@suse.cz>

Thanks,
Rafael

[RFC][PATCH 3/10] PCI PM: Power-manage devices without drivers during suspend-resume

[Rafael J. Wysocki]

PCI devices without drivers can be put into low power states during
suspend with the help of pci_prepare_to_sleep() and prevented from
generating wake-up events during resume with the help of
pci_enable_wake().  However, it's better not to put bridges into
low power states during suspend, because that might result in entire
bus segments being powered off.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |   33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

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
@@ -421,6 +421,31 @@ static int pci_legacy_resume_early(struc
 	return error;
 }
 
+/* Auxiliary functions used by the new power management framework */
+
+static bool pci_is_bridge(struct pci_dev *pci_dev)
+{
+	return !!(pci_dev->subordinate);
+}
+
+static int pci_pm_default_resume(struct pci_dev *pci_dev)
+{
+	if (!pci_is_bridge(pci_dev))
+		pci_enable_wake(pci_dev, PCI_D0, false);
+
+	return pci_default_pm_resume_late(pci_dev);
+}
+
+static void pci_pm_default_suspend(struct pci_dev *pci_dev)
+{
+	pci_default_pm_suspend_early(pci_dev);
+
+	if (!pci_is_bridge(pci_dev))
+		pci_prepare_to_sleep(pci_dev);
+}
+
+/* New power management framework */
+
 static int pci_pm_prepare(struct device *dev)
 {
 	struct device_driver *drv = dev->driver;
@@ -456,7 +481,7 @@ static int pci_pm_suspend(struct device 
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
 	} else {
-		pci_default_pm_suspend_early(pci_dev);
+		pci_pm_default_suspend(pci_dev);
 	}
 
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
@@ -498,7 +523,7 @@ static int pci_pm_resume(struct device *
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_resume(dev);
 	} else {
-		error = pci_default_pm_resume_late(pci_dev);
+		error = pci_pm_default_resume(pci_dev);
 	}
 
 	return error;
@@ -628,7 +653,7 @@ static int pci_pm_poweroff(struct device
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend(dev, PMSG_HIBERNATE);
 	} else {
-		pci_default_pm_suspend_early(pci_dev);
+		pci_pm_default_suspend(pci_dev);
 	}
 
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
@@ -667,7 +692,7 @@ static int pci_pm_restore(struct device 
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_resume(dev);
 	} else {
-		error = pci_default_pm_resume_late(pci_dev);
+		error = pci_pm_default_resume(pci_dev);
 	}
 
 	return error;

Re: [RFC][PATCH 3/10] PCI PM: Power-manage devices without drivers during suspend-resume

[Pavel Machek]

> 
> PCI devices without drivers can be put into low power states during
> suspend with the help of pci_prepare_to_sleep() and prevented from
> generating wake-up events during resume with the help of
> pci_enable_wake().  However, it's better not to put bridges into
> low power states during suspend, because that might result in entire
> bus segments being powered off.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Acked-by: Pavel Machek <Pavel@suse.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC][PATCH 4/10] PCI PM: Move pci_has_legacy_pm_support

[Rafael J. Wysocki]

Move pci_has_legacy_pm_support() closer to the functions that
call it.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

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
@@ -300,14 +300,6 @@ static void pci_device_shutdown(struct d
 
 #ifdef CONFIG_PM_SLEEP
 
-static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)
-{
-	struct pci_driver *drv = pci_dev->driver;
-
-	return drv && (drv->suspend || drv->suspend_late || drv->resume
-		|| drv->resume_early);
-}
-
 /*
  * Default "suspend" method for devices that have no driver provided suspend,
  * or not even a driver at all (first part).
@@ -444,6 +436,14 @@ static void pci_pm_default_suspend(struc
 		pci_prepare_to_sleep(pci_dev);
 }
 
+static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)
+{
+	struct pci_driver *drv = pci_dev->driver;
+
+	return drv && (drv->suspend || drv->suspend_late || drv->resume
+		|| drv->resume_early);
+}
+
 /* New power management framework */
 
 static int pci_pm_prepare(struct device *dev)

Re: [RFC][PATCH 4/10] PCI PM: Move pci_has_legacy_pm_support

[Pavel Machek]

> 
> Move pci_has_legacy_pm_support() closer to the functions that
> call it.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Acked-by: Pavel Machek <pavel@suse.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC][PATCH 5/10] PCI PM: Avoid touching devices behind bridges in unknown state

[Rafael J. Wysocki]

It generally is better to avoid accessing devices behind bridges that
may not be in the D0 power state, because in that case the bridges'
secondary buses may not be accessible.  For this reason, during the
early phase of resume (ie. with interrupts disabled), before
restoring the standard config registers of a device, check the power
state of the bridge the device is behind and postpone the restoration
of the device's config space, as well as any other operations that
would involve accessing the device, if that state is not D0.

In such cases the restoration of the device's config space will be
retried during the "normal" phase of resume (ie. with interrupts
enabled), so that the bridge can be put into D0 before that happens.

Also, save standard configuration registers of PCI devices during the
"normal" phase of suspend (ie. with interrupts enabled), so that the
bridges the devices are behind can be put into low power states (we
don't put bridges into low power states at the moment, but we may
want to do it in the future and it seems reasonable to design for
that).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |  109 +++++++++++++++++++++++++++++++----------------
 drivers/pci/pci.c        |    2 
 drivers/pci/pci.h        |    1 
 3 files changed, 74 insertions(+), 38 deletions(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -526,7 +526,7 @@ pci_raw_set_power_state(struct pci_dev *
  * @dev: PCI device to handle.
  * @state: State to cache in case the device doesn't have the PM capability
  */
-static void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
+void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
 {
 	if (dev->pm_cap) {
 		u16 pmcsr;
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -40,6 +40,7 @@ struct pci_platform_pm_ops {
 };
 
 extern int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
+extern void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 extern void pci_disable_enabled_device(struct pci_dev *dev);
 extern void pci_pm_init(struct pci_dev *dev);
 extern void pci_allocate_cap_save_buffers(struct pci_dev *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
@@ -302,21 +302,10 @@ static void pci_device_shutdown(struct d
 
 /*
  * Default "suspend" method for devices that have no driver provided suspend,
- * or not even a driver at all (first part).
- */
-static void pci_default_pm_suspend_early(struct pci_dev *pci_dev)
-{
-	/* If device is enabled at this point, disable it */
-	pci_disable_enabled_device(pci_dev);
-}
-
-/*
- * Default "suspend" method for devices that have no driver provided suspend,
  * or not even a driver at all (second part).
  */
 static void pci_default_pm_suspend_late(struct pci_dev *pci_dev)
 {
-	pci_save_state(pci_dev);
 	/*
 	 * mark its power state as "unknown", since we don't know if
 	 * e.g. the BIOS will change its device state when we suspend.
@@ -327,16 +316,6 @@ static void pci_default_pm_suspend_late(
 
 /*
  * Default "resume" method for devices that have no driver provided resume,
- * or not even a driver at all (first part).
- */
-static void pci_default_pm_resume_early(struct pci_dev *pci_dev)
-{
-	/* restore the PCI config space */
-	pci_restore_state(pci_dev);
-}
-
-/*
- * Default "resume" method for devices that have no driver provided resume,
  * or not even a driver at all (second part).
  */
 static int pci_default_pm_resume_late(struct pci_dev *pci_dev)
@@ -365,9 +344,10 @@ static int pci_legacy_suspend(struct dev
 		i = drv->suspend(pci_dev, state);
 		suspend_report_result(drv->suspend, i);
 	} else {
+		pci_save_state(pci_dev);
 		/*
-		 * For compatibility with existing code with legacy PM support
-		 * don't call pci_default_pm_suspend_early() here.
+		 * This is for compatibility with existing code with legacy PM
+		 * support.
 		 */
 		pci_default_pm_suspend_late(pci_dev);
 	}
@@ -396,7 +376,8 @@ static int pci_legacy_resume(struct devi
 	if (drv && drv->resume) {
 		error = drv->resume(pci_dev);
 	} else {
-		pci_default_pm_resume_early(pci_dev);
+		/* restore the PCI config space */
+		pci_restore_state(pci_dev);
 		error = pci_default_pm_resume_late(pci_dev);
 	}
 	return error;
@@ -415,22 +396,72 @@ static int pci_legacy_resume_early(struc
 
 /* Auxiliary functions used by the new power management framework */
 
+static int pci_restore_standard_config(struct pci_dev *pci_dev)
+{
+	struct pci_dev *parent = pci_dev->bus->self;
+	int error = 0;
+
+	/* Check if the device's bus is operational */
+	if (!parent || parent->current_state == PCI_D0) {
+		pci_restore_state(pci_dev);
+		pci_update_current_state(pci_dev, PCI_D0);
+	} else {
+		dev_warn(&pci_dev->dev, "unable to restore config, "
+			"bridge %s in low power state D%d\n", pci_name(parent),
+			parent->current_state);
+		pci_dev->current_state = PCI_UNKNOWN;
+		error = -EAGAIN;
+	}
+
+	return error;
+}
+
 static bool pci_is_bridge(struct pci_dev *pci_dev)
 {
 	return !!(pci_dev->subordinate);
 }
 
+static void pci_pm_default_resume_noirq(struct pci_dev *pci_dev)
+{
+	if (pci_restore_standard_config(pci_dev))
+		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+}
+
 static int pci_pm_default_resume(struct pci_dev *pci_dev)
 {
+	int error;
+
+	/*
+	 * pci_restore_standard_config() should have been called once already,
+	 * but it would have failed if the device's parent bridge had not been
+	 * in power state D0 at that time.  Check it and try again if necessary.
+	 */
+	error = pci_restore_standard_config(pci_dev);
+	if (error)
+		return error;
+
+	pci_fixup_device(pci_fixup_resume, pci_dev);
+
 	if (!pci_is_bridge(pci_dev))
 		pci_enable_wake(pci_dev, PCI_D0, false);
 
 	return pci_default_pm_resume_late(pci_dev);
 }
 
+static void pci_pm_default_suspend_generic(struct pci_dev *pci_dev)
+{
+	/* If device is enabled at this point, disable it */
+	pci_disable_enabled_device(pci_dev);
+	/*
+	 * Save state with interrupts enabled, because in principle the bus the
+	 * device is on may be put into a low power state after this code runs.
+	 */
+	pci_save_state(pci_dev);
+}
+
 static void pci_pm_default_suspend(struct pci_dev *pci_dev)
 {
-	pci_default_pm_suspend_early(pci_dev);
+	pci_pm_default_suspend_generic(pci_dev);
 
 	if (!pci_is_bridge(pci_dev))
 		pci_prepare_to_sleep(pci_dev);
@@ -515,12 +546,13 @@ static int pci_pm_resume(struct device *
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	pci_fixup_device(pci_fixup_resume, pci_dev);
-
 	if (drv && drv->pm) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
+
 		if (drv->pm->resume)
 			error = drv->pm->resume(dev);
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
 		error = pci_legacy_resume(dev);
 	} else {
 		error = pci_pm_default_resume(pci_dev);
@@ -535,15 +567,16 @@ static int pci_pm_resume_noirq(struct de
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	pci_fixup_device(pci_fixup_resume_early, to_pci_dev(dev));
-
 	if (drv && drv->pm) {
+		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+
 		if (drv->pm->resume_noirq)
 			error = drv->pm->resume_noirq(dev);
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume_early, pci_dev);
 		error = pci_legacy_resume_early(dev);
 	} else {
-		pci_default_pm_resume_early(pci_dev);
+		pci_pm_default_resume_noirq(pci_dev);
 	}
 
 	return error;
@@ -575,7 +608,7 @@ static int pci_pm_freeze(struct device *
 		error = pci_legacy_suspend(dev, PMSG_FREEZE);
 		pci_fixup_device(pci_fixup_suspend, pci_dev);
 	} else {
-		pci_default_pm_suspend_early(pci_dev);
+		pci_pm_default_suspend_generic(pci_dev);
 	}
 
 	return error;
@@ -633,7 +666,7 @@ static int pci_pm_thaw_noirq(struct devi
 		pci_fixup_device(pci_fixup_resume_early, to_pci_dev(dev));
 		error = pci_legacy_resume_early(dev);
 	} else {
-		pci_default_pm_resume_early(pci_dev);
+		pci_update_current_state(pci_dev, PCI_D0);
 	}
 
 	return error;
@@ -684,12 +717,13 @@ static int pci_pm_restore(struct device 
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	pci_fixup_device(pci_fixup_resume, pci_dev);
-
 	if (drv && drv->pm) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
+
 		if (drv->pm->restore)
 			error = drv->pm->restore(dev);
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
 		error = pci_legacy_resume(dev);
 	} else {
 		error = pci_pm_default_resume(pci_dev);
@@ -704,15 +738,16 @@ static int pci_pm_restore_noirq(struct d
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	pci_fixup_device(pci_fixup_resume_early, pci_dev);
-
 	if (drv && drv->pm) {
+		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+
 		if (drv->pm->restore_noirq)
 			error = drv->pm->restore_noirq(dev);
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume_early, pci_dev);
 		error = pci_legacy_resume_early(dev);
 	} else {
-		pci_default_pm_resume_early(pci_dev);
+		pci_pm_default_resume_noirq(pci_dev);
 	}
 
 	return error;

[RFC][PATCH 5/10] PCI PM: Avoid touching devices behind bridges in unknown state (rev. 2)

[Rafael J. Wysocki]

On Tuesday 30 December 2008, Rafael J. Wysocki wrote:
> 
> It generally is better to avoid accessing devices behind bridges that
> may not be in the D0 power state, because in that case the bridges'
> secondary buses may not be accessible.  For this reason, during the
> early phase of resume (ie. with interrupts disabled), before
> restoring the standard config registers of a device, check the power
> state of the bridge the device is behind and postpone the restoration
> of the device's config space, as well as any other operations that
> would involve accessing the device, if that state is not D0.
> 
> In such cases the restoration of the device's config space will be
> retried during the "normal" phase of resume (ie. with interrupts
> enabled), so that the bridge can be put into D0 before that happens.
> 
> Also, save standard configuration registers of PCI devices during the
> "normal" phase of suspend (ie. with interrupts enabled), so that the
> bridges the devices are behind can be put into low power states (we
> don't put bridges into low power states at the moment, but we may
> want to do it in the future and it seems reasonable to design for
> that).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

I found a small mistake in this patch.  Namely, the code in
pci_pm_default_resume doesn't follow the comment and the config spaces of
devices without drivers are restored twice in a row during resume (once with
interrupts enabled and once with interrupts disabled.  That shouldn't hurt, but
is not correct nevertheless.

Corrected patch follows.

Thanks,
Rafael

---
Subject: PCI PM: Avoid touching devices behind bridges in unknown state (rev. 2)
From: Rafael J. Wysocki <rjw@sisk.pl>

It generally is better to avoid accessing devices behind bridges that
may not be in the D0 power state, because in that case the bridges'
secondary buses may not be accessible.  For this reason, during the
early phase of resume (ie. with interrupts disabled), before
restoring the standard config registers of a device, check the power
state of the bridge the device is behind and postpone the restoration
of the device's config space, as well as any other operations that
would involve accessing the device, if that state is not D0.

In such cases the restoration of the device's config space will be
retried during the "normal" phase of resume (ie. with interrupts
enabled), so that the bridge can be put into D0 before that happens.

Also, save standard configuration registers of PCI devices during the
"normal" phase of suspend (ie. with interrupts enabled), so that the
bridges the devices are behind can be put into low power states (we
don't put bridges into low power states at the moment, but we may
want to do it in the future and it seems reasonable to design for
that).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |  109 +++++++++++++++++++++++++++++++----------------
 drivers/pci/pci.c        |    2 
 drivers/pci/pci.h        |    1 
 3 files changed, 74 insertions(+), 38 deletions(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -526,7 +526,7 @@ pci_raw_set_power_state(struct pci_dev *
  * @dev: PCI device to handle.
  * @state: State to cache in case the device doesn't have the PM capability
  */
-static void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
+void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
 {
 	if (dev->pm_cap) {
 		u16 pmcsr;
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -40,6 +40,7 @@ struct pci_platform_pm_ops {
 };
 
 extern int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
+extern void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 extern void pci_disable_enabled_device(struct pci_dev *dev);
 extern void pci_pm_init(struct pci_dev *dev);
 extern void pci_allocate_cap_save_buffers(struct pci_dev *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
@@ -302,21 +302,10 @@ static void pci_device_shutdown(struct d
 
 /*
  * Default "suspend" method for devices that have no driver provided suspend,
- * or not even a driver at all (first part).
- */
-static void pci_default_pm_suspend_early(struct pci_dev *pci_dev)
-{
-	/* If device is enabled at this point, disable it */
-	pci_disable_enabled_device(pci_dev);
-}
-
-/*
- * Default "suspend" method for devices that have no driver provided suspend,
  * or not even a driver at all (second part).
  */
 static void pci_default_pm_suspend_late(struct pci_dev *pci_dev)
 {
-	pci_save_state(pci_dev);
 	/*
 	 * mark its power state as "unknown", since we don't know if
 	 * e.g. the BIOS will change its device state when we suspend.
@@ -327,16 +316,6 @@ static void pci_default_pm_suspend_late(
 
 /*
  * Default "resume" method for devices that have no driver provided resume,
- * or not even a driver at all (first part).
- */
-static void pci_default_pm_resume_early(struct pci_dev *pci_dev)
-{
-	/* restore the PCI config space */
-	pci_restore_state(pci_dev);
-}
-
-/*
- * Default "resume" method for devices that have no driver provided resume,
  * or not even a driver at all (second part).
  */
 static int pci_default_pm_resume_late(struct pci_dev *pci_dev)
@@ -365,9 +344,10 @@ static int pci_legacy_suspend(struct dev
 		i = drv->suspend(pci_dev, state);
 		suspend_report_result(drv->suspend, i);
 	} else {
+		pci_save_state(pci_dev);
 		/*
-		 * For compatibility with existing code with legacy PM support
-		 * don't call pci_default_pm_suspend_early() here.
+		 * This is for compatibility with existing code with legacy PM
+		 * support.
 		 */
 		pci_default_pm_suspend_late(pci_dev);
 	}
@@ -396,7 +376,8 @@ static int pci_legacy_resume(struct devi
 	if (drv && drv->resume) {
 		error = drv->resume(pci_dev);
 	} else {
-		pci_default_pm_resume_early(pci_dev);
+		/* restore the PCI config space */
+		pci_restore_state(pci_dev);
 		error = pci_default_pm_resume_late(pci_dev);
 	}
 	return error;
@@ -415,22 +396,72 @@ static int pci_legacy_resume_early(struc
 
 /* Auxiliary functions used by the new power management framework */
 
+static int pci_restore_standard_config(struct pci_dev *pci_dev)
+{
+	struct pci_dev *parent = pci_dev->bus->self;
+	int error = 0;
+
+	/* Check if the device's bus is operational */
+	if (!parent || parent->current_state == PCI_D0) {
+		pci_restore_state(pci_dev);
+		pci_update_current_state(pci_dev, PCI_D0);
+	} else {
+		dev_warn(&pci_dev->dev, "unable to restore config, "
+			"bridge %s in low power state D%d\n", pci_name(parent),
+			parent->current_state);
+		pci_dev->current_state = PCI_UNKNOWN;
+		error = -EAGAIN;
+	}
+
+	return error;
+}
+
 static bool pci_is_bridge(struct pci_dev *pci_dev)
 {
 	return !!(pci_dev->subordinate);
 }
 
+static void pci_pm_default_resume_noirq(struct pci_dev *pci_dev)
+{
+	if (pci_restore_standard_config(pci_dev))
+		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+}
+
 static int pci_pm_default_resume(struct pci_dev *pci_dev)
 {
+	/*
+	 * pci_restore_standard_config() should have been called once already,
+	 * but it would have failed if the device's parent bridge had not been
+	 * in power state D0 at that time.  Check it and try again if necessary.
+	 */
+	if (pci_dev->current_state == PCI_UNKNOWN) {
+		int error = pci_restore_standard_config(pci_dev);
+		if (error)
+			return error;
+	}
+
+	pci_fixup_device(pci_fixup_resume, pci_dev);
+
 	if (!pci_is_bridge(pci_dev))
 		pci_enable_wake(pci_dev, PCI_D0, false);
 
 	return pci_default_pm_resume_late(pci_dev);
 }
 
+static void pci_pm_default_suspend_generic(struct pci_dev *pci_dev)
+{
+	/* If device is enabled at this point, disable it */
+	pci_disable_enabled_device(pci_dev);
+	/*
+	 * Save state with interrupts enabled, because in principle the bus the
+	 * device is on may be put into a low power state after this code runs.
+	 */
+	pci_save_state(pci_dev);
+}
+
 static void pci_pm_default_suspend(struct pci_dev *pci_dev)
 {
-	pci_default_pm_suspend_early(pci_dev);
+	pci_pm_default_suspend_generic(pci_dev);
 
 	if (!pci_is_bridge(pci_dev))
 		pci_prepare_to_sleep(pci_dev);
@@ -515,12 +546,13 @@ static int pci_pm_resume(struct device *
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	pci_fixup_device(pci_fixup_resume, pci_dev);
-
 	if (drv && drv->pm) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
+
 		if (drv->pm->resume)
 			error = drv->pm->resume(dev);
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
 		error = pci_legacy_resume(dev);
 	} else {
 		error = pci_pm_default_resume(pci_dev);
@@ -535,15 +567,16 @@ static int pci_pm_resume_noirq(struct de
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	pci_fixup_device(pci_fixup_resume_early, to_pci_dev(dev));
-
 	if (drv && drv->pm) {
+		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+
 		if (drv->pm->resume_noirq)
 			error = drv->pm->resume_noirq(dev);
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume_early, pci_dev);
 		error = pci_legacy_resume_early(dev);
 	} else {
-		pci_default_pm_resume_early(pci_dev);
+		pci_pm_default_resume_noirq(pci_dev);
 	}
 
 	return error;
@@ -575,7 +608,7 @@ static int pci_pm_freeze(struct device *
 		error = pci_legacy_suspend(dev, PMSG_FREEZE);
 		pci_fixup_device(pci_fixup_suspend, pci_dev);
 	} else {
-		pci_default_pm_suspend_early(pci_dev);
+		pci_pm_default_suspend_generic(pci_dev);
 	}
 
 	return error;
@@ -633,7 +666,7 @@ static int pci_pm_thaw_noirq(struct devi
 		pci_fixup_device(pci_fixup_resume_early, to_pci_dev(dev));
 		error = pci_legacy_resume_early(dev);
 	} else {
-		pci_default_pm_resume_early(pci_dev);
+		pci_update_current_state(pci_dev, PCI_D0);
 	}
 
 	return error;
@@ -684,12 +717,13 @@ static int pci_pm_restore(struct device 
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	pci_fixup_device(pci_fixup_resume, pci_dev);
-
 	if (drv && drv->pm) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
+
 		if (drv->pm->restore)
 			error = drv->pm->restore(dev);
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
 		error = pci_legacy_resume(dev);
 	} else {
 		error = pci_pm_default_resume(pci_dev);
@@ -704,15 +738,16 @@ static int pci_pm_restore_noirq(struct d
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	pci_fixup_device(pci_fixup_resume_early, pci_dev);
-
 	if (drv && drv->pm) {
+		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+
 		if (drv->pm->restore_noirq)
 			error = drv->pm->restore_noirq(dev);
 	} else if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume_early, pci_dev);
 		error = pci_legacy_resume_early(dev);
 	} else {
-		pci_default_pm_resume_early(pci_dev);
+		pci_pm_default_resume_noirq(pci_dev);
 	}
 
 	return error;

Re: [RFC][PATCH 5/10] PCI PM: Avoid touching devices behind bridges in unknown state (rev. 2)

[Pavel Machek]

Hi!

> Subject: PCI PM: Avoid touching devices behind bridges in unknown state (rev. 2)
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> It generally is better to avoid accessing devices behind bridges that
> may not be in the D0 power state, because in that case the bridges'
> secondary buses may not be accessible.  For this reason, during the
> early phase of resume (ie. with interrupts disabled), before
> restoring the standard config registers of a device, check the power
> state of the bridge the device is behind and postpone the restoration
> of the device's config space, as well as any other operations that
> would involve accessing the device, if that state is not D0.

I'm not sure if this is good idea.

Either pci config space needs to be restored early, or it can wait.

Sometimes restoring it early and sometimes restoring it late seems
harmful: it will make code harder to understand and harder to test.

> In such cases the restoration of the device's config space will be
> retried during the "normal" phase of resume (ie. with interrupts
> enabled), so that the bridge can be put into D0 before that happens.

If drivers have to assume it is restored late, anyway... why not do it
late, always?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH 5/10] PCI PM: Avoid touching devices behind bridges in unknown state (rev. 2)

[Rafael J. Wysocki]

On Monday 05 January 2009, Pavel Machek wrote:
> Hi!
> 
> > Subject: PCI PM: Avoid touching devices behind bridges in unknown state (rev. 2)
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > It generally is better to avoid accessing devices behind bridges that
> > may not be in the D0 power state, because in that case the bridges'
> > secondary buses may not be accessible.  For this reason, during the
> > early phase of resume (ie. with interrupts disabled), before
> > restoring the standard config registers of a device, check the power
> > state of the bridge the device is behind and postpone the restoration
> > of the device's config space, as well as any other operations that
> > would involve accessing the device, if that state is not D0.
> 
> I'm not sure if this is good idea.
> 
> Either pci config space needs to be restored early, or it can wait.
> 
> Sometimes restoring it early and sometimes restoring it late seems
> harmful: it will make code harder to understand and harder to test.

Unfortunately, we need to restore it early at least for some devices (bridges
and PCI Express ports) and I don't think it is generally safe to go and restore
it early for every device (as explained in this changelog).

> > In such cases the restoration of the device's config space will be
> > retried during the "normal" phase of resume (ie. with interrupts
> > enabled), so that the bridge can be put into D0 before that happens.
> 
> If drivers have to assume it is restored late, anyway... why not do it
> late, always?

Because it breaks.

Thanks,
Rafael

Re: [RFC][PATCH 5/10] PCI PM: Avoid touching devices behind bridges in unknown state (rev. 2)

[Pavel Machek]

On Mon 2009-01-05 14:30:54, Rafael J. Wysocki wrote:
> On Monday 05 January 2009, Pavel Machek wrote:
> > Hi!
> > 
> > > Subject: PCI PM: Avoid touching devices behind bridges in unknown state (rev. 2)
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > It generally is better to avoid accessing devices behind bridges that
> > > may not be in the D0 power state, because in that case the bridges'
> > > secondary buses may not be accessible.  For this reason, during the
> > > early phase of resume (ie. with interrupts disabled), before
> > > restoring the standard config registers of a device, check the power
> > > state of the bridge the device is behind and postpone the restoration
> > > of the device's config space, as well as any other operations that
> > > would involve accessing the device, if that state is not D0.
> > 
> > I'm not sure if this is good idea.
> > 
> > Either pci config space needs to be restored early, or it can wait.
> > 
> > Sometimes restoring it early and sometimes restoring it late seems
> > harmful: it will make code harder to understand and harder to test.
> 
> Unfortunately, we need to restore it early at least for some devices (bridges
> and PCI Express ports) and I don't think it is generally safe to go and restore
> it early for every device (as explained in this changelog).

Could we make it so that it is restored early exactly on bridges and
PCIe ports?

Idea that restore of one specific device is called early or late
depending if it is connected directly or behind bridge scares me...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH 5/10] PCI PM: Avoid touching devices behind bridges in unknown state (rev. 2)

[Rafael J. Wysocki]

On Wednesday 07 January 2009, Pavel Machek wrote:
> On Mon 2009-01-05 14:30:54, Rafael J. Wysocki wrote:
> > On Monday 05 January 2009, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > Subject: PCI PM: Avoid touching devices behind bridges in unknown state (rev. 2)
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > It generally is better to avoid accessing devices behind bridges that
> > > > may not be in the D0 power state, because in that case the bridges'
> > > > secondary buses may not be accessible.  For this reason, during the
> > > > early phase of resume (ie. with interrupts disabled), before
> > > > restoring the standard config registers of a device, check the power
> > > > state of the bridge the device is behind and postpone the restoration
> > > > of the device's config space, as well as any other operations that
> > > > would involve accessing the device, if that state is not D0.
> > > 
> > > I'm not sure if this is good idea.
> > > 
> > > Either pci config space needs to be restored early, or it can wait.
> > > 
> > > Sometimes restoring it early and sometimes restoring it late seems
> > > harmful: it will make code harder to understand and harder to test.
> > 
> > Unfortunately, we need to restore it early at least for some devices (bridges
> > and PCI Express ports) and I don't think it is generally safe to go and restore
> > it early for every device (as explained in this changelog).
> 
> Could we make it so that it is restored early exactly on bridges and
> PCIe ports?
> 
> Idea that restore of one specific device is called early or late
> depending if it is connected directly or behind bridge scares me...

There's nothing scary in it IMO and in fact the device's config space is
always restored as soon as reasonably possible.

It will be restored early even if it is behind a bridge, unless that bridge
itself is in a low power state, in which case clearly we _can't_ restore it
early.

Thanks,
Rafael

[RFC][PATCH 6/10] PCI PM: Rearrange code in pci-driver.c

[Rafael J. Wysocki]

Rename two functions and rearrange code in drivers/pci/pci-driver.c
so that it's easier to follow.  In particular, separate invocations
of the legacy callbacks from the rest of the new callbacks' code.

No functional changes should result from this.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |  104 +++++++++++++++++++++++++++++------------------
 1 file changed, 65 insertions(+), 39 deletions(-)

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
@@ -304,7 +304,7 @@ static void pci_device_shutdown(struct d
  * Default "suspend" method for devices that have no driver provided suspend,
  * or not even a driver at all (second part).
  */
-static void pci_default_pm_suspend_late(struct pci_dev *pci_dev)
+static void pci_pm_set_unknown_state(struct pci_dev *pci_dev)
 {
 	/*
 	 * mark its power state as "unknown", since we don't know if
@@ -318,7 +318,7 @@ static void pci_default_pm_suspend_late(
  * Default "resume" method for devices that have no driver provided resume,
  * or not even a driver at all (second part).
  */
-static int pci_default_pm_resume_late(struct pci_dev *pci_dev)
+static int pci_pm_reenable_device(struct pci_dev *pci_dev)
 {
 	int retval;
 
@@ -349,7 +349,7 @@ static int pci_legacy_suspend(struct dev
 		 * This is for compatibility with existing code with legacy PM
 		 * support.
 		 */
-		pci_default_pm_suspend_late(pci_dev);
+		pci_pm_set_unknown_state(pci_dev);
 	}
 	return i;
 }
@@ -378,7 +378,7 @@ static int pci_legacy_resume(struct devi
 	} else {
 		/* restore the PCI config space */
 		pci_restore_state(pci_dev);
-		error = pci_default_pm_resume_late(pci_dev);
+		error = pci_pm_reenable_device(pci_dev);
 	}
 	return error;
 }
@@ -445,7 +445,7 @@ static int pci_pm_default_resume(struct 
 	if (!pci_is_bridge(pci_dev))
 		pci_enable_wake(pci_dev, PCI_D0, false);
 
-	return pci_default_pm_resume_late(pci_dev);
+	return pci_pm_reenable_device(pci_dev);
 }
 
 static void pci_pm_default_suspend_generic(struct pci_dev *pci_dev)
@@ -504,17 +504,21 @@ static int pci_pm_suspend(struct device 
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pci_has_legacy_pm_support(pci_dev)) {
+		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
+		goto Exit;
+	}
+
 	if (drv && drv->pm) {
 		if (drv->pm->suspend) {
 			error = drv->pm->suspend(dev);
 			suspend_report_result(drv->pm->suspend, error);
 		}
-	} else if (pci_has_legacy_pm_support(pci_dev)) {
-		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
 	} else {
 		pci_pm_default_suspend(pci_dev);
 	}
 
+ Exit:
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
 
 	return error;
@@ -526,15 +530,16 @@ static int pci_pm_suspend_noirq(struct d
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pci_has_legacy_pm_support(pci_dev))
+		return pci_legacy_suspend_late(dev, PMSG_SUSPEND);
+
 	if (drv && drv->pm) {
 		if (drv->pm->suspend_noirq) {
 			error = drv->pm->suspend_noirq(dev);
 			suspend_report_result(drv->pm->suspend_noirq, error);
 		}
-	} else if (pci_has_legacy_pm_support(pci_dev)) {
-		error = pci_legacy_suspend_late(dev, PMSG_SUSPEND);
 	} else {
-		pci_default_pm_suspend_late(pci_dev);
+		pci_pm_set_unknown_state(pci_dev);
 	}
 
 	return error;
@@ -546,14 +551,16 @@ static int pci_pm_resume(struct device *
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
+		return pci_legacy_resume(dev);
+	}
+
 	if (drv && drv->pm) {
 		pci_fixup_device(pci_fixup_resume, pci_dev);
 
 		if (drv->pm->resume)
 			error = drv->pm->resume(dev);
-	} else if (pci_has_legacy_pm_support(pci_dev)) {
-		pci_fixup_device(pci_fixup_resume, pci_dev);
-		error = pci_legacy_resume(dev);
 	} else {
 		error = pci_pm_default_resume(pci_dev);
 	}
@@ -567,14 +574,16 @@ static int pci_pm_resume_noirq(struct de
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+		return pci_legacy_resume_early(dev);
+	}
+
 	if (drv && drv->pm) {
 		pci_fixup_device(pci_fixup_resume_early, pci_dev);
 
 		if (drv->pm->resume_noirq)
 			error = drv->pm->resume_noirq(dev);
-	} else if (pci_has_legacy_pm_support(pci_dev)) {
-		pci_fixup_device(pci_fixup_resume_early, pci_dev);
-		error = pci_legacy_resume_early(dev);
 	} else {
 		pci_pm_default_resume_noirq(pci_dev);
 	}
@@ -599,14 +608,17 @@ static int pci_pm_freeze(struct device *
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pci_has_legacy_pm_support(pci_dev)) {
+		error = pci_legacy_suspend(dev, PMSG_FREEZE);
+		pci_fixup_device(pci_fixup_suspend, pci_dev);
+		return error;
+	}
+
 	if (drv && drv->pm) {
 		if (drv->pm->freeze) {
 			error = drv->pm->freeze(dev);
 			suspend_report_result(drv->pm->freeze, error);
 		}
-	} else if (pci_has_legacy_pm_support(pci_dev)) {
-		error = pci_legacy_suspend(dev, PMSG_FREEZE);
-		pci_fixup_device(pci_fixup_suspend, pci_dev);
 	} else {
 		pci_pm_default_suspend_generic(pci_dev);
 	}
@@ -620,15 +632,16 @@ static int pci_pm_freeze_noirq(struct de
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pci_has_legacy_pm_support(pci_dev))
+		return pci_legacy_suspend_late(dev, PMSG_FREEZE);
+
 	if (drv && drv->pm) {
 		if (drv->pm->freeze_noirq) {
 			error = drv->pm->freeze_noirq(dev);
 			suspend_report_result(drv->pm->freeze_noirq, error);
 		}
-	} else if (pci_has_legacy_pm_support(pci_dev)) {
-		error = pci_legacy_suspend_late(dev, PMSG_FREEZE);
 	} else {
-		pci_default_pm_suspend_late(pci_dev);
+		pci_pm_set_unknown_state(pci_dev);
 	}
 
 	return error;
@@ -640,14 +653,16 @@ static int pci_pm_thaw(struct device *de
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
+		return pci_legacy_resume(dev);
+	}
+
 	if (drv && drv->pm) {
 		if (drv->pm->thaw)
 			error =  drv->pm->thaw(dev);
-	} else if (pci_has_legacy_pm_support(pci_dev)) {
-		pci_fixup_device(pci_fixup_resume, pci_dev);
-		error = pci_legacy_resume(dev);
 	} else {
-		pci_default_pm_resume_late(pci_dev);
+		pci_pm_reenable_device(pci_dev);
 	}
 
 	return error;
@@ -659,12 +674,14 @@ static int pci_pm_thaw_noirq(struct devi
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume_early, to_pci_dev(dev));
+		return pci_legacy_resume_early(dev);
+	}
+
 	if (drv && drv->pm) {
 		if (drv->pm->thaw_noirq)
 			error = drv->pm->thaw_noirq(dev);
-	} else if (pci_has_legacy_pm_support(pci_dev)) {
-		pci_fixup_device(pci_fixup_resume_early, to_pci_dev(dev));
-		error = pci_legacy_resume_early(dev);
 	} else {
 		pci_update_current_state(pci_dev, PCI_D0);
 	}
@@ -678,17 +695,21 @@ static int pci_pm_poweroff(struct device
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pci_has_legacy_pm_support(pci_dev)) {
+		error = pci_legacy_suspend(dev, PMSG_HIBERNATE);
+		goto Exit;
+	}
+
 	if (drv && drv->pm) {
 		if (drv->pm->poweroff) {
 			error = drv->pm->poweroff(dev);
 			suspend_report_result(drv->pm->poweroff, error);
 		}
-	} else if (pci_has_legacy_pm_support(pci_dev)) {
-		error = pci_legacy_suspend(dev, PMSG_HIBERNATE);
 	} else {
 		pci_pm_default_suspend(pci_dev);
 	}
 
+ Exit:
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
 
 	return error;
@@ -699,13 +720,14 @@ static int pci_pm_poweroff_noirq(struct 
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pci_has_legacy_pm_support(to_pci_dev(dev)))
+		return pci_legacy_suspend_late(dev, PMSG_HIBERNATE);
+
 	if (drv && drv->pm) {
 		if (drv->pm->poweroff_noirq) {
 			error = drv->pm->poweroff_noirq(dev);
 			suspend_report_result(drv->pm->poweroff_noirq, error);
 		}
-	} else if (pci_has_legacy_pm_support(to_pci_dev(dev))) {
-		error = pci_legacy_suspend_late(dev, PMSG_HIBERNATE);
 	}
 
 	return error;
@@ -717,14 +739,16 @@ static int pci_pm_restore(struct device 
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
+		return pci_legacy_resume(dev);
+	}
+
 	if (drv && drv->pm) {
 		pci_fixup_device(pci_fixup_resume, pci_dev);
 
 		if (drv->pm->restore)
 			error = drv->pm->restore(dev);
-	} else if (pci_has_legacy_pm_support(pci_dev)) {
-		pci_fixup_device(pci_fixup_resume, pci_dev);
-		error = pci_legacy_resume(dev);
 	} else {
 		error = pci_pm_default_resume(pci_dev);
 	}
@@ -738,14 +762,16 @@ static int pci_pm_restore_noirq(struct d
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+		return pci_legacy_resume_early(dev);
+	}
+
 	if (drv && drv->pm) {
 		pci_fixup_device(pci_fixup_resume_early, pci_dev);
 
 		if (drv->pm->restore_noirq)
 			error = drv->pm->restore_noirq(dev);
-	} else if (pci_has_legacy_pm_support(pci_dev)) {
-		pci_fixup_device(pci_fixup_resume_early, pci_dev);
-		error = pci_legacy_resume_early(dev);
 	} else {
 		pci_pm_default_resume_noirq(pci_dev);
 	}

Re: [RFC][PATCH 6/10] PCI PM: Rearrange code in pci-driver.c

[Pavel Machek]

Hi!

> Rename two functions and rearrange code in drivers/pci/pci-driver.c
> so that it's easier to follow.  In particular, separate invocations
> of the legacy callbacks from the rest of the new callbacks' code.
> 
> No functional changes should result from this.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/pci/pci-driver.c |  104 +++++++++++++++++++++++++++++------------------
>  1 file changed, 65 insertions(+), 39 deletions(-)
>

> @@ -504,17 +504,21 @@ static int pci_pm_suspend(struct device 
>  	struct device_driver *drv = dev->driver;
>  	int error = 0;
>  
> +	if (pci_has_legacy_pm_support(pci_dev)) {
> +		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
> +		goto Exit;
> +	}
> +
>  	if (drv && drv->pm) {
>  		if (drv->pm->suspend) {
>  			error = drv->pm->suspend(dev);
>  			suspend_report_result(drv->pm->suspend, error);
>  		}
> -	} else if (pci_has_legacy_pm_support(pci_dev)) {
> -		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
>  	} else {
>  		pci_pm_default_suspend(pci_dev);
>  	}
>

Does this mean that pci_has_legacy_pm_support() => !(drv && drv->pm) ?
Should pci_has_legacy_pm_support() check for that and WARN() in case
both sets of callbacks are set?
									Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH 6/10] PCI PM: Rearrange code in pci-driver.c

[Rafael J. Wysocki]

On Monday 05 January 2009, Pavel Machek wrote:
> Hi!
> 
> > Rename two functions and rearrange code in drivers/pci/pci-driver.c
> > so that it's easier to follow.  In particular, separate invocations
> > of the legacy callbacks from the rest of the new callbacks' code.
> > 
> > No functional changes should result from this.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/pci/pci-driver.c |  104 +++++++++++++++++++++++++++++------------------
> >  1 file changed, 65 insertions(+), 39 deletions(-)
> >
> 
> > @@ -504,17 +504,21 @@ static int pci_pm_suspend(struct device 
> >  	struct device_driver *drv = dev->driver;
> >  	int error = 0;
> >  
> > +	if (pci_has_legacy_pm_support(pci_dev)) {
> > +		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
> > +		goto Exit;
> > +	}
> > +
> >  	if (drv && drv->pm) {
> >  		if (drv->pm->suspend) {
> >  			error = drv->pm->suspend(dev);
> >  			suspend_report_result(drv->pm->suspend, error);
> >  		}
> > -	} else if (pci_has_legacy_pm_support(pci_dev)) {
> > -		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
> >  	} else {
> >  		pci_pm_default_suspend(pci_dev);
> >  	}
> >
> 
> Does this mean that pci_has_legacy_pm_support() => !(drv && drv->pm) ?

Yes, it does.

> Should pci_has_legacy_pm_support() check for that and WARN() in case
> both sets of callbacks are set?

Yes, I think I can add a WARN_ON() in there.

Thanks,
Rafael

[RFC][PATCH 7/10] PCI PM: Call pci_fixup_device from legacy routines

[Rafael J. Wysocki]

The size of drivers/pci/pci-driver.c can be reduced quite a bit
if pci_fixup_device() is called from the legacy PM callbacks, so make
it happen.

No functional changes should result from this.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |   52 +++++++++++++++++------------------------------
 1 file changed, 19 insertions(+), 33 deletions(-)

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
@@ -351,6 +351,9 @@ static int pci_legacy_suspend(struct dev
 		 */
 		pci_pm_set_unknown_state(pci_dev);
 	}
+
+	pci_fixup_device(pci_fixup_suspend, pci_dev);
+
 	return i;
 }
 
@@ -373,6 +376,8 @@ static int pci_legacy_resume(struct devi
 	struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * drv = pci_dev->driver;
 
+	pci_fixup_device(pci_fixup_resume, pci_dev);
+
 	if (drv && drv->resume) {
 		error = drv->resume(pci_dev);
 	} else {
@@ -389,6 +394,8 @@ static int pci_legacy_resume_early(struc
 	struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * drv = pci_dev->driver;
 
+	pci_fixup_device(pci_fixup_resume_early, pci_dev);
+
 	if (drv && drv->resume_early)
 		error = drv->resume_early(pci_dev);
 	return error;
@@ -504,10 +511,8 @@ static int pci_pm_suspend(struct device 
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	if (pci_has_legacy_pm_support(pci_dev)) {
-		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
-		goto Exit;
-	}
+	if (pci_has_legacy_pm_support(pci_dev))
+		return pci_legacy_suspend(dev, PMSG_SUSPEND);
 
 	if (drv && drv->pm) {
 		if (drv->pm->suspend) {
@@ -518,7 +523,6 @@ static int pci_pm_suspend(struct device 
 		pci_pm_default_suspend(pci_dev);
 	}
 
- Exit:
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
 
 	return error;
@@ -551,10 +555,8 @@ static int pci_pm_resume(struct device *
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	if (pci_has_legacy_pm_support(pci_dev)) {
-		pci_fixup_device(pci_fixup_resume, pci_dev);
+	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);
-	}
 
 	if (drv && drv->pm) {
 		pci_fixup_device(pci_fixup_resume, pci_dev);
@@ -574,10 +576,8 @@ static int pci_pm_resume_noirq(struct de
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	if (pci_has_legacy_pm_support(pci_dev)) {
-		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
-	}
 
 	if (drv && drv->pm) {
 		pci_fixup_device(pci_fixup_resume_early, pci_dev);
@@ -608,11 +608,8 @@ static int pci_pm_freeze(struct device *
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	if (pci_has_legacy_pm_support(pci_dev)) {
-		error = pci_legacy_suspend(dev, PMSG_FREEZE);
-		pci_fixup_device(pci_fixup_suspend, pci_dev);
-		return error;
-	}
+	if (pci_has_legacy_pm_support(pci_dev))
+		return pci_legacy_suspend(dev, PMSG_FREEZE);
 
 	if (drv && drv->pm) {
 		if (drv->pm->freeze) {
@@ -653,10 +650,8 @@ static int pci_pm_thaw(struct device *de
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	if (pci_has_legacy_pm_support(pci_dev)) {
-		pci_fixup_device(pci_fixup_resume, pci_dev);
+	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);
-	}
 
 	if (drv && drv->pm) {
 		if (drv->pm->thaw)
@@ -674,10 +669,8 @@ static int pci_pm_thaw_noirq(struct devi
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	if (pci_has_legacy_pm_support(pci_dev)) {
-		pci_fixup_device(pci_fixup_resume_early, to_pci_dev(dev));
+	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
-	}
 
 	if (drv && drv->pm) {
 		if (drv->pm->thaw_noirq)
@@ -695,10 +688,8 @@ static int pci_pm_poweroff(struct device
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	if (pci_has_legacy_pm_support(pci_dev)) {
-		error = pci_legacy_suspend(dev, PMSG_HIBERNATE);
-		goto Exit;
-	}
+	if (pci_has_legacy_pm_support(pci_dev))
+		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
 
 	if (drv && drv->pm) {
 		if (drv->pm->poweroff) {
@@ -709,7 +700,6 @@ static int pci_pm_poweroff(struct device
 		pci_pm_default_suspend(pci_dev);
 	}
 
- Exit:
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
 
 	return error;
@@ -739,10 +729,8 @@ static int pci_pm_restore(struct device 
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	if (pci_has_legacy_pm_support(pci_dev)) {
-		pci_fixup_device(pci_fixup_resume, pci_dev);
+	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);
-	}
 
 	if (drv && drv->pm) {
 		pci_fixup_device(pci_fixup_resume, pci_dev);
@@ -762,10 +750,8 @@ static int pci_pm_restore_noirq(struct d
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	if (pci_has_legacy_pm_support(pci_dev)) {
-		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
-	}
 
 	if (drv && drv->pm) {
 		pci_fixup_device(pci_fixup_resume_early, pci_dev);

Re: [RFC][PATCH 7/10] PCI PM: Call pci_fixup_device from legacy routines

[Pavel Machek]

> 
> The size of drivers/pci/pci-driver.c can be reduced quite a bit
> if pci_fixup_device() is called from the legacy PM callbacks, so make
> it happen.
> 
> No functional changes should result from this.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Acked-by: Pavel Machek <pavel@suse.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC][PATCH 8/10] PCI PM: Register power state of devices during initialization

[Rafael J. Wysocki]

Use the observation that the power state of a PCI device can be
loaded into its pci_dev structure as soon as pci_pm_init() is run for
it and make that happen.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c |    8 ++++++--
 1 file changed, 6 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
@@ -1259,14 +1259,15 @@ void pci_pm_init(struct pci_dev *dev)
 	/* find PCI PM capability in list */
 	pm = pci_find_capability(dev, PCI_CAP_ID_PM);
 	if (!pm)
-		return;
+		goto Exit;
+
 	/* Check device's ability to generate PME# */
 	pci_read_config_word(dev, pm + PCI_PM_PMC, &pmc);
 
 	if ((pmc & PCI_PM_CAP_VER_MASK) > 3) {
 		dev_err(&dev->dev, "unsupported PM cap regs version (%u)\n",
 			pmc & PCI_PM_CAP_VER_MASK);
-		return;
+		goto Exit;
 	}
 
 	dev->pm_cap = pm;
@@ -1305,6 +1306,9 @@ void pci_pm_init(struct pci_dev *dev)
 	} else {
 		dev->pme_support = 0;
 	}
+
+ Exit:
+	pci_update_current_state(dev, PCI_D0);
 }
 
 /**

Re: [RFC][PATCH 8/10] PCI PM: Register power state of devices during initialization

[Pavel Machek]

> 
> Use the observation that the power state of a PCI device can be
> loaded into its pci_dev structure as soon as pci_pm_init() is run for
> it and make that happen.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Acked-by: Pavel Machek <Pavel@suse.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC][PATCH 9/10] PCI PM: Run default PM callbacks for all devices using new framework

[Rafael J. Wysocki]

It should be quite clear that it generally makes sense to execute
the default PM callbacks (ie. the callbacks used for handling
suspend, hibernation and resume of PCI devices without drivers) for
all devices.  Of course, the drivers that provide legacy PCI PM
support (ie. the ->suspend, ->suspend_late, ->resume_early
or ->resume hooks in the pci_driver structure), carry out these
operations too, so we can't do it for devices with such drivers.
Still, we can make the default PM callbacks run for devices with
drivers using the new framework (ie. implement the pm object), since
there are no such drivers at the moment.

This also simplifies the code and reduces its size.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |  135 ++++++++++++++++++-----------------------------
 1 file changed, 53 insertions(+), 82 deletions(-)

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
@@ -472,6 +472,8 @@ static void pci_pm_default_suspend(struc
 
 	if (!pci_is_bridge(pci_dev))
 		pci_prepare_to_sleep(pci_dev);
+
+	pci_fixup_device(pci_fixup_suspend, pci_dev);
 }
 
 static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)
@@ -514,16 +516,13 @@ static int pci_pm_suspend(struct device 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_SUSPEND);
 
-	if (drv && drv->pm) {
-		if (drv->pm->suspend) {
-			error = drv->pm->suspend(dev);
-			suspend_report_result(drv->pm->suspend, error);
-		}
-	} else {
-		pci_pm_default_suspend(pci_dev);
+	if (drv && drv->pm && drv->pm->suspend) {
+		error = drv->pm->suspend(dev);
+		suspend_report_result(drv->pm->suspend, error);
 	}
 
-	pci_fixup_device(pci_fixup_suspend, pci_dev);
+	if (!error)
+		pci_pm_default_suspend(pci_dev);
 
 	return error;
 }
@@ -537,15 +536,14 @@ static int pci_pm_suspend_noirq(struct d
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend_late(dev, PMSG_SUSPEND);
 
-	if (drv && drv->pm) {
-		if (drv->pm->suspend_noirq) {
-			error = drv->pm->suspend_noirq(dev);
-			suspend_report_result(drv->pm->suspend_noirq, error);
-		}
-	} else {
-		pci_pm_set_unknown_state(pci_dev);
+	if (drv && drv->pm && drv->pm->suspend_noirq) {
+		error = drv->pm->suspend_noirq(dev);
+		suspend_report_result(drv->pm->suspend_noirq, error);
 	}
 
+	if (!error)
+		pci_pm_set_unknown_state(pci_dev);
+
 	return error;
 }
 
@@ -558,14 +556,10 @@ static int pci_pm_resume(struct device *
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);
 
-	if (drv && drv->pm) {
-		pci_fixup_device(pci_fixup_resume, pci_dev);
+	error = pci_pm_default_resume(pci_dev);
 
-		if (drv->pm->resume)
-			error = drv->pm->resume(dev);
-	} else {
-		error = pci_pm_default_resume(pci_dev);
-	}
+	if (!error && drv && drv->pm && drv->pm->resume)
+		error = drv->pm->resume(dev);
 
 	return error;
 }
@@ -579,14 +573,10 @@ static int pci_pm_resume_noirq(struct de
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
 
-	if (drv && drv->pm) {
-		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+	pci_pm_default_resume_noirq(pci_dev);
 
-		if (drv->pm->resume_noirq)
-			error = drv->pm->resume_noirq(dev);
-	} else {
-		pci_pm_default_resume_noirq(pci_dev);
-	}
+	if (drv && drv->pm && drv->pm->resume_noirq)
+		error = drv->pm->resume_noirq(dev);
 
 	return error;
 }
@@ -611,15 +601,14 @@ static int pci_pm_freeze(struct device *
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_FREEZE);
 
-	if (drv && drv->pm) {
-		if (drv->pm->freeze) {
-			error = drv->pm->freeze(dev);
-			suspend_report_result(drv->pm->freeze, error);
-		}
-	} else {
-		pci_pm_default_suspend_generic(pci_dev);
+	if (drv && drv->pm && drv->pm->freeze) {
+		error = drv->pm->freeze(dev);
+		suspend_report_result(drv->pm->freeze, error);
 	}
 
+	if (!error)
+		pci_pm_default_suspend_generic(pci_dev);
+
 	return error;
 }
 
@@ -632,15 +621,14 @@ static int pci_pm_freeze_noirq(struct de
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend_late(dev, PMSG_FREEZE);
 
-	if (drv && drv->pm) {
-		if (drv->pm->freeze_noirq) {
-			error = drv->pm->freeze_noirq(dev);
-			suspend_report_result(drv->pm->freeze_noirq, error);
-		}
-	} else {
-		pci_pm_set_unknown_state(pci_dev);
+	if (drv && drv->pm && drv->pm->freeze_noirq) {
+		error = drv->pm->freeze_noirq(dev);
+		suspend_report_result(drv->pm->freeze_noirq, error);
 	}
 
+	if (!error)
+		pci_pm_set_unknown_state(pci_dev);
+
 	return error;
 }
 
@@ -653,12 +641,10 @@ static int pci_pm_thaw(struct device *de
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);
 
-	if (drv && drv->pm) {
-		if (drv->pm->thaw)
-			error =  drv->pm->thaw(dev);
-	} else {
-		pci_pm_reenable_device(pci_dev);
-	}
+	pci_pm_reenable_device(pci_dev);
+
+	if (drv && drv->pm && drv->pm->thaw)
+		error =  drv->pm->thaw(dev);
 
 	return error;
 }
@@ -672,12 +658,10 @@ static int pci_pm_thaw_noirq(struct devi
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
 
-	if (drv && drv->pm) {
-		if (drv->pm->thaw_noirq)
-			error = drv->pm->thaw_noirq(dev);
-	} else {
-		pci_update_current_state(pci_dev, PCI_D0);
-	}
+	pci_update_current_state(pci_dev, PCI_D0);
+
+	if (drv && drv->pm && drv->pm->thaw_noirq)
+		error = drv->pm->thaw_noirq(dev);
 
 	return error;
 }
@@ -691,16 +675,13 @@ static int pci_pm_poweroff(struct device
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
 
-	if (drv && drv->pm) {
-		if (drv->pm->poweroff) {
-			error = drv->pm->poweroff(dev);
-			suspend_report_result(drv->pm->poweroff, error);
-		}
-	} else {
-		pci_pm_default_suspend(pci_dev);
+	if (drv && drv->pm && drv->pm->poweroff) {
+		error = drv->pm->poweroff(dev);
+		suspend_report_result(drv->pm->poweroff, error);
 	}
 
-	pci_fixup_device(pci_fixup_suspend, pci_dev);
+	if (!error)
+		pci_pm_default_suspend(pci_dev);
 
 	return error;
 }
@@ -713,11 +694,9 @@ static int pci_pm_poweroff_noirq(struct 
 	if (pci_has_legacy_pm_support(to_pci_dev(dev)))
 		return pci_legacy_suspend_late(dev, PMSG_HIBERNATE);
 
-	if (drv && drv->pm) {
-		if (drv->pm->poweroff_noirq) {
-			error = drv->pm->poweroff_noirq(dev);
-			suspend_report_result(drv->pm->poweroff_noirq, error);
-		}
+	if (drv && drv->pm && drv->pm->poweroff_noirq) {
+		error = drv->pm->poweroff_noirq(dev);
+		suspend_report_result(drv->pm->poweroff_noirq, error);
 	}
 
 	return error;
@@ -732,14 +711,10 @@ static int pci_pm_restore(struct device 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);
 
-	if (drv && drv->pm) {
-		pci_fixup_device(pci_fixup_resume, pci_dev);
+	error = pci_pm_default_resume(pci_dev);
 
-		if (drv->pm->restore)
-			error = drv->pm->restore(dev);
-	} else {
-		error = pci_pm_default_resume(pci_dev);
-	}
+	if (!error && drv && drv->pm && drv->pm->restore)
+		error = drv->pm->restore(dev);
 
 	return error;
 }
@@ -753,14 +728,10 @@ static int pci_pm_restore_noirq(struct d
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
 
-	if (drv && drv->pm) {
-		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+	pci_pm_default_resume_noirq(pci_dev);
 
-		if (drv->pm->restore_noirq)
-			error = drv->pm->restore_noirq(dev);
-	} else {
-		pci_pm_default_resume_noirq(pci_dev);
-	}
+	if (drv && drv->pm && drv->pm->restore_noirq)
+		error = drv->pm->restore_noirq(dev);
 
 	return error;
 }

Re: [RFC][PATCH 9/10] PCI PM: Run default PM callbacks for all devices using new framework

[Pavel Machek]

> 
> It should be quite clear that it generally makes sense to execute
> the default PM callbacks (ie. the callbacks used for handling
> suspend, hibernation and resume of PCI devices without drivers) for
> all devices.  Of course, the drivers that provide legacy PCI PM
> support (ie. the ->suspend, ->suspend_late, ->resume_early
> or ->resume hooks in the pci_driver structure), carry out these
> operations too, so we can't do it for devices with such drivers.
> Still, we can make the default PM callbacks run for devices with
> drivers using the new framework (ie. implement the pm object), since
> there are no such drivers at the moment.
> 
> This also simplifies the code and reduces its size.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Acked-by: Pavel Machek <Pavel@suse.cz>
> ---
>  drivers/pci/pci-driver.c |  135 ++++++++++++++++++-----------------------------
>  1 file changed, 53 insertions(+), 82 deletions(-)
> 
> 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
> @@ -472,6 +472,8 @@ static void pci_pm_default_suspend(struc
>  
>  	if (!pci_is_bridge(pci_dev))
>  		pci_prepare_to_sleep(pci_dev);
> +
> +	pci_fixup_device(pci_fixup_suspend, pci_dev);
>  }
>  
>  static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)
> @@ -514,16 +516,13 @@ static int pci_pm_suspend(struct device 
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_suspend(dev, PMSG_SUSPEND);
>  
> -	if (drv && drv->pm) {
> -		if (drv->pm->suspend) {
> -			error = drv->pm->suspend(dev);
> -			suspend_report_result(drv->pm->suspend, error);
> -		}
> -	} else {
> -		pci_pm_default_suspend(pci_dev);
> +	if (drv && drv->pm && drv->pm->suspend) {
> +		error = drv->pm->suspend(dev);
> +		suspend_report_result(drv->pm->suspend, error);
>  	}
>  
> -	pci_fixup_device(pci_fixup_suspend, pci_dev);
> +	if (!error)
> +		pci_pm_default_suspend(pci_dev);
>  
>  	return error;
>  }
> @@ -537,15 +536,14 @@ static int pci_pm_suspend_noirq(struct d
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_suspend_late(dev, PMSG_SUSPEND);
>  
> -	if (drv && drv->pm) {
> -		if (drv->pm->suspend_noirq) {
> -			error = drv->pm->suspend_noirq(dev);
> -			suspend_report_result(drv->pm->suspend_noirq, error);
> -		}
> -	} else {
> -		pci_pm_set_unknown_state(pci_dev);
> +	if (drv && drv->pm && drv->pm->suspend_noirq) {
> +		error = drv->pm->suspend_noirq(dev);
> +		suspend_report_result(drv->pm->suspend_noirq, error);
>  	}
>  
> +	if (!error)
> +		pci_pm_set_unknown_state(pci_dev);
> +
>  	return error;
>  }
>  
> @@ -558,14 +556,10 @@ static int pci_pm_resume(struct device *
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume(dev);
>  
> -	if (drv && drv->pm) {
> -		pci_fixup_device(pci_fixup_resume, pci_dev);
> +	error = pci_pm_default_resume(pci_dev);
>  
> -		if (drv->pm->resume)
> -			error = drv->pm->resume(dev);
> -	} else {
> -		error = pci_pm_default_resume(pci_dev);
> -	}
> +	if (!error && drv && drv->pm && drv->pm->resume)
> +		error = drv->pm->resume(dev);
>  
>  	return error;
>  }
> @@ -579,14 +573,10 @@ static int pci_pm_resume_noirq(struct de
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume_early(dev);
>  
> -	if (drv && drv->pm) {
> -		pci_fixup_device(pci_fixup_resume_early, pci_dev);
> +	pci_pm_default_resume_noirq(pci_dev);
>  
> -		if (drv->pm->resume_noirq)
> -			error = drv->pm->resume_noirq(dev);
> -	} else {
> -		pci_pm_default_resume_noirq(pci_dev);
> -	}
> +	if (drv && drv->pm && drv->pm->resume_noirq)
> +		error = drv->pm->resume_noirq(dev);
>  
>  	return error;
>  }
> @@ -611,15 +601,14 @@ static int pci_pm_freeze(struct device *
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_suspend(dev, PMSG_FREEZE);
>  
> -	if (drv && drv->pm) {
> -		if (drv->pm->freeze) {
> -			error = drv->pm->freeze(dev);
> -			suspend_report_result(drv->pm->freeze, error);
> -		}
> -	} else {
> -		pci_pm_default_suspend_generic(pci_dev);
> +	if (drv && drv->pm && drv->pm->freeze) {
> +		error = drv->pm->freeze(dev);
> +		suspend_report_result(drv->pm->freeze, error);
>  	}
>  
> +	if (!error)
> +		pci_pm_default_suspend_generic(pci_dev);
> +
>  	return error;
>  }
>  
> @@ -632,15 +621,14 @@ static int pci_pm_freeze_noirq(struct de
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_suspend_late(dev, PMSG_FREEZE);
>  
> -	if (drv && drv->pm) {
> -		if (drv->pm->freeze_noirq) {
> -			error = drv->pm->freeze_noirq(dev);
> -			suspend_report_result(drv->pm->freeze_noirq, error);
> -		}
> -	} else {
> -		pci_pm_set_unknown_state(pci_dev);
> +	if (drv && drv->pm && drv->pm->freeze_noirq) {
> +		error = drv->pm->freeze_noirq(dev);
> +		suspend_report_result(drv->pm->freeze_noirq, error);
>  	}
>  
> +	if (!error)
> +		pci_pm_set_unknown_state(pci_dev);
> +
>  	return error;
>  }
>  
> @@ -653,12 +641,10 @@ static int pci_pm_thaw(struct device *de
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume(dev);
>  
> -	if (drv && drv->pm) {
> -		if (drv->pm->thaw)
> -			error =  drv->pm->thaw(dev);
> -	} else {
> -		pci_pm_reenable_device(pci_dev);
> -	}
> +	pci_pm_reenable_device(pci_dev);
> +
> +	if (drv && drv->pm && drv->pm->thaw)
> +		error =  drv->pm->thaw(dev);
>  
>  	return error;
>  }
> @@ -672,12 +658,10 @@ static int pci_pm_thaw_noirq(struct devi
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume_early(dev);
>  
> -	if (drv && drv->pm) {
> -		if (drv->pm->thaw_noirq)
> -			error = drv->pm->thaw_noirq(dev);
> -	} else {
> -		pci_update_current_state(pci_dev, PCI_D0);
> -	}
> +	pci_update_current_state(pci_dev, PCI_D0);
> +
> +	if (drv && drv->pm && drv->pm->thaw_noirq)
> +		error = drv->pm->thaw_noirq(dev);
>  
>  	return error;
>  }
> @@ -691,16 +675,13 @@ static int pci_pm_poweroff(struct device
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
>  
> -	if (drv && drv->pm) {
> -		if (drv->pm->poweroff) {
> -			error = drv->pm->poweroff(dev);
> -			suspend_report_result(drv->pm->poweroff, error);
> -		}
> -	} else {
> -		pci_pm_default_suspend(pci_dev);
> +	if (drv && drv->pm && drv->pm->poweroff) {
> +		error = drv->pm->poweroff(dev);
> +		suspend_report_result(drv->pm->poweroff, error);
>  	}
>  
> -	pci_fixup_device(pci_fixup_suspend, pci_dev);
> +	if (!error)
> +		pci_pm_default_suspend(pci_dev);
>  
>  	return error;
>  }
> @@ -713,11 +694,9 @@ static int pci_pm_poweroff_noirq(struct 
>  	if (pci_has_legacy_pm_support(to_pci_dev(dev)))
>  		return pci_legacy_suspend_late(dev, PMSG_HIBERNATE);
>  
> -	if (drv && drv->pm) {
> -		if (drv->pm->poweroff_noirq) {
> -			error = drv->pm->poweroff_noirq(dev);
> -			suspend_report_result(drv->pm->poweroff_noirq, error);
> -		}
> +	if (drv && drv->pm && drv->pm->poweroff_noirq) {
> +		error = drv->pm->poweroff_noirq(dev);
> +		suspend_report_result(drv->pm->poweroff_noirq, error);
>  	}
>  
>  	return error;
> @@ -732,14 +711,10 @@ static int pci_pm_restore(struct device 
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume(dev);
>  
> -	if (drv && drv->pm) {
> -		pci_fixup_device(pci_fixup_resume, pci_dev);
> +	error = pci_pm_default_resume(pci_dev);
>  
> -		if (drv->pm->restore)
> -			error = drv->pm->restore(dev);
> -	} else {
> -		error = pci_pm_default_resume(pci_dev);
> -	}
> +	if (!error && drv && drv->pm && drv->pm->restore)
> +		error = drv->pm->restore(dev);
>  
>  	return error;
>  }
> @@ -753,14 +728,10 @@ static int pci_pm_restore_noirq(struct d
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume_early(dev);
>  
> -	if (drv && drv->pm) {
> -		pci_fixup_device(pci_fixup_resume_early, pci_dev);
> +	pci_pm_default_resume_noirq(pci_dev);
>  
> -		if (drv->pm->restore_noirq)
> -			error = drv->pm->restore_noirq(dev);
> -	} else {
> -		pci_pm_default_resume_noirq(pci_dev);
> -	}
> +	if (drv && drv->pm && drv->pm->restore_noirq)
> +		error = drv->pm->restore_noirq(dev);
>  
>  	return error;
>  }
> 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC][PATCH 10/10] PCI PM: Put PM callbacks in the order of execution

[Rafael J. Wysocki]

Put PM callbacks in drivers/pci/pci-driver.c in the order in which
they are executed which makes it much easier to follow the code.

No functional changes should result from this.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |   86 +++++++++++++++++++++++------------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

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
@@ -370,6 +370,19 @@ static int pci_legacy_suspend_late(struc
 	return i;
 }
 
+static int pci_legacy_resume_early(struct device *dev)
+{
+	int error = 0;
+	struct pci_dev * pci_dev = to_pci_dev(dev);
+	struct pci_driver * drv = pci_dev->driver;
+
+	pci_fixup_device(pci_fixup_resume_early, pci_dev);
+
+	if (drv && drv->resume_early)
+		error = drv->resume_early(pci_dev);
+	return error;
+}
+
 static int pci_legacy_resume(struct device *dev)
 {
 	int error;
@@ -388,19 +401,6 @@ static int pci_legacy_resume(struct devi
 	return error;
 }
 
-static int pci_legacy_resume_early(struct device *dev)
-{
-	int error = 0;
-	struct pci_dev * pci_dev = to_pci_dev(dev);
-	struct pci_driver * drv = pci_dev->driver;
-
-	pci_fixup_device(pci_fixup_resume_early, pci_dev);
-
-	if (drv && drv->resume_early)
-		error = drv->resume_early(pci_dev);
-	return error;
-}
-
 /* Auxiliary functions used by the new power management framework */
 
 static int pci_restore_standard_config(struct pci_dev *pci_dev)
@@ -547,36 +547,36 @@ static int pci_pm_suspend_noirq(struct d
 	return error;
 }
 
-static int pci_pm_resume(struct device *dev)
+static int pci_pm_resume_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
 	if (pci_has_legacy_pm_support(pci_dev))
-		return pci_legacy_resume(dev);
+		return pci_legacy_resume_early(dev);
 
-	error = pci_pm_default_resume(pci_dev);
+	pci_pm_default_resume_noirq(pci_dev);
 
-	if (!error && drv && drv->pm && drv->pm->resume)
-		error = drv->pm->resume(dev);
+	if (drv && drv->pm && drv->pm->resume_noirq)
+		error = drv->pm->resume_noirq(dev);
 
 	return error;
 }
 
-static int pci_pm_resume_noirq(struct device *dev)
+static int pci_pm_resume(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
 	if (pci_has_legacy_pm_support(pci_dev))
-		return pci_legacy_resume_early(dev);
+		return pci_legacy_resume(dev);
 
-	pci_pm_default_resume_noirq(pci_dev);
+	error = pci_pm_default_resume(pci_dev);
 
-	if (drv && drv->pm && drv->pm->resume_noirq)
-		error = drv->pm->resume_noirq(dev);
+	if (!error && drv && drv->pm && drv->pm->resume)
+		error = drv->pm->resume(dev);
 
 	return error;
 }
@@ -632,36 +632,36 @@ static int pci_pm_freeze_noirq(struct de
 	return error;
 }
 
-static int pci_pm_thaw(struct device *dev)
+static int pci_pm_thaw_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
 	if (pci_has_legacy_pm_support(pci_dev))
-		return pci_legacy_resume(dev);
+		return pci_legacy_resume_early(dev);
 
-	pci_pm_reenable_device(pci_dev);
+	pci_update_current_state(pci_dev, PCI_D0);
 
-	if (drv && drv->pm && drv->pm->thaw)
-		error =  drv->pm->thaw(dev);
+	if (drv && drv->pm && drv->pm->thaw_noirq)
+		error = drv->pm->thaw_noirq(dev);
 
 	return error;
 }
 
-static int pci_pm_thaw_noirq(struct device *dev)
+static int pci_pm_thaw(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
 	if (pci_has_legacy_pm_support(pci_dev))
-		return pci_legacy_resume_early(dev);
+		return pci_legacy_resume(dev);
 
-	pci_update_current_state(pci_dev, PCI_D0);
+	pci_pm_reenable_device(pci_dev);
 
-	if (drv && drv->pm && drv->pm->thaw_noirq)
-		error = drv->pm->thaw_noirq(dev);
+	if (drv && drv->pm && drv->pm->thaw)
+		error =  drv->pm->thaw(dev);
 
 	return error;
 }
@@ -702,36 +702,36 @@ static int pci_pm_poweroff_noirq(struct 
 	return error;
 }
 
-static int pci_pm_restore(struct device *dev)
+static int pci_pm_restore_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
 	if (pci_has_legacy_pm_support(pci_dev))
-		return pci_legacy_resume(dev);
+		return pci_legacy_resume_early(dev);
 
-	error = pci_pm_default_resume(pci_dev);
+	pci_pm_default_resume_noirq(pci_dev);
 
-	if (!error && drv && drv->pm && drv->pm->restore)
-		error = drv->pm->restore(dev);
+	if (drv && drv->pm && drv->pm->restore_noirq)
+		error = drv->pm->restore_noirq(dev);
 
 	return error;
 }
 
-static int pci_pm_restore_noirq(struct device *dev)
+static int pci_pm_restore(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
 	if (pci_has_legacy_pm_support(pci_dev))
-		return pci_legacy_resume_early(dev);
+		return pci_legacy_resume(dev);
 
-	pci_pm_default_resume_noirq(pci_dev);
+	error = pci_pm_default_resume(pci_dev);
 
-	if (drv && drv->pm && drv->pm->restore_noirq)
-		error = drv->pm->restore_noirq(dev);
+	if (!error && drv && drv->pm && drv->pm->restore)
+		error = drv->pm->restore(dev);
 
 	return error;
 }

Re: [RFC][PATCH 10/10] PCI PM: Put PM callbacks in the order of execution

[Pavel Machek]

> 
> Put PM callbacks in drivers/pci/pci-driver.c in the order in which
> they are executed which makes it much easier to follow the code.
> 
> No functional changes should result from this.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Acked-by: Pavel Machek <Pavel@suse.cz

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html