[PATCH 0/7] PCI PM: Fixes and refinements

[PATCH 0/7] PCI PM: Fixes and refinements

[Rafael J. Wysocki]

Hi,

The following series of patches contains some fixes and refinements of the
PCI PM framework.

[1/7] fixes the problem that some driverless devices don't like to be power
managed and break things if we attempt to do it.

[2/7] and [3/7] fix the problem with devices in PCI_UNKNOWN that are
mishandled by the new code.

[4/7] fixes regression related to PCIe port suspend-resume.

[5/7] fixes the bug that bridges (and PCIe ports) are disabled during suspend,
althouth they shouldn't be.

[6/7] makes pci_restore_standard_config() read the current power state of the
device from the device itself after attempting to change it.

[7/7] makes the PCI PM core handle devices more carefully (details in the
changelog).

Please disregard any previous versions of these patches I sent to you.

Thanks,
Rafael

[PATCH 1/7] PCI PM: Fix handling of devices without drivers

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Suspend to RAM is reported to break on some machines as a result of
attempting to put one of driverless PCI devices into a low power
state.  Avoid that by not attepmting to power manage driverless
devices during suspend.

Fix up pci_pm_poweroff() after a previous incomplete fix for the same
thing during hibernation.

This patch is reported to fix the regression from 2.6.28 tracked as
http://bugzilla.kernel.org/show_bug.cgi?id=12605

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Reported-and-tested-by: Eric Sesterhenn <snakebyte@gmx.de>
---
 drivers/pci/pci-driver.c |   27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 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
@@ -445,11 +445,11 @@ static void pci_pm_default_suspend_gener
 	pci_save_state(pci_dev);
 }
 
-static void pci_pm_default_suspend(struct pci_dev *pci_dev)
+static void pci_pm_default_suspend(struct pci_dev *pci_dev, bool prepare)
 {
 	pci_pm_default_suspend_generic(pci_dev);
 
-	if (!pci_is_bridge(pci_dev))
+	if (prepare && !pci_is_bridge(pci_dev))
 		pci_prepare_to_sleep(pci_dev);
 
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
@@ -497,19 +497,19 @@ static void pci_pm_complete(struct devic
 static int pci_pm_suspend(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	struct device_driver *drv = dev->driver;
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int error = 0;
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_SUSPEND);
 
-	if (drv && drv->pm && drv->pm->suspend) {
-		error = drv->pm->suspend(dev);
-		suspend_report_result(drv->pm->suspend, error);
+	if (pm && pm->suspend) {
+		error = pm->suspend(dev);
+		suspend_report_result(pm->suspend, error);
 	}
 
 	if (!error)
-		pci_pm_default_suspend(pci_dev);
+		pci_pm_default_suspend(pci_dev, !!pm);
 
 	return error;
 }
@@ -663,22 +663,19 @@ static int pci_pm_thaw(struct device *de
 static int pci_pm_poweroff(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	struct device_driver *drv = dev->driver;
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int error = 0;
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
 
-	if (!drv || !drv->pm)
-		return 0;
-
-	if (drv->pm->poweroff) {
-		error = drv->pm->poweroff(dev);
-		suspend_report_result(drv->pm->poweroff, error);
+	if (pm && pm->poweroff) {
+		error = pm->poweroff(dev);
+		suspend_report_result(pm->poweroff, error);
 	}
 
 	if (!error)
-		pci_pm_default_suspend(pci_dev);
+		pci_pm_default_suspend(pci_dev, !!pm);
 
 	return error;
 }

[PATCH 2/7] PCI PM: Check if the state has been saved before trying to restore it

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Check if the standard configuration registers of a PCI device have
been saved during suspend before trying to restore them during
resume.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Reported-By: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/pci/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -1421,7 +1421,7 @@ int pci_restore_standard_config(struct p
 	dev->current_state = PCI_D0;
 
  Restore:
-	return pci_restore_state(dev);
+	return dev->state_saved ? pci_restore_state(dev) : 0;
 }
 
 /**

[PATCH 3/7] PCI PM: Fix saving of device state in pci_legacy_suspend

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Make pci_legacy_suspend() save the state of the device if it is
in PCI_UNKNOWN after its suspend callback has run and warn only if
the power state of the device has been changed by its suspend
callback.

Also, use WARN_ONCE(), which is more useful, in pci_legacy_suspend(),
so that the name of the offending function is printed.

Additionaly, remove the unnecessary line of code setting
pci_dev->state_saved.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/pci/pci-driver.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 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
@@ -355,6 +355,8 @@ static int pci_legacy_suspend(struct dev
 	int i = 0;
 
 	if (drv && drv->suspend) {
+		pci_power_t prev = pci_dev->current_state;
+
 		pci_dev->state_saved = false;
 
 		i = drv->suspend(pci_dev, state);
@@ -365,12 +367,16 @@ static int pci_legacy_suspend(struct dev
 		if (pci_dev->state_saved)
 			goto Fixup;
 
-		if (WARN_ON_ONCE(pci_dev->current_state != PCI_D0))
+		if (pci_dev->current_state != PCI_D0
+		    && pci_dev->current_state != PCI_UNKNOWN) {
+			WARN_ONCE(pci_dev->current_state != prev,
+				"PCI PM: Device state not saved by %pF\n",
+				drv->suspend);
 			goto Fixup;
+		}
 	}
 
 	pci_save_state(pci_dev);
-	pci_dev->state_saved = true;
 	/*
 	 * This is for compatibility with existing code with legacy PM support.
 	 */

[PATCH 4/7] PCI PCIe portdrv: Simplify suspend and resume

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Simplify suspend and resume of the PCI Express port driver that
doesn't need to save and restore the standard configuration space
of the device any more, since this is now done by the PCI PM core
layer.

This patch is reported to fix the regression tracked as
http://bugzilla.kernel.org/show_bug.cgi?id=12598

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Reported-and-tested-by: Parag Warudkar <parag.lkml@gmail.com>
---
 drivers/pci/pcie/portdrv_pci.c |   16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

Index: linux-2.6/drivers/pci/pcie/portdrv_pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pcie/portdrv_pci.c
+++ linux-2.6/drivers/pci/pcie/portdrv_pci.c
@@ -55,25 +55,13 @@ static int pcie_portdrv_suspend(struct p
 
 }
 
-static int pcie_portdrv_suspend_late(struct pci_dev *dev, pm_message_t state)
-{
-	return pci_save_state(dev);
-}
-
-static int pcie_portdrv_resume_early(struct pci_dev *dev)
-{
-	return pci_restore_state(dev);
-}
-
 static int pcie_portdrv_resume(struct pci_dev *dev)
 {
-	pcie_portdrv_restore_config(dev);
+	pci_set_master(dev);
 	return pcie_port_device_resume(dev);
 }
 #else
 #define pcie_portdrv_suspend NULL
-#define pcie_portdrv_suspend_late NULL
-#define pcie_portdrv_resume_early NULL
 #define pcie_portdrv_resume NULL
 #endif
 
@@ -292,8 +280,6 @@ static struct pci_driver pcie_portdriver
 	.remove		= pcie_portdrv_remove,
 
 	.suspend	= pcie_portdrv_suspend,
-	.suspend_late	= pcie_portdrv_suspend_late,
-	.resume_early	= pcie_portdrv_resume_early,
 	.resume		= pcie_portdrv_resume,
 
 	.err_handler 	= &pcie_portdrv_err_handler,

[PATCH 5/7] PCI PM: Do not disable and enable bridges during suspend-resume

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

It is a mistake to disable and enable PCI bridges and PCI Express
ports during suspend-resume, at least at the time when it is
currently done.  Disabling them may lead to problems with accessing
devices behind them and they should be automatically enabled when
their standard config spaces are restored.  Fix this by not attempting
to disable bridges during suspend and enable them during resume.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |   10 ++++++----
 1 file changed, 6 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
@@ -434,16 +434,18 @@ static int pci_pm_default_resume(struct 
 {
 	pci_fixup_device(pci_fixup_resume, pci_dev);
 
-	if (!pci_is_bridge(pci_dev))
-		pci_enable_wake(pci_dev, PCI_D0, false);
+	if (pci_is_bridge(pci_dev))
+		return 0;
 
+	pci_enable_wake(pci_dev, PCI_D0, false);
 	return pci_pm_reenable_device(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);
+	/* If a non-bridge device is enabled at this point, disable it */
+	if (!pci_is_bridge(pci_dev))
+		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.

[PATCH 6/7] PCI PM: Read power state from device after trying to change it on resume

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

pci_restore_standard_config() unconditionally changes current_state
to PCI_D0 after attempting to change the device's power state, but
it should rather read the actual current power state from the
device.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -1418,7 +1418,7 @@ int pci_restore_standard_config(struct p
 		break;
 	}
 
-	dev->current_state = PCI_D0;
+	pci_update_current_state(dev, PCI_D0);
 
  Restore:
 	return dev->state_saved ? pci_restore_state(dev) : 0;

[PATCH 7/7] PCI PM: Let the core be more careful with respect to drivers using new framework

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Currently, the PM core always attempts to manage devices with drivers
that use the new PM framework.  In particular, it attempts to disable
the devices (which is unnecessary), to save their state (which may be
undesirable if the driver has done that already) and to put them into
low power states (again, this may be undesirable if the driver has
already put the device into a low power state).  That need not be
the right thing to do, so make the core be more careful in this
respect.

Generally, there are the following categories of devices to consider:
* bridge devices without drivers
* non-bridge devices without drivers
* bridge devices with drivers
* non-bridge devices with drivers
and each of them should be handled differently.

For bridge devices without drivers the PCI PM core will save their
state on suspend and restore it (early) during resume, after putting
them into D0 if necessary.  It will not attempt to do anything else
to these devices.

For non-bridge devices without drivers the PCI PM core will disable
them and save their state on suspend.  During resume, it will put
them into D0, if necessary, restore their state (early) and reenable
them.

For bridge devices with drivers the PCI PM core will only save
their state on suspend if the driver hasn't done that already.
Still, the core will restore their state (early) during resume,
after putting them into D0, if necessary.

For non-bridge devices with drivers the PCI PM core will only save
their state on suspend if the driver hasn't done that already.  Also,
if the state of the device hasn't been saved by the driver, the core
will attempt to put the device into a low power state.  During
resume the core will restore the state of the device (early), after
putting it into D0, if necessary.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |  145 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 93 insertions(+), 52 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
@@ -430,39 +430,22 @@ static void pci_pm_default_resume_noirq(
 	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 }
 
-static int pci_pm_default_resume(struct pci_dev *pci_dev)
+static void pci_pm_default_resume(struct pci_dev *pci_dev)
 {
 	pci_fixup_device(pci_fixup_resume, pci_dev);
 
-	if (pci_is_bridge(pci_dev))
-		return 0;
-
-	pci_enable_wake(pci_dev, PCI_D0, false);
-	return pci_pm_reenable_device(pci_dev);
+	if (!pci_is_bridge(pci_dev))
+		pci_enable_wake(pci_dev, PCI_D0, false);
 }
 
-static void pci_pm_default_suspend_generic(struct pci_dev *pci_dev)
+static void pci_pm_default_suspend(struct pci_dev *pci_dev)
 {
-	/* If a non-bridge device is enabled at this point, disable it */
+	/* Disable non-bridge devices without PM support */
 	if (!pci_is_bridge(pci_dev))
 		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, bool prepare)
-{
-	pci_pm_default_suspend_generic(pci_dev);
-
-	if (prepare && !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)
 {
 	struct pci_driver *drv = pci_dev->driver;
@@ -506,20 +489,48 @@ static int pci_pm_suspend(struct device 
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int error = 0;
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_SUSPEND);
 
-	if (pm && pm->suspend) {
+	if (!pm) {
+		pci_pm_default_suspend(pci_dev);
+		goto Fixup;
+	}
+
+	pci_dev->state_saved = false;
+
+	if (pm->suspend) {
+		pci_power_t prev = pci_dev->current_state;
+		int error;
+
 		error = pm->suspend(dev);
 		suspend_report_result(pm->suspend, error);
+		if (error)
+			return error;
+
+		if (pci_dev->state_saved)
+			goto Fixup;
+
+		if (pci_dev->current_state != PCI_D0
+		    && pci_dev->current_state != PCI_UNKNOWN) {
+			WARN_ONCE(pci_dev->current_state != prev,
+				"PCI PM: State of device not saved by %pF\n",
+				pm->suspend);
+			goto Fixup;
+		}
 	}
 
-	if (!error)
-		pci_pm_default_suspend(pci_dev, !!pm);
+	if (!pci_dev->state_saved) {
+		pci_save_state(pci_dev);
+		if (!pci_is_bridge(pci_dev))
+			pci_prepare_to_sleep(pci_dev);
+	}
 
-	return error;
+ Fixup:
+	pci_fixup_device(pci_fixup_suspend, pci_dev);
+
+	return 0;
 }
 
 static int pci_pm_suspend_noirq(struct device *dev)
@@ -562,7 +573,7 @@ static int pci_pm_resume_noirq(struct de
 static int pci_pm_resume(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	struct device_driver *drv = dev->driver;
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int error = 0;
 
 	/*
@@ -575,12 +586,16 @@ static int pci_pm_resume(struct device *
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);
 
-	error = pci_pm_default_resume(pci_dev);
+	pci_pm_default_resume(pci_dev);
 
-	if (!error && drv && drv->pm && drv->pm->resume)
-		error = drv->pm->resume(dev);
+	if (pm) {
+		if (pm->resume)
+			error = pm->resume(dev);
+	} else {
+		pci_pm_reenable_device(pci_dev);
+	}
 
-	return error;
+	return 0;
 }
 
 #else /* !CONFIG_SUSPEND */
@@ -597,21 +612,31 @@ static int pci_pm_resume(struct device *
 static int pci_pm_freeze(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	struct device_driver *drv = dev->driver;
-	int error = 0;
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_FREEZE);
 
-	if (drv && drv->pm && drv->pm->freeze) {
-		error = drv->pm->freeze(dev);
-		suspend_report_result(drv->pm->freeze, error);
+	if (!pm) {
+		pci_pm_default_suspend(pci_dev);
+		return 0;
 	}
 
-	if (!error)
-		pci_pm_default_suspend_generic(pci_dev);
+	pci_dev->state_saved = false;
 
-	return error;
+	if (pm->freeze) {
+		int error;
+
+		error = pm->freeze(dev);
+		suspend_report_result(pm->freeze, error);
+		if (error)
+			return error;
+	}
+
+	if (!pci_dev->state_saved)
+		pci_save_state(pci_dev);
+
+	return 0;
 }
 
 static int pci_pm_freeze_noirq(struct device *dev)
@@ -654,16 +679,18 @@ static int pci_pm_thaw_noirq(struct devi
 static int pci_pm_thaw(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	struct device_driver *drv = dev->driver;
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int error = 0;
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);
 
-	pci_pm_reenable_device(pci_dev);
-
-	if (drv && drv->pm && drv->pm->thaw)
-		error =  drv->pm->thaw(dev);
+	if (pm) {
+		if (pm->thaw)
+			error = pm->thaw(dev);
+	} else {
+		pci_pm_reenable_device(pci_dev);
+	}
 
 	return error;
 }
@@ -677,13 +704,23 @@ static int pci_pm_poweroff(struct device
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
 
-	if (pm && pm->poweroff) {
+	if (!pm) {
+		pci_pm_default_suspend(pci_dev);
+		goto Fixup;
+	}
+
+	pci_dev->state_saved = false;
+
+	if (pm->poweroff) {
 		error = pm->poweroff(dev);
 		suspend_report_result(pm->poweroff, error);
 	}
 
-	if (!error)
-		pci_pm_default_suspend(pci_dev, !!pm);
+	if (!pci_dev->state_saved && !pci_is_bridge(pci_dev))
+		pci_prepare_to_sleep(pci_dev);
+
+ Fixup:
+	pci_fixup_device(pci_fixup_suspend, pci_dev);
 
 	return error;
 }
@@ -724,7 +761,7 @@ static int pci_pm_restore_noirq(struct d
 static int pci_pm_restore(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	struct device_driver *drv = dev->driver;
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int error = 0;
 
 	/*
@@ -737,10 +774,14 @@ static int pci_pm_restore(struct device 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume(dev);
 
-	error = pci_pm_default_resume(pci_dev);
+	pci_pm_default_resume(pci_dev);
 
-	if (!error && drv && drv->pm && drv->pm->restore)
-		error = drv->pm->restore(dev);
+	if (pm) {
+		if (pm->restore)
+			error = pm->restore(dev);
+	} else {
+		pci_pm_reenable_device(pci_dev);
+	}
 
 	return error;
 }

Re: [PATCH 1/7] PCI PM: Fix handling of devices without drivers

[Benjamin Herrenschmidt]

> -static void pci_pm_default_suspend(struct pci_dev *pci_dev)
> +static void pci_pm_default_suspend(struct pci_dev *pci_dev, bool prepare)
>  {
>  	pci_pm_default_suspend_generic(pci_dev);
>  
> -	if (!pci_is_bridge(pci_dev))
> +	if (prepare && !pci_is_bridge(pci_dev))
>  		pci_prepare_to_sleep(pci_dev);

Minor nit: It took me a while to understand the patch because of the use
of the word "prepare" which we easily confuse with the prepare suspend
phase.

Might be worth using a different terminology...

Cheers,
Ben.

Re: [PATCH 2/7] PCI PM: Check if the state has been saved before trying to restore it

[Benjamin Herrenschmidt]

On Wed, 2009-02-04 at 01:57 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Check if the standard configuration registers of a PCI device have
> been saved during suspend before trying to restore them during
> resume.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Acked-By: Benjamin Herrenschmidt <benh@kernel.crashing.org>


> ---
>  drivers/pci/pci.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/pci/pci.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci.c
> +++ linux-2.6/drivers/pci/pci.c
> @@ -1421,7 +1421,7 @@ int pci_restore_standard_config(struct p
>  	dev->current_state = PCI_D0;
>  
>   Restore:
> -	return pci_restore_state(dev);
> +	return dev->state_saved ? pci_restore_state(dev) : 0;
>  }
>  
>  /**

Re: [PATCH 3/7] PCI PM: Fix saving of device state in pci_legacy_suspend

[Benjamin Herrenschmidt]

On Wed, 2009-02-04 at 01:59 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Make pci_legacy_suspend() save the state of the device if it is
> in PCI_UNKNOWN after its suspend callback has run and warn only if
> the power state of the device has been changed by its suspend
> callback.
> 
> Also, use WARN_ONCE(), which is more useful, in pci_legacy_suspend(),
> so that the name of the offending function is printed.
> 
> Additionaly, remove the unnecessary line of code setting
> pci_dev->state_saved.

Minor nit: Should the warning be preceeded by a message ? The reason is,
right now, all we get is a backtrace, it doesn't actually tell you which
device or driver caused it which makes it pretty pointless.

I think you should add a printk(KERN_ERR... just before that which gives
those informations along with a little blurb along the lines of "driver
changed device state without saving config space state").

> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  drivers/pci/pci-driver.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 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
> @@ -355,6 +355,8 @@ static int pci_legacy_suspend(struct dev
>  	int i = 0;
>  
>  	if (drv && drv->suspend) {
> +		pci_power_t prev = pci_dev->current_state;
> +
>  		pci_dev->state_saved = false;
>  
>  		i = drv->suspend(pci_dev, state);
> @@ -365,12 +367,16 @@ static int pci_legacy_suspend(struct dev
>  		if (pci_dev->state_saved)
>  			goto Fixup;
>  
> -		if (WARN_ON_ONCE(pci_dev->current_state != PCI_D0))
> +		if (pci_dev->current_state != PCI_D0
> +		    && pci_dev->current_state != PCI_UNKNOWN) {
> +			WARN_ONCE(pci_dev->current_state != prev,
> +				"PCI PM: Device state not saved by %pF\n",
> +				drv->suspend);
>  			goto Fixup;
> +		}
>  	}
>  
>  	pci_save_state(pci_dev);
> -	pci_dev->state_saved = true;
>  	/*
>  	 * This is for compatibility with existing code with legacy PM support.
>  	 */

Re: [PATCH 7/7] PCI PM: Let the core be more careful with respect to drivers using new framework

[Benjamin Herrenschmidt]

> For non-bridge devices without drivers the PCI PM core will disable
> them and save their state on suspend.  During resume, it will put
> them into D0, if necessary, restore their state (early) and reenable
> them.

This is "without drivers" only or does it include devices that have a
driver and no PM ops ?

In the later case, the worry is that the driver will potentially still
take shared interrupts after you have disabled the device. I know the
normal disable path only disables bus mastering, which is a GOOD
thing :-) but the pcibios hook might do more here ...

I would recommend only doing that disabling in the "noirq" phase of
suspend to avoid problems here. Do you see any reason that wouldn't
work ?

Cheers,
Ben.

Re: [PATCH 1/7] PCI PM: Fix handling of devices without drivers

[Linus Torvalds]

On Wed, 4 Feb 2009, Rafael J. Wysocki wrote:
> 
> Suspend to RAM is reported to break on some machines as a result of
> attempting to put one of driverless PCI devices into a low power
> state.  Avoid that by not attepmting to power manage driverless
> devices during suspend.
> 
> Fix up pci_pm_poweroff() after a previous incomplete fix for the same
> thing during hibernation.

Ok, I really don't like this patch, because:

> -static void pci_pm_default_suspend(struct pci_dev *pci_dev)
> +static void pci_pm_default_suspend(struct pci_dev *pci_dev, bool prepare)
>  {
>  	pci_pm_default_suspend_generic(pci_dev);
>  
> -	if (!pci_is_bridge(pci_dev))
> +	if (prepare && !pci_is_bridge(pci_dev))
>  		pci_prepare_to_sleep(pci_dev);
>  
>  	pci_fixup_device(pci_fixup_suspend, pci_dev);

This "helper" function really isn't helping anything at all any more. It's 
really just confusing things.

Now that was true even before this all; mostly because your naming in this 
area _really_ sucks. I mean, what the heck is the difference between 
"pci_pm_default_suspend_generic()" and "pci_pm_default_suspend()", and 
what do they do?

But you just made it worse. This trivial function that doesn't do anything 
interesting, and isn't well-named enough to actually explain what it is 
doing now became EVEN WORSE.  Now it's a trivial function that does two 
things, except it does one of those things only if the magic flag (that is 
also not helpfully named) is set.

Argh.

To make it worse, it's not at all obvious what the logic is:

> +	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +		pci_pm_default_suspend(pci_dev, !!pm);

Whaa? This is basically totally obfuscated code both in the caller _and_ 
in the callee.

Now, it looks like this all then goes away in PATCH 7/7, so I guess I 
shouldn't complain too much, but I just don't see much point in carrying 
this broken patch around in the series, since it's then going away and 
rewritten almost immediately again.

Apart from that complaints, Acked-by: for the series. 

			Linus

Re: [PATCH 1/7] PCI PM: Fix handling of devices without drivers

[Benjamin Herrenschmidt]

On Tue, 2009-02-03 at 18:23 -0800, Linus Torvalds wrote:
> Whaa? This is basically totally obfuscated code both in the caller _and_ 
> in the callee.
> 
> Now, it looks like this all then goes away in PATCH 7/7, so I guess I 
> shouldn't complain too much, but I just don't see much point in carrying 
> this broken patch around in the series, since it's then going away and 
> rewritten almost immediately again.
> 
> Apart from that complaints, Acked-by: for the series. 

Same comments. It took me a while to figure the logic out, which I end
up agreeing with but the code is pretty obfuscated ... And yes, patch 7
seems to improve things, but then, that makes reading the patch itself
nearly impossible :-) I ended up patching the file and -then- reading
it.

I tried the whole thing on various powerbooks, along with some patches
I'm about to send for radeonfb, atyfb and aty128fb to make them properly
call pci_save_state() before they hand-change the D state, and things
appear to be working fine. (well, appart from some unrelated problems
leading to some might_sleep() hits but as I said, they are unrelated to
this series)

I need a couple more tests of the fbdev patches on some machine variants
and once done, I'll post them out. Hopefully tonight.

Cheers,
Ben.

Re: [PATCH 3/7] PCI PM: Fix saving of device state in pci_legacy_suspend

[Frans Pop]

Benjamin Herrenschmidt wrote:
> Minor nit: Should the warning be preceeded by a message ? The reason is,
> right now, all we get is a backtrace, it doesn't actually tell you which
> device or driver caused it which makes it pretty pointless.
> 
> I think you should add a printk(KERN_ERR... just before that which gives
> those informations along with a little blurb along the lines of "driver
> changed device state without saving config space state").

That is actually already included in the patch (through drv->suspend):

> -             if (WARN_ON_ONCE(pci_dev->current_state != PCI_D0))
> +             if (pci_dev->current_state != PCI_D0
> +                 && pci_dev->current_state != PCI_UNKNOWN) {
> +                     WARN_ONCE(pci_dev->current_state != prev,
> +                             "PCI PM: Device state not saved by %pF\n",
> +                             drv->suspend);

Cheers,
FJP

Re: [PATCH 1/7] PCI PM: Fix handling of devices without drivers

[Rafael J. Wysocki]

On Wednesday 04 February 2009, Linus Torvalds wrote:
> 
> On Wed, 4 Feb 2009, Rafael J. Wysocki wrote:
> > 
> > Suspend to RAM is reported to break on some machines as a result of
> > attempting to put one of driverless PCI devices into a low power
> > state.  Avoid that by not attepmting to power manage driverless
> > devices during suspend.
> > 
> > Fix up pci_pm_poweroff() after a previous incomplete fix for the same
> > thing during hibernation.
> 
> Ok, I really don't like this patch, because:
> 
> > -static void pci_pm_default_suspend(struct pci_dev *pci_dev)
> > +static void pci_pm_default_suspend(struct pci_dev *pci_dev, bool prepare)
> >  {
> >  	pci_pm_default_suspend_generic(pci_dev);
> >  
> > -	if (!pci_is_bridge(pci_dev))
> > +	if (prepare && !pci_is_bridge(pci_dev))
> >  		pci_prepare_to_sleep(pci_dev);
> >  
> >  	pci_fixup_device(pci_fixup_suspend, pci_dev);
> 
> This "helper" function really isn't helping anything at all any more. It's 
> really just confusing things.
> 
> Now that was true even before this all; mostly because your naming in this 
> area _really_ sucks. I mean, what the heck is the difference between 
> "pci_pm_default_suspend_generic()" and "pci_pm_default_suspend()", and 
> what do they do?
> 
> But you just made it worse. This trivial function that doesn't do anything 
> interesting, and isn't well-named enough to actually explain what it is 
> doing now became EVEN WORSE.  Now it's a trivial function that does two 
> things, except it does one of those things only if the magic flag (that is 
> also not helpfully named) is set.
> 
> Argh.
> 
> To make it worse, it's not at all obvious what the logic is:
> 
> > +	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > +		pci_pm_default_suspend(pci_dev, !!pm);
> 
> Whaa? This is basically totally obfuscated code both in the caller _and_ 
> in the callee.
> 
> Now, it looks like this all then goes away in PATCH 7/7, so I guess I 
> shouldn't complain too much, but I just don't see much point in carrying 
> this broken patch around in the series, since it's then going away and 
> rewritten almost immediately again.

This is what has been tested by the bug reporter.

OK, I should have created a more sophisticated version, but that would go
away with patch 7/7 too, so ...

> Apart from that complaints, Acked-by: for the series. 

Thanks!

Rafael

Re: [PATCH 1/7] PCI PM: Fix handling of devices without drivers

[Rafael J. Wysocki]

On Wednesday 04 February 2009, Benjamin Herrenschmidt wrote:
> On Tue, 2009-02-03 at 18:23 -0800, Linus Torvalds wrote:
> > Whaa? This is basically totally obfuscated code both in the caller _and_ 
> > in the callee.
> > 
> > Now, it looks like this all then goes away in PATCH 7/7, so I guess I 
> > shouldn't complain too much, but I just don't see much point in carrying 
> > this broken patch around in the series, since it's then going away and 
> > rewritten almost immediately again.
> > 
> > Apart from that complaints, Acked-by: for the series. 
> 
> Same comments. It took me a while to figure the logic out, which I end
> up agreeing with but the code is pretty obfuscated ... And yes, patch 7
> seems to improve things, but then, that makes reading the patch itself
> nearly impossible :-) I ended up patching the file and -then- reading
> it.

Well, yeah, sorry.  I should have been more careful.

> I tried the whole thing on various powerbooks, along with some patches
> I'm about to send for radeonfb, atyfb and aty128fb to make them properly
> call pci_save_state() before they hand-change the D state, and things
> appear to be working fine. (well, appart from some unrelated problems
> leading to some might_sleep() hits but as I said, they are unrelated to
> this series)
> 
> I need a couple more tests of the fbdev patches on some machine variants
> and once done, I'll post them out. Hopefully tonight.

Thanks a lot for your help!

Best,
Rafael

Re: [PATCH 3/7] PCI PM: Fix saving of device state in pci_legacy_suspend

[Benjamin Herrenschmidt]

On Wed, 2009-02-04 at 11:14 +0100, Frans Pop wrote:
> Benjamin Herrenschmidt wrote:
> > Minor nit: Should the warning be preceeded by a message ? The reason is,
> > right now, all we get is a backtrace, it doesn't actually tell you which
> > device or driver caused it which makes it pretty pointless.
> > 
> > I think you should add a printk(KERN_ERR... just before that which gives
> > those informations along with a little blurb along the lines of "driver
> > changed device state without saving config space state").
> 
> That is actually already included in the patch (through drv->suspend):

How so ? I've had such backtraces with no other info in my log...

Ben.

Re: [PATCH 3/7] PCI PM: Fix saving of device state in pci_legacy_suspend

[Frans Pop]

On Wednesday 04 February 2009, Benjamin Herrenschmidt wrote:
> On Wed, 2009-02-04 at 11:14 +0100, Frans Pop wrote:
> > Benjamin Herrenschmidt wrote:
> > > Minor nit: Should the warning be preceeded by a message ? The
> > > reason is, right now, all we get is a backtrace, it doesn't
> > > actually tell you which device or driver caused it which makes it
> > > pretty pointless.
> > >
> > > I think you should add a printk(KERN_ERR... just before that which
> > > gives those informations along with a little blurb along the lines
> > > of "driver changed device state without saving config space
> > > state").
> >
> > That is actually already included in the patch (through
> > drv->suspend):
>
> How so ? I've had such backtraces with no other info in my log...

Previous versions did not have the info, but with this patch I get:
WARNING: at drivers/pci/pci-driver.c:374 pci_legacy_suspend+0x9b/0xde()
Hardware name: HP Compaq 2510p Notebook PC
PCI PM: Device state not saved by iwl_pci_suspend+0x0/0x4c [iwlagn]
[...]

Driver and function causing the warning are in the last line.

Cheers,
FJP

Re: [PATCH 3/7] PCI PM: Fix saving of device state in pci_legacy_suspend

[Linus Torvalds]

On Thu, 5 Feb 2009, Benjamin Herrenschmidt wrote:
> 
> How so ? I've had such backtraces with no other info in my log...

_with_ that patch?

It very much replaces a warning with no further info with

+                       WARN_ONCE(pci_dev->current_state != prev,
+                               "PCI PM: Device state not saved by %pF\n",
+                               drv->suspend);

And notice the "%pF" and the 'drv->suspend': it should tell you exactly 
which suspend routine it was that messed up.

		Linus

Re: [PATCH 3/7] PCI PM: Fix saving of device state in pci_legacy_suspend

[Benjamin Herrenschmidt]

On Wed, 2009-02-04 at 13:56 -0800, Linus Torvalds wrote:
> 
> On Thu, 5 Feb 2009, Benjamin Herrenschmidt wrote:
> > 
> > How so ? I've had such backtraces with no other info in my log...
> 
> _with_ that patch?
> 
> It very much replaces a warning with no further info with
> 
> +                       WARN_ONCE(pci_dev->current_state != prev,
> +                               "PCI PM: Device state not saved by %pF\n",
> +                               drv->suspend);
> 
> And notice the "%pF" and the 'drv->suspend': it should tell you exactly 
> which suspend routine it was that messed up.

Yup, missed that change in the patch. In fact, I even missed the
existence of a WARN_ONCE that takes those text arguments... baaaah. I
suppose I should try to read more of lkml :-)

Cheers,
Ben.

Re: [PATCH 7/7] PCI PM: Let the core be more careful with respect to drivers using new framework

[Rafael J. Wysocki]

On Wednesday 04 February 2009, Benjamin Herrenschmidt wrote:
> 
> > For non-bridge devices without drivers the PCI PM core will disable
> > them and save their state on suspend.  During resume, it will put
> > them into D0, if necessary, restore their state (early) and reenable
> > them.
> 
> This is "without drivers" only or does it include devices that have a
> driver and no PM ops ?

It includes devices with drivers and without PM ops (either "legacy" or "new"),
the idea being that in the simplest cases the driver should not be required to
implement PM ops and the device should be handled nevertheless.

> In the later case, the worry is that the driver will potentially still
> take shared interrupts after you have disabled the device. I know the
> normal disable path only disables bus mastering, which is a GOOD
> thing :-) but the pcibios hook might do more here ...

I don't know about ppc, but on x86 the pcibios hook is only supposed to
disable the device's interrupt link.

> I would recommend only doing that disabling in the "noirq" phase of
> suspend to avoid problems here. Do you see any reason that wouldn't
> work ?

Potentially, calling into the AML interpreter.

When we implement the changes discussed in the "PCI PM: Restore standard
config registers of all devices early" thread, that part will end up in
suspend_noirq.

Thanks,
Rafael

Re: [PATCH 3/7] PCI PM: Fix saving of device state in pci_legacy_suspend

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

On Thu, 2009-02-05 at 11:40 +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2009-02-04 at 13:56 -0800, Linus Torvalds wrote:
> > 
> > On Thu, 5 Feb 2009, Benjamin Herrenschmidt wrote:
> > > 
> > > How so ? I've had such backtraces with no other info in my log...
> > 
> > _with_ that patch?
> > 
> > It very much replaces a warning with no further info with
> > 
> > +                       WARN_ONCE(pci_dev->current_state != prev,
> > +                               "PCI PM: Device state not saved by %pF\n",
> > +                               drv->suspend);
> > 
> > And notice the "%pF" and the 'drv->suspend': it should tell you exactly 
> > which suspend routine it was that messed up.
> 
> Yup, missed that change in the patch. In fact, I even missed the
> existence of a WARN_ONCE that takes those text arguments... baaaah. I
> suppose I should try to read more of lkml :-)

But don't expect to see them on powerpc, they never make it to the
console.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 7/7] PCI PM: Let the core be more careful with respect to drivers using new framework

[Benjamin Herrenschmidt]

> I don't know about ppc, but on x86 the pcibios hook is only supposed to
> disable the device's interrupt link.

Right now on ppc, I may disable the clocks too, but then:

 - As soon as you have implemented the new scheme, I'll replace all that
with using the same hooks as ACPI so that won't be an issue anymore

 - Today, it's probably a non issue too because none of the devices for
which I have that sort of manual clock control also shares interrupts.

> Potentially, calling into the AML interpreter.

Right, but we are fixing that anyway with the noirq phase being AML safe
by no ? :-)
 
> When we implement the changes discussed in the "PCI PM: Restore standard
> config registers of all devices early" thread, that part will end up in
> suspend_noirq.

Yup.

Cheers,
Ben.

Re: [PATCH 0/7] PCI PM: Fixes and refinements

[Jesse Barnes]

On Tuesday, February 3, 2009 4:54 pm Rafael J. Wysocki wrote:
> Hi,
>
> The following series of patches contains some fixes and refinements of the
> PCI PM framework.
>
> [1/7] fixes the problem that some driverless devices don't like to be power
> managed and break things if we attempt to do it.
>
> [2/7] and [3/7] fix the problem with devices in PCI_UNKNOWN that are
> mishandled by the new code.
>
> [4/7] fixes regression related to PCIe port suspend-resume.
>
> [5/7] fixes the bug that bridges (and PCIe ports) are disabled during
> suspend, althouth they shouldn't be.
>
> [6/7] makes pci_restore_standard_config() read the current power state of
> the device from the device itself after attempting to change it.
>
> [7/7] makes the PCI PM core handle devices more carefully (details in the
> changelog).
>
> Please disregard any previous versions of these patches I sent to you.

Ok, I pushed this series into my for-linus branch.  Aside from the minor 
issues in 7/7 that I already mentioned on irc things looked fine, and those 
were really just cleanups anyway.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/7] PCI PM: Fix saving of device state in pci_legacy_suspend

[Benjamin Herrenschmidt]

> > Yup, missed that change in the patch. In fact, I even missed the
> > existence of a WARN_ONCE that takes those text arguments... baaaah. I
> > suppose I should try to read more of lkml :-)
> 
> But don't expect to see them on powerpc, they never make it to the
> console.

Allright, that's the problem with our implementation of WARN using a
conditional trap, we don't carry all the variable arguments for the
printf over.

I'm tempted to turn our implementation back to normal C code with an if
and a branch out of line, but Michael seems to say that even with
appropriate use of unlikely() etc... gcc decides to generate the worst
possible code every single time ...

<rant>
such as always inlining the whole printf mumbo jumbo and having the
not-warning case branch over it, not -all- CPUs have good branch
prediction dude, so the icache is going to scream but gcc folks know
better of course....
</rant>

Anyway, I'll see Michael what exactly the situation is here and if we
can fix it a way or another.

Ben.

Re: [PATCH 0/7] PCI PM: Fixes and refinements

[Rafael J. Wysocki]

On Thursday 05 February 2009, Jesse Barnes wrote:
> On Tuesday, February 3, 2009 4:54 pm Rafael J. Wysocki wrote:
> > Hi,
> >
> > The following series of patches contains some fixes and refinements of the
> > PCI PM framework.
> >
> > [1/7] fixes the problem that some driverless devices don't like to be power
> > managed and break things if we attempt to do it.
> >
> > [2/7] and [3/7] fix the problem with devices in PCI_UNKNOWN that are
> > mishandled by the new code.
> >
> > [4/7] fixes regression related to PCIe port suspend-resume.
> >
> > [5/7] fixes the bug that bridges (and PCIe ports) are disabled during
> > suspend, althouth they shouldn't be.
> >
> > [6/7] makes pci_restore_standard_config() read the current power state of
> > the device from the device itself after attempting to change it.
> >
> > [7/7] makes the PCI PM core handle devices more carefully (details in the
> > changelog).
> >
> > Please disregard any previous versions of these patches I sent to you.
> 
> Ok, I pushed this series into my for-linus branch.  Aside from the minor 
> issues in 7/7 that I already mentioned on irc things looked fine, and those 
> were really just cleanups anyway.

Thanks a lot!

Rafael

Re: [PATCH 3/7] PCI PM: Fix saving of device state in pci_legacy_suspend

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 4055 bytes --]

On Thu, 2009-02-05 at 12:29 +1100, Benjamin Herrenschmidt wrote:
> > > Yup, missed that change in the patch. In fact, I even missed the
> > > existence of a WARN_ONCE that takes those text arguments... baaaah. I
> > > suppose I should try to read more of lkml :-)
> > 
> > But don't expect to see them on powerpc, they never make it to the
> > console.
> 
> Allright, that's the problem with our implementation of WARN using a
> conditional trap, we don't carry all the variable arguments for the
> printf over.
> 
> I'm tempted to turn our implementation back to normal C code with an if
> and a branch out of line, but Michael seems to say that even with
> appropriate use of unlikely() etc... gcc decides to generate the worst
> possible code every single time ...
> 
> <rant>
> such as always inlining the whole printf mumbo jumbo and having the
> not-warning case branch over it, not -all- CPUs have good branch
> prediction dude, so the icache is going to scream but gcc folks know
> better of course....
> </rant>
> 
> Anyway, I'll see Michael what exactly the situation is here and if we
> can fix it a way or another.

Well I'm glad you listened to me when I told you not to fire off a rant
email before I had another look at it ;)

Because Arjan has patched the generic bug.h so that we at least get the
printk on powerpc, and then use our version of __WARN().

So you should see the message at least.


As far as the code gen, with gcc 4.3.1 (debian).

WARN looks like:

#define WARN(condition, format...) ({                                           \
        int __ret_warn_on = !!(condition);                              \
        if (unlikely(__ret_warn_on))                                    \
                __WARN_printf(format);                                  \
        unlikely(__ret_warn_on);                                        \
})


Called from:

int smp_request_message_ipi(int virq, int msg)
{
        int err;
...
        err = request_irq(virq, smp_ipi_action[msg], IRQF_DISABLED|IRQF_PERCPU,
                          smp_ipi_name[msg], 0);
        WARN(err < 0, "unable to request_irq %d for %s (rc %d)\n",
                virq, smp_ipi_name[msg], err);

        return err;
}


Comes out as:

c00000000002c9e8:       48 0a 88 c9     bl      c0000000000d52b0 <.request_irq>
c00000000002c9ec:       60 00 00 00     nop
c00000000002c9f0:       2f 83 00 00     cmpwi   cr7,r3,0
c00000000002c9f4:       7c 7f 1b 78     mr      r31,r3
c00000000002c9f8:       40 bc 00 20     bge+    cr7,c00000000002ca18 <.smp_request_message_ipi+0x84>
c00000000002c9fc:       7c bc e8 2a     ldx     r5,r28,r29
c00000000002ca00:       e8 7e 80 d0     ld      r3,-32560(r30)
c00000000002ca04:       7f 64 db 78     mr      r4,r27
c00000000002ca08:       7f e6 fb 78     mr      r6,r31
c00000000002ca0c:       48 5f 53 7d     bl      c000000000621d88 <.printk>
c00000000002ca10:       60 00 00 00     nop     
c00000000002ca14:       0f e0 00 00     twi     31,r0,0
c00000000002ca18:       38 21 00 a0     addi    r1,r1,160
c00000000002ca1c:       7f e3 fb 78     mr      r3,r31
c00000000002ca20:       e8 01 00 10     ld      r0,16(r1)
c00000000002ca24:       eb 61 ff d8     ld      r27,-40(r1)
c00000000002ca28:       eb 81 ff e0     ld      r28,-32(r1)
c00000000002ca2c:       eb a1 ff e8     ld      r29,-24(r1)
c00000000002ca30:       eb c1 ff f0     ld      r30,-16(r1)
c00000000002ca34:       eb e1 ff f8     ld      r31,-8(r1)
c00000000002ca38:       7c 08 03 a6     mtlr    r0
c00000000002ca3c:       4e 80 00 20     blr    


So it's jumping over the if body, rather than moving it out of the
straight line. It is hinting it right, so we should check if that helps
us at all.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/7] PCI PM: Fix saving of device state in pci_legacy_suspend

[Benjamin Herrenschmidt]

> Well I'm glad you listened to me when I told you not to fire off a rant
> email before I had another look at it ;)
> 
> Because Arjan has patched the generic bug.h so that we at least get the
> printk on powerpc, and then use our version of __WARN().

Right. A deep breath later or two ... the current upstream code in
pci-driver.c doesn't have a message, which is why I didn't see it. The
-new- code from Rafael does, and it would work thanks to Arjan fix. Now
I should indeed stop listening to you telling me it's broken before I
get a chance to verify it :-)

> As far as the code gen, with gcc 4.3.1 (debian).

 .../...

> So it's jumping over the if body, rather than moving it out of the
> straight line. It is hinting it right, so we should check if that helps
> us at all.

Right, gcc is full of sh*t as usual. Now try replacing the magic trap
which we still use in there, with a normal C function call (ie branch
out of line), see if it makes a difference. The trap statement uses an
inline asm statement and I know for a fact that the mere existence of
such a thing will make gcc already dubious idea of whether to go out of
line or not completely bonkers.

Cheers,
Ben.