[PATCH 0/3] Fix hibernation regression on Toshiba Portege R500

[PATCH 0/3] Fix hibernation regression on Toshiba Portege R500

[Rafael J. Wysocki]

Hi,

The following three patches address the hibernation/suspend issue described in
http://bugzilla.kernel.org/show_bug.cgi?id=12121 and in the very long thread at
http://lkml.org/lkml/2008/12/1/382.

In short, the problem is that resume (from hibernation and/or suspend-to-RAM)
occasionally fails (approximately 20-25% of attempts) in the middle of resuming
PCI devices.  We were able to find a specific layout of devices within the
memory address space in which the failure appeared to be extremely unlikely,
but this layout was no really valid for other reasons.  We also found out that
using the NMI watchdog decreased the probablitily of failure which indicated
that the problem could be timing-related.

Next, we started to look at the PCI resume code and we generally agreed that
it would be a good idea to restore the standard PCI configuration registers
with interrupts disabled.  Also, we thought we could move the saving of those
registers for some devices into functions executed with interrupts disabled.

I have followed these observations and created the three following patches.
With all of these patches applied, I'm not able to reproduce the problem.

Thanks,
Rafael

[PATCH 1/3] PCI: Rework default handling of suspend and resume

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PCI: Rework default handling of suspend and resume

Rework the handling of suspend and resume of PCI devices which have
no drivers or the drivers of which do not provide any suspend-resume
callbacks in such a way that their standard PCI configuration
registers will be saved and restored with interrupts disabled.  This
should prevent such devices, including PCI bridges, from being
resumed too late to be able to function correctly during the resume
of the other PCI devices that may depend on them.

Also, to remove one possible source of future confusion, drop the
default handling of suspend and resume for PCI devices with drivers
providing the 'pm' object introduced by the new suspend-resume
framework (there are no such PCI drivers at the moment).

This patch addresses the regression from 2.6.26 tracked as
http://bugzilla.kernel.org/show_bug.cgi?id=12121 .

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |   90 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 59 insertions(+), 31 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,6 +300,14 @@ 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.
@@ -317,14 +325,22 @@ static void pci_default_pm_suspend(struc
 
 /*
  * Default "resume" method for devices that have no driver provided resume,
- * or not even a driver at all.
+ * or not even a driver at all (first part).
  */
-static int pci_default_pm_resume(struct pci_dev *pci_dev)
+static void pci_default_pm_resume_early(struct pci_dev *pci_dev)
 {
-	int retval = 0;
-
 	/* 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)
+{
+	int retval;
+
 	/* if the device was enabled before suspend, reenable */
 	retval = pci_reenable_device(pci_dev);
 	/*
@@ -371,10 +387,12 @@ static int pci_legacy_resume(struct devi
 	struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * drv = pci_dev->driver;
 
-	if (drv && drv->resume)
+	if (drv && drv->resume) {
 		error = drv->resume(pci_dev);
-	else
-		error = pci_default_pm_resume(pci_dev);
+	} else {
+		pci_default_pm_resume_early(pci_dev);
+		error = pci_default_pm_resume_late(pci_dev);
+	}
 	return error;
 }
 
@@ -420,10 +438,8 @@ static int pci_pm_suspend(struct device 
 		if (drv->pm->suspend) {
 			error = drv->pm->suspend(dev);
 			suspend_report_result(drv->pm->suspend, error);
-		} else {
-			pci_default_pm_suspend(pci_dev);
 		}
-	} else {
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
 	}
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
@@ -442,8 +458,10 @@ static int pci_pm_suspend_noirq(struct d
 			error = drv->pm->suspend_noirq(dev);
 			suspend_report_result(drv->pm->suspend_noirq, error);
 		}
-	} else {
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend_late(dev, PMSG_SUSPEND);
+	} else {
+		pci_default_pm_suspend(pci_dev);
 	}
 
 	return error;
@@ -453,15 +471,17 @@ static int pci_pm_resume(struct device *
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
-	int error;
+	int error = 0;
 
 	pci_fixup_device(pci_fixup_resume, pci_dev);
 
 	if (drv && drv->pm) {
-		error = drv->pm->resume ? drv->pm->resume(dev) :
-			pci_default_pm_resume(pci_dev);
-	} else {
+		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);
 	}
 
 	return error;
@@ -478,8 +498,10 @@ static int pci_pm_resume_noirq(struct de
 	if (drv && drv->pm) {
 		if (drv->pm->resume_noirq)
 			error = drv->pm->resume_noirq(dev);
-	} else {
+	} 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;
@@ -506,10 +528,8 @@ static int pci_pm_freeze(struct device *
 		if (drv->pm->freeze) {
 			error = drv->pm->freeze(dev);
 			suspend_report_result(drv->pm->freeze, error);
-		} else {
-			pci_default_pm_suspend(pci_dev);
 		}
-	} else {
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend(dev, PMSG_FREEZE);
 		pci_fixup_device(pci_fixup_suspend, pci_dev);
 	}
@@ -528,8 +548,10 @@ static int pci_pm_freeze_noirq(struct de
 			error = drv->pm->freeze_noirq(dev);
 			suspend_report_result(drv->pm->freeze_noirq, error);
 		}
-	} else {
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend_late(dev, PMSG_FREEZE);
+	} else {
+		pci_default_pm_suspend(pci_dev);
 	}
 
 	return error;
@@ -537,14 +559,15 @@ static int pci_pm_freeze_noirq(struct de
 
 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 (drv && drv->pm) {
 		if (drv->pm->thaw)
 			error =  drv->pm->thaw(dev);
-	} else {
-		pci_fixup_device(pci_fixup_resume, to_pci_dev(dev));
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
 		error = pci_legacy_resume(dev);
 	}
 
@@ -560,7 +583,7 @@ static int pci_pm_thaw_noirq(struct devi
 	if (drv && drv->pm) {
 		if (drv->pm->thaw_noirq)
 			error = drv->pm->thaw_noirq(dev);
-	} else {
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		pci_fixup_device(pci_fixup_resume_early, pci_dev);
 		error = pci_legacy_resume_early(dev);
 	}
@@ -570,17 +593,18 @@ static int pci_pm_thaw_noirq(struct devi
 
 static int pci_pm_poweroff(struct device *dev)
 {
+	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	pci_fixup_device(pci_fixup_suspend, to_pci_dev(dev));
+	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 {
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend(dev, PMSG_HIBERNATE);
 	}
 
@@ -598,7 +622,7 @@ static int pci_pm_poweroff_noirq(struct 
 			error = drv->pm->poweroff_noirq(dev);
 			suspend_report_result(drv->pm->poweroff_noirq, error);
 		}
-	} else {
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend_late(dev, PMSG_HIBERNATE);
 	}
 
@@ -609,13 +633,15 @@ static int pci_pm_restore(struct device 
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
-	int error;
+	int error = 0;
 
 	if (drv && drv->pm) {
-		error = drv->pm->restore ? drv->pm->restore(dev) :
-			pci_default_pm_resume(pci_dev);
-	} else {
+		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);
 	}
 	pci_fixup_device(pci_fixup_resume, pci_dev);
 
@@ -633,8 +659,10 @@ static int pci_pm_restore_noirq(struct d
 	if (drv && drv->pm) {
 		if (drv->pm->restore_noirq)
 			error = drv->pm->restore_noirq(dev);
-	} else {
+	} 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);
 

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Linus Torvalds]

On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> 
> Rework the handling of suspend and resume of PCI devices which have
> no drivers or the drivers of which do not provide any suspend-resume
> callbacks in such a way that their standard PCI configuration
> registers will be saved and restored with interrupts disabled.

Ok, I think this is good, but I _also_ think that we should do one more 
fix:

 - if a device uses the new-format suspend/resume structure, we should do 
   the low-level save-restore _unconditionally_ in the PCI layer.

Because apparently there is only a single user of the new format, and that 
single user got it wrong. So wouldn't it be much nicer to just _remove_ 
the code from the USB host controllers that does the save/restore thing.

Quite frankly, the USB code really does look wrong. Not just in that it 
enables the BAR's before restoring them, but on the suspend side it 
actually puts the device into D3_hot _before_ it then does the whole 
"pci_enable_wake()", which I'm not at all sure will necessarily work. I'm 
pretty sure that you should enable wakeup events _before_ going to sleep.

If the generic PCI layer unconditionally did the suspend as the last thing 
it does (and the resume as the first thing), then drivers couldn't do 
insane things like that, even by mistake.

Hmm?

				Linus

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Rafael J. Wysocki]

On Saturday, 6 of December 2008, Linus Torvalds wrote:
> 
> On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > 
> > Rework the handling of suspend and resume of PCI devices which have
> > no drivers or the drivers of which do not provide any suspend-resume
> > callbacks in such a way that their standard PCI configuration
> > registers will be saved and restored with interrupts disabled.
> 
> Ok, I think this is good, but I _also_ think that we should do one more 
> fix:
> 
>  - if a device uses the new-format suspend/resume structure, we should do 
>    the low-level save-restore _unconditionally_ in the PCI layer.
> 
> Because apparently there is only a single user of the new format, and that 
> single user got it wrong. So wouldn't it be much nicer to just _remove_ 
> the code from the USB host controllers that does the save/restore thing.

USB doesn't use that for PCI suspend-resume, it uses it for suspend-resume of
USB devices behind the controller.

> Quite frankly, the USB code really does look wrong. Not just in that it 
> enables the BAR's before restoring them, but on the suspend side it 
> actually puts the device into D3_hot _before_ it then does the whole 
> "pci_enable_wake()", which I'm not at all sure will necessarily work. I'm 
> pretty sure that you should enable wakeup events _before_ going to sleep.

Yeah.  Or simply use pci_prepare_to_sleep() and be done with it.

> If the generic PCI layer unconditionally did the suspend as the last thing 
> it does (and the resume as the first thing), then drivers couldn't do 
> insane things like that, even by mistake.
> 
> Hmm?

OK

But then we will save the device's registers in the "sleeping" state.  Is this
going to be entirely correct in all possible cases?  [pci_save_state() doesn't
save the PM registers, so that _should_ be correct, but I don't have _that_
much experience with these things.]

Rafael

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Linus Torvalds]

On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> 
> USB doesn't use that for PCI suspend-resume, it uses it for suspend-resume of
> USB devices behind the controller.

Oh, in that case there are no PCI users of this at all, and what the PCI 
driver does is immaterial ;)

> But then we will save the device's registers in the "sleeping" state. 

No no. The rule would be that a PCI driver - if it uses the new 
infrastructure, which apparently nobody does _as_ a PCI driver - simply 
would never do the whole "pci_set_power_state(PCI_D3hot)" etc crud AT ALL.

So a PCI driver would only do higher-level stuff in its suspend/resume 
code. For example, a USB host controller would initiate the USB bus level 
stuff, and likely just stop the controller (not suspend it - just stop 
it).

			Linus

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Rafael J. Wysocki]

On Saturday, 6 of December 2008, Linus Torvalds wrote:
> 
> On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > 
> > USB doesn't use that for PCI suspend-resume, it uses it for suspend-resume of
> > USB devices behind the controller.
> 
> Oh, in that case there are no PCI users of this at all, and what the PCI 
> driver does is immaterial ;)
> 
> > But then we will save the device's registers in the "sleeping" state. 
> 
> No no. The rule would be that a PCI driver - if it uses the new 
> infrastructure, which apparently nobody does _as_ a PCI driver - simply 
> would never do the whole "pci_set_power_state(PCI_D3hot)" etc crud AT ALL.

Now _that_ sounds good. :-)

> So a PCI driver would only do higher-level stuff in its suspend/resume 
> code. For example, a USB host controller would initiate the USB bus level 
> stuff, and likely just stop the controller (not suspend it - just stop 
> it).

I like this idea very much.

So, to fix the issue at hand, I'd like the $subject patch to go first.  Then,
there is a major update of the new framework waiting for .29 in the Greg's
tree (that's the main reason why nobody uses it so far, BTW) and I'd really
prefer it to go next.  After it's been merged, I'm going to add the mandatory
suspend-resume things (save state and go to a low power state on suspend,
restore state on resume) to the new framework in a separete patch.

Is this plan acceptable?

Rafael

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Linus Torvalds]

On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> 
> So, to fix the issue at hand, I'd like the $subject patch to go first.  Then,
> there is a major update of the new framework waiting for .29 in the Greg's
> tree (that's the main reason why nobody uses it so far, BTW) and I'd really
> prefer it to go next.  After it's been merged, I'm going to add the mandatory
> suspend-resume things (save state and go to a low power state on suspend,
> restore state on resume) to the new framework in a separete patch.
> 
> Is this plan acceptable?

Sounds good to me. And assuming Jesse/Greg are all aboard, I'll just wait 
for the pull requests from Jesse and Greg.

The only thing I'll do right now is to send off my "print out ICH6+ 
LPC resources" patch again to Jesse, with a changelog etc. It can probably 
go in as-is (it really just adds printk's), but since it didn't matter 
anyway we migth as well just do it as a PCI thing for 2.6.29 too.

On a similar note, I wonder what we should do about the whole "transparent 
bridge resource allocation" thing. It also didn't end up really mattering, 
even if it apparently made a difference for Frans. The question is just 
whether we would be better off with IO windows for transparent buses (the 
way we try to set things up now), or with a simpler PCI resource tree that 
just takes advantage of the transparency.

The bridge windows _may_ result in better PCI throughput behind such a 
bridge, so there is some argument for keeping them. On the other hand, 
transparent bridges aren't generally for high-performance stuff anyway, 
and one advantage of the transparency is the flexibility it allows (ie we 
don't _need_ to set up the static bridging windows).

I dunno. I wonder what Windows does. Following Windows in areas like this 
tends to have the advantage that it's what the firmware and the hardware 
has generally been tested with most. At the same time, I'm not sure this 
is necessarily a very bug-prone area for either firmware or hardware. If 
there's actual bridge bugs wrt the windows, I suspect such a bridge would 
be broken enough to be unusable regardless.

			Linus

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Rafael J. Wysocki]

On Saturday, 6 of December 2008, Linus Torvalds wrote:
> 
> On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > 
> > So, to fix the issue at hand, I'd like the $subject patch to go first.  Then,
> > there is a major update of the new framework waiting for .29 in the Greg's
> > tree (that's the main reason why nobody uses it so far, BTW) and I'd really
> > prefer it to go next.  After it's been merged, I'm going to add the mandatory
> > suspend-resume things (save state and go to a low power state on suspend,
> > restore state on resume) to the new framework in a separete patch.
> > 
> > Is this plan acceptable?
> 
> Sounds good to me. And assuming Jesse/Greg are all aboard, I'll just wait 
> for the pull requests from Jesse and Greg.
> 
> The only thing I'll do right now is to send off my "print out ICH6+ 
> LPC resources" patch again to Jesse, with a changelog etc. It can probably 
> go in as-is (it really just adds printk's), but since it didn't matter 
> anyway we migth as well just do it as a PCI thing for 2.6.29 too.
> 
> On a similar note, I wonder what we should do about the whole "transparent 
> bridge resource allocation" thing. It also didn't end up really mattering, 
> even if it apparently made a difference for Frans. The question is just 
> whether we would be better off with IO windows for transparent buses (the 
> way we try to set things up now), or with a simpler PCI resource tree that 
> just takes advantage of the transparency.
> 
> The bridge windows _may_ result in better PCI throughput behind such a 
> bridge, so there is some argument for keeping them. On the other hand, 
> transparent bridges aren't generally for high-performance stuff anyway, 
> and one advantage of the transparency is the flexibility it allows (ie we 
> don't _need_ to set up the static bridging windows).

The static bridging windows help understand the system topology a bit IMO,
because you can just look at /proc/iomem and see what resources are
behind the bridge.

> I dunno. I wonder what Windows does. Following Windows in areas like this 
> tends to have the advantage that it's what the firmware and the hardware 
> has generally been tested with most. At the same time, I'm not sure this 
> is necessarily a very bug-prone area for either firmware or hardware. If 
> there's actual bridge bugs wrt the windows, I suspect such a bridge would 
> be broken enough to be unusable regardless.

I think Intel people should be able to find out what Windows does in this
area.

Thanks,
Rafael

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Jesse Barnes]

On Saturday, December 6, 2008 10:00 am Linus Torvalds wrote:
> On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > So, to fix the issue at hand, I'd like the $subject patch to go first. 
> > Then, there is a major update of the new framework waiting for .29 in the
> > Greg's tree (that's the main reason why nobody uses it so far, BTW) and
> > I'd really prefer it to go next.  After it's been merged, I'm going to
> > add the mandatory suspend-resume things (save state and go to a low power
> > state on suspend, restore state on resume) to the new framework in a
> > separete patch.
> >
> > Is this plan acceptable?
>
> Sounds good to me. And assuming Jesse/Greg are all aboard, I'll just wait
> for the pull requests from Jesse and Greg.
>
> The only thing I'll do right now is to send off my "print out ICH6+
> LPC resources" patch again to Jesse, with a changelog etc. It can probably
> go in as-is (it really just adds printk's), but since it didn't matter
> anyway we migth as well just do it as a PCI thing for 2.6.29 too.

Ok, I applied the set (Rafael's 1-2 and your ICH patch) to my linux-next 
branch.  We should get a little build coverage this week at least, hopefully 
nothing breaks too badly.

> On a similar note, I wonder what we should do about the whole "transparent
> bridge resource allocation" thing. It also didn't end up really mattering,
> even if it apparently made a difference for Frans. The question is just
> whether we would be better off with IO windows for transparent buses (the
> way we try to set things up now), or with a simpler PCI resource tree that
> just takes advantage of the transparency.
>
> The bridge windows _may_ result in better PCI throughput behind such a
> bridge, so there is some argument for keeping them. On the other hand,
> transparent bridges aren't generally for high-performance stuff anyway,
> and one advantage of the transparency is the flexibility it allows (ie we
> don't _need_ to set up the static bridging windows).
>
> I dunno. I wonder what Windows does. Following Windows in areas like this
> tends to have the advantage that it's what the firmware and the hardware
> has generally been tested with most. At the same time, I'm not sure this
> is necessarily a very bug-prone area for either firmware or hardware. If
> there's actual bridge bugs wrt the windows, I suspect such a bridge would
> be broken enough to be unusable regardless.

Just so happens that I'm working with some people internally on transparent 
bridge related issues atm, I'll see what I can dig up.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Greg KH]

On Sat, Dec 06, 2008 at 10:00:35AM -0800, Linus Torvalds wrote:
> 
> 
> On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > 
> > So, to fix the issue at hand, I'd like the $subject patch to go first.  Then,
> > there is a major update of the new framework waiting for .29 in the Greg's
> > tree (that's the main reason why nobody uses it so far, BTW) and I'd really
> > prefer it to go next.  After it's been merged, I'm going to add the mandatory
> > suspend-resume things (save state and go to a low power state on suspend,
> > restore state on resume) to the new framework in a separete patch.
> > 
> > Is this plan acceptable?
> 
> Sounds good to me. And assuming Jesse/Greg are all aboard, I'll just wait 
> for the pull requests from Jesse and Greg.

No objection from me, I'll wait for Jesse to "go first" in the .29 merge
window.

thanks,

greg k-h

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Rafael J. Wysocki]

On Sunday, 7 of December 2008, Greg KH wrote:
> On Sat, Dec 06, 2008 at 10:00:35AM -0800, Linus Torvalds wrote:
> > 
> > 
> > On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > > 
> > > So, to fix the issue at hand, I'd like the $subject patch to go first.  Then,
> > > there is a major update of the new framework waiting for .29 in the Greg's
> > > tree (that's the main reason why nobody uses it so far, BTW) and I'd really
> > > prefer it to go next.  After it's been merged, I'm going to add the mandatory
> > > suspend-resume things (save state and go to a low power state on suspend,
> > > restore state on resume) to the new framework in a separete patch.
> > > 
> > > Is this plan acceptable?
> > 
> > Sounds good to me. And assuming Jesse/Greg are all aboard, I'll just wait 
> > for the pull requests from Jesse and Greg.
> 
> No objection from me, I'll wait for Jesse to "go first" in the .29 merge
> window.

Unfortunately, the merge of the $subject patch with the one in your tree
results in code that doesn't compile.  Namely, some lines of code that the
$subject patch relies on are removed by the patch in your tree.

If there is no objection from Jesse and if you don't mind, I'll prepare a
version of the $subject patch on top of the patch in your tree and send it to
you.

Thanks,
Rafael

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Linus Torvalds]

On Sun, 7 Dec 2008, Rafael J. Wysocki wrote:
> 
> If there is no objection from Jesse and if you don't mind, I'll prepare a
> version of the $subject patch on top of the patch in your tree and send it to
> you.

Rafael: there's a bug in your 1/3 patch. 

It looks like "pci_save_state()" is currently unhappy with being called 
with interrupts disabled. Or at least "pci_save_pci[ex]_state()" state 
are. They both do a kzalloc(GFP_KERNEL).

So you should change that GFP_KERNEL into a GFP_ATOMIC. Or do something 
more complicated like pre-allocate them during early suspend, but just 
changing it to GFP_ATOMIC seems to be the trivial fix.

I'm not seeing any other issues with saving/restoring things with irq's 
disabled, but people should be on the lookout for details like this.

			Linus

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Greg KH]

On Sun, Dec 07, 2008 at 01:47:18PM +0100, Rafael J. Wysocki wrote:
> On Sunday, 7 of December 2008, Greg KH wrote:
> > On Sat, Dec 06, 2008 at 10:00:35AM -0800, Linus Torvalds wrote:
> > > 
> > > 
> > > On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > > > 
> > > > So, to fix the issue at hand, I'd like the $subject patch to go first.  Then,
> > > > there is a major update of the new framework waiting for .29 in the Greg's
> > > > tree (that's the main reason why nobody uses it so far, BTW) and I'd really
> > > > prefer it to go next.  After it's been merged, I'm going to add the mandatory
> > > > suspend-resume things (save state and go to a low power state on suspend,
> > > > restore state on resume) to the new framework in a separete patch.
> > > > 
> > > > Is this plan acceptable?
> > > 
> > > Sounds good to me. And assuming Jesse/Greg are all aboard, I'll just wait 
> > > for the pull requests from Jesse and Greg.
> > 
> > No objection from me, I'll wait for Jesse to "go first" in the .29 merge
> > window.
> 
> Unfortunately, the merge of the $subject patch with the one in your tree
> results in code that doesn't compile.  Namely, some lines of code that the
> $subject patch relies on are removed by the patch in your tree.
> 
> If there is no objection from Jesse and if you don't mind, I'll prepare a
> version of the $subject patch on top of the patch in your tree and send it to
> you.

I sure don't mind.

thanks,

greg k-h

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Rafael J. Wysocki]

On Sunday, 7 of December 2008, Linus Torvalds wrote:
> 
> On Sun, 7 Dec 2008, Rafael J. Wysocki wrote:
> > 
> > If there is no objection from Jesse and if you don't mind, I'll prepare a
> > version of the $subject patch on top of the patch in your tree and send it to
> > you.
> 
> Rafael: there's a bug in your 1/3 patch. 
> 
> It looks like "pci_save_state()" is currently unhappy with being called 
> with interrupts disabled. Or at least "pci_save_pci[ex]_state()" state 
> are. They both do a kzalloc(GFP_KERNEL).
> 
> So you should change that GFP_KERNEL into a GFP_ATOMIC. Or do something 
> more complicated like pre-allocate them during early suspend, but just 
> changing it to GFP_ATOMIC seems to be the trivial fix.
> 
> I'm not seeing any other issues with saving/restoring things with irq's 
> disabled, but people should be on the lookout for details like this.

I overlooked that, sorry.

What about doing the following in addition to patch [1/3]?

Rafael

---
 drivers/pci/pci.c   |   73 ++++++++++++++++++++++++++++++++++++----------------
 drivers/pci/pci.h   |    1 
 drivers/pci/probe.c |    3 ++
 3 files changed, 55 insertions(+), 22 deletions(-)

Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -958,6 +958,9 @@ static void pci_init_capabilities(struct
 	/* MSI/MSI-X list */
 	pci_msi_init_pci_dev(dev);
 
+	/* Buffers for saving PCIe and PCI-X capabilities */
+	pci_allocate_cap_save_buffers(dev);
+
 	/* Power Management */
 	pci_pm_init(dev);
 
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -41,6 +41,7 @@ struct pci_platform_pm_ops {
 
 extern int pci_set_platform_pm(struct pci_platform_pm_ops *ops);
 extern void pci_pm_init(struct pci_dev *dev);
+extern void pci_allocate_cap_save_buffers(struct pci_dev *dev);
 
 extern int pci_user_read_config_byte(struct pci_dev *dev, int where, u8 *val);
 extern int pci_user_read_config_word(struct pci_dev *dev, int where, u16 *val);
Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -640,19 +640,14 @@ static int pci_save_pcie_state(struct pc
 	int pos, i = 0;
 	struct pci_cap_saved_state *save_state;
 	u16 *cap;
-	int found = 0;
 
 	pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
 	if (pos <= 0)
 		return 0;
 
 	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
-	if (!save_state)
-		save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 4, GFP_KERNEL);
-	else
-		found = 1;
 	if (!save_state) {
-		dev_err(&dev->dev, "out of memory in pci_save_pcie_state\n");
+		dev_err(&dev->dev, "buffer not found in %s\n", __FUNCTION__);
 		return -ENOMEM;
 	}
 	cap = (u16 *)&save_state->data[0];
@@ -661,9 +656,7 @@ static int pci_save_pcie_state(struct pc
 	pci_read_config_word(dev, pos + PCI_EXP_LNKCTL, &cap[i++]);
 	pci_read_config_word(dev, pos + PCI_EXP_SLTCTL, &cap[i++]);
 	pci_read_config_word(dev, pos + PCI_EXP_RTCTL, &cap[i++]);
-	save_state->cap_nr = PCI_CAP_ID_EXP;
-	if (!found)
-		pci_add_saved_cap(dev, save_state);
+
 	return 0;
 }
 
@@ -688,30 +681,21 @@ static void pci_restore_pcie_state(struc
 
 static int pci_save_pcix_state(struct pci_dev *dev)
 {
-	int pos, i = 0;
+	int pos;
 	struct pci_cap_saved_state *save_state;
-	u16 *cap;
-	int found = 0;
 
 	pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
 	if (pos <= 0)
 		return 0;
 
 	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
-	if (!save_state)
-		save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
-	else
-		found = 1;
 	if (!save_state) {
-		dev_err(&dev->dev, "out of memory in pci_save_pcie_state\n");
+		dev_err(&dev->dev, "buffer not found in %s\n", __FUNCTION__);
 		return -ENOMEM;
 	}
-	cap = (u16 *)&save_state->data[0];
 
-	pci_read_config_word(dev, pos + PCI_X_CMD, &cap[i++]);
-	save_state->cap_nr = PCI_CAP_ID_PCIX;
-	if (!found)
-		pci_add_saved_cap(dev, save_state);
+	pci_read_config_word(dev, pos + PCI_X_CMD, (u16 *)save_state->data);
+
 	return 0;
 }
 
@@ -1301,6 +1285,51 @@ void pci_pm_init(struct pci_dev *dev)
 }
 
 /**
+ * pci_add_save_buffer - allocate buffer for saving given capability registers
+ * @dev: the PCI device
+ * @cap: the capability to allocate the buffer for
+ * @size: requested size of the buffer
+ */
+static int pci_add_cap_save_buffer(
+	struct pci_dev *dev, char cap, unsigned int size)
+{
+	int pos;
+	struct pci_cap_saved_state *save_state;
+
+	pos = pci_find_capability(dev, cap);
+	if (pos <= 0)
+		return 0;
+
+	save_state = kzalloc(sizeof(*save_state) + size, GFP_KERNEL);
+	if (!save_state)
+		return -ENOMEM;
+
+	save_state->cap_nr = cap;
+	pci_add_saved_cap(dev, save_state);
+
+	return 0;
+}
+
+/**
+ * pci_allocate_cap_save_buffers - allocate buffers for saving capabilities
+ * @dev: the PCI device
+ */
+void pci_allocate_cap_save_buffers(struct pci_dev *dev)
+{
+	int error;
+
+	error = pci_add_cap_save_buffer(dev, PCI_CAP_ID_EXP, 4 * sizeof(u16));
+	if (error)
+		dev_err(&dev->dev,
+			"unable to preallocate PCI Express save buffer\n");
+
+	error = pci_add_cap_save_buffer(dev, PCI_CAP_ID_PCIX, sizeof(u16));
+	if (error)
+		dev_err(&dev->dev,
+			"unable to preallocate PCI-X save buffer\n");
+}
+
+/**
  * pci_enable_ari - enable ARI forwarding if hardware support it
  * @dev: the PCI device
  */

[PATCH 1/3] PCI: Rework default handling of suspend and resume (rebased)

[Rafael J. Wysocki]

On Sunday, 7 of December 2008, Greg KH wrote:
> On Sun, Dec 07, 2008 at 01:47:18PM +0100, Rafael J. Wysocki wrote:
> > On Sunday, 7 of December 2008, Greg KH wrote:
> > > On Sat, Dec 06, 2008 at 10:00:35AM -0800, Linus Torvalds wrote:
> > > > 
> > > > 
> > > > On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > > > > 
> > > > > So, to fix the issue at hand, I'd like the $subject patch to go first.  Then,
> > > > > there is a major update of the new framework waiting for .29 in the Greg's
> > > > > tree (that's the main reason why nobody uses it so far, BTW) and I'd really
> > > > > prefer it to go next.  After it's been merged, I'm going to add the mandatory
> > > > > suspend-resume things (save state and go to a low power state on suspend,
> > > > > restore state on resume) to the new framework in a separete patch.
> > > > > 
> > > > > Is this plan acceptable?
> > > > 
> > > > Sounds good to me. And assuming Jesse/Greg are all aboard, I'll just wait 
> > > > for the pull requests from Jesse and Greg.
> > > 
> > > No objection from me, I'll wait for Jesse to "go first" in the .29 merge
> > > window.
> > 
> > Unfortunately, the merge of the $subject patch with the one in your tree
> > results in code that doesn't compile.  Namely, some lines of code that the
> > $subject patch relies on are removed by the patch in your tree.
> > 
> > If there is no objection from Jesse and if you don't mind, I'll prepare a
> > version of the $subject patch on top of the patch in your tree and send it to
> > you.
> 
> I sure don't mind.

OK, thanks.

So, please add the patch below to your tree (Jesse doesn't object).

The GFP_KERNEL issue noticed by Linus will be fixed by a separate patch going
through Jesse.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PCI: Rework default handling of suspend and resume

Rework the handling of suspend and resume of PCI devices which have
no drivers or the drivers of which do not provide any suspend-resume
callbacks in such a way that their standard PCI configuration
registers will be saved and restored with interrupts disabled.  This
should prevent such devices, including PCI bridges, from being
resumed too late to be able to function correctly during the resume
of the other PCI devices that may depend on them.

Also, to remove one possible source of future confusion, drop the
default handling of suspend and resume for PCI devices with drivers
providing the 'pm' object introduced by the new suspend-resume
framework (there are no such PCI drivers at the moment).

This patch addresses the regression from 2.6.26 tracked as
http://bugzilla.kernel.org/show_bug.cgi?id=12121 .

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |   94 +++++++++++++++++++++++++++++++----------------
 1 file changed, 63 insertions(+), 31 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,6 +300,14 @@ 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.
@@ -317,14 +325,22 @@ static void pci_default_pm_suspend(struc
 
 /*
  * Default "resume" method for devices that have no driver provided resume,
- * or not even a driver at all.
+ * or not even a driver at all (first part).
  */
-static int pci_default_pm_resume(struct pci_dev *pci_dev)
+static void pci_default_pm_resume_early(struct pci_dev *pci_dev)
 {
-	int retval = 0;
-
 	/* 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)
+{
+	int retval;
+
 	/* if the device was enabled before suspend, reenable */
 	retval = pci_reenable_device(pci_dev);
 	/*
@@ -371,10 +387,12 @@ static int pci_legacy_resume(struct devi
 	struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * drv = pci_dev->driver;
 
-	if (drv && drv->resume)
+	if (drv && drv->resume) {
 		error = drv->resume(pci_dev);
-	else
-		error = pci_default_pm_resume(pci_dev);
+	} else {
+		pci_default_pm_resume_early(pci_dev);
+		error = pci_default_pm_resume_late(pci_dev);
+	}
 	return error;
 }
 
@@ -420,10 +438,8 @@ static int pci_pm_suspend(struct device 
 		if (drv->pm->suspend) {
 			error = drv->pm->suspend(dev);
 			suspend_report_result(drv->pm->suspend, error);
-		} else {
-			pci_default_pm_suspend(pci_dev);
 		}
-	} else {
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend(dev, PMSG_SUSPEND);
 	}
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
@@ -433,6 +449,7 @@ static int pci_pm_suspend(struct device 
 
 static int pci_pm_suspend_noirq(struct device *dev)
 {
+	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
@@ -441,8 +458,10 @@ static int pci_pm_suspend_noirq(struct d
 			error = drv->pm->suspend_noirq(dev);
 			suspend_report_result(drv->pm->suspend_noirq, error);
 		}
-	} else {
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend_late(dev, PMSG_SUSPEND);
+	} else {
+		pci_default_pm_suspend(pci_dev);
 	}
 
 	return error;
@@ -452,15 +471,17 @@ static int pci_pm_resume(struct device *
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
-	int error;
+	int error = 0;
 
 	pci_fixup_device(pci_fixup_resume, pci_dev);
 
 	if (drv && drv->pm) {
-		error = drv->pm->resume ? drv->pm->resume(dev) :
-			pci_default_pm_resume(pci_dev);
-	} else {
+		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);
 	}
 
 	return error;
@@ -468,6 +489,7 @@ static int pci_pm_resume(struct device *
 
 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;
 
@@ -476,8 +498,10 @@ static int pci_pm_resume_noirq(struct de
 	if (drv && drv->pm) {
 		if (drv->pm->resume_noirq)
 			error = drv->pm->resume_noirq(dev);
-	} else {
+	} 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;
@@ -504,10 +528,8 @@ static int pci_pm_freeze(struct device *
 		if (drv->pm->freeze) {
 			error = drv->pm->freeze(dev);
 			suspend_report_result(drv->pm->freeze, error);
-		} else {
-			pci_default_pm_suspend(pci_dev);
 		}
-	} else {
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend(dev, PMSG_FREEZE);
 		pci_fixup_device(pci_fixup_suspend, pci_dev);
 	}
@@ -517,6 +539,7 @@ static int pci_pm_freeze(struct device *
 
 static int pci_pm_freeze_noirq(struct device *dev)
 {
+	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
@@ -525,8 +548,10 @@ static int pci_pm_freeze_noirq(struct de
 			error = drv->pm->freeze_noirq(dev);
 			suspend_report_result(drv->pm->freeze_noirq, error);
 		}
-	} else {
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend_late(dev, PMSG_FREEZE);
+	} else {
+		pci_default_pm_suspend(pci_dev);
 	}
 
 	return error;
@@ -534,14 +559,15 @@ static int pci_pm_freeze_noirq(struct de
 
 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 (drv && drv->pm) {
 		if (drv->pm->thaw)
 			error =  drv->pm->thaw(dev);
-	} else {
-		pci_fixup_device(pci_fixup_resume, to_pci_dev(dev));
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
+		pci_fixup_device(pci_fixup_resume, pci_dev);
 		error = pci_legacy_resume(dev);
 	}
 
@@ -550,13 +576,14 @@ static int pci_pm_thaw(struct device *de
 
 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 (drv && drv->pm) {
 		if (drv->pm->thaw_noirq)
 			error = drv->pm->thaw_noirq(dev);
-	} else {
+	} 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);
 	}
@@ -566,17 +593,18 @@ static int pci_pm_thaw_noirq(struct devi
 
 static int pci_pm_poweroff(struct device *dev)
 {
+	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	pci_fixup_device(pci_fixup_suspend, to_pci_dev(dev));
+	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 {
+	} else if (pci_has_legacy_pm_support(pci_dev)) {
 		error = pci_legacy_suspend(dev, PMSG_HIBERNATE);
 	}
 
@@ -593,7 +621,7 @@ static int pci_pm_poweroff_noirq(struct 
 			error = drv->pm->poweroff_noirq(dev);
 			suspend_report_result(drv->pm->poweroff_noirq, error);
 		}
-	} else {
+	} else if (pci_has_legacy_pm_support(to_pci_dev(dev))) {
 		error = pci_legacy_suspend_late(dev, PMSG_HIBERNATE);
 	}
 
@@ -604,13 +632,15 @@ static int pci_pm_restore(struct device 
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
-	int error;
+	int error = 0;
 
 	if (drv && drv->pm) {
-		error = drv->pm->restore ? drv->pm->restore(dev) :
-			pci_default_pm_resume(pci_dev);
-	} else {
+		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);
 	}
 	pci_fixup_device(pci_fixup_resume, pci_dev);
 
@@ -628,8 +658,10 @@ static int pci_pm_restore_noirq(struct d
 	if (drv && drv->pm) {
 		if (drv->pm->restore_noirq)
 			error = drv->pm->restore_noirq(dev);
-	} else {
+	} 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);
 

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Alan Stern]

On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:

> On Saturday, 6 of December 2008, Linus Torvalds wrote:
> > 
> > On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > > 
> > > USB doesn't use that for PCI suspend-resume, it uses it for suspend-resume of
> > > USB devices behind the controller.
> > 
> > Oh, in that case there are no PCI users of this at all, and what the PCI 
> > driver does is immaterial ;)
> > 
> > > But then we will save the device's registers in the "sleeping" state. 
> > 
> > No no. The rule would be that a PCI driver - if it uses the new 
> > infrastructure, which apparently nobody does _as_ a PCI driver - simply 
> > would never do the whole "pci_set_power_state(PCI_D3hot)" etc crud AT ALL.
> 
> Now _that_ sounds good. :-)
> 
> > So a PCI driver would only do higher-level stuff in its suspend/resume 
> > code. For example, a USB host controller would initiate the USB bus level 
> > stuff, and likely just stop the controller (not suspend it - just stop 
> > it).
> 
> I like this idea very much.

Rafael, I'd be happy to help with fixing up the USB PCI PM code.  At
this point I'm not sure exactly what's needed, though.  For instance,
is there any compelling reason to switch over to the new dev_pm_ops
approach?  And what should the correct sequence of calls be?

Alan Stern

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Alan Cox]

> prefer it to go next.  After it's been merged, I'm going to add the mandatory
> suspend-resume things (save state and go to a low power state on suspend,
> restore state on resume) to the new framework in a separete patch.
> 
> Is this plan acceptable?

I have at least two drivers I look after where if you put the device into
D3 you lost. We survive because on a successful suspend/resume sequence
the BIOS puts it back coming out of suspend but that means we must not
put those devices into D3 ourselves ever - including during a suspend
before we are 100% comitted to the suspend completing or reboot.

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Rafael J. Wysocki]

On Saturday, 6 of December 2008, Alan Cox wrote:
> > prefer it to go next.  After it's been merged, I'm going to add the mandatory
> > suspend-resume things (save state and go to a low power state on suspend,
> > restore state on resume) to the new framework in a separete patch.
> > 
> > Is this plan acceptable?
> 
> I have at least two drivers I look after where if you put the device into
> D3 you lost. We survive because on a successful suspend/resume sequence
> the BIOS puts it back coming out of suspend but that means we must not
> put those devices into D3 ourselves ever - including during a suspend
> before we are 100% comitted to the suspend completing or reboot.

We can mark them as devices not to put into D3.  There already is a
mechanism for that in place.

Thanks,
Rafael

[PATCH 2/3] PCI: Suspend and resume PCI Express ports with interrupts disabled

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PCI: Suspend and resume PCI Express ports with interrupts disabled

I don't see why the suspend and resume of PCI Express ports should be
handled with interrupts enabled and it may even lead to problems in
some situations.  For this reason, move the suspending and resuming
of PCI Express ports into ->suspend_late() and ->resume_early()
callbacks executed with interrupts disabled.

This patch addresses the regression from 2.6.26 tracked as
http://bugzilla.kernel.org/show_bug.cgi?id=12121 .

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pcie/portdrv_pci.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 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
@@ -50,7 +50,7 @@ static int pcie_portdrv_restore_config(s
 }
 
 #ifdef CONFIG_PM
-static int pcie_portdrv_suspend(struct pci_dev *dev, pm_message_t state)
+static int pcie_portdrv_suspend_late(struct pci_dev *dev, pm_message_t state)
 {
 	int ret = pcie_port_device_suspend(dev, state);
 
@@ -59,14 +59,14 @@ static int pcie_portdrv_suspend(struct p
 	return ret;
 }
 
-static int pcie_portdrv_resume(struct pci_dev *dev)
+static int pcie_portdrv_resume_early(struct pci_dev *dev)
 {
 	pcie_portdrv_restore_config(dev);
 	return pcie_port_device_resume(dev);
 }
 #else
-#define pcie_portdrv_suspend NULL
-#define pcie_portdrv_resume NULL
+#define pcie_portdrv_suspend_late NULL
+#define pcie_portdrv_resume_early NULL
 #endif
 
 /*
@@ -282,8 +282,8 @@ static struct pci_driver pcie_portdriver
 	.probe		= pcie_portdrv_probe,
 	.remove		= pcie_portdrv_remove,
 
-	.suspend	= pcie_portdrv_suspend,
-	.resume		= pcie_portdrv_resume,
+	.suspend_late	= pcie_portdrv_suspend_late,
+	.resume_early	= pcie_portdrv_resume_early,
 
 	.err_handler 	= &pcie_portdrv_err_handler,
 };

[PATCH 3/3] Sound (HDA Intel): Restore PCI configuration space with interrupts off

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: Sound (HDA Intel): Restore PCI configuration space with interrupts off

Move the restoration of the standard PCI configuration registers
in the snd_hda_intel driver to a ->resume_early() callback executed
with interrupts disabled, since doing that with interrupts enabled
may lead to problems in some cases.

This patch addresses the regression from 2.6.26 tracked as
http://bugzilla.kernel.org/show_bug.cgi?id=12121 .

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 sound/pci/hda/hda_intel.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Index: linux-2.6/sound/pci/hda/hda_intel.c
===================================================================
--- linux-2.6.orig/sound/pci/hda/hda_intel.c
+++ linux-2.6/sound/pci/hda/hda_intel.c
@@ -1951,13 +1951,16 @@ static int azx_suspend(struct pci_dev *p
 	return 0;
 }
 
+static int azx_resume_early(struct pci_dev *pci)
+{
+	return pci_restore_state(pci);
+}
+
 static int azx_resume(struct pci_dev *pci)
 {
 	struct snd_card *card = pci_get_drvdata(pci);
 	struct azx *chip = card->private_data;
 
-	pci_set_power_state(pci, PCI_D0);
-	pci_restore_state(pci);
 	if (pci_enable_device(pci) < 0) {
 		printk(KERN_ERR "hda-intel: pci_enable_device failed, "
 		       "disabling device\n");
@@ -2465,6 +2468,7 @@ static struct pci_driver driver = {
 	.remove = __devexit_p(azx_remove),
 #ifdef CONFIG_PM
 	.suspend = azx_suspend,
+	.resume_early = azx_resume_early,
 	.resume = azx_resume,
 #endif
 };

Re: [PATCH 2/3] PCI: Suspend and resume PCI Express ports with interrupts disabled

[Linus Torvalds]

On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> 
> I don't see why the suspend and resume of PCI Express ports should be
> handled with interrupts enabled and it may even lead to problems in
> some situations.

Absolutely. A PCI Express port is really just a PCI bridge, with some odd 
rules. We need to enable them early, exacly like regular PCI bridges, or 
we cannot walk the PCI bus hierarchy correctly.

Anyway, ack, ack, ack for the whole series.

			Linus

Re: [PATCH 2/3] PCI: Suspend and resume PCI Express ports with interrupts disabled

[Rafael J. Wysocki]

On Saturday, 6 of December 2008, Linus Torvalds wrote:
> 
> On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > 
> > I don't see why the suspend and resume of PCI Express ports should be
> > handled with interrupts enabled and it may even lead to problems in
> > some situations.
> 
> Absolutely. A PCI Express port is really just a PCI bridge, with some odd 
> rules. We need to enable them early, exacly like regular PCI bridges, or 
> we cannot walk the PCI bus hierarchy correctly.
> 
> Anyway, ack, ack, ack for the whole series.

Thanks! :-)

I think it should go through Jesse?

Rafael

Re: [PATCH 2/3] PCI: Suspend and resume PCI Express ports with interrupts disabled

[Linus Torvalds]

On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> 
> I think it should go through Jesse?

Probably correct. And we want it in -next, so that it can get some testing 
even before I open the merge window. Because I hope everybody realizes 
that there's no way we're doing this in 2.6.28, and we'll leave the broken 
and unreliable suspend.

Because afaik this is not a new bug (I tried to push a patch to do 
suspend_late/resume_early for the PCI code a _loong_ time ago, but it 
never got merged), and the only reason it showed up as a regression was 
almost certainly simply that we've always had this.

IOW, suspend/resume has always been dodgy wrt interrupts, and there's some 
luck involved. And your machine just happened to get unlucky.

I'd love to fix this in 2.6.28, but it's just not reasonable - it needs 
widespread testing with an early -rc merge. And if it turns out to fix a 
lot of machines, and there are no regressions, we can always back-port it 
later.

Jesse?

			Linus

Re: [PATCH 2/3] PCI: Suspend and resume PCI Express ports with interrupts disabled

[Rafael J. Wysocki]

On Saturday, 6 of December 2008, Linus Torvalds wrote:
> 
> On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > 
> > I think it should go through Jesse?
> 
> Probably correct. And we want it in -next, so that it can get some testing 
> even before I open the merge window. Because I hope everybody realizes 
> that there's no way we're doing this in 2.6.28, and we'll leave the broken 
> and unreliable suspend.
> 
> Because afaik this is not a new bug (I tried to push a patch to do 
> suspend_late/resume_early for the PCI code a _loong_ time ago, but it 
> never got merged), and the only reason it showed up as a regression was 
> almost certainly simply that we've always had this.
> 
> IOW, suspend/resume has always been dodgy wrt interrupts, and there's some 
> luck involved. And your machine just happened to get unlucky.
> 
> I'd love to fix this in 2.6.28, but it's just not reasonable - it needs 
> widespread testing with an early -rc merge. And if it turns out to fix a 
> lot of machines, and there are no regressions, we can always back-port it 
> later.

I agree.

Thanks,
Rafael

Re: [PATCH 2/3] PCI: Suspend and resume PCI Express ports with interrupts disabled

[Jesse Barnes]

On Saturday, December 6, 2008 9:46 am Rafael J. Wysocki wrote:
> On Saturday, 6 of December 2008, Linus Torvalds wrote:
> > On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > > I think it should go through Jesse?
> >
> > Probably correct. And we want it in -next, so that it can get some
> > testing even before I open the merge window. Because I hope everybody
> > realizes that there's no way we're doing this in 2.6.28, and we'll leave
> > the broken and unreliable suspend.
> >
> > Because afaik this is not a new bug (I tried to push a patch to do
> > suspend_late/resume_early for the PCI code a _loong_ time ago, but it
> > never got merged), and the only reason it showed up as a regression was
> > almost certainly simply that we've always had this.
> >
> > IOW, suspend/resume has always been dodgy wrt interrupts, and there's
> > some luck involved. And your machine just happened to get unlucky.
> >
> > I'd love to fix this in 2.6.28, but it's just not reasonable - it needs
> > widespread testing with an early -rc merge. And if it turns out to fix a
> > lot of machines, and there are no regressions, we can always back-port it
> > later.
>
> I agree.

I'll stuff it into my -next branch tonight.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/3] PCI: Suspend and resume PCI Express ports with interrupts disabled

[Rafael J. Wysocki]

On Sunday, 7 of December 2008, Jesse Barnes wrote:
> On Saturday, December 6, 2008 9:46 am Rafael J. Wysocki wrote:
> > On Saturday, 6 of December 2008, Linus Torvalds wrote:
> > > On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > > > I think it should go through Jesse?
> > >
> > > Probably correct. And we want it in -next, so that it can get some
> > > testing even before I open the merge window. Because I hope everybody
> > > realizes that there's no way we're doing this in 2.6.28, and we'll leave
> > > the broken and unreliable suspend.
> > >
> > > Because afaik this is not a new bug (I tried to push a patch to do
> > > suspend_late/resume_early for the PCI code a _loong_ time ago, but it
> > > never got merged), and the only reason it showed up as a regression was
> > > almost certainly simply that we've always had this.
> > >
> > > IOW, suspend/resume has always been dodgy wrt interrupts, and there's
> > > some luck involved. And your machine just happened to get unlucky.
> > >
> > > I'd love to fix this in 2.6.28, but it's just not reasonable - it needs
> > > widespread testing with an early -rc merge. And if it turns out to fix a
> > > lot of machines, and there are no regressions, we can always back-port it
> > > later.
> >
> > I agree.
> 
> I'll stuff it into my -next branch tonight.

Well, if the [1/3] patch goes into your tree as is, there will be a bad merge
conflict between your tree and the Greg's one.  I think it's better if I rebase
that patch on top of the Greg's tree and push it to him (please see my last
message to Greg, http://lkml.org/lkml/2008/12/7/58).

The $subject patch is safe to pick up, though.

Thanks,
Rafael

Re: [PATCH 0/3] Fix hibernation regression on Toshiba Portege R500

[Frans Pop]

> The following three patches address the hibernation/suspend issue
> described in http://bugzilla.kernel.org/show_bug.cgi?id=12121 and in
> the very long thread at http://lkml.org/lkml/2008/12/1/382.

I've just built a kernel with your three patches, and without the earlier
"revert/debug" and "ignore transparent bridge" patches.

It also includes my patch that somewhat improves USB resume and my fix for 
the ohci1394: "irq 19: nobody cared" issue. (Although I understand that 
it's likely we'll get a much more structural improvement for USB than my 
naive patch.)

I'll run my notebook with this kernel for the next few days and let you 
know the results. First suspend/resume cycle was fine and showed, as 
expected, a lot of config restores moved up, including HDA intel and 
pcieport-driver.

It's nice to see something of significance happening before the ricoh-mmc 
controller gets disabled :-P

Cheers,
FJP

Re: [PATCH 3/3] Sound (HDA Intel): Restore PCI configuration space with interrupts off

[Jesse Barnes]

On Saturday, December 6, 2008 6:09 am Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: Sound (HDA Intel): Restore PCI configuration space with interrupts
> off
>
> Move the restoration of the standard PCI configuration registers
> in the snd_hda_intel driver to a ->resume_early() callback executed
> with interrupts disabled, since doing that with interrupts enabled
> may lead to problems in some cases.
>
> This patch addresses the regression from 2.6.26 tracked as
> http://bugzilla.kernel.org/show_bug.cgi?id=12121 .

Since I only applied 1 and 2 you'll need to send this one through Takashi.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/3] Sound (HDA Intel): Restore PCI configuration space with interrupts off

[Takashi Iwai]

At Sat, 6 Dec 2008 20:45:35 -0800,
Jesse Barnes wrote:
> 
> On Saturday, December 6, 2008 6:09 am Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > Subject: Sound (HDA Intel): Restore PCI configuration space with interrupts
> > off
> >
> > Move the restoration of the standard PCI configuration registers
> > in the snd_hda_intel driver to a ->resume_early() callback executed
> > with interrupts disabled, since doing that with interrupts enabled
> > may lead to problems in some cases.
> >
> > This patch addresses the regression from 2.6.26 tracked as
> > http://bugzilla.kernel.org/show_bug.cgi?id=12121 .
> 
> Since I only applied 1 and 2 you'll need to send this one through Takashi.

OK, I merged it to for-next branch now.
It should appear in the linux-next tree of tomorrow.


thanks,

Takashi

Re: [PATCH 3/3] Sound (HDA Intel): Restore PCI configuration space with interrupts off

[Takashi Iwai]

At Sun, 07 Dec 2008 10:47:56 +0100,
I wrote:
> 
> At Sat, 6 Dec 2008 20:45:35 -0800,
> Jesse Barnes wrote:
> > 
> > On Saturday, December 6, 2008 6:09 am Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > Subject: Sound (HDA Intel): Restore PCI configuration space with interrupts
> > > off
> > >
> > > Move the restoration of the standard PCI configuration registers
> > > in the snd_hda_intel driver to a ->resume_early() callback executed
> > > with interrupts disabled, since doing that with interrupts enabled
> > > may lead to problems in some cases.
> > >
> > > This patch addresses the regression from 2.6.26 tracked as
> > > http://bugzilla.kernel.org/show_bug.cgi?id=12121 .
> > 
> > Since I only applied 1 and 2 you'll need to send this one through Takashi.
> 
> OK, I merged it to for-next branch now.
> It should appear in the linux-next tree of tomorrow.

There is no build errors at least on linux-next, but I guess the
testing about PM has been rarely done on linux-next kernel...

BTW, Rafael, is this particular patch (against hda_intel.c) works in
general or dependent on other two patches?


thanks,

Takashi

Re: [PATCH 3/3] Sound (HDA Intel): Restore PCI configuration space with interrupts off

[Rafael J. Wysocki]

On Thursday, 11 of December 2008, Takashi Iwai wrote:
> At Sun, 07 Dec 2008 10:47:56 +0100,
> I wrote:
> > 
> > At Sat, 6 Dec 2008 20:45:35 -0800,
> > Jesse Barnes wrote:
> > > 
> > > On Saturday, December 6, 2008 6:09 am Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > Subject: Sound (HDA Intel): Restore PCI configuration space with interrupts
> > > > off
> > > >
> > > > Move the restoration of the standard PCI configuration registers
> > > > in the snd_hda_intel driver to a ->resume_early() callback executed
> > > > with interrupts disabled, since doing that with interrupts enabled
> > > > may lead to problems in some cases.
> > > >
> > > > This patch addresses the regression from 2.6.26 tracked as
> > > > http://bugzilla.kernel.org/show_bug.cgi?id=12121 .
> > > 
> > > Since I only applied 1 and 2 you'll need to send this one through Takashi.
> > 
> > OK, I merged it to for-next branch now.
> > It should appear in the linux-next tree of tomorrow.
> 
> There is no build errors at least on linux-next, but I guess the
> testing about PM has been rarely done on linux-next kernel...
> 
> BTW, Rafael, is this particular patch (against hda_intel.c) works in
> general or dependent on other two patches?

It should be safe without the other patches too.

Thanks,
Rafael

Re: [PATCH 3/3] Sound (HDA Intel): Restore PCI configuration space with interrupts off

[Takashi Iwai]

At Thu, 11 Dec 2008 21:03:16 +0100,
Rafael J. Wysocki wrote:
> 
> On Thursday, 11 of December 2008, Takashi Iwai wrote:
> > At Sun, 07 Dec 2008 10:47:56 +0100,
> > I wrote:
> > > 
> > > At Sat, 6 Dec 2008 20:45:35 -0800,
> > > Jesse Barnes wrote:
> > > > 
> > > > On Saturday, December 6, 2008 6:09 am Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > Subject: Sound (HDA Intel): Restore PCI configuration space with interrupts
> > > > > off
> > > > >
> > > > > Move the restoration of the standard PCI configuration registers
> > > > > in the snd_hda_intel driver to a ->resume_early() callback executed
> > > > > with interrupts disabled, since doing that with interrupts enabled
> > > > > may lead to problems in some cases.
> > > > >
> > > > > This patch addresses the regression from 2.6.26 tracked as
> > > > > http://bugzilla.kernel.org/show_bug.cgi?id=12121 .
> > > > 
> > > > Since I only applied 1 and 2 you'll need to send this one through Takashi.
> > > 
> > > OK, I merged it to for-next branch now.
> > > It should appear in the linux-next tree of tomorrow.
> > 
> > There is no build errors at least on linux-next, but I guess the
> > testing about PM has been rarely done on linux-next kernel...
> > 
> > BTW, Rafael, is this particular patch (against hda_intel.c) works in
> > general or dependent on other two patches?
> 
> It should be safe without the other patches too.

OK, but this alone doesn't make much sense without others, right?

I'm asking this because I'm not pretty sure how this should be handled.
Certainly it must be in 2.6.29, but about for 2.6.28...


thanks,

Takashi

Re: [PATCH 3/3] Sound (HDA Intel): Restore PCI configuration space with interrupts off

[Rafael J. Wysocki]

On Thursday, 11 of December 2008, Takashi Iwai wrote:
> At Thu, 11 Dec 2008 21:03:16 +0100,
> Rafael J. Wysocki wrote:
> > 
> > On Thursday, 11 of December 2008, Takashi Iwai wrote:
> > > At Sun, 07 Dec 2008 10:47:56 +0100,
> > > I wrote:
> > > > 
> > > > At Sat, 6 Dec 2008 20:45:35 -0800,
> > > > Jesse Barnes wrote:
> > > > > 
> > > > > On Saturday, December 6, 2008 6:09 am Rafael J. Wysocki wrote:
> > > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > Subject: Sound (HDA Intel): Restore PCI configuration space with interrupts
> > > > > > off
> > > > > >
> > > > > > Move the restoration of the standard PCI configuration registers
> > > > > > in the snd_hda_intel driver to a ->resume_early() callback executed
> > > > > > with interrupts disabled, since doing that with interrupts enabled
> > > > > > may lead to problems in some cases.
> > > > > >
> > > > > > This patch addresses the regression from 2.6.26 tracked as
> > > > > > http://bugzilla.kernel.org/show_bug.cgi?id=12121 .
> > > > > 
> > > > > Since I only applied 1 and 2 you'll need to send this one through Takashi.
> > > > 
> > > > OK, I merged it to for-next branch now.
> > > > It should appear in the linux-next tree of tomorrow.
> > > 
> > > There is no build errors at least on linux-next, but I guess the
> > > testing about PM has been rarely done on linux-next kernel...
> > > 
> > > BTW, Rafael, is this particular patch (against hda_intel.c) works in
> > > general or dependent on other two patches?
> > 
> > It should be safe without the other patches too.
> 
> OK, but this alone doesn't make much sense without others, right?
> 
> I'm asking this because I'm not pretty sure how this should be handled.
> Certainly it must be in 2.6.29, but about for 2.6.28...

Well, I think 2.6.29 would be fine.

Thanks,
Rafael

Re: [PATCH 3/3] Sound (HDA Intel): Restore PCI configuration space with interrupts off

[Takashi Iwai]

At Thu, 11 Dec 2008 21:38:56 +0100,
Rafael J. Wysocki wrote:
> 
> On Thursday, 11 of December 2008, Takashi Iwai wrote:
> > At Thu, 11 Dec 2008 21:03:16 +0100,
> > Rafael J. Wysocki wrote:
> > > 
> > > On Thursday, 11 of December 2008, Takashi Iwai wrote:
> > > > At Sun, 07 Dec 2008 10:47:56 +0100,
> > > > I wrote:
> > > > > 
> > > > > At Sat, 6 Dec 2008 20:45:35 -0800,
> > > > > Jesse Barnes wrote:
> > > > > > 
> > > > > > On Saturday, December 6, 2008 6:09 am Rafael J. Wysocki wrote:
> > > > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > > Subject: Sound (HDA Intel): Restore PCI configuration space with interrupts
> > > > > > > off
> > > > > > >
> > > > > > > Move the restoration of the standard PCI configuration registers
> > > > > > > in the snd_hda_intel driver to a ->resume_early() callback executed
> > > > > > > with interrupts disabled, since doing that with interrupts enabled
> > > > > > > may lead to problems in some cases.
> > > > > > >
> > > > > > > This patch addresses the regression from 2.6.26 tracked as
> > > > > > > http://bugzilla.kernel.org/show_bug.cgi?id=12121 .
> > > > > > 
> > > > > > Since I only applied 1 and 2 you'll need to send this one through Takashi.
> > > > > 
> > > > > OK, I merged it to for-next branch now.
> > > > > It should appear in the linux-next tree of tomorrow.
> > > > 
> > > > There is no build errors at least on linux-next, but I guess the
> > > > testing about PM has been rarely done on linux-next kernel...
> > > > 
> > > > BTW, Rafael, is this particular patch (against hda_intel.c) works in
> > > > general or dependent on other two patches?
> > > 
> > > It should be safe without the other patches too.
> > 
> > OK, but this alone doesn't make much sense without others, right?
> > 
> > I'm asking this because I'm not pretty sure how this should be handled.
> > Certainly it must be in 2.6.29, but about for 2.6.28...
> 
> Well, I think 2.6.29 would be fine.

OK, thanks.
If any, this could be pushed later from stable tree, too.


Takashi

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Rafael J. Wysocki]

Hi Alan,

On Saturday, 6 of December 2008, Alan Stern wrote:
> On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> 
> > On Saturday, 6 of December 2008, Linus Torvalds wrote:
> > > 
> > > On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > > > 
> > > > USB doesn't use that for PCI suspend-resume, it uses it for suspend-resume of
> > > > USB devices behind the controller.
> > > 
> > > Oh, in that case there are no PCI users of this at all, and what the PCI 
> > > driver does is immaterial ;)
> > > 
> > > > But then we will save the device's registers in the "sleeping" state. 
> > > 
> > > No no. The rule would be that a PCI driver - if it uses the new 
> > > infrastructure, which apparently nobody does _as_ a PCI driver - simply 
> > > would never do the whole "pci_set_power_state(PCI_D3hot)" etc crud AT ALL.
> > 
> > Now _that_ sounds good. :-)
> > 
> > > So a PCI driver would only do higher-level stuff in its suspend/resume 
> > > code. For example, a USB host controller would initiate the USB bus level 
> > > stuff, and likely just stop the controller (not suspend it - just stop 
> > > it).
> > 
> > I like this idea very much.
> 
> Rafael, I'd be happy to help with fixing up the USB PCI PM code.  At
> this point I'm not sure exactly what's needed, though.  For instance,
> is there any compelling reason to switch over to the new dev_pm_ops
> approach?

Certainly not at the moment.  There will be a reason some time after .29.

That said, it apparently is possible to clean up the resume callbacks of PCI
USB controllers, as mentioned here: http://lkml.org/lkml/2008/12/6/38

> And what should the correct sequence of calls be?

Well, that's something I'm not exactly sure about myself.  Surely it seems
reasonable to call pci_restore_state() with interrupts disabled and do the rest
of resume after that.  Also, I think that the core could execute things like
pci_enable_device() during resume and pci_set_power_state()/pci_enable_wake()
on suspend so that the drivers didn't have to.  This way we could reduce code
duplication quite a bit.

However, I'm not quite sure about the freeing and requesting IRQs during
suspend and resume.  Many drivers do that, many others don't.  Still,
apparently some drivers don't work correctly after resume if this is not done.
So, if that should generally be done, I also think that moving it to the core
might be a good idea.

Thanks,
Rafael

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Linus Torvalds]

On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> 
> However, I'm not quite sure about the freeing and requesting IRQs during
> suspend and resume.  Many drivers do that, many others don't.  Still,
> apparently some drivers don't work correctly after resume if this is not done.
> So, if that should generally be done, I also think that moving it to the core
> might be a good idea.

I'd suggest against it.

A lot of drivers that want to disable (or unregister) interrupts almost 
certainly want to do it simply because they are not ready and willing to 
handle any interrupts after having run their "suspend()" function. 

So if the generic layer does it _after_ calling ->suspend() (or at 
suspend_late()) time, it's too late.

And the generic layer certainly must not disable/unregister interrupts 
_before_ calling ->suspend(), since the driver may well need to handle 
interrupts for suspending.

So there is no right time for the generic layer to do this. Not to mention 
that the generic layer doesn't even know what kind of interrupt (if any - 
or if perhaps even _multiple_) that the driver has registered.

I also suspect that a lot of drivers simply do not want or need to 
unregister the interrupt handler. I'm personally pretty sure that the only 
reason that drivers do this in the first place is exactly because they do 
their suspend() thing with interrupts enabled in the first place, and 
moving the core suspend routines to inside the irq-off region just means 
that they don't even want/need to do anything about interrupts.

			Linus

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Arjan van de Ven]

On Sat, 6 Dec 2008 14:24:55 -0800 (PST)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> > 
> > However, I'm not quite sure about the freeing and requesting IRQs
> > during suspend and resume.  Many drivers do that, many others
> > don't.  Still, apparently some drivers don't work correctly after
> > resume if this is not done. So, if that should generally be done, I
> > also think that moving it to the core might be a good idea.
> 
> I'd suggest against it.
> 
> A lot of drivers that want to disable (or unregister) interrupts
> almost certainly want to do it simply because they are not ready and
> willing to handle any interrupts after having run their "suspend()"
> function. 

the problem is that the system bios can have reassigned interrupts
after resume, and afaik we need to re-evaluate the ACPI methods to
get the new mapping.
So we need to unregister + re-register to make that happen

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Alan Cox]

> So we need to unregister + re-register to make that happen

Agreed - and to cope with coming back up with some masked IRQs for those
lovely hardware vendors whose idea of amusement is handing the resumed
system a pending IRQ. To be fair its often hardware flagging things like
'device has become ready' from power up events...

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Linus Torvalds]

On Sat, 6 Dec 2008, Arjan van de Ven wrote:
> 
> the problem is that the system bios can have reassigned interrupts
> after resume, and afaik we need to re-evaluate the ACPI methods to
> get the new mapping.
> So we need to unregister + re-register to make that happen

Can you give actual examples of real life situations?

Because quite frankly, it sounds less and less likely for any relevant 
hardware. It's a non-issue for MSI, for example. And it's a non-issue for 
any sane interrupt source I can think of.

In other words, I've heard that claim before - and I just don't believe 
it. I've never heard a realistic explanation of why it would happen for a 
normal PCI driver. And I still claim that it's a very odd and special case 
if it does.

And btw, I'm talking suspend, not hibernate. 

			Linus

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Linus Torvalds]

On Sat, 6 Dec 2008, Linus Torvalds wrote:
> 
> And btw, I'm talking suspend, not hibernate. 

And, btw, even if anybody actually does this, it should be up to the 
interrupt controller logic to re-initialize the interrupts so that they 
are back where they belong. IOW, we should never show such _idiotic_ 
brokenness to any actual driver, it should all be remapped and handled 
below them.

And I still have never heard any valid reason to do it in the first place, 
so until somebody actually gives a real example and an explanation, I 
would suggest ignoring the whole issue as some insane rumblings from crazy 
hw/firmare people doing idiotic things.

			Linus

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Takashi Iwai]

At Sat, 6 Dec 2008 22:00:59 -0800 (PST),
Linus Torvalds wrote:
> 
> 
> 
> On Sat, 6 Dec 2008, Arjan van de Ven wrote:
> > 
> > the problem is that the system bios can have reassigned interrupts
> > after resume, and afaik we need to re-evaluate the ACPI methods to
> > get the new mapping.
> > So we need to unregister + re-register to make that happen
> 
> Can you give actual examples of real life situations?

There were such cases on intel8x0 and maestro3 on-board sound devices,
but all they were about hibernate, IIRC.  Just though a quick git 
log search, I found the following:
	http://bugzilla.kernel.org/show_bug.cgi?id=4416


Takashi

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Rafael J. Wysocki]

[-- Attachment #1.1: Type: text/plain, Size: 952 bytes --]

On Sunday, 7 of December 2008, Takashi Iwai wrote:
> At Sat, 6 Dec 2008 22:00:59 -0800 (PST),
> Linus Torvalds wrote:
> > 
> > 
> > 
> > On Sat, 6 Dec 2008, Arjan van de Ven wrote:
> > > 
> > > the problem is that the system bios can have reassigned interrupts
> > > after resume, and afaik we need to re-evaluate the ACPI methods to
> > > get the new mapping.
> > > So we need to unregister + re-register to make that happen
> > 
> > Can you give actual examples of real life situations?
> 
> There were such cases on intel8x0 and maestro3 on-board sound devices,
> but all they were about hibernate, IIRC.  Just though a quick git 
> log search, I found the following:
> 	http://bugzilla.kernel.org/show_bug.cgi?id=4416

Heh, I didn't remember _I_ had this issue. :-)

Well, I must admit my understanding of things at that time was not too clear ...

Anyway, the box is still available to me, so I'll check if that is still relevant.

Thanks,
Rafael

[-- Attachment #1.2: Type: text/html, Size: 1381 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Rafael J. Wysocki]

On Sunday, 7 of December 2008, Linus Torvalds wrote:
> 
> On Sat, 6 Dec 2008, Linus Torvalds wrote:
> > 
> > And btw, I'm talking suspend, not hibernate. 

Even as far as hibernation is concerned, I wouldn't _expect_ any BIOS to do
anything like this as long as we use the ACPI facility to enter S4.

> And, btw, even if anybody actually does this, it should be up to the 
> interrupt controller logic to re-initialize the interrupts so that they 
> are back where they belong. IOW, we should never show such _idiotic_ 
> brokenness to any actual driver, it should all be remapped and handled 
> below them.
> 
> And I still have never heard any valid reason to do it in the first place, 
> so until somebody actually gives a real example and an explanation, I 
> would suggest ignoring the whole issue as some insane rumblings from crazy 
> hw/firmare people doing idiotic things.

While I'd really like to do that, I also think that we should make all drivers
behave in the same way in this area.

Also, we need to state clearly what the _recommended_way of doing that is,
at least as a guidance for driver writers if not for anything else.

So, can we just say that PCI drivers shouldn't free IRQs during suspend and
request them during resume, and if there's any problem that leads to, then it
should be solved differently?

Rafael

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Linus Torvalds]

On Sun, 7 Dec 2008, Rafael J. Wysocki wrote:
> 
> So, can we just say that PCI drivers shouldn't free IRQs during suspend and
> request them during resume, and if there's any problem that leads to, then it
> should be solved differently?

Well, there are reasons why _individual_ drivers might want to free and 
re-request irq's during suspend, so I wouldn't say it's wrong either.

For example, let's say that driver xyzzy has a suspend function (note: not 
"suspend_late" or "suspend_noirq"), and that in that suspend routine it 
turns off some slow part of itself (ie it doesn't go into D3, but let's 
say that it's a wireless device and it turns off its radio).

And maybe that driver is written in such a way that the interrupt routine 
wants to access the radio chip.

Now, the driver has two choices:

 - just make the irq handler happy with the partially suspended state (and 
   admittedly this is likely the _sane_ choice and interrupt handlers 
   should always be robust, but never mind)

 - or just make the suspend routine make sure that the chip doesn't 
   generate any interrupts, and release the interrupt handler (the latter 
   is needed because of shared interrupts - even if _that_ chip doesn't 
   generate any interrupts, the interrupt handler will still get called if 
   there are shared interrupts, of course)

IOW, I think an _acceptable_ solution to a problem like this is "hey, I'm 
turning myself off, so I'll also turn off my interrupts and disable my irq 
handler).

I just don't think it's a very -good- approach. I think it's an acceptable 
one, but it sure as hell shouldn't be the _default_ one. Especially not 
for a lot of simple devices that can probably do all of their save/restore 
entirely inside the "noirq" window, so they would never have this kind of 
issue anyway.

			Linus

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Arjan van de Ven]

On Sun, 7 Dec 2008 14:39:27 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Sunday, 7 of December 2008, Linus Torvalds wrote:
> > 
> > On Sat, 6 Dec 2008, Linus Torvalds wrote:
> > > 
> > > And btw, I'm talking suspend, not hibernate. 
> 
> Even as far as hibernation is concerned, I wouldn't _expect_ any BIOS
> to do anything like this as long as we use the ACPI facility to enter
> S4.

there are funky scenarios where the BIOS ends up .. not knowing.
Like

you boot your laptop
you then hot-dock your laptop
then you suspend (say S4)

..
during resume, the bios sees a very different system than it saw before.
I can totally imagine not all of them getting it right, esp if other
OSes would just re-register interrupts

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Pavel Machek]

On Sun 2008-12-07 08:34:43, Linus Torvalds wrote:
> On Sun, 7 Dec 2008, Rafael J. Wysocki wrote:
> > 
> > So, can we just say that PCI drivers shouldn't free IRQs during suspend and
> > request them during resume, and if there's any problem that leads to, then it
> > should be solved differently?
> 
> Well, there are reasons why _individual_ drivers might want to free and 
> re-request irq's during suspend, so I wouldn't say it's wrong either.

Another (not too good) reason why you may want to unregister the
interrupt is similarity between suspend and rmmod (and resume and
insmod).

In some cases you can get away with sharing code between those two
paths...
								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: Rework default handling of suspend and resume

[Alan Stern]

On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:

> > Rafael, I'd be happy to help with fixing up the USB PCI PM code.  At
> > this point I'm not sure exactly what's needed, though.  For instance,
> > is there any compelling reason to switch over to the new dev_pm_ops
> > approach?
> 
> Certainly not at the moment.  There will be a reason some time after .29.
> 
> That said, it apparently is possible to clean up the resume callbacks of PCI
> USB controllers, as mentioned here: http://lkml.org/lkml/2008/12/6/38
> 
> > And what should the correct sequence of calls be?
> 
> Well, that's something I'm not exactly sure about myself.  Surely it seems
> reasonable to call pci_restore_state() with interrupts disabled and do the rest
> of resume after that.  Also, I think that the core could execute things like
> pci_enable_device() during resume and pci_set_power_state()/pci_enable_wake()
> on suspend so that the drivers didn't have to.  This way we could reduce code
> duplication quite a bit.

Do you plan to change the PCI core to do these things any time soon?  
Wouldn't that require changing a whole bunch of PCI drivers too?  I
tend to agree that having the core take care of these choreographed
activities would be good -- it would leave less room for drivers to
make mistakes.

So for now maybe it would be best just to rearrange the existing calls
in USB, and wait for the core changes before doing anything more
ambitious.

> However, I'm not quite sure about the freeing and requesting IRQs during
> suspend and resume.  Many drivers do that, many others don't.  Still,
> apparently some drivers don't work correctly after resume if this is not done.
> So, if that should generally be done, I also think that moving it to the core
> might be a good idea.

For USB this doesn't matter; we don't free the IRQs during suspend.

Alan Stern

Re: [PATCH 1/3] PCI: Rework default handling of suspend and resume

[Rafael J. Wysocki]

On Sunday, 7 of December 2008, Alan Stern wrote:
> On Sat, 6 Dec 2008, Rafael J. Wysocki wrote:
> 
> > > Rafael, I'd be happy to help with fixing up the USB PCI PM code.  At
> > > this point I'm not sure exactly what's needed, though.  For instance,
> > > is there any compelling reason to switch over to the new dev_pm_ops
> > > approach?
> > 
> > Certainly not at the moment.  There will be a reason some time after .29.
> > 
> > That said, it apparently is possible to clean up the resume callbacks of PCI
> > USB controllers, as mentioned here: http://lkml.org/lkml/2008/12/6/38
> > 
> > > And what should the correct sequence of calls be?
> > 
> > Well, that's something I'm not exactly sure about myself.  Surely it seems
> > reasonable to call pci_restore_state() with interrupts disabled and do the rest
> > of resume after that.  Also, I think that the core could execute things like
> > pci_enable_device() during resume and pci_set_power_state()/pci_enable_wake()
> > on suspend so that the drivers didn't have to.  This way we could reduce code
> > duplication quite a bit.
> 
> Do you plan to change the PCI core to do these things any time soon?  

I'm going to do that after the patches from this series are merged.

> Wouldn't that require changing a whole bunch of PCI drivers too?

Only those that start to use the new framework before this happens
(which probably is only MMC at this point).

> I tend to agree that having the core take care of these choreographed
> activities would be good -- it would leave less room for drivers to
> make mistakes.
> 
> So for now maybe it would be best just to rearrange the existing calls
> in USB, and wait for the core changes before doing anything more
> ambitious.

Sounds good.

Thanks,
Rafael