[PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Rafael J. Wysocki]

[following off-list discussion]

On Thursday 15 January 2009, Rafael J. Wysocki wrote:
> On Thursday 15 January 2009, Linus Torvalds wrote:
> > 
> > On Thu, 15 Jan 2009, Rafael J. Wysocki wrote:
> > 
> > > On Thursday 15 January 2009, Linus Torvalds wrote:
> > > > > 
> > > > > Yes, it does, via pci_prepare_to_sleep().
> > > > 
> > > > Umm. On the _early_wakeup_ path.
> > > 
> > > Nope.  pci_prepare_to_sleep() is called during suspend, from
> > > pci_pm_default_suspend(), with interrupts on.
> > > 
> > > On resume pci_set_power_state(dev, PCI_D0) is called from
> > > pci_reenable_device(), with interrupts on.
> > 
> > Rafael - that was my POINT.
> > 
> > I claimed that the new PM infrastructure did NOT wake up the device at the 
> > relevant time. That was all.
> > 
> > I made that comment as a reply to you saying that the new PM model fixes 
> > the problems with the old. It does NOT fix anything at all in this area. 
> > That was my whole point. It does not wake up the low-level PCI code during 
> > early wakeup, so it cannot.
> > 
> > > The only solution I can see would be to use a special version of
> > > pci_set_power_state() to power up devices during resume with interrupts off
> > > that will use mdelay() to wait for the ones in D3 to reach D0.  In fact it
> > > could even wait once for multiple devices.
> > 
> > Yes.
> > 
> > > Bridges would be problematic, but still possible to handle.
> > > 
> > > There might be a problem with the devices that are power-managed via ACPI only,
> > > but they _should_ be in D0 during resume anyway (otherwise we won't be able to
> > > access their config spaces at all).
> > > 
> > > How does that sound?
> > 
> > I do suspect that resume time is special enough that yes, it's worth doing 
> > something different than what a driver would normally do at device detect 
> > (or open) time.
> 
> OK, so I'll do that and let's see how it works.

The patch is appended.  Well, I'm not particularly proud of it, but it seems to
do the trick.

It survived some initial testing, but I tested it on top of the patch from
http://lkml.org/lkml/2009/1/13/144 (this shouldn't matter, but still).

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PCI PM: Restore standard config registers of all devices early

There is a problem in our handling of suspend-resume of PCI devices
that many of them have their standard config registers restored with
interrupts enabled and they are put into the full power state with
interrupts enabled as well.  This may lead to the following scenario:
* an interrupt vector is shared between two or more devices
* one device is resumed earlier and generates an interrupt
* the interrupt handler of another device tries to handle it and
  attempts to access the device the config space of which hasn't
  been restored yet and/or which still is in a low power state
* the system crashes as a result

To prevent this from happening we should restore the standard
configuration registers of all devices with interrupts disabled and
we should put them into the D0 power state right after that.
Unfortunately, this cannot be done using the existing
pci_set_power_state(), because it can sleep.  Also, to do it we have
to make sure that the config spaces of all devices were actually
saved during suspend.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |   98 ++++++++++++++++-------------------------------
 drivers/pci/pci.c        |   63 +++++++++++++++++++++++++++---
 drivers/pci/pci.h        |    6 ++
 include/linux/pci.h      |    5 ++
 4 files changed, 102 insertions(+), 70 deletions(-)

Index: linux-2.6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-driver.c
+++ linux-2.6/drivers/pci/pci-driver.c
@@ -355,17 +355,34 @@ static int pci_legacy_suspend(struct dev
 	int i = 0;
 
 	if (drv && drv->suspend) {
+		pci_dev->state_saved = false;
+
 		i = drv->suspend(pci_dev, state);
 		suspend_report_result(drv->suspend, i);
-	} else {
-		pci_save_state(pci_dev);
-		/*
-		 * This is for compatibility with existing code with legacy PM
-		 * support.
-		 */
-		pci_pm_set_unknown_state(pci_dev);
+		if (i)
+			return i;
+
+		if (pci_dev->state_saved)
+			goto Fixup;
+
+		if (pci_dev->current_state != PCI_D0) {
+			/*
+			 * Warn users if the driver puts the device into a low
+			 * power state, but doesn't bother to save its config
+			 * registers.
+			 */
+			WARN_ON(true);
+			goto Fixup;
+		}
 	}
 
+	pci_save_state(pci_dev);
+	/*
+	 * This is for compatibility with existing code with legacy PM support.
+	 */
+	pci_pm_set_unknown_state(pci_dev);
+
+ Fixup:
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
 
 	return i;
@@ -386,81 +403,34 @@ static int pci_legacy_suspend_late(struc
 
 static int pci_legacy_resume_early(struct device *dev)
 {
-	int error = 0;
 	struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * drv = pci_dev->driver;
 
-	pci_fixup_device(pci_fixup_resume_early, pci_dev);
-
-	if (drv && drv->resume_early)
-		error = drv->resume_early(pci_dev);
-	return error;
+	return drv && drv->resume_early ?
+			drv->resume_early(pci_dev) : 0;
 }
 
 static int pci_legacy_resume(struct device *dev)
 {
-	int error;
 	struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * drv = pci_dev->driver;
 
 	pci_fixup_device(pci_fixup_resume, pci_dev);
 
-	if (drv && drv->resume) {
-		error = drv->resume(pci_dev);
-	} else {
-		/* restore the PCI config space */
-		pci_restore_state(pci_dev);
-		error = pci_pm_reenable_device(pci_dev);
-	}
-	return error;
+	return drv && drv->resume ?
+			drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);
 }
 
 /* Auxiliary functions used by the new power management framework */
 
-static int pci_restore_standard_config(struct pci_dev *pci_dev)
-{
-	struct pci_dev *parent = pci_dev->bus->self;
-	int error = 0;
-
-	/* Check if the device's bus is operational */
-	if (!parent || parent->current_state == PCI_D0) {
-		pci_restore_state(pci_dev);
-		pci_update_current_state(pci_dev, PCI_D0);
-	} else {
-		dev_warn(&pci_dev->dev, "unable to restore config, "
-			"bridge %s in low power state D%d\n", pci_name(parent),
-			parent->current_state);
-		pci_dev->current_state = PCI_UNKNOWN;
-		error = -EAGAIN;
-	}
-
-	return error;
-}
-
-static bool pci_is_bridge(struct pci_dev *pci_dev)
-{
-	return !!(pci_dev->subordinate);
-}
-
 static void pci_pm_default_resume_noirq(struct pci_dev *pci_dev)
 {
-	if (pci_restore_standard_config(pci_dev))
-		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+	pci_restore_standard_config(pci_dev);
+	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 }
 
 static int pci_pm_default_resume(struct pci_dev *pci_dev)
 {
-	/*
-	 * pci_restore_standard_config() should have been called once already,
-	 * but it would have failed if the device's parent bridge had not been
-	 * in power state D0 at that time.  Check it and try again if necessary.
-	 */
-	if (pci_dev->current_state == PCI_UNKNOWN) {
-		int error = pci_restore_standard_config(pci_dev);
-		if (error)
-			return error;
-	}
-
 	pci_fixup_device(pci_fixup_resume, pci_dev);
 
 	if (!pci_is_bridge(pci_dev))
@@ -575,11 +545,11 @@ static int pci_pm_resume_noirq(struct de
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	pci_pm_default_resume_noirq(pci_dev);
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
 
-	pci_pm_default_resume_noirq(pci_dev);
-
 	if (drv && drv->pm && drv->pm->resume_noirq)
 		error = drv->pm->resume_noirq(dev);
 
@@ -730,11 +700,11 @@ static int pci_pm_restore_noirq(struct d
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	pci_pm_default_resume_noirq(pci_dev);
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
 
-	pci_pm_default_resume_noirq(pci_dev);
-
 	if (drv && drv->pm && drv->pm->restore_noirq)
 		error = drv->pm->restore_noirq(dev);
 
Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -22,7 +22,7 @@
 #include <asm/dma.h>	/* isa_dma_bridge_buggy */
 #include "pci.h"
 
-unsigned int pci_pm_d3_delay = 10;
+unsigned int pci_pm_d3_delay = PCI_PM_D3_WAIT;
 
 #ifdef CONFIG_PCI_DOMAINS
 int pci_domains_supported = 1;
@@ -426,6 +426,7 @@ static inline int platform_pci_sleep_wak
  *                           given PCI device
  * @dev: PCI device to handle.
  * @state: PCI power state (D0, D1, D2, D3hot) to put the device into.
+ * @wait: If 'true', wait for the device to change its power state
  *
  * RETURN VALUE:
  * -EINVAL if the requested state is invalid.
@@ -435,7 +436,7 @@ static inline int platform_pci_sleep_wak
  * 0 if device's power state has been successfully changed.
  */
 static int
-pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
+pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state, bool wait)
 {
 	u16 pmcsr;
 	bool need_restore = false;
@@ -480,8 +481,10 @@ pci_raw_set_power_state(struct pci_dev *
 		break;
 	case PCI_UNKNOWN: /* Boot-up */
 		if ((pmcsr & PCI_PM_CTRL_STATE_MASK) == PCI_D3hot
-		 && !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET))
+		 && !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET)) {
 			need_restore = true;
+			wait = true;
+		}
 		/* Fall-through: force to D0 */
 	default:
 		pmcsr = 0;
@@ -491,12 +494,15 @@ pci_raw_set_power_state(struct pci_dev *
 	/* enter specified state */
 	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
 
+	if (!wait)
+		return 0;
+
 	/* Mandatory power management transition delays */
 	/* see PCI PM 1.1 5.6.1 table 18 */
 	if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
 		msleep(pci_pm_d3_delay);
 	else if (state == PCI_D2 || dev->current_state == PCI_D2)
-		udelay(200);
+		udelay(PCI_PM_D2_DELAY);
 
 	dev->current_state = state;
 
@@ -515,7 +521,7 @@ pci_raw_set_power_state(struct pci_dev *
 	if (need_restore)
 		pci_restore_bars(dev);
 
-	if (dev->bus->self)
+	if (wait && dev->bus->self)
 		pcie_aspm_pm_state_change(dev->bus->self);
 
 	return 0;
@@ -585,7 +591,7 @@ int pci_set_power_state(struct pci_dev *
 	if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
 		return 0;
 
-	error = pci_raw_set_power_state(dev, state);
+	error = pci_raw_set_power_state(dev, state, true);
 
 	if (state > PCI_D0 && platform_pci_power_manageable(dev)) {
 		/* Allow the platform to finalize the transition */
@@ -730,6 +736,7 @@ pci_save_state(struct pci_dev *dev)
 	/* XXX: 100% dword access ok here? */
 	for (i = 0; i < 16; i++)
 		pci_read_config_dword(dev, i * 4,&dev->saved_config_space[i]);
+	dev->state_saved = true;
 	if ((i = pci_save_pcie_state(dev)) != 0)
 		return i;
 	if ((i = pci_save_pcix_state(dev)) != 0)
@@ -1378,6 +1385,50 @@ void pci_allocate_cap_save_buffers(struc
 }
 
 /**
+ * pci_restore_standard_config - restore standard config registers of PCI device
+ * @dev: PCI device to handle
+ *
+ * This function assumes that the device's configuration space is accessible.
+ * If the device needs to be powered up, the function will wait for it to
+ * change the state.
+ */
+int pci_restore_standard_config(struct pci_dev *dev)
+{
+	pci_power_t prev_state;
+	int error;
+
+	pci_restore_state(dev);
+	pci_update_current_state(dev, PCI_D0);
+
+	prev_state = dev->current_state;
+	if (prev_state == PCI_D0)
+		return 0;
+
+	error = pci_raw_set_power_state(dev, PCI_D0, false);
+	if (error)
+		return error;
+
+	if (pci_is_bridge(dev)) {
+		if (prev_state > PCI_D1)
+			mdelay(PCI_PM_BUS_WAIT);
+	} else {
+		switch(prev_state) {
+		case PCI_D3cold:
+		case PCI_D3hot:
+			mdelay(pci_pm_d3_delay);
+			break;
+		case PCI_D2:
+			udelay(PCI_PM_D2_DELAY);
+			break;
+		}
+	}
+
+	dev->current_state = PCI_D0;
+
+	return 0;
+}
+
+/**
  * pci_enable_ari - enable ARI forwarding if hardware support it
  * @dev: the PCI device
  */
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -117,6 +117,10 @@ typedef int __bitwise pci_power_t;
 #define PCI_UNKNOWN	((pci_power_t __force) 5)
 #define PCI_POWER_ERROR	((pci_power_t __force) -1)
 
+#define PCI_PM_D2_DELAY	200
+#define PCI_PM_D3_WAIT	10
+#define PCI_PM_BUS_WAIT	50
+
 /** The pci_channel state describes connectivity between the CPU and
  *  the pci device.  If some PCI bus between here and the pci device
  *  has crashed or locked up, this info is reflected here.
@@ -252,6 +256,7 @@ struct pci_dev {
 	unsigned int	ari_enabled:1;	/* ARI forwarding */
 	unsigned int	is_managed:1;
 	unsigned int	is_pcie:1;
+	unsigned int	state_saved:1;
 	pci_dev_flags_t dev_flags;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */
 
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -49,6 +49,12 @@ extern void pci_disable_enabled_device(s
 extern void pci_pm_init(struct pci_dev *dev);
 extern void platform_pci_wakeup_init(struct pci_dev *dev);
 extern void pci_allocate_cap_save_buffers(struct pci_dev *dev);
+extern int pci_restore_standard_config(struct pci_dev *dev);
+
+static inline bool pci_is_bridge(struct pci_dev *pci_dev)
+{
+	return !!(pci_dev->subordinate);
+}
 
 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);

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Alan Stern]

On Fri, 16 Jan 2009, Rafael J. Wysocki wrote:

> There is a problem in our handling of suspend-resume of PCI devices
> that many of them have their standard config registers restored with
> interrupts enabled and they are put into the full power state with
> interrupts enabled as well.  This may lead to the following scenario:
> * an interrupt vector is shared between two or more devices
> * one device is resumed earlier and generates an interrupt
> * the interrupt handler of another device tries to handle it and
>   attempts to access the device the config space of which hasn't
>   been restored yet and/or which still is in a low power state
> * the system crashes as a result
> 
> To prevent this from happening we should restore the standard
> configuration registers of all devices with interrupts disabled and
> we should put them into the D0 power state right after that.
> Unfortunately, this cannot be done using the existing
> pci_set_power_state(), because it can sleep.  Also, to do it we have
> to make sure that the config spaces of all devices were actually
> saved during suspend.

Alternatively, PCI drivers should be smart enough to realize that a
shared interrupt arriving while their device is suspended could not
possibly be generated by that device.  In such a case the driver can
return IRQ_NONE directly, without ever touching the device.

USB does this.  However I admit that fixing every PCI driver would be a 
big chore...

Alan Stern

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Rafael J. Wysocki]

On Friday 16 January 2009, Alan Stern wrote:
> On Fri, 16 Jan 2009, Rafael J. Wysocki wrote:
> 
> > There is a problem in our handling of suspend-resume of PCI devices
> > that many of them have their standard config registers restored with
> > interrupts enabled and they are put into the full power state with
> > interrupts enabled as well.  This may lead to the following scenario:
> > * an interrupt vector is shared between two or more devices
> > * one device is resumed earlier and generates an interrupt
> > * the interrupt handler of another device tries to handle it and
> >   attempts to access the device the config space of which hasn't
> >   been restored yet and/or which still is in a low power state
> > * the system crashes as a result
> > 
> > To prevent this from happening we should restore the standard
> > configuration registers of all devices with interrupts disabled and
> > we should put them into the D0 power state right after that.
> > Unfortunately, this cannot be done using the existing
> > pci_set_power_state(), because it can sleep.  Also, to do it we have
> > to make sure that the config spaces of all devices were actually
> > saved during suspend.
> 
> Alternatively, PCI drivers should be smart enough to realize that a
> shared interrupt arriving while their device is suspended could not
> possibly be generated by that device.  In such a case the driver can
> return IRQ_NONE directly, without ever touching the device.
> 
> USB does this.  However I admit that fixing every PCI driver would be a 
> big chore...

Well, that's the point. :-)

Thanks,
Rafael

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Linus Torvalds]

On Fri, 16 Jan 2009, Alan Stern wrote:
> 
> USB does this.  However I admit that fixing every PCI driver would be a 
> big chore...

I suspect USB is one of the very few drivers that may get suspend/resume 
very close to right, partly because _everybody_ has a USB controller these 
days, partly because USB interrupts are very commonly shared, and partly 
because there are several core people who work on USB.

That's actually very rare (especially the last point - most drivers hardly 
have _any_ maintainer, much less anybody who can see the big picture and 
knows about the rest of the system).

So if everybody did what USB does, we'd indeed be fine, and suspend and 
resume would probably work on practically every machine out there. But as 
you note, it's not really an option. The common case for 99% of all 
drivers is probably that none of the core kernel people can even test 
them, and they likely do not exist even in some distro test area.

So I'm the one who pushed Rafael towards his patch, because we've worked 
on suspend/resume for years, but while we're _much_ better than we used to 
be (especially on machines that are really bog-standard and only have the 
common chips), I don't think we'll ever really "get there" if we rely on 
the drivers always getting things right.

And getting things right does mean changing every single PCI interrupt 
handler to know about suspend/resume - not to mention do the 
suspend/resume sequence right in the first place. Neither of the two is 
very likely to really happen.

I did suggest that we could also add some test-infrastructure like the 
DEBUG_SHIRQ thing we already have, which sends an interrupt immediately on 
resume after interrupts have been enabled. That will likely uncover a 
_lot_ of problems, and it's almost certainly worth doing regardless.

But even then, it would just be so much _simpler_ if the generic layer 
just took care of this issue, so that drivers could just ignore it, and a 
driver would only have to worry about its _own_ resume issues. 

So yes, doing it in the generic PCI layer clearly has some problems, and 
no, it's not "perfect". In a perfect world, doing it in the driver has 
real advantages - it's the most flexible approach, and it allows the 
driver to do what it wants. But from all I've ever seen, I'm personally 
pretty convinced that when it comes to drivers, the less the driver writer 
has to worry about, the better off we are.

I'm franklly hoping that most PCI drivers would not need to have a 
suspend/resume method at all - the PCI layer should get the normal PCI 
suspend parts right, and the upper layers should do the rest. For example, 
a network driver shouldn't have to close itself down - the network layer 
should just do it for it (all the netif_stop_queue() etc crud to make 
sure that the network layer doesn't try to touch it). 

So we're clearly not there yet. But after having yet another random 
machine that just didn't resume because of yet another new random driver, 
I'm just not seeing anything else that is viable long-term.

IOW, we can ask driver writers to write perfect code, or we can spend some 
effort on trying to make things work even when they don't. I know which 
approach I consider to be realistic.

			Linus

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Rafael J. Wysocki]

On Friday 16 January 2009, Linus Torvalds wrote:
> 
> On Fri, 16 Jan 2009, Alan Stern wrote:
> > 
> > USB does this.  However I admit that fixing every PCI driver would be a 
> > big chore...
> 
> I suspect USB is one of the very few drivers that may get suspend/resume 
> very close to right, partly because _everybody_ has a USB controller these 
> days, partly because USB interrupts are very commonly shared, and partly 
> because there are several core people who work on USB.
> 
> That's actually very rare (especially the last point - most drivers hardly 
> have _any_ maintainer, much less anybody who can see the big picture and 
> knows about the rest of the system).
> 
> So if everybody did what USB does, we'd indeed be fine, and suspend and 
> resume would probably work on practically every machine out there. But as 
> you note, it's not really an option. The common case for 99% of all 
> drivers is probably that none of the core kernel people can even test 
> them, and they likely do not exist even in some distro test area.
> 
> So I'm the one who pushed Rafael towards his patch, because we've worked 
> on suspend/resume for years, but while we're _much_ better than we used to 
> be (especially on machines that are really bog-standard and only have the 
> common chips), I don't think we'll ever really "get there" if we rely on 
> the drivers always getting things right.
> 
> And getting things right does mean changing every single PCI interrupt 
> handler to know about suspend/resume - not to mention do the 
> suspend/resume sequence right in the first place. Neither of the two is 
> very likely to really happen.
> 
> I did suggest that we could also add some test-infrastructure like the 
> DEBUG_SHIRQ thing we already have, which sends an interrupt immediately on 
> resume after interrupts have been enabled. That will likely uncover a 
> _lot_ of problems, and it's almost certainly worth doing regardless.

For the record, I put that onto my todo list, which unfortunately is quite long
at the moment (and getting longer).

> But even then, it would just be so much _simpler_ if the generic layer 
> just took care of this issue, so that drivers could just ignore it, and a 
> driver would only have to worry about its _own_ resume issues. 
> 
> So yes, doing it in the generic PCI layer clearly has some problems, and 
> no, it's not "perfect". In a perfect world, doing it in the driver has 
> real advantages - it's the most flexible approach, and it allows the 
> driver to do what it wants. But from all I've ever seen, I'm personally 
> pretty convinced that when it comes to drivers, the less the driver writer 
> has to worry about, the better off we are.

I agree.

IMO suspend-resume is quite difficult to implement in a PCI driver unless
the driver writer knows very well how PCI PM is supposed to work and interact
with things like interrupts control, ACPI etc.

> I'm franklly hoping that most PCI drivers would not need to have a 
> suspend/resume method at all - the PCI layer should get the normal PCI 
> suspend parts right, and the upper layers should do the rest. For example, 
> a network driver shouldn't have to close itself down - the network layer 
> should just do it for it (all the netif_stop_queue() etc crud to make 
> sure that the network layer doesn't try to touch it). 
> 
> So we're clearly not there yet. But after having yet another random 
> machine that just didn't resume because of yet another new random driver, 
> I'm just not seeing anything else that is viable long-term.
> 
> IOW, we can ask driver writers to write perfect code, or we can spend some 
> effort on trying to make things work even when they don't. I know which 
> approach I consider to be realistic.

So, does it mean the patch looks reasonable? ;-)

Rafael

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Linus Torvalds]

On Fri, 16 Jan 2009, Rafael J. Wysocki wrote:
> 
> So, does it mean the patch looks reasonable? ;-)

I haven't tested it myself, and I probably won't have time to before I 
leave for LCA, but it looks reasonable. I'd suggest replacing this:

+               if (pci_dev->current_state != PCI_D0) {
+                       /*
+                        * Warn users if the driver puts the device into a low
+                        * power state, but doesn't bother to save its config
+                        * registers.
+                        */
+                       WARN_ON(true);
+                       goto Fixup;

with the much simpler

	if (WARN_ON_ONCE(pci_dev->current_state != PCI_D0))
		goto Fixup;

instead, because we don't want to get millions of warnings, and it's kind 
of silly to have WARN_ON(true) inside a conditional code-sequence.

			Linus

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Alan Stern]

On Fri, 16 Jan 2009, Rafael J. Wysocki wrote:

> IMO suspend-resume is quite difficult to implement in a PCI driver unless
> the driver writer knows very well how PCI PM is supposed to work and interact
> with things like interrupts control, ACPI etc.

I agree.  There's plenty of boilerplate PCI stuff in the USB
suspend/resume routines which really belongs in the PCI core.  When we
switch over to the new framework (perhaps as soon as 2.6.30) the driver
code will be a lot simpler.

Alan Stern

[PATCH] irq: provide debug_poll_all_shared_irqs() method under CONFIG_DEBUG_SHIRQ

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> > I did suggest that we could also add some test-infrastructure like the 
> > DEBUG_SHIRQ thing we already have, which sends an interrupt 
> > immediately on resume after interrupts have been enabled. That will 
> > likely uncover a _lot_ of problems, and it's almost certainly worth 
> > doing regardless.
> 
> For the record, I put that onto my todo list, which unfortunately is 
> quite long at the moment (and getting longer).

as i mentioned it in the private thread to Linus, kernel/irq/spurious.c's 
misrouted_irq(-1) function function is precisely what we want to call at 
resume time.

See the patch below, it factors this functionality out and provides a 
debug_poll_shared_irqs() function that you can call from the resume path. 

In the !DEBUG_SHIRQ case it will do nothing, in the DEBUG_SHIRQ case it 
will iterate through all registered IRQs (which are capable of handling 
shared irq lines) and call them (if they are not disabled explicitly).

The sooner we start seeing these kinds of problems the better - delaying 
debug patches to after feature work is never a good idea - delaying 
feature changes works much better ;-)

	Ingo

------------------>
>From 74296a8ed6aa3c5bf672808ada690de7ba323ecc Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Fri, 16 Jan 2009 17:43:50 +0100
Subject: [PATCH] irq: provide debug_poll_all_shared_irqs() method under CONFIG_DEBUG_SHIRQ

Provide a shared interrupt debug facility under CONFIG_DEBUG_SHIRQ:
it uses the existing irqpoll facilities to iterate through all
registered interrupt handlers and call those which can handle shared
IRQ lines.

This can be handy for suspend/resume debugging: if we call this function
early during resume we can trigger crashes in those drivers which have
incorrect assumptions about when exactly their ISRs will be called
during suspend/resume.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/interrupt.h |    6 ++++++
 kernel/irq/spurious.c     |   14 +++++++++++++-
 2 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 9127f6b..468e3a2 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -462,6 +462,12 @@ static inline void init_irq_proc(void)
 }
 #endif
 
+#if defined(CONFIG_GENERIC_HARDIRQS) && defined(CONFIG_DEBUG_SHIRQ)
+extern void debug_poll_all_shared_irqs(void);
+#else
+static inline void debug_poll_all_shared_irqs(void) { }
+#endif
+
 int show_interrupts(struct seq_file *p, void *v);
 
 struct irq_desc;
diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
index dd364c1..4d56829 100644
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -104,7 +104,7 @@ static int misrouted_irq(int irq)
 	return ok;
 }
 
-static void poll_spurious_irqs(unsigned long dummy)
+static void poll_all_shared_irqs(void)
 {
 	struct irq_desc *desc;
 	int i;
@@ -123,11 +123,23 @@ static void poll_spurious_irqs(unsigned long dummy)
 
 		try_one_irq(i, desc);
 	}
+}
+
+static void poll_spurious_irqs(unsigned long dummy)
+{
+	poll_all_shared_irqs();
 
 	mod_timer(&poll_spurious_irq_timer,
 		  jiffies + POLL_SPURIOUS_IRQ_INTERVAL);
 }
 
+#ifdef CONFIG_DEBUG_SHIRQ
+void debug_poll_all_shared_irqs(void)
+{
+	poll_all_shared_irqs();
+}
+#endif
+
 /*
  * If 99,900 of the previous 100,000 interrupts have not been handled
  * then assume that the IRQ is stuck in some manner. Drop a diagnostic

Re: [PATCH] irq: provide debug_poll_all_shared_irqs() method under CONFIG_DEBUG_SHIRQ

[Linus Torvalds]

On Fri, 16 Jan 2009, Ingo Molnar wrote:
> 
> The sooner we start seeing these kinds of problems the better - delaying 
> debug patches to after feature work is never a good idea - delaying 
> feature changes works much better ;-)

The problem with this particular debug feature is that when it finds 
something, the machine usually just hangs because the interrupt handler is 
just looping over reading some status register that returns 0xff, and 
thinks that means "interrupt pending".

Or maybe it's not looping, it's using some value as an index, or just 
isn't ready to be called at all because it shut down things and as a 
result it actually crashes with a NULL pointer dereference or something.

And if you are in the middle of a resume, the result is not pretty. The 
crash is often totally undebuggable. You can test it by using the 
'no_console_suspend' and 'acpi_sleep=s3_bios' kernel parameters, and 
_maybe_ it will get things working. You're probably more likely to get a 
working setup if you use VGA text mode rather than FBCON, though.

In other words: the actual problems you hit tend to be very similar to the 
nasty problems that DEBUG_SHIRQ can uncover - but with the _normal_ 
DEBUG_SHIRQ you at least usually have a console, so you can add printk's 
to see where it happened, and you get oopses etc.

So I do think this is worth doing as a "make it easier to _see_ all the 
nasty suspend/resume problems", but at the same time it's going to be 
really really painful to debug, and it's not an option we can likely ever 
enable in some distro kernel - because a regular user can't even send a 
bug-report (just a "resume used to work, now it stopped working" thing).

			Linus

Re: [PATCH] irq: provide debug_poll_all_shared_irqs() method under CONFIG_DEBUG_SHIRQ

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, 16 Jan 2009, Ingo Molnar wrote:
> > 
> > The sooner we start seeing these kinds of problems the better - 
> > delaying debug patches to after feature work is never a good idea - 
> > delaying feature changes works much better ;-)
> 
> The problem with this particular debug feature is that when it finds 
> something, the machine usually just hangs because the interrupt handler 
> is just looping over reading some status register that returns 0xff, and 
> thinks that means "interrupt pending".
> 
> Or maybe it's not looping, it's using some value as an index, or just 
> isn't ready to be called at all because it shut down things and as a 
> result it actually crashes with a NULL pointer dereference or something.
> 
> And if you are in the middle of a resume, the result is not pretty. The 
> crash is often totally undebuggable. You can test it by using the 
> 'no_console_suspend' and 'acpi_sleep=s3_bios' kernel parameters, and 
> _maybe_ it will get things working. You're probably more likely to get a 
> working setup if you use VGA text mode rather than FBCON, though.
> 
> In other words: the actual problems you hit tend to be very similar to 
> the nasty problems that DEBUG_SHIRQ can uncover - but with the _normal_ 
> DEBUG_SHIRQ you at least usually have a console, so you can add printk's 
> to see where it happened, and you get oopses etc.
> 
> So I do think this is worth doing as a "make it easier to _see_ all the 
> nasty suspend/resume problems", but at the same time it's going to be 
> really really painful to debug, and it's not an option we can likely 
> ever enable in some distro kernel - because a regular user can't even 
> send a bug-report (just a "resume used to work, now it stopped working" 
> thing).

Yeah, indeed. Perhaps a sysctl that turns it off by default might help? 
Under CONFIG_PM_DEBUG or so.

	Ingo

Re: [PATCH] irq: provide debug_poll_all_shared_irqs() method under CONFIG_DEBUG_SHIRQ

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > So I do think this is worth doing as a "make it easier to _see_ all 
> > the nasty suspend/resume problems", but at the same time it's going to 
> > be really really painful to debug, and it's not an option we can 
> > likely ever enable in some distro kernel - because a regular user 
> > can't even send a bug-report (just a "resume used to work, now it 
> > stopped working" thing).
> 
> Yeah, indeed. Perhaps a sysctl that turns it off by default might help? 
> Under CONFIG_PM_DEBUG or so.

btw., i think the main problem with debugging is whether one can 'see' the 
problem, literally.

Would it be possible to have a s2ram debug/test mode that puts as much 
stuff into low-power mode as possible - except for the console. That would 
still include a fair amount of random drivers that is at issue here. And 
once the crash/hang happens while it can be seen where it hangs/crashes, 
it becomes a lot more debuggable IMO.

Or is this impossible / not feasible / inefficient?

	Ingo

Re: [PATCH] irq: provide debug_poll_all_shared_irqs() method under CONFIG_DEBUG_SHIRQ

[Linus Torvalds]

On Fri, 16 Jan 2009, Ingo Molnar wrote:
> 
> Would it be possible to have a s2ram debug/test mode that puts as much 
> stuff into low-power mode as possible - except for the console.

Yeah, well.. Our own Documentation says that "no_console_suspend" is known 
to work with serial consoles and VGA, but that's not actually true. It 
_often_ works with those, especially if the serial line and the graphics 
are basically on the southbridge (ie integrated). But we do not really 
track any more complex topology.

But we could certainly do a "put everything but PCI bridge chips into D3" 
mode, and then do the whole "echo core > /sys/power/pm_test" thing. Right 
now I _think_ we don't actually put chips into D3 by default, because it 
has caused so many problems (ie only drivers with real suspend routines 
will put their chips into D3).

Of course, if the devices aren't on the power rail, they will lose power 
if we do a _real_ suspend (as opposed to that pm_test thing) and then come 
up in D3 anyway, so that means that we literally lack a lot of test 
coverage on this. 

You can try it out: right now I think we do

	pci_pm_set_unknown_state(pci_dev);

And you could replace it with something like

	/* Bridge chip or VGA controller? */
	if (pci_dev->subordinate || dev_is_graphics(pci_dev))
		pci_pm_set_unknown_state(pci_dev);
	else
		pci_set_power_state(PCI_D3cold);

and see what happens. I suspect a lot of machines are unhappy, and I 
suspect they are unhappy exactly because of the fact that this will 
suddenly show all the irq problems more.

But that, together with 'no_console_suspend', your irq injection, _and_ 
then using pm_test=core should probably be able to give some interesting 
debug data.

I dunno. I'm handwaving. There may be other problems.

			Linus

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Rafael J. Wysocki]

On Friday 16 January 2009, Linus Torvalds wrote:
> 
> On Fri, 16 Jan 2009, Rafael J. Wysocki wrote:
> > 
> > So, does it mean the patch looks reasonable? ;-)
> 
> I haven't tested it myself, and I probably won't have time to before I 
> leave for LCA, but it looks reasonable. I'd suggest replacing this:
> 
> +               if (pci_dev->current_state != PCI_D0) {
> +                       /*
> +                        * Warn users if the driver puts the device into a low
> +                        * power state, but doesn't bother to save its config
> +                        * registers.
> +                        */
> +                       WARN_ON(true);
> +                       goto Fixup;
> 
> with the much simpler
> 
> 	if (WARN_ON_ONCE(pci_dev->current_state != PCI_D0))
> 		goto Fixup;
> 
> instead, because we don't want to get millions of warnings, and it's kind 
> of silly to have WARN_ON(true) inside a conditional code-sequence.

I'll do that, thanks.

Somehow, I didn't realize that WARN_ON_ONCE() returned !!(its argument).

Rafael

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Rafael J. Wysocki]

On Friday 16 January 2009, Rafael J. Wysocki wrote:
> On Friday 16 January 2009, Linus Torvalds wrote:
> > 
> > On Fri, 16 Jan 2009, Rafael J. Wysocki wrote:
> > > 
> > > So, does it mean the patch looks reasonable? ;-)
> > 
> > I haven't tested it myself, and I probably won't have time to before I 
> > leave for LCA, but it looks reasonable. I'd suggest replacing this:
> > 
> > +               if (pci_dev->current_state != PCI_D0) {
> > +                       /*
> > +                        * Warn users if the driver puts the device into a low
> > +                        * power state, but doesn't bother to save its config
> > +                        * registers.
> > +                        */
> > +                       WARN_ON(true);
> > +                       goto Fixup;
> > 
> > with the much simpler
> > 
> > 	if (WARN_ON_ONCE(pci_dev->current_state != PCI_D0))
> > 		goto Fixup;
> > 
> > instead, because we don't want to get millions of warnings, and it's kind 
> > of silly to have WARN_ON(true) inside a conditional code-sequence.
> 
> I'll do that, thanks.
> 
> Somehow, I didn't realize that WARN_ON_ONCE() returned !!(its argument).

Updated patch follows.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PCI PM: Restore standard config registers of all devices early

There is a problem in our handling of suspend-resume of PCI devices
that many of them have their standard config registers restored with
interrupts enabled and they are put into the full power state with
interrupts enabled as well.  This may lead to the following scenario:
* an interrupt vector is shared between two or more devices
* one device is resumed earlier and generates an interrupt
* the interrupt handler of another device tries to handle it and
  attempts to access the device the config space of which hasn't
  been restored yet and/or which still is in a low power state
* the system crashes as a result

To prevent this from happening we should restore the standard
configuration registers of all devices with interrupts disabled and
we should put them into the D0 power state right after that.
Unfortunately, this cannot be done using the existing
pci_set_power_state(), because it can sleep.  Also, to do it we have
to make sure that the config spaces of all devices were actually
saved during suspend.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci-driver.c |   91 +++++++++++++----------------------------------
 drivers/pci/pci.c        |   63 +++++++++++++++++++++++++++++---
 drivers/pci/pci.h        |    6 +++
 include/linux/pci.h      |    5 ++
 4 files changed, 95 insertions(+), 70 deletions(-)

Index: linux-2.6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-driver.c
+++ linux-2.6/drivers/pci/pci-driver.c
@@ -355,17 +355,27 @@ static int pci_legacy_suspend(struct dev
 	int i = 0;
 
 	if (drv && drv->suspend) {
+		pci_dev->state_saved = false;
+
 		i = drv->suspend(pci_dev, state);
 		suspend_report_result(drv->suspend, i);
-	} else {
-		pci_save_state(pci_dev);
-		/*
-		 * This is for compatibility with existing code with legacy PM
-		 * support.
-		 */
-		pci_pm_set_unknown_state(pci_dev);
+		if (i)
+			return i;
+
+		if (pci_dev->state_saved)
+			goto Fixup;
+
+		if (WARN_ON_ONCE(pci_dev->current_state != PCI_D0))
+			goto Fixup;
 	}
 
+	pci_save_state(pci_dev);
+	/*
+	 * This is for compatibility with existing code with legacy PM support.
+	 */
+	pci_pm_set_unknown_state(pci_dev);
+
+ Fixup:
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
 
 	return i;
@@ -386,81 +396,34 @@ static int pci_legacy_suspend_late(struc
 
 static int pci_legacy_resume_early(struct device *dev)
 {
-	int error = 0;
 	struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * drv = pci_dev->driver;
 
-	pci_fixup_device(pci_fixup_resume_early, pci_dev);
-
-	if (drv && drv->resume_early)
-		error = drv->resume_early(pci_dev);
-	return error;
+	return drv && drv->resume_early ?
+			drv->resume_early(pci_dev) : 0;
 }
 
 static int pci_legacy_resume(struct device *dev)
 {
-	int error;
 	struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * drv = pci_dev->driver;
 
 	pci_fixup_device(pci_fixup_resume, pci_dev);
 
-	if (drv && drv->resume) {
-		error = drv->resume(pci_dev);
-	} else {
-		/* restore the PCI config space */
-		pci_restore_state(pci_dev);
-		error = pci_pm_reenable_device(pci_dev);
-	}
-	return error;
+	return drv && drv->resume ?
+			drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);
 }
 
 /* Auxiliary functions used by the new power management framework */
 
-static int pci_restore_standard_config(struct pci_dev *pci_dev)
-{
-	struct pci_dev *parent = pci_dev->bus->self;
-	int error = 0;
-
-	/* Check if the device's bus is operational */
-	if (!parent || parent->current_state == PCI_D0) {
-		pci_restore_state(pci_dev);
-		pci_update_current_state(pci_dev, PCI_D0);
-	} else {
-		dev_warn(&pci_dev->dev, "unable to restore config, "
-			"bridge %s in low power state D%d\n", pci_name(parent),
-			parent->current_state);
-		pci_dev->current_state = PCI_UNKNOWN;
-		error = -EAGAIN;
-	}
-
-	return error;
-}
-
-static bool pci_is_bridge(struct pci_dev *pci_dev)
-{
-	return !!(pci_dev->subordinate);
-}
-
 static void pci_pm_default_resume_noirq(struct pci_dev *pci_dev)
 {
-	if (pci_restore_standard_config(pci_dev))
-		pci_fixup_device(pci_fixup_resume_early, pci_dev);
+	pci_restore_standard_config(pci_dev);
+	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 }
 
 static int pci_pm_default_resume(struct pci_dev *pci_dev)
 {
-	/*
-	 * pci_restore_standard_config() should have been called once already,
-	 * but it would have failed if the device's parent bridge had not been
-	 * in power state D0 at that time.  Check it and try again if necessary.
-	 */
-	if (pci_dev->current_state == PCI_UNKNOWN) {
-		int error = pci_restore_standard_config(pci_dev);
-		if (error)
-			return error;
-	}
-
 	pci_fixup_device(pci_fixup_resume, pci_dev);
 
 	if (!pci_is_bridge(pci_dev))
@@ -575,11 +538,11 @@ static int pci_pm_resume_noirq(struct de
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	pci_pm_default_resume_noirq(pci_dev);
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
 
-	pci_pm_default_resume_noirq(pci_dev);
-
 	if (drv && drv->pm && drv->pm->resume_noirq)
 		error = drv->pm->resume_noirq(dev);
 
@@ -730,11 +693,11 @@ static int pci_pm_restore_noirq(struct d
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
+	pci_pm_default_resume_noirq(pci_dev);
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
 
-	pci_pm_default_resume_noirq(pci_dev);
-
 	if (drv && drv->pm && drv->pm->restore_noirq)
 		error = drv->pm->restore_noirq(dev);
 
Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -22,7 +22,7 @@
 #include <asm/dma.h>	/* isa_dma_bridge_buggy */
 #include "pci.h"
 
-unsigned int pci_pm_d3_delay = 10;
+unsigned int pci_pm_d3_delay = PCI_PM_D3_WAIT;
 
 #ifdef CONFIG_PCI_DOMAINS
 int pci_domains_supported = 1;
@@ -426,6 +426,7 @@ static inline int platform_pci_sleep_wak
  *                           given PCI device
  * @dev: PCI device to handle.
  * @state: PCI power state (D0, D1, D2, D3hot) to put the device into.
+ * @wait: If 'true', wait for the device to change its power state
  *
  * RETURN VALUE:
  * -EINVAL if the requested state is invalid.
@@ -435,7 +436,7 @@ static inline int platform_pci_sleep_wak
  * 0 if device's power state has been successfully changed.
  */
 static int
-pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
+pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state, bool wait)
 {
 	u16 pmcsr;
 	bool need_restore = false;
@@ -480,8 +481,10 @@ pci_raw_set_power_state(struct pci_dev *
 		break;
 	case PCI_UNKNOWN: /* Boot-up */
 		if ((pmcsr & PCI_PM_CTRL_STATE_MASK) == PCI_D3hot
-		 && !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET))
+		 && !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET)) {
 			need_restore = true;
+			wait = true;
+		}
 		/* Fall-through: force to D0 */
 	default:
 		pmcsr = 0;
@@ -491,12 +494,15 @@ pci_raw_set_power_state(struct pci_dev *
 	/* enter specified state */
 	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
 
+	if (!wait)
+		return 0;
+
 	/* Mandatory power management transition delays */
 	/* see PCI PM 1.1 5.6.1 table 18 */
 	if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
 		msleep(pci_pm_d3_delay);
 	else if (state == PCI_D2 || dev->current_state == PCI_D2)
-		udelay(200);
+		udelay(PCI_PM_D2_DELAY);
 
 	dev->current_state = state;
 
@@ -515,7 +521,7 @@ pci_raw_set_power_state(struct pci_dev *
 	if (need_restore)
 		pci_restore_bars(dev);
 
-	if (dev->bus->self)
+	if (wait && dev->bus->self)
 		pcie_aspm_pm_state_change(dev->bus->self);
 
 	return 0;
@@ -585,7 +591,7 @@ int pci_set_power_state(struct pci_dev *
 	if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
 		return 0;
 
-	error = pci_raw_set_power_state(dev, state);
+	error = pci_raw_set_power_state(dev, state, true);
 
 	if (state > PCI_D0 && platform_pci_power_manageable(dev)) {
 		/* Allow the platform to finalize the transition */
@@ -730,6 +736,7 @@ pci_save_state(struct pci_dev *dev)
 	/* XXX: 100% dword access ok here? */
 	for (i = 0; i < 16; i++)
 		pci_read_config_dword(dev, i * 4,&dev->saved_config_space[i]);
+	dev->state_saved = true;
 	if ((i = pci_save_pcie_state(dev)) != 0)
 		return i;
 	if ((i = pci_save_pcix_state(dev)) != 0)
@@ -1378,6 +1385,50 @@ void pci_allocate_cap_save_buffers(struc
 }
 
 /**
+ * pci_restore_standard_config - restore standard config registers of PCI device
+ * @dev: PCI device to handle
+ *
+ * This function assumes that the device's configuration space is accessible.
+ * If the device needs to be powered up, the function will wait for it to
+ * change the state.
+ */
+int pci_restore_standard_config(struct pci_dev *dev)
+{
+	pci_power_t prev_state;
+	int error;
+
+	pci_restore_state(dev);
+	pci_update_current_state(dev, PCI_D0);
+
+	prev_state = dev->current_state;
+	if (prev_state == PCI_D0)
+		return 0;
+
+	error = pci_raw_set_power_state(dev, PCI_D0, false);
+	if (error)
+		return error;
+
+	if (pci_is_bridge(dev)) {
+		if (prev_state > PCI_D1)
+			mdelay(PCI_PM_BUS_WAIT);
+	} else {
+		switch(prev_state) {
+		case PCI_D3cold:
+		case PCI_D3hot:
+			mdelay(pci_pm_d3_delay);
+			break;
+		case PCI_D2:
+			udelay(PCI_PM_D2_DELAY);
+			break;
+		}
+	}
+
+	dev->current_state = PCI_D0;
+
+	return 0;
+}
+
+/**
  * pci_enable_ari - enable ARI forwarding if hardware support it
  * @dev: the PCI device
  */
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -117,6 +117,10 @@ typedef int __bitwise pci_power_t;
 #define PCI_UNKNOWN	((pci_power_t __force) 5)
 #define PCI_POWER_ERROR	((pci_power_t __force) -1)
 
+#define PCI_PM_D2_DELAY	200
+#define PCI_PM_D3_WAIT	10
+#define PCI_PM_BUS_WAIT	50
+
 /** The pci_channel state describes connectivity between the CPU and
  *  the pci device.  If some PCI bus between here and the pci device
  *  has crashed or locked up, this info is reflected here.
@@ -252,6 +256,7 @@ struct pci_dev {
 	unsigned int	ari_enabled:1;	/* ARI forwarding */
 	unsigned int	is_managed:1;
 	unsigned int	is_pcie:1;
+	unsigned int	state_saved:1;
 	pci_dev_flags_t dev_flags;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */
 
Index: linux-2.6/drivers/pci/pci.h
===================================================================
--- linux-2.6.orig/drivers/pci/pci.h
+++ linux-2.6/drivers/pci/pci.h
@@ -49,6 +49,12 @@ extern void pci_disable_enabled_device(s
 extern void pci_pm_init(struct pci_dev *dev);
 extern void platform_pci_wakeup_init(struct pci_dev *dev);
 extern void pci_allocate_cap_save_buffers(struct pci_dev *dev);
+extern int pci_restore_standard_config(struct pci_dev *dev);
+
+static inline bool pci_is_bridge(struct pci_dev *pci_dev)
+{
+	return !!(pci_dev->subordinate);
+}
 
 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);

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Jesse Barnes]

On Friday, January 16, 2009 12:54 pm Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PCI PM: Restore standard config registers of all devices early
>
> There is a problem in our handling of suspend-resume of PCI devices
> that many of them have their standard config registers restored with
> interrupts enabled and they are put into the full power state with
> interrupts enabled as well.  This may lead to the following scenario:
> * an interrupt vector is shared between two or more devices
> * one device is resumed earlier and generates an interrupt
> * the interrupt handler of another device tries to handle it and
>   attempts to access the device the config space of which hasn't
>   been restored yet and/or which still is in a low power state
> * the system crashes as a result
>
> To prevent this from happening we should restore the standard
> configuration registers of all devices with interrupts disabled and
> we should put them into the D0 power state right after that.
> Unfortunately, this cannot be done using the existing
> pci_set_power_state(), because it can sleep.  Also, to do it we have
> to make sure that the config spaces of all devices were actually
> saved during suspend.
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Thanks Rafael, I stuffed this one into my for-linus branch.  I'd like it to go 
through a linux-next cycle before pushing it to Linus, but if he's impatient 
to get it into -rc2 he can always pull sooner (there's only one other small 
fix in there atm).

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Rafael J. Wysocki]

On Friday 16 January 2009, Jesse Barnes wrote:
> On Friday, January 16, 2009 12:54 pm Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > Subject: PCI PM: Restore standard config registers of all devices early
> >
> > There is a problem in our handling of suspend-resume of PCI devices
> > that many of them have their standard config registers restored with
> > interrupts enabled and they are put into the full power state with
> > interrupts enabled as well.  This may lead to the following scenario:
> > * an interrupt vector is shared between two or more devices
> > * one device is resumed earlier and generates an interrupt
> > * the interrupt handler of another device tries to handle it and
> >   attempts to access the device the config space of which hasn't
> >   been restored yet and/or which still is in a low power state
> > * the system crashes as a result
> >
> > To prevent this from happening we should restore the standard
> > configuration registers of all devices with interrupts disabled and
> > we should put them into the D0 power state right after that.
> > Unfortunately, this cannot be done using the existing
> > pci_set_power_state(), because it can sleep.  Also, to do it we have
> > to make sure that the config spaces of all devices were actually
> > saved during suspend.
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Thanks Rafael, I stuffed this one into my for-linus branch.

Thanks!

> I'd like it to go through a linux-next cycle before pushing it to Linus,

Me too.

> but if he's impatient to get it into -rc2 he can always pull sooner (there's
> only one other small fix in there atm).

Well, -rc2 is upon us already.

Thanks,
Rafael

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Linus Torvalds]

On Fri, 16 Jan 2009, Rafael J. Wysocki wrote:
> 
> So, does it mean the patch looks reasonable? ;-)

So not only did it look reasonable, it does seem to actually help my EeePC 
suspend/restore with the wireless driver enabled.

There's something fishy there in that it restores immediately rather than 
waiting for me to press the power button, but I guess that's quite 
possibly the (unrelated) issue with the PM wake event changes that went 
into -rc1. 

It also seems to have trouble restoring the second time, but it still 
seems like a big improvement to not restoring at all ;)

			Linus

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Linus Torvalds]

On Tue, 20 Jan 2009, Linus Torvalds wrote:
> 
> It also seems to have trouble restoring the second time, but it still 
> seems like a big improvement to not restoring at all ;)

.. and this issue seems to be about X restore, not about the kernel. I can 
suspend/restore many many times if I use "acpi_sleep=s3_bios,s3_mode" and 
just stay in text-mode. But X won't restore the second time around.

So this machine does have several issues, but your patch definitely 
improves on parts of it.

			Linus

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Jesse Barnes]

On Monday, January 19, 2009 2:19 pm Linus Torvalds wrote:
> On Tue, 20 Jan 2009, Linus Torvalds wrote:
> > It also seems to have trouble restoring the second time, but it still
> > seems like a big improvement to not restoring at all ;)
>
> .. and this issue seems to be about X restore, not about the kernel. I can
> suspend/restore many many times if I use "acpi_sleep=s3_bios,s3_mode" and
> just stay in text-mode. But X won't restore the second time around.
>
> So this machine does have several issues, but your patch definitely
> improves on parts of it.

Is i915 loaded?  Hopefully you can suspend/resume in text mode using its 
suspend/resume code w/o using the s3_bios stuff?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Rafael J. Wysocki]

On Monday 19 January 2009, Linus Torvalds wrote:
> 
> On Tue, 20 Jan 2009, Linus Torvalds wrote:
> > 
> > It also seems to have trouble restoring the second time, but it still 
> > seems like a big improvement to not restoring at all ;)
> 
> .. and this issue seems to be about X restore, not about the kernel. I can 
> suspend/restore many many times if I use "acpi_sleep=s3_bios,s3_mode" and 
> just stay in text-mode. But X won't restore the second time around.
> 
> So this machine does have several issues, but your patch definitely 
> improves on parts of it.

Good, thanks for testing! :-)

The patch is in the Jesse's tree now.

Thanks,
Rafael

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Linus Torvalds]

On Mon, 19 Jan 2009, Jesse Barnes wrote:
> 
> Is i915 loaded?  Hopefully you can suspend/resume in text mode using its 
> suspend/resume code w/o using the s3_bios stuff?

You're right, that works fine. So text-mode suspends and (immediately) 
resumes without any issues, multiple times. But X doesn't. The second 
resume will just hang when X re-initializes (sometimes I see the cursor, 
so I know X actually started up)

			Linus

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Daniel Qarras]

Hi!

> There's something fishy there in that it restores
> immediately rather than waiting for me to press the power
> button, but I guess that's quite possibly the (unrelated)
> issue with the PM wake event changes that went into -rc1. 

Not sure is this anything related that all but I reported similar issues few days ago, something also happened between 2.6.23 and 2.6.24:

https://lists.linux-foundation.org/pipermail/linux-pm/2009-January/019225.html

It was diagnosed that one workaround was to unload ehci-hcd:

https://lists.linux-foundation.org/pipermail/linux-pm/2009-January/019251.html

Cheers!




      

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Jesse Barnes]

On Monday, January 19, 2009 8:56 pm Linus Torvalds wrote:
> On Mon, 19 Jan 2009, Jesse Barnes wrote:
> > Is i915 loaded?  Hopefully you can suspend/resume in text mode using its
> > suspend/resume code w/o using the s3_bios stuff?
>
> You're right, that works fine. So text-mode suspends and (immediately)
> resumes without any issues, multiple times. But X doesn't. The second
> resume will just hang when X re-initializes (sometimes I see the cursor,
> so I know X actually started up)

Getting register dumps before and after resume (and also preferably before and 
after X VT switches) might help us figure out what's going on (sounds like 
the driver's VT enter routine is broken somehow)...  The xf86-video-intel 
repo at git://git.freedesktop.org/git/xorg/driver/xf86-video-intel has a tool 
called "intel_reg_dumper" in src/reg_dumper that you can use to capture the 
dumps.  If you're having trouble building it just check out an old version 
like 2.5.1; current git needs a very new libdrm to build.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Linus Torvalds]

On Tue, 20 Jan 2009, Jesse Barnes wrote:
>
> On Monday, January 19, 2009 8:56 pm Linus Torvalds wrote:
> > On Mon, 19 Jan 2009, Jesse Barnes wrote:
> > > Is i915 loaded?  Hopefully you can suspend/resume in text mode using its
> > > suspend/resume code w/o using the s3_bios stuff?
> >
> > You're right, that works fine. So text-mode suspends and (immediately)
> > resumes without any issues, multiple times. But X doesn't. The second
> > resume will just hang when X re-initializes (sometimes I see the cursor,
> > so I know X actually started up)
> 
> Getting register dumps before and after resume (and also preferably before and 
> after X VT switches) might help us figure out what's going on (sounds like 
> the driver's VT enter routine is broken somehow)...

So just looking at the Xorg.0.log after rebooting, I can tell that the 
failure looks to be around the time when we do the DRI mapping. A 
successful suspend-resume cycle will look like:

	...
	(WW) intel(0): PRB0_CTL (0x0001f001) indicates ring buffer enabled
	(WW) intel(0): Existing errors found in hardware state.
	(II) intel(0): using SSC reference clock of 100 MHz
	(II) intel(0): Selecting standard 18 bit TMDS pixel format.
	(II) intel(0): Output configuration:
	(II) intel(0):   Pipe A is off
	(II) intel(0):   Display plane A is now disabled and connected to pipe A.
	(II) intel(0):   Pipe B is on
	(II) intel(0):   Display plane B is now enabled and connected to pipe B.
	(II) intel(0):   Output VGA is connected to pipe none
	(II) intel(0):   Output LVDS is connected to pipe B
	(II) intel(0): [drm] mapped front buffer at 0xd1000000, handle = 0xd1000000
	(II) intel(0): [drm] mapped back buffer at 0xd0c00000, handle = 0xd0c00000
	(II) intel(0): [drm] mapped depth buffer at 0xd0800000, handle = 0xd0800000
	(II) intel(0): RandR 1.2 enabled, ignore the following RandR disabled message.
	...

and an unsuccessful one will always get stuck before it logs that "[drm] 
mapped front buffer" thing, but after doing the "Output LVDS is connected 
to pipe B".  I checked a couple of times - the log always ended at that 
same place.

No interesting kernel logs survive this thing (and it's obviously in 
graphics mode, so I can't see any oops if any), so I can't tell what 
happens. But it looks DRI-related.

And btw, I was wrong - it's not always on the second suspend. It's 
happened on the first suspends too. 

			Linus

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Jesse Barnes]

On Tuesday, January 20, 2009 11:22 am Linus Torvalds wrote:
> So just looking at the Xorg.0.log after rebooting, I can tell that the
> failure looks to be around the time when we do the DRI mapping. A
> successful suspend-resume cycle will look like:
>
> 	...
> 	(WW) intel(0): PRB0_CTL (0x0001f001) indicates ring buffer enabled
> 	(WW) intel(0): Existing errors found in hardware state.
> 	(II) intel(0): using SSC reference clock of 100 MHz
> 	(II) intel(0): Selecting standard 18 bit TMDS pixel format.
> 	(II) intel(0): Output configuration:
> 	(II) intel(0):   Pipe A is off
> 	(II) intel(0):   Display plane A is now disabled and connected to pipe A.
> 	(II) intel(0):   Pipe B is on
> 	(II) intel(0):   Display plane B is now enabled and connected to pipe B.
> 	(II) intel(0):   Output VGA is connected to pipe none
> 	(II) intel(0):   Output LVDS is connected to pipe B
> 	(II) intel(0): [drm] mapped front buffer at 0xd1000000, handle =
> 0xd1000000 (II) intel(0): [drm] mapped back buffer at 0xd0c00000, handle =
> 0xd0c00000 (II) intel(0): [drm] mapped depth buffer at 0xd0800000, handle =
> 0xd0800000 (II) intel(0): RandR 1.2 enabled, ignore the following RandR
> disabled message. ...
>
> and an unsuccessful one will always get stuck before it logs that "[drm]
> mapped front buffer" thing, but after doing the "Output LVDS is connected
> to pipe B".  I checked a couple of times - the log always ended at that
> same place.
>
> No interesting kernel logs survive this thing (and it's obviously in
> graphics mode, so I can't see any oops if any), so I can't tell what
> happens. But it looks DRI-related.
>
> And btw, I was wrong - it's not always on the second suspend. It's
> happened on the first suspends too.

Hm it's probably not display programming related then... Sounds like the 
i830_update_dri_buffers() call is failing for some reason.  The kernel driver 
does do some I/O mapping stuff at enter/leavevt time (and therefore 
suspend/resume time), can you reproduce the issue just by VT switching?  If 
so you might be able to see something useful in the kernel log...
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Linus Torvalds]

On Tue, 20 Jan 2009, Jesse Barnes wrote:
> 
> Hm it's probably not display programming related then... Sounds like the 
> i830_update_dri_buffers() call is failing for some reason.  The kernel driver 
> does do some I/O mapping stuff at enter/leavevt time (and therefore 
> suspend/resume time), can you reproduce the issue just by VT switching?  If 
> so you might be able to see something useful in the kernel log...

I was going to say that I've been VT switching and it has worked, but I 
decided to double-check. And yes, I can reproduce it that way. And no, no 
kernel messages that are visible that way either. 

		Linus

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Jesse Barnes]

On Tuesday, January 20, 2009 11:45 am Linus Torvalds wrote:
> On Tue, 20 Jan 2009, Jesse Barnes wrote:
> > Hm it's probably not display programming related then... Sounds like the
> > i830_update_dri_buffers() call is failing for some reason.  The kernel
> > driver does do some I/O mapping stuff at enter/leavevt time (and
> > therefore suspend/resume time), can you reproduce the issue just by VT
> > switching?  If so you might be able to see something useful in the kernel
> > log...
>
> I was going to say that I've been VT switching and it has worked, but I
> decided to double-check. And yes, I can reproduce it that way. And no, no
> kernel messages that are visible that way either.

So the machine is alive enough for you to see the logs?  Can you get a 
backtrace (both kernel & userspace)?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Jesse Barnes]

On Tuesday, January 20, 2009 12:09 pm Jesse Barnes wrote:
> On Tuesday, January 20, 2009 11:45 am Linus Torvalds wrote:
> > On Tue, 20 Jan 2009, Jesse Barnes wrote:
> > > Hm it's probably not display programming related then... Sounds like
> > > the i830_update_dri_buffers() call is failing for some reason.  The
> > > kernel driver does do some I/O mapping stuff at enter/leavevt time (and
> > > therefore suspend/resume time), can you reproduce the issue just by VT
> > > switching?  If so you might be able to see something useful in the
> > > kernel log...
> >
> > I was going to say that I've been VT switching and it has worked, but I
> > decided to double-check. And yes, I can reproduce it that way. And no, no
> > kernel messages that are visible that way either.
>
> So the machine is alive enough for you to see the logs?  Can you get a
> backtrace (both kernel & userspace)?

Oops, just realized by "no messages visible" you probably meant that it really 
was hung rather than you saw no useful messages.  That'll make things more 
difficult.  Which 2D driver are you running?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Jesse Barnes]

On Tuesday, January 20, 2009 12:09 pm Jesse Barnes wrote:
> On Tuesday, January 20, 2009 11:45 am Linus Torvalds wrote:
> > On Tue, 20 Jan 2009, Jesse Barnes wrote:
> > > Hm it's probably not display programming related then... Sounds like
> > > the i830_update_dri_buffers() call is failing for some reason.  The
> > > kernel driver does do some I/O mapping stuff at enter/leavevt time (and
> > > therefore suspend/resume time), can you reproduce the issue just by VT
> > > switching?  If so you might be able to see something useful in the
> > > kernel log...
> >
> > I was going to say that I've been VT switching and it has worked, but I
> > decided to double-check. And yes, I can reproduce it that way. And no, no
> > kernel messages that are visible that way either.
>
> So the machine is alive enough for you to see the logs?  Can you get a
> backtrace (both kernel & userspace)?

Oops, just realized by "no messages visible" you probably meant that it really 
was hung rather than you saw no useful messages.  That'll make things more 
difficult.  Which 2D driver are you running?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Linus Torvalds]

On Wed, 21 Jan 2009, Linus Torvalds wrote:
> 
> I was going to say that I've been VT switching and it has worked, but I 
> decided to double-check. And yes, I can reproduce it that way. And no, no 
> kernel messages that are visible that way either. 

Ahhah!

No kernel messages, because the kernel isn't actually dead. Just X is. 

I verified that by doing a

	while : ; do echo t > /proc/sys/sysrq-trigger; sleep 5; done

while doing these things. I'm travelling, it's 7AM in Hobart now, and I 
don't have another machine, so when X dies in graphics mode, there's not a 
whole lot else I can do to debug it. No other machine to use to ssh into 
the machine from etc..

Also, the reason I at one point thought that VT switching works is that 
quite often it _does_ work. I seem to need some timing or interrupt thing 
happening to trigger the problem, because when I wasn't doing anything 
else, I could VT switch back and forth something like 10 times without 
problems.

That probably also explains why it then happens all the time on 
suspend/resume: other things are most definitely happening.

So I did another shell loop (that also gives an audible clue that things 
are working, even if the machine appears dead):

	while : ; do cat /bin/echo ; sleep 5; done > /dev/audio

and the VT switch failed the very next time I tried it. 

And since I had the sysrq loop running, and since it still made progress, 
I can now say that X seems to hang here:

	Xorg          S f52a3b98  6088  2252   2251
	 f68db340 00003082 c06ea2f8 f52a3b98 c07bb73c c07bec40 c07bec40 c07bec4
	 c07bec40 f4a58980 f4a58be8 c2010c40 00000000 00000000 bfc1f838 0000020
	 bfc1f838 f4a58be8 fffe9b5a 000a037f 00003282 c022d7ff f514fe98 f4a58c5
	Call Trace:
	 [<c022d7ff>] lock_timer_base+0x19/0x35
	 [<c022d983>] __mod_timer+0xba/0xc3
	 [<c05a65ef>] schedule_timeout+0x9f/0xbb
	 [<c022d522>] process_timeout+0x0/0x5
	 [<c05a65ea>] schedule_timeout+0x9a/0xbb
	 [<c038c494>] drm_wait_vblank+0x2d8/0x396
	 [<c0221c4c>] default_wake_function+0x0/0x8
	 [<c038a5ab>] drm_ioctl+0x1a6/0x21e
	 [<c038c1bc>] drm_wait_vblank+0x0/0x396
	 [<c0288d5e>] vfs_ioctl+0x49/0x5f
	 [<c0289527>] do_vfs_ioctl+0x464/0x49f
	 [<c02895a3>] sys_ioctl+0x41/0x58
	 [<c0202d01>] sysenter_do_call+0x12/0x21

ie it seems to be some vblank race. 

Looks like your commit dc1336ff4fe08ae7cfe8301bfd7f0b2cfd31d20a wasn't a
complete solution? Because it's definitely something I have, and I have
compiz, and I see the compiz hangs at VT switch that it claims to have
fixed. 

Added the other people that the git logs say worked on the vblank code
to the Cc.  

What an odd way to debug X lockups. What do you guys usually do? Just 
always make sure to have another machine handy?

				Linus

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Jesse Barnes]

On Tuesday, January 20, 2009 12:15 pm Jesse Barnes wrote:
> On Tuesday, January 20, 2009 12:09 pm Jesse Barnes wrote:
> > On Tuesday, January 20, 2009 11:45 am Linus Torvalds wrote:
> > > On Tue, 20 Jan 2009, Jesse Barnes wrote:
> > > > Hm it's probably not display programming related then... Sounds like
> > > > the i830_update_dri_buffers() call is failing for some reason.  The
> > > > kernel driver does do some I/O mapping stuff at enter/leavevt time
> > > > (and therefore suspend/resume time), can you reproduce the issue just
> > > > by VT switching?  If so you might be able to see something useful in
> > > > the kernel log...
> > >
> > > I was going to say that I've been VT switching and it has worked, but I
> > > decided to double-check. And yes, I can reproduce it that way. And no,
> > > no kernel messages that are visible that way either.
> >
> > So the machine is alive enough for you to see the logs?  Can you get a
> > backtrace (both kernel & userspace)?
>
> Oops, just realized by "no messages visible" you probably meant that it
> really was hung rather than you saw no useful messages.  That'll make
> things more difficult.  Which 2D driver are you running?

And I assume you've tried keeping an ssh session open on the machine while VT 
switching (that's *usually* sufficient for me, in all but the worst of 
crashes)...

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Jesse Barnes]

On Tuesday, January 20, 2009 12:19 pm Linus Torvalds wrote:
> On Wed, 21 Jan 2009, Linus Torvalds wrote:
> > I was going to say that I've been VT switching and it has worked, but I
> > decided to double-check. And yes, I can reproduce it that way. And no, no
> > kernel messages that are visible that way either.
>
> Ahhah!
>
> No kernel messages, because the kernel isn't actually dead. Just X is.
>
> I verified that by doing a
>
> 	while : ; do echo t > /proc/sys/sysrq-trigger; sleep 5; done
>
> while doing these things. I'm travelling, it's 7AM in Hobart now, and I
> don't have another machine, so when X dies in graphics mode, there's not a
> whole lot else I can do to debug it. No other machine to use to ssh into
> the machine from etc..

Ah ok, yeah having one machine to debug with makes things harder, but at the 
last LCA it was all I had too. :)

> 	Xorg          S f52a3b98  6088  2252   2251
> 	 f68db340 00003082 c06ea2f8 f52a3b98 c07bb73c c07bec40 c07bec40 c07bec4
> 	 c07bec40 f4a58980 f4a58be8 c2010c40 00000000 00000000 bfc1f838 0000020
> 	 bfc1f838 f4a58be8 fffe9b5a 000a037f 00003282 c022d7ff f514fe98 f4a58c5
> 	Call Trace:
> 	 [<c022d7ff>] lock_timer_base+0x19/0x35
> 	 [<c022d983>] __mod_timer+0xba/0xc3
> 	 [<c05a65ef>] schedule_timeout+0x9f/0xbb
> 	 [<c022d522>] process_timeout+0x0/0x5
> 	 [<c05a65ea>] schedule_timeout+0x9a/0xbb
> 	 [<c038c494>] drm_wait_vblank+0x2d8/0x396
> 	 [<c0221c4c>] default_wake_function+0x0/0x8
> 	 [<c038a5ab>] drm_ioctl+0x1a6/0x21e
> 	 [<c038c1bc>] drm_wait_vblank+0x0/0x396
> 	 [<c0288d5e>] vfs_ioctl+0x49/0x5f
> 	 [<c0289527>] do_vfs_ioctl+0x464/0x49f
> 	 [<c02895a3>] sys_ioctl+0x41/0x58
> 	 [<c0202d01>] sysenter_do_call+0x12/0x21
>
> ie it seems to be some vblank race.
>
> Looks like your commit dc1336ff4fe08ae7cfe8301bfd7f0b2cfd31d20a wasn't a
> complete solution? Because it's definitely something I have, and I have
> compiz, and I see the compiz hangs at VT switch that it claims to have
> fixed.
>
> Added the other people that the git logs say worked on the vblank code
> to the Cc.

Part of the fix for the vblank hang was in libdrm.  X handles cursor movement 
using SIGALM, so it's pretty easy to get stuck restarting ioctl.  And in the 
case of the vblank ioctl, restart will prevent the kernel timeout code from 
ever triggering, so you'll just see a hang.

> What an odd way to debug X lockups. What do you guys usually do? Just
> always make sure to have another machine handy?

Pretty much; until people start using KMS, having a separate machine is really 
the only way to get info on a machine where X is misbehaving (well that and 
doing what you did with scripts running & sleeping in the background).

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] PCI PM: Restore standard config registers of all devices early (was: Re: EeePC resume failure - timers)

[Jesse Barnes]

On Tuesday, January 20, 2009 12:40 pm Jesse Barnes wrote:
> On Tuesday, January 20, 2009 12:19 pm Linus Torvalds wrote:
> > Added the other people that the git logs say worked on the vblank code
> > to the Cc.
>
> Part of the fix for the vblank hang was in libdrm.  X handles cursor
> movement using SIGALM, so it's pretty easy to get stuck restarting ioctl. 
> And in the case of the vblank ioctl, restart will prevent the kernel
> timeout code from ever triggering, so you'll just see a hang.

Sorry, misspoke here, X's core client scheduler uses SIGALRM (with a 20ms 
default timeslice), not the cursor movement code.