[PATCH 0/3] PCI PM: More refinements of suspend-resume framework

[PATCH 0/3] PCI PM: More refinements of suspend-resume framework

[Rafael J. Wysocki]

Hi,

After the recent discussion with Linus, I have the following three fixes of the
PCI PM framework.

The first patch fixes the bug that bridges (and PCIe ports) are disabled
during suspend, althouth they shouldn't.

The second one makes the PCI PM core handle devices more carefully (details
in the changelog). [Note to Linus: devices are still put into low power states
with interrupts on after this patch.  Moving that to the late suspend phase
will be the next step.]

The last patch makes the warning in pci_legacy_suspend() more useful.

Thanks,
Rafael

[PATCH 1/3] 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
@@ -428,16 +428,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.

Re: [PATCH 1/3] PCI PM: Do not disable and enable bridges during suspend-resume

[Pavel Machek]

Hi!

> their standard config spaces are restored.  Fix this by not attempting
> to disable bridges during suspend and enable them during resume.

...

> 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
> @@ -428,16 +428,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);
>  }

Are you sure? This goes from doing reenable_device to not doing it for
bridges, seemingly contradicting changelog?
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/3] PCI PM: Do not disable and enable bridges during suspend-resume

[Rafael J. Wysocki]

On Saturday 07 February 2009, Pavel Machek wrote:
> Hi!
> 
> > their standard config spaces are restored.  Fix this by not attempting
> > to disable bridges during suspend and enable them during resume.
> 
> ...
> 
> > 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
> > @@ -428,16 +428,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);
> >  }
> 
> Are you sure? This goes from doing reenable_device to not doing it for
> bridges, seemingly contradicting changelog?

Can you explain what you mean, please?

Thanks,
Rafael

Re: [PATCH 1/3] PCI PM: Do not disable and enable bridges during suspend-resume

[Pavel Machek]

Hi!

> > > their standard config spaces are restored.  Fix this by not attempting
> > > to disable bridges during suspend and enable them during resume.
> > ...
> > > @@ -428,16 +428,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);
> > >  }
> > 
> > Are you sure? This goes from doing reenable_device to not doing it for
> > bridges, seemingly contradicting changelog?
> 
> Can you explain what you mean, please?

It looks to me like the patch does not match the changelog.

Changelog says "enable bridge during resume", but code does return 0
if it seems bridge.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/3] PCI PM: Do not disable and enable bridges during suspend-resume

[Rafael J. Wysocki]

On Sunday 08 February 2009, Pavel Machek wrote:
> Hi!
> 
> > > > their standard config spaces are restored.  Fix this by not attempting
> > > > to disable bridges during suspend and enable them during resume.
> > > ...
> > > > @@ -428,16 +428,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);
> > > >  }
> > > 
> > > Are you sure? This goes from doing reenable_device to not doing it for
> > > bridges, seemingly contradicting changelog?
> > 
> > Can you explain what you mean, please?
> 
> It looks to me like the patch does not match the changelog.
> 
> Changelog says "enable bridge during resume", but code does return 0
> if it seems bridge.

Ah, I see.  The changelog was supposed to mean that the bridges would not
be disabled during suspend and also would not be enabled during resume
(which shouldn't be necessary, since they would be automatically enabled
as a result of restoring their configuration registers).

Thanks,
Rafael


> 									Pavel


-- 
Everyone knows that debugging is twice as hard as writing a program
in the first place.  So if you're as clever as you can be when you write it,
how will you ever debug it? --- Brian Kernighan

Re: [PATCH 1/3] PCI PM: Do not disable and enable bridges during suspend-resume

[Pavel Machek]

> On Sunday 08 February 2009, Pavel Machek wrote:
> > Hi!
> > 
> > > > > their standard config spaces are restored.  Fix this by not attempting
> > > > > to disable bridges during suspend and enable them during resume.
> > > > ...
> > > > > @@ -428,16 +428,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);
> > > > >  }
> > > > 
> > > > Are you sure? This goes from doing reenable_device to not doing it for
> > > > bridges, seemingly contradicting changelog?
> > > 
> > > Can you explain what you mean, please?
> > 
> > It looks to me like the patch does not match the changelog.
> > 
> > Changelog says "enable bridge during resume", but code does return 0
> > if it seems bridge.
> 
> Ah, I see.  The changelog was supposed to mean that the bridges would not
> be disabled during suspend and also would not be enabled during resume
> (which shouldn't be necessary, since they would be automatically enabled
> as a result of restoring their configuration registers).

You can add my acked-by: when you resubmit with fixed
changelog... :-).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/3] PCI PM: Do not disable and enable bridges during suspend-resume

[Rafael J. Wysocki]

On Monday 09 February 2009, Pavel Machek wrote:
> > On Sunday 08 February 2009, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > > > their standard config spaces are restored.  Fix this by not attempting
> > > > > > to disable bridges during suspend and enable them during resume.
> > > > > ...
> > > > > > @@ -428,16 +428,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);
> > > > > >  }
> > > > > 
> > > > > Are you sure? This goes from doing reenable_device to not doing it for
> > > > > bridges, seemingly contradicting changelog?
> > > > 
> > > > Can you explain what you mean, please?
> > > 
> > > It looks to me like the patch does not match the changelog.
> > > 
> > > Changelog says "enable bridge during resume", but code does return 0
> > > if it seems bridge.
> > 
> > Ah, I see.  The changelog was supposed to mean that the bridges would not
> > be disabled during suspend and also would not be enabled during resume
> > (which shouldn't be necessary, since they would be automatically enabled
> > as a result of restoring their configuration registers).
> 
> You can add my acked-by: when you resubmit with fixed
> changelog... :-).

It's merged, so that wouldn't be of any use.

Thanks,
Rafael

[PATCH 2/3] 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 attepmt 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 without 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.

For all devices the core will disable wake-up during resume.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |  138 ++++++++++++++++++++++++++++-------------------
 1 file changed, 85 insertions(+), 53 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,7 +370,6 @@ static int pci_legacy_suspend(struct dev
 	}
 
 	pci_save_state(pci_dev);
-	pci_dev->state_saved = true;
 	/*
 	 * This is for compatibility with existing code with legacy PM support.
 	 */
@@ -424,39 +423,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;
@@ -500,20 +482,40 @@ 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) {
+		int error;
+
 		error = pm->suspend(dev);
 		suspend_report_result(pm->suspend, error);
+		if (error)
+			return error;
+		if (!pci_dev->state_saved)
+			WARN_ONCE(pci_dev->current_state != PCI_D0,
+				"PCI PM: State of device not saved by %pF\n",
+				pm->suspend);
 	}
 
-	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)
@@ -556,7 +558,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;
 
 	/*
@@ -569,12 +571,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 */
@@ -591,21 +597,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)
@@ -648,16 +664,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;
 }
@@ -671,13 +689,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;
 }
@@ -718,7 +746,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;
 
 	/*
@@ -731,10 +759,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: [linux-pm] [PATCH 2/3] PCI PM: Let the core be more careful with respect to drivers using new framework

[Nigel Cunningham]

Hi Rafael.

Just a couple of typos in the comment:

On Sun, 2009-02-01 at 22:33 +0100, Rafael J. Wysocki wrote:
> 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 attepmt to do anything else

s/attepmt/attempt/

> 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 without drivers the PCI PM core will only save

s/without/with/

> 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.
> 
> For all devices the core will disable wake-up during resume.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

I'm not pretending to understand the gory details of the code itself,
and so don't want you to add a Reviewed-by or such like for me, thanks.

Regards,

Nigel

Re: [linux-pm] [PATCH 2/3] PCI PM: Let the core be more careful with respect to drivers using new framework

[Rafael J. Wysocki]

On Monday 02 February 2009, Nigel Cunningham wrote:
> Hi Rafael.
> 
> Just a couple of typos in the comment:
> 
> On Sun, 2009-02-01 at 22:33 +0100, Rafael J. Wysocki wrote:
> > 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 attepmt to do anything else
> 
> s/attepmt/attempt/

OK

> > 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 without drivers the PCI PM core will only save
> 
> s/without/with/

Ouch, thanks.

> > 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.
> > 
> > For all devices the core will disable wake-up during resume.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> I'm not pretending to understand the gory details of the code itself,
> and so don't want you to add a Reviewed-by or such like for me, thanks.

Thanks for the fixes, I'll resend the patch with updated changelog.

Best,
Rafael

Re: [linux-pm] [PATCH 2/3] PCI PM: Let the core be more careful with respect to drivers using new framework

[Rafael J. Wysocki]

On Monday 02 February 2009, Rafael J. Wysocki wrote:
> On Monday 02 February 2009, Nigel Cunningham wrote:
> > Hi Rafael.
> > 
> > Just a couple of typos in the comment:
> > 
> > On Sun, 2009-02-01 at 22:33 +0100, Rafael J. Wysocki wrote:
> > > 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 attepmt to do anything else
> > 
> > s/attepmt/attempt/
> 
> OK
> 
> > > 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 without drivers the PCI PM core will only save
> > 
> > s/without/with/
> 
> Ouch, thanks.
> 
> > > 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.
> > > 
> > > For all devices the core will disable wake-up during resume.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > I'm not pretending to understand the gory details of the code itself,
> > and so don't want you to add a Reviewed-by or such like for me, thanks.
> 
> Thanks for the fixes, I'll resend the patch with updated changelog.

Appended.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PCI PM: Let the core be more careful with respect to drivers using new framework

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.

For all devices the core will disable wake-up during resume.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |  138 ++++++++++++++++++++++++++++-------------------
 1 file changed, 85 insertions(+), 53 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,7 +370,6 @@ static int pci_legacy_suspend(struct dev
 	}
 
 	pci_save_state(pci_dev);
-	pci_dev->state_saved = true;
 	/*
 	 * This is for compatibility with existing code with legacy PM support.
 	 */
@@ -424,39 +423,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;
@@ -500,20 +482,40 @@ 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) {
+		int error;
+
 		error = pm->suspend(dev);
 		suspend_report_result(pm->suspend, error);
+		if (error)
+			return error;
+		if (!pci_dev->state_saved)
+			WARN_ONCE(pci_dev->current_state != PCI_D0,
+				"PCI PM: State of device not saved by %pF\n",
+				pm->suspend);
 	}
 
-	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)
@@ -556,7 +558,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;
 
 	/*
@@ -569,12 +571,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 */
@@ -591,21 +597,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)
@@ -648,16 +664,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;
 }
@@ -671,13 +689,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;
 }
@@ -718,7 +746,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;
 
 	/*
@@ -731,10 +759,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;
 }

[PATCH 3/3] PCI PM: Make warning in pci_legacy_suspend more useful

[Rafael J. Wysocki]

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

The warning in pci_legacy_suspend() would be much more useful if it
printed the name of the function that did the wrong thing.  Make it
do so.

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

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
@@ -365,7 +365,9 @@ 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 (WARN_ONCE(pci_dev->current_state != PCI_D0,
+				"PCI PM: Device state not saved by %pF\n",
+				drv->suspend))
 			goto Fixup;
 	}
 

Re: [linux-pm] [PATCH 3/3] PCI PM: Make warning in pci_legacy_suspend more useful

[Nigel Cunningham]

Hi again.

On Sun, 2009-02-01 at 22:34 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The warning in pci_legacy_suspend() would be much more useful if it
> printed the name of the function that did the wrong thing.  Make it
> do so.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/pci/pci-driver.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 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
> @@ -365,7 +365,9 @@ 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 (WARN_ONCE(pci_dev->current_state != PCI_D0,
> +				"PCI PM: Device state not saved by %pF\n",
> +				drv->suspend))
>  			goto Fixup;
>  	}

Am I right in thinking that WARN_ONCE will only warn about the first
driver that has the problem? If so, wouldn't it be better to make it
warn about all drivers that have the problem, but only once per device?

Regards,

Nigel

Re: [linux-pm] [PATCH 3/3] PCI PM: Make warning in pci_legacy_suspend more useful

[Rafael J. Wysocki]

On Monday 02 February 2009, Nigel Cunningham wrote:
> Hi again.
> 
> On Sun, 2009-02-01 at 22:34 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The warning in pci_legacy_suspend() would be much more useful if it
> > printed the name of the function that did the wrong thing.  Make it
> > do so.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/pci/pci-driver.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > 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
> > @@ -365,7 +365,9 @@ 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 (WARN_ONCE(pci_dev->current_state != PCI_D0,
> > +				"PCI PM: Device state not saved by %pF\n",
> > +				drv->suspend))
> >  			goto Fixup;
> >  	}
> 
> Am I right in thinking that WARN_ONCE will only warn about the first
> driver that has the problem? If so, wouldn't it be better to make it
> warn about all drivers that have the problem, but only once per device?

That would be better, but I don't think we are going to see many of these
anyway. :-)

Thanks,
Rafael

Re: [PATCH 0/3] PCI PM: More refinements of suspend-resume framework

[Jesse Barnes]

On Sunday, February 1, 2009 1:29 pm Rafael J. Wysocki wrote:
> Hi,
>
> After the recent discussion with Linus, I have the following three fixes of
> the PCI PM framework.
>
> The first patch fixes the bug that bridges (and PCIe ports) are disabled
> during suspend, althouth they shouldn't.
>
> The second one makes the PCI PM core handle devices more carefully (details
> in the changelog). [Note to Linus: devices are still put into low power
> states with interrupts on after this patch.  Moving that to the late
> suspend phase will be the next step.]
>
> The last patch makes the warning in pci_legacy_suspend() more useful.

Linus, do you want these as part of the next pull request?  If so, I'll check 
them out, pull them in, and let them run the linux-next build gauntlet for a 
day before sending them.  Otherwise I'll just send a pull request today with 
the 8 or so fixes I have queued now.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 0/3] PCI PM: More refinements of suspend-resume framework

[Linus Torvalds]

On Mon, 2 Feb 2009, Jesse Barnes wrote:
> >
> > The last patch makes the warning in pci_legacy_suspend() more useful.
> 
> Linus, do you want these as part of the next pull request?  If so, I'll check 
> them out, pull them in, and let them run the linux-next build gauntlet for a 
> day before sending them.  Otherwise I'll just send a pull request today with 
> the 8 or so fixes I have queued now.

I want them eventually, but I don't know if we got confirmation that this 
particular series actually fixed the PCI-E bridge issue? The one that 
Parag reported.. It would be good to have confirmation. Maybe we do 
already and I missed that email?

		Linus

Re: [PATCH 0/3] PCI PM: More refinements of suspend-resume framework

[Rafael J. Wysocki]

On Tuesday 03 February 2009, Linus Torvalds wrote:
> 
> On Mon, 2 Feb 2009, Jesse Barnes wrote:
> > >
> > > The last patch makes the warning in pci_legacy_suspend() more useful.
> > 
> > Linus, do you want these as part of the next pull request?  If so, I'll check 
> > them out, pull them in, and let them run the linux-next build gauntlet for a 
> > day before sending them.  Otherwise I'll just send a pull request today with 
> > the 8 or so fixes I have queued now.
> 
> I want them eventually, but I don't know if we got confirmation that this 
> particular series actually fixed the PCI-E bridge issue? The one that 
> Parag reported.. It would be good to have confirmation. Maybe we do 
> already and I missed that email?

An additional patch against the PCIe port driver will be necessary to fix this
issue, but I'm waiting for feedback from Parag about the patches already
posted to find out what's the minimal fix.

Thanks,
Rafael