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

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

USB suspend and resume for PCI host controllers

[Alan Stern]

Rafael:

Here's the current version of my patch.  All of the PCI stuff has been 
moved to the suspend_late and resume_early routines.

Now maybe that's not the right thing to do, or at least not until some 
further fixes have been added.  All I can tell you is that this looks 
right to me, but it doesn't work.  When I tried it, all the I/O and 
MMIO accesses after resume_early returned nothing but 0xffffffff.

Here's a sample extracted from the log:

[  472.248029] ehci_hcd 0000:00:1d.7: resume from previous PCI D3: 0
[  472.248139] ehci_hcd 0000:00:1d.7: restoring config space at offset 0xf (was 0x400, writing 0x0)
[  472.248283] ehci_hcd 0000:00:1d.7: restoring config space at offset 0xd (was 0x50, writing 0x0)
[  472.248430] ehci_hcd 0000:00:1d.7: restoring config space at offset 0xb (was 0x52478086, writing 0x0)
[  472.248591] ehci_hcd 0000:00:1d.7: restoring config space at offset 0x2 (was 0xc032002, writing 0x0)
[  472.248735] ehci_hcd 0000:00:1d.7: restoring config space at offset 0x1 (was 0x2900000, writing 0x0)
[  472.248878] ehci_hcd 0000:00:1d.7: restoring config space at offset 0x0 (was 0x24cd8086, writing 0x0)
[  472.249037] ehci_hcd 0000:00:1d.7: enabling device (0000 -> 0002)
[  472.249136] ehci_hcd 0000:00:1d.7: PCI INT D -> GSI 23 (level, low) -> IRQ 23
[  472.249237] ehci_hcd 0000:00:1d.7: setting latency timer to 64
[  472.264026] ohci_hcd 0000:01:00.0: resume from previous PCI D3: 0
[  472.264130] ohci_hcd 0000:01:00.0: restoring config space at offset 0xf (was 0x2a01010b, writing 0x0)
[  472.264275] ohci_hcd 0000:01:00.0: restoring config space at offset 0xd (was 0x40, writing 0x0)
[  472.264418] ohci_hcd 0000:01:00.0: restoring config space at offset 0xb (was 0x351033, writing 0x0)
[  472.264571] ohci_hcd 0000:01:00.0: restoring config space at offset 0x4 (was 0xff8fd000, writing 0x0)
[  472.264713] ohci_hcd 0000:01:00.0: restoring config space at offset 0x3 (was 0x802008, writing 0x0)
[  472.264855] ohci_hcd 0000:01:00.0: restoring config space at offset 0x2 (was 0xc031043, writing 0x0)
[  472.264997] ohci_hcd 0000:01:00.0: restoring config space at offset 0x1 (was 0x2100000, writing 0x0)
[  472.265139] ohci_hcd 0000:01:00.0: restoring config space at offset 0x0 (was 0x351033, writing 0x0)
[  472.265289] ohci_hcd 0000:01:00.0: enabling device (0000 -> 0002)
[  472.265385] ohci_hcd 0000:01:00.0: PCI INT A -> GSI 21 (level, low) -> IRQ 21
[  472.265484] ohci_hcd 0000:01:00.0: setting latency timer to 64
...
[  472.350778] ehci_hcd 0000:00:1d.7: lost power, restarting
[  472.350869] usb usb6: root hub lost power or was reset
[  472.350964] ehci_hcd 0000:00:1d.7: reset command ffffffff park=3 ithresh=63 LReset IAAD Async Periodic period=?? R
[  472.351120] ehci_hcd 0000:00:1d.7: debug port 1 IN USE
[  472.351216] ehci_hcd 0000:00:1d.7: cache line size of 128 is not supported
[  472.352143] ohci_hcd 0000:01:00.0: BIOS/SMM active, control ffffffff
[  472.352237] ohci_hcd 0000:01:00.0: USB HC TakeOver from BIOS/SMM
[  480.352022] ohci_hcd 0000:01:00.0: USB HC takeover failed!  (BIOS/SMM bug)
[  480.368072] ohci_hcd 0000:01:00.0: USB HC reset timed out!
[  480.368165] ohci_hcd 0000:01:00.0: can't restart, -1
[  480.368253] usb usb9: root hub lost power or was reset
[  480.368343] ohci_hcd 0000:01:00.0: already suspended

Alan Stern



Index: usb-2.6/drivers/usb/core/hcd.h
===================================================================
--- usb-2.6.orig/drivers/usb/core/hcd.h
+++ usb-2.6/drivers/usb/core/hcd.h
@@ -256,7 +256,9 @@ extern int usb_hcd_pci_probe(struct pci_
 extern void usb_hcd_pci_remove(struct pci_dev *dev);
 
 #ifdef CONFIG_PM
-extern int usb_hcd_pci_suspend(struct pci_dev *dev, pm_message_t state);
+extern int usb_hcd_pci_suspend(struct pci_dev *dev, pm_message_t msg);
+extern int usb_hcd_pci_suspend_late(struct pci_dev *dev, pm_message_t msg);
+extern int usb_hcd_pci_resume_early(struct pci_dev *dev);
 extern int usb_hcd_pci_resume(struct pci_dev *dev);
 #endif /* CONFIG_PM */
 
Index: usb-2.6/drivers/usb/core/hcd-pci.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/hcd-pci.c
+++ usb-2.6/drivers/usb/core/hcd-pci.c
@@ -191,17 +191,15 @@ EXPORT_SYMBOL_GPL(usb_hcd_pci_remove);
 /**
  * usb_hcd_pci_suspend - power management suspend of a PCI-based HCD
  * @dev: USB Host Controller being suspended
- * @message: semantics in flux
+ * @message: Power Management message describing this state transition
  *
- * Store this function in the HCD's struct pci_driver as suspend().
+ * Store this function in the HCD's struct pci_driver as .suspend.
  */
 int usb_hcd_pci_suspend(struct pci_dev *dev, pm_message_t message)
 {
-	struct usb_hcd		*hcd;
+	struct usb_hcd		*hcd = pci_get_drvdata(dev);
 	int			retval = 0;
-	int			has_pci_pm;
 
-	hcd = pci_get_drvdata(dev);
 
 	/* Root hub suspend should have stopped all downstream traffic,
 	 * and all bus master traffic.  And done so for both the interface
@@ -212,96 +210,86 @@ int usb_hcd_pci_suspend(struct pci_dev *
 	 * otherwise the swsusp will save (and restore) garbage state.
 	 */
 	if (!(hcd->state == HC_STATE_SUSPENDED ||
-			hcd->state == HC_STATE_HALT))
-		return -EBUSY;
+			hcd->state == HC_STATE_HALT)) {
+		dev_warn(&dev->dev, "Root hub is not suspended\n");
+		retval = -EBUSY;
 
-	if (hcd->driver->pci_suspend) {
+	} else if (hcd->driver->pci_suspend) {
 		retval = hcd->driver->pci_suspend(hcd, message);
 		suspend_report_result(hcd->driver->pci_suspend, retval);
-		if (retval)
-			goto done;
 	}
-	synchronize_irq(dev->irq);
 
-	/* FIXME until the generic PM interfaces change a lot more, this
-	 * can't use PCI D1 and D2 states.  For example, the confusion
-	 * between messages and states will need to vanish, and messages
-	 * will need to provide a target system state again.
-	 *
-	 * It'll be important to learn characteristics of the target state,
-	 * especially on embedded hardware where the HCD will often be in
-	 * charge of an external VBUS power supply and one or more clocks.
-	 * Some target system states will leave them active; others won't.
-	 * (With PCI, that's often handled by platform BIOS code.)
-	 */
+	if (retval == 0)
+		synchronize_irq(dev->irq);
+	return retval;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_pci_suspend);
+
+/**
+ * usb_hcd_pci_suspend_late - suspend a PCI-based HCD after IRQs are disabled
+ * @dev: USB Host Controller being suspended
+ * @message: Power Management message describing this state transition
+ *
+ * Store this function in the HCD's struct pci_driver as .suspend_late.
+ */
+int usb_hcd_pci_suspend_late(struct pci_dev *dev, pm_message_t message)
+{
+	int			retval = 0;
+	int			has_pci_pm;
+	struct usb_hcd		*hcd = pci_get_drvdata(dev);
+	int			wake;
+
+	/* We might already be suspended (runtime PM -- not yet written) */
+	if (dev->current_state != PCI_D0)
+		goto done;
+
+	/* Don't change state if we don't need to */
+	if (message.event == PM_EVENT_FREEZE ||
+			message.event == PM_EVENT_PRETHAW) {
+		dev_dbg(&dev->dev, "--> no state change\n");
+		goto done;
+	}
 
-	/* even when the PCI layer rejects some of the PCI calls
-	 * below, HCs can try global suspend and reduce DMA traffic.
-	 * PM-sensitive HCDs may already have done this.
-	 */
 	has_pci_pm = pci_find_capability(dev, PCI_CAP_ID_PM);
+	if (!has_pci_pm) {
+		dev_dbg(&dev->dev, "--> PCI D0/legacy\n");
+		goto done;
+	}
+
+	/* Ignore these return values.  We rely on pci code to
+	 * reject requests the hardware can't implement, rather
+	 * than coding the same thing.
+	 */
+	wake = (hcd->state == HC_STATE_SUSPENDED &&
+			device_may_wakeup(&dev->dev));
+	(void) pci_enable_wake(dev, PCI_D3hot, wake);
+	(void) pci_enable_wake(dev, PCI_D3cold, wake);
 
 	/* Downstream ports from this root hub should already be quiesced, so
 	 * there will be no DMA activity.  Now we can shut down the upstream
 	 * link (except maybe for PME# resume signaling) and enter some PCI
 	 * low power state, if the hardware allows.
 	 */
-	if (hcd->state == HC_STATE_SUSPENDED) {
-
-		/* no DMA or IRQs except when HC is active */
-		if (dev->current_state == PCI_D0) {
-			pci_save_state(dev);
-			pci_disable_device(dev);
-		}
-
-		if (message.event == PM_EVENT_FREEZE ||
-				message.event == PM_EVENT_PRETHAW) {
-			dev_dbg(hcd->self.controller, "--> no state change\n");
-			goto done;
-		}
-
-		if (!has_pci_pm) {
-			dev_dbg(hcd->self.controller, "--> PCI D0/legacy\n");
-			goto done;
-		}
+	pci_disable_device(dev);
 
-		/* NOTE:  dev->current_state becomes nonzero only here, and
-		 * only for devices that support PCI PM.  Also, exiting
-		 * PCI_D3 (but not PCI_D1 or PCI_D2) is allowed to reset
-		 * some device state (e.g. as part of clock reinit).
-		 */
-		retval = pci_set_power_state(dev, PCI_D3hot);
-		suspend_report_result(pci_set_power_state, retval);
-		if (retval == 0) {
-			int wake = device_can_wakeup(&hcd->self.root_hub->dev);
-
-			wake = wake && device_may_wakeup(hcd->self.controller);
-
-			dev_dbg(hcd->self.controller, "--> PCI D3%s\n",
-					wake ? "/wakeup" : "");
-
-			/* Ignore these return values.  We rely on pci code to
-			 * reject requests the hardware can't implement, rather
-			 * than coding the same thing.
-			 */
-			(void) pci_enable_wake(dev, PCI_D3hot, wake);
-			(void) pci_enable_wake(dev, PCI_D3cold, wake);
-		} else {
-			dev_dbg(&dev->dev, "PCI D3 suspend fail, %d\n",
-					retval);
-			(void) usb_hcd_pci_resume(dev);
-		}
-
-	} else if (hcd->state != HC_STATE_HALT) {
-		dev_dbg(hcd->self.controller, "hcd state %d; not suspended\n",
-			hcd->state);
-		WARN_ON(1);
-		retval = -EINVAL;
+	/* NOTE:  dev->current_state becomes nonzero only here, and
+	 * only for devices that support PCI PM.  Also, exiting
+	 * PCI_D3 (but not PCI_D1 or PCI_D2) is allowed to reset
+	 * some device state (e.g. as part of clock reinit).
+	 */
+	retval = pci_set_power_state(dev, PCI_D3hot);
+	suspend_report_result(pci_set_power_state, retval);
+	if (retval == 0) {
+		dev_dbg(hcd->self.controller, "--> PCI D3%s\n",
+				wake ? "/wakeup" : "");
+	} else {
+		dev_dbg(&dev->dev, "PCI D3 suspend fail, %d\n", retval);
+		(void) usb_hcd_pci_resume(dev);
 	}
 
-done:
-	if (retval == 0) {
+ done:
 #ifdef CONFIG_PPC_PMAC
+	if (retval == 0) {
 		/* Disable ASIC clocks for USB */
 		if (machine_is(powermac)) {
 			struct device_node	*of_node;
@@ -311,30 +299,23 @@ done:
 				pmac_call_feature(PMAC_FTR_USB_ENABLE,
 							of_node, 0, 0);
 		}
-#endif
 	}
+#endif
 
 	return retval;
 }
-EXPORT_SYMBOL_GPL(usb_hcd_pci_suspend);
+EXPORT_SYMBOL_GPL(usb_hcd_pci_suspend_late);
 
 /**
- * usb_hcd_pci_resume - power management resume of a PCI-based HCD
+ * usb_hcd_pci_resume_early - resume a PCI-based HCD before IRQs are enabled
  * @dev: USB Host Controller being resumed
  *
- * Store this function in the HCD's struct pci_driver as resume().
+ * Store this function in the HCD's struct pci_driver as .resume_early.
  */
-int usb_hcd_pci_resume(struct pci_dev *dev)
+int usb_hcd_pci_resume_early(struct pci_dev *dev)
 {
-	struct usb_hcd		*hcd;
-	int			retval;
-
-	hcd = pci_get_drvdata(dev);
-	if (hcd->state != HC_STATE_SUSPENDED) {
-		dev_dbg(hcd->self.controller,
-				"can't resume, not suspended!\n");
-		return 0;
-	}
+	int		retval = 0;
+	pci_power_t	state = dev->current_state;
 
 #ifdef CONFIG_PPC_PMAC
 	/* Reenable ASIC clocks for USB */
@@ -352,7 +333,7 @@ int usb_hcd_pci_resume(struct pci_dev *d
 	 * calls "standby", "suspend to RAM", and so on).  There are also
 	 * dirty cases when swsusp fakes a suspend in "shutdown" mode.
 	 */
-	if (dev->current_state != PCI_D0) {
+	if (state != PCI_D0) {
 #ifdef	DEBUG
 		int	pci_pm;
 		u16	pmcr;
@@ -364,8 +345,7 @@ int usb_hcd_pci_resume(struct pci_dev *d
 			/* Clean case:  power to USB and to HC registers was
 			 * maintained; remote wakeup is easy.
 			 */
-			dev_dbg(hcd->self.controller, "resume from PCI D%d\n",
-					pmcr);
+			dev_dbg(&dev->dev, "resume from PCI D%d\n", pmcr);
 		} else {
 			/* Clean:  HC lost Vcc power, D0 uninitialized
 			 *   + Vaux may have preserved port and transceiver
@@ -376,33 +356,56 @@ int usb_hcd_pci_resume(struct pci_dev *d
 			 *   + after BIOS init
 			 *   + after Linux init (HCD statically linked)
 			 */
-			dev_dbg(hcd->self.controller,
-				"PCI D0, from previous PCI D%d\n",
-				dev->current_state);
+			dev_dbg(&dev->dev, "resume from previous PCI D%d\n",
+					state);
 		}
 #endif
-		/* yes, ignore these results too... */
-		(void) pci_enable_wake(dev, dev->current_state, 0);
-		(void) pci_enable_wake(dev, PCI_D3cold, 0);
+
+		retval = pci_set_power_state(dev, PCI_D0);
 	} else {
 		/* Same basic cases: clean (powered/not), dirty */
-		dev_dbg(hcd->self.controller, "PCI legacy resume\n");
+		dev_dbg(&dev->dev, "PCI legacy resume\n");
 	}
 
-	/* NOTE:  the PCI API itself is asymmetric here.  We don't need to
-	 * pci_set_power_state(PCI_D0) since that's part of re-enabling;
-	 * but that won't re-enable bus mastering.  Yet pci_disable_device()
-	 * explicitly disables bus mastering...
-	 */
-	retval = pci_enable_device(dev);
 	if (retval < 0) {
-		dev_err(hcd->self.controller,
-			"can't re-enable after resume, %d!\n", retval);
+		dev_err(&dev->dev, "can't resume: %d\n", retval);
 		return retval;
 	}
-	pci_set_master(dev);
+
 	pci_restore_state(dev);
 
+	/* yes, ignore these results too... */
+	(void) pci_enable_wake(dev, PCI_D3hot, 0);
+	(void) pci_enable_wake(dev, PCI_D3cold, 0);
+
+	retval = pci_enable_device(dev);
+	if (retval >= 0)
+		pci_set_master(dev);
+	else
+		dev_err(&dev->dev, "can't re-enable after resume, %d!\n",
+				retval);
+	return retval;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_pci_resume_early);
+
+/**
+ * usb_hcd_pci_resume - power management resume of a PCI-based HCD
+ * @dev: USB Host Controller being resumed
+ *
+ * Store this function in the HCD's struct pci_driver as .resume.
+ */
+int usb_hcd_pci_resume(struct pci_dev *dev)
+{
+	struct usb_hcd		*hcd;
+	int			retval = 0;
+
+	hcd = pci_get_drvdata(dev);
+	if (hcd->state != HC_STATE_SUSPENDED) {
+		dev_dbg(hcd->self.controller,
+				"can't resume, not suspended!\n");
+		return 0;
+	}
+
 	clear_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
 
 	if (hcd->driver->pci_resume) {
@@ -413,7 +416,6 @@ int usb_hcd_pci_resume(struct pci_dev *d
 			usb_hc_died(hcd);
 		}
 	}
-
 	return retval;
 }
 EXPORT_SYMBOL_GPL(usb_hcd_pci_resume);
Index: usb-2.6/drivers/usb/host/ehci-pci.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-pci.c
+++ usb-2.6/drivers/usb/host/ehci-pci.c
@@ -428,6 +428,8 @@ static struct pci_driver ehci_pci_driver
 
 #ifdef	CONFIG_PM
 	.suspend =	usb_hcd_pci_suspend,
+	.suspend_late =	usb_hcd_pci_suspend_late,
+	.resume_early =	usb_hcd_pci_resume_early,
 	.resume =	usb_hcd_pci_resume,
 #endif
 	.shutdown = 	usb_hcd_pci_shutdown,
Index: usb-2.6/drivers/usb/host/ohci-pci.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ohci-pci.c
+++ usb-2.6/drivers/usb/host/ohci-pci.c
@@ -487,6 +487,8 @@ static struct pci_driver ohci_pci_driver
 
 #ifdef	CONFIG_PM
 	.suspend =	usb_hcd_pci_suspend,
+	.suspend_late =	usb_hcd_pci_suspend_late,
+	.resume_early =	usb_hcd_pci_resume_early,
 	.resume =	usb_hcd_pci_resume,
 #endif
 
Index: usb-2.6/drivers/usb/host/uhci-hcd.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/uhci-hcd.c
+++ usb-2.6/drivers/usb/host/uhci-hcd.c
@@ -942,6 +942,8 @@ static struct pci_driver uhci_pci_driver
 
 #ifdef	CONFIG_PM
 	.suspend =	usb_hcd_pci_suspend,
+	.suspend_late =	usb_hcd_pci_suspend_late,
+	.resume_early =	usb_hcd_pci_resume_early,
 	.resume =	usb_hcd_pci_resume,
 #endif	/* PM */
 };

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

[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
  */

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