ACPI vs device ordering on resume

During the pci_restore process, the MSI information and the PCI command register are restored properly. But later during resume, inside the ACPI evaluation of the WAK method, the PCI_COMMAND INTX_DISABLE (0x400) flag is being cleared. My guess is that the BIOS ends up doing some resetting of devices.

If I do a suspend-to-ram then resume on a Sony Vaio laptop with sky2 driver, the first interrupt gets misrouted to the original shared IRQ, rather than to the MSI irq expected.

During the pci_restore process, the MSI information and the PCI command register are restored properly. But later during resume, inside the ACPI evaluation of the WAK method, the PCI_COMMAND INTX_DISABLE (0x400) flag is being cleared. My guess is that the BIOS ends up doing some resetting of devices.

I may be able to workaround the problem for this one device, but it brings up a more general issue about what the ordering should be during resume. If ACPI evaluation (which I assume talks to the BIOS), might change device state, it seems that ACPI code should execute before resuming devices not after. But changing the order here seems drastic.

An alternate solution would be to have two pm_ops, one for early_resume and another for late, and split the ACPI work.

kernel/power/main.c +0 -0

@@ -132,12 +132,12 @@
 
 static void suspend_finish(suspend_state_t state)
 {

Quoted 24 lines · Stephen Hemminger

If I do a suspend-to-ram then resume on a Sony Vaio laptop with sky2 driver, the first interrupt gets misrouted to the original shared IRQ, rather than to the MSI irq expected.

During the pci_restore process, the MSI information and the PCI command register are restored properly. But later during resume, inside the ACPI evaluation of the WAK method, the PCI_COMMAND INTX_DISABLE (0x400) flag is being cleared. My guess is that the BIOS ends up doing some resetting of devices.

I may be able to workaround the problem for this one device, but it brings up a more general issue about what the ordering should be during resume. If ACPI evaluation (which I assume talks to the BIOS), might change device state, it seems that ACPI code should execute before resuming devices not after. But changing the order here seems drastic.

An alternate solution would be to have two pm_ops, one for early_resume and another for late, and split the ACPI work.

kernel/power/main.c +0 -0

@@ -132,12 +132,12 @@
 
 static void suspend_finish(suspend_state_t state)
 {

kernel/power/main.c line 142

Quoted 10 lines

kernel/power/main.c +2 -2

+	if (pm_ops && pm_ops->finish)
+		pm_ops->finish(state);
 	device_resume();
 	resume_console();
 	thaw_processes();
 	enable_nonboot_cpus();
-	if (pm_ops && pm_ops->finish)
-		pm_ops->finish(state);
 	pm_restore_console();
 }

Yes, I agree that _WAK needs to come before device_resume().
Need to let any BIOS nasties happen and get over with before we restore device drivers. This is consistent with the wording in ACPI 3.0b (section 7.4) that says
11. _WAK is run
12. OSPM notifies all native device drivefrs of the return from the sleep state transition

However, commit 1a38416cea8ac801ae8f261074721f35317613dc says that _WAK must follow INIT -- ie finish() must come after enable_nonboot_cpus(), and this patch as it stands would violate that.

So it looks like we need this sequence:

enable_nonboot_cpus() /* INIT */
finish() /* _WAK */
device_resume()

So it looks like we need this sequence:

enable_nonboot_cpus() /* INIT */
finish() /* _WAK */
device_resume()

Quoted 5 lines

So it looks like we need this sequence:

enable_nonboot_cpus() /* INIT */
finish() /* _WAK */
device_resume()

Can somebody remind me about this immediately after 2.6.19?

No way am I going to make that kind of a major ordering change right now, especially as the thing isn't apparently even a real regression (just more fallout from enabling MSI).

It would be good if people who are affected (and people in general that are interested in power management) would test out the patch.

In particular, this is an even bigger change than the one suggested by Stephen. It means, for example, that we will have device resume (and process thawing) being called when we're in SMP mode - and that may or may not have issues. So this is a really scary patch to me, no way in hell will I apply it right now.

But if people try it out, it would be good..

Btw, this is a clear example of where it might be good to start actively using the "early_resume" thing, and do PCI bus resume there. We might want to do early_resume _before_ calling the bios (I'm not at all convinced that the firmware will do the right thing without the PCI buses set up, for example), and _before_ thawing processes. Then, we might let individual devices do their "device resume" in the normal late resume phase.

Greg already carries the patch around for that PCI bus early resume thing, I think.

We need to test these things.

Linus

----

kernel/power/main.c +3 -3

index 873228c..2989609 100644
@@ -132,12 +132,12 @@ int suspend_enter(suspend_state_t state)
 
 static void suspend_finish(suspend_state_t state)
 {
-	device_resume();
-	resume_console();
-	thaw_processes();
 	enable_nonboot_cpus();
 	if (pm_ops && pm_ops->finish)
 		pm_ops->finish(state);
+	device_resume();
+	resume_console();
+	thaw_processes();
 	pm_restore_console();
 }
 

Hi!

Quoted 2 lines

Quoted 2 lines

So it looks like we need this sequence:

Quoted 5 lines

Quoted 3 lines

enable_nonboot_cpus() /* INIT */
finish() /* _WAK */
device_resume()

Can somebody remind me about this immediately after 2.6.19?

Remind. But note that freezer is not yet SMP safe... Rafael is working on that.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

So it looks like we need this sequence:

enable_nonboot_cpus() /* INIT */
finish() /* _WAK */
device_resume()

Can somebody remind me about this immediately after 2.6.19?

Quoted 2 lines

Remind. But note that freezer is not yet SMP safe... Rafael is working on that.

Yup.

BTW, have you looked at the last version of the patch for the handling of stopped tasks (appended just in case, full discussion at:
http://lists.osdl.org/pipermail/linux-pm/2006-November/004214.html)?

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 kernel/power/process.c |   36 ++++++++++++++++++++++++++++++------
 kernel/signal.c        |    2 +-
 2 files changed, 31 insertions(+), 7 deletions(-)

Index: linux-2.6.19-rc6-mm2/kernel/power/process.c
===================================================================

kernel/power/process.c +30 -6

@@ -28,8 +28,7 @@ static inline int freezeable(struct task
 	if ((p == current) || 
 	    (p->flags & PF_NOFREEZE) ||
 	    (p->exit_state == EXIT_ZOMBIE) ||
-	    (p->exit_state == EXIT_DEAD) ||
-	    (p->state == TASK_STOPPED))
+	    (p->exit_state == EXIT_DEAD))
 		return 0;
 	return 1;
 }

@@ -81,6 +80,11 @@ static void cancel_freezing(struct task_
 	}
 }
 
+static inline int stopped_and_freezing(struct task_struct *p)
+{
+	return p->state == TASK_STOPPED && freezing(p);
+}
+
 static inline int is_user_space(struct task_struct *p)
 {
 	return p->mm && !(p->flags & PF_BORROWED_MM);

@@ -103,9 +107,11 @@ static unsigned int try_to_freeze_tasks(
 			if (frozen(p))
 				continue;
 
-			if (p->state == TASK_TRACED &&
-			    (frozen(p->parent) ||
-			     p->parent->state == TASK_STOPPED)) {
+			if (stopped_and_freezing(p))
+				continue;
+
+			if (p->state == TASK_TRACED && (frozen(p->parent) ||
+			    stopped_and_freezing(p->parent))) {
 				cancel_freezing(p);
 				continue;
 			}

@@ -149,7 +155,8 @@ static unsigned int try_to_freeze_tasks(
 			if (is_user_space(p) == !freeze_user_space)
 				continue;
 
-			if (freezeable(p) && !frozen(p))
+			if (freezeable(p) && !frozen(p) &&
+			    p->state != TASK_STOPPED && p->state != TASK_TRACED)
 				printk(KERN_ERR " %s\n", p->comm);
 
 			cancel_freezing(p);

@@ -185,6 +192,18 @@ int freeze_processes(void)
 	return 0;
 }
 
+static void release_stopped_tasks(void)
+{
+	struct task_struct *g, *p;
+
+	read_lock(&tasklist_lock);
+	do_each_thread(g, p) {
+		if (stopped_and_freezing(p))
+			cancel_freezing(p);
+	} while_each_thread(g, p);
+	read_unlock(&tasklist_lock);
+}
+
 static void thaw_tasks(int thaw_user_space)
 {
 	struct task_struct *g, *p;

@@ -197,6 +216,10 @@ static void thaw_tasks(int thaw_user_spa
 		if (is_user_space(p) == !thaw_user_space)
 			continue;
 
+		if (!frozen(p) &&
+		    (p->state == TASK_STOPPED || p->state == TASK_TRACED))
+			continue;
+
 		if (!thaw_process(p))
 			printk(KERN_WARNING " Strange, %s not stopped\n",
 				p->comm );

@@ -207,6 +230,7 @@ static void thaw_tasks(int thaw_user_spa
 void thaw_processes(void)
 {
 	printk("Restarting tasks ... ");
+	release_stopped_tasks();
 	thaw_tasks(FREEZER_KERNEL_THREADS);
 	thaw_tasks(FREEZER_USER_SPACE);
 	schedule();

Index: linux-2.6.19-rc6-mm2/kernel/signal.c
===================================================================

kernel/signal.c +1 -1

@@ -1937,9 +1937,9 @@ int get_signal_to_deliver(siginfo_t *inf
 	sigset_t *mask = &current->blocked;
 	int signr = 0;
 
+relock:
 	try_to_freeze();
 
-relock:
 	spin_lock_irq(&current->sighand->siglock);
 	for (;;) {
 		struct k_sigaction *ka;

Hi!

Quoted 13 lines

Quoted 10 lines

Quoted 7 lines

Quoted 5 lines

So it looks like we need this sequence:

enable_nonboot_cpus() /* INIT */
finish() /* _WAK */
device_resume()

Can somebody remind me about this immediately after 2.6.19?

Remind. But note that freezer is not yet SMP safe... Rafael is working on that.

Yup.

Quoted 3 lines

BTW, have you looked at the last version of the patch for the handling of stopped tasks (appended just in case, full discussion at:
http://lists.osdl.org/pipermail/linux-pm/2006-November/004214.html)?

Well, I took a look.. and decided I'd like to find the place in kernel where I can add try_to_freeze() and fix the TASK_STOPPED processes. I hope such place exists.

Quoted 6 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 kernel/power/process.c |   36 ++++++++++++++++++++++++++++++------
 kernel/signal.c        |    2 +-
 2 files changed, 31 insertions(+), 7 deletions(-)

kernel/power/process.c static inline int freezeable(struct task · line 31

Quoted 11 lines

Index: linux-2.6.19-rc6-mm2/kernel/power/process.c
===================================================================

kernel/power/process.c +1 -2

@@ -28,8 +28,7 @@ static inline int freezeable(struct task
 	if ((p == current) || 
 	    (p->flags & PF_NOFREEZE) ||
 	    (p->exit_state == EXIT_ZOMBIE) ||
-	    (p->exit_state == EXIT_DEAD) ||
-	    (p->state == TASK_STOPPED))
+	    (p->exit_state == EXIT_DEAD))

Quoted 19 lines

kernel/power/process.c +5 -0

 		return 0;
 	return 1;
 }

@@ -81,6 +80,11 @@ static void cancel_freezing(struct task_
 	}
 }
 
+static inline int stopped_and_freezing(struct task_struct *p)
+{
+	return p->state == TASK_STOPPED && freezing(p);
+}
+
 static inline int is_user_space(struct task_struct *p)
 {
 	return p->mm && !(p->flags & PF_BORROWED_MM);

@@ -103,9 +107,11 @@ static unsigned int try_to_freeze_tasks(
 			if (frozen(p))
 				continue;
 

kernel/power/process.c static unsigned int try_to_freeze_tasks( · line 111

Quoted 5 lines

kernel/power/process.c +2 -3

-			if (p->state == TASK_TRACED &&
-			    (frozen(p->parent) ||
-			     p->parent->state == TASK_STOPPED)) {
+			if (stopped_and_freezing(p))
+				continue;

kernel/power/process.c static unsigned int try_to_freeze_tasks( · line 114

Quoted 3 lines

kernel/power/process.c +3 -0

+
+			if (p->state == TASK_TRACED && (frozen(p->parent) ||
+			    stopped_and_freezing(p->parent))) {

kernel/power/process.c static unsigned int try_to_freeze_tasks( · line 160

Quoted 11 lines

kernel/power/process.c +2 -1

 				cancel_freezing(p);
 				continue;
 			}

@@ -149,7 +155,8 @@ static unsigned int try_to_freeze_tasks(
 			if (is_user_space(p) == !freeze_user_space)
 				continue;
 
-			if (freezeable(p) && !frozen(p))
+			if (freezeable(p) && !frozen(p) &&
+			    p->state != TASK_STOPPED && p->state != TASK_TRACED)
 				printk(KERN_ERR " %s\n", p->comm);

Quoted 25 lines

kernel/power/process.c +12 -0

 			cancel_freezing(p);

@@ -185,6 +192,18 @@ int freeze_processes(void)
 	return 0;
 }
 
+static void release_stopped_tasks(void)
+{
+	struct task_struct *g, *p;
+
+	read_lock(&tasklist_lock);
+	do_each_thread(g, p) {
+		if (stopped_and_freezing(p))
+			cancel_freezing(p);
+	} while_each_thread(g, p);
+	read_unlock(&tasklist_lock);
+}
+
 static void thaw_tasks(int thaw_user_space)
 {
 	struct task_struct *g, *p;

@@ -197,6 +216,10 @@ static void thaw_tasks(int thaw_user_spa
 		if (is_user_space(p) == !thaw_user_space)
 			continue;
 

kernel/power/process.c static void thaw_tasks(int thaw_user_spa · line 222

Quoted 4 lines

kernel/power/process.c +4 -0

+		if (!frozen(p) &&
+		    (p->state == TASK_STOPPED || p->state == TASK_TRACED))
+			continue;
+

kernel/power/process.c static void thaw_tasks(int thaw_user_spa · line 233

Quoted 8 lines

kernel/power/process.c +1 -0

 		if (!thaw_process(p))
 			printk(KERN_WARNING " Strange, %s not stopped\n",
 				p->comm );

@@ -207,6 +230,7 @@ static void thaw_tasks(int thaw_user_spa
 void thaw_processes(void)
 {
 	printk("Restarting tasks ... ");
+	release_stopped_tasks();

Quoted 11 lines

kernel/power/process.c +0 -0

 	thaw_tasks(FREEZER_KERNEL_THREADS);
 	thaw_tasks(FREEZER_USER_SPACE);
 	schedule();

Index: linux-2.6.19-rc6-mm2/kernel/signal.c
===================================================================

kernel/signal.c +0 -0

@@ -1937,9 +1937,9 @@ int get_signal_to_deliver(siginfo_t *inf
 	sigset_t *mask = &current->blocked;
 	int signr = 0;
 

kernel/signal.c int get_signal_to_deliver(siginfo_t *inf · line 1945

Quoted 7 lines

kernel/signal.c +1 -1

+relock:
 	try_to_freeze();
 
-relock:
 	spin_lock_irq(&current->sighand->siglock);
 	for (;;) {
 		struct k_sigaction *ka;

Quoted 7 lines

Quoted 5 lines

So it looks like we need this sequence:

enable_nonboot_cpus() /* INIT */
finish() /* _WAK */
device_resume()

Can somebody remind me about this immediately after 2.6.19?

Remind. But note that freezer is not yet SMP safe... Rafael is working on that.

Quoted 11 lines · Pavel Machek

Quoted 8 lines

Quoted 6 lines

So it looks like we need this sequence:

enable_nonboot_cpus() /* INIT */
finish() /* _WAK */
device_resume()

Can somebody remind me about this immediately after 2.6.19?

Remind. But note that freezer is not yet SMP safe... Rafael is working on that.

Thanks.

On the other hand, I really wonder (and suspect) whether the problem isn't really the freezer or even the kernel resume ordering, but simply an ACPI internal resume ordering thing.

Doesn't ACPI have per-device "WAK" calls anyway? Shouldn't we just call those _individually_ as we walk the device tree (perhaps in the "early_resume" stage) rather than calling them all in one chunk?

Linus

Quoted 25 lines · Linus Torvalds

Quoted 11 lines · Pavel Machek

Quoted 8 lines

Quoted 6 lines

So it looks like we need this sequence:

enable_nonboot_cpus() /* INIT */
finish() /* _WAK */
device_resume()

Can somebody remind me about this immediately after 2.6.19?

Remind. But note that freezer is not yet SMP safe... Rafael is working on that.

Thanks.

On the other hand, I really wonder (and suspect) whether the problem isn't really the freezer or even the kernel resume ordering, but simply an ACPI internal resume ordering thing.

Doesn't ACPI have per-device "WAK" calls anyway? Shouldn't we just call those _individually_ as we walk the device tree (perhaps in the "early_resume" stage) rather than calling them all in one chunk?

Quoted 2 lines

Linus

_WAK method is system-wide. Individual objects do not have their own resume methods.
One way of reordering internal ACPI resume is done in patch series to 7122, I mentioned that earlier.
It's possible to resume ACPI devices after execution of _WAK in pm->finish.

Quoted 29 lines · On Fri, 01 Dec 2006 20:45:51 +0300 Alexey Starikovskiy…

Quoted 27 lines · Linus Torvalds

Quoted 7 lines · Pavel Machek

Quoted 7 lines

Quoted 7 lines

So it looks like we need this sequence:

enable_nonboot_cpus() /* INIT */
finish() /* _WAK */
device_resume()

Quoted 7 lines

Quoted 3 lines

Can somebody remind me about this immediately after 2.6.19?

Remind. But note that freezer is not yet SMP safe... Rafael is working on that.

Thanks.

On the other hand, I really wonder (and suspect) whether the problem isn't really the freezer or even the kernel resume ordering, but simply an ACPI internal resume ordering thing.

Doesn't ACPI have per-device "WAK" calls anyway? Shouldn't we just call those _individually_ as we walk the device tree (perhaps in the "early_resume" stage) rather than calling them all in one chunk?

Linus

Quoted 4 lines

_WAK method is system-wide. Individual objects do not have their own resume methods. One way of reordering internal ACPI resume is done in patch series to 7122, I mentioned that earlier. It's possible to resume ACPI devices after execution of _WAK in pm->finish.

Does it solve the original problem where finish() was getting run after device_resume(), and finish() was corrupting PCI register settings like MSI?

Quoted 24 lines · Stephen Hemminger

If I do a suspend-to-ram then resume on a Sony Vaio laptop with sky2 driver, the first interrupt gets misrouted to the original shared IRQ, rather than to the MSI irq expected.

During the pci_restore process, the MSI information and the PCI command register are restored properly. But later during resume, inside the ACPI evaluation of the WAK method, the PCI_COMMAND INTX_DISABLE (0x400) flag is being cleared. My guess is that the BIOS ends up doing some resetting of devices.

I may be able to workaround the problem for this one device, but it brings up a more general issue about what the ordering should be during resume. If ACPI evaluation (which I assume talks to the BIOS), might change device state, it seems that ACPI code should execute before resuming devices not after. But changing the order here seems drastic.

An alternate solution would be to have two pm_ops, one for early_resume and another for late, and split the ACPI work.

kernel/power/main.c +0 -0

@@ -132,12 +132,12 @@
 
 static void suspend_finish(suspend_state_t state)
 {

kernel/power/main.c line 142

Quoted 10 lines

kernel/power/main.c +2 -2

+	if (pm_ops && pm_ops->finish)
+		pm_ops->finish(state);
 	device_resume();
 	resume_console();
 	thaw_processes();
 	enable_nonboot_cpus();
-	if (pm_ops && pm_ops->finish)
-		pm_ops->finish(state);
 	pm_restore_console();
 }

Yes, I agree that _WAK needs to come before device_resume().
Need to let any BIOS nasties happen and get over with before we restore device drivers. This is consistent with the wording in ACPI 3.0b (section 7.4) that says
11. _WAK is run
12. OSPM notifies all native device drivefrs of the return from the sleep state transition

However, commit 1a38416cea8ac801ae8f261074721f35317613dc says that _WAK must follow INIT -- ie finish() must come after enable_nonboot_cpus(), and this patch as it stands would violate that.

So it looks like we need this sequence:

enable_nonboot_cpus() /* INIT */
finish() /* _WAK */
device_resume()

Quoted 48 lines · On Wed, 15 Nov 2006 02:03:30 -0500 Len Brown <[email protected]> wrote:

Quoted 24 lines · Stephen Hemminger

If I do a suspend-to-ram then resume on a Sony Vaio laptop with sky2 driver, the first interrupt gets misrouted to the original shared IRQ, rather than to the MSI irq expected.

During the pci_restore process, the MSI information and the PCI command register are restored properly. But later during resume, inside the ACPI evaluation of the WAK method, the PCI_COMMAND INTX_DISABLE (0x400) flag is being cleared. My guess is that the BIOS ends up doing some resetting of devices.

I may be able to workaround the problem for this one device, but it brings up a more general issue about what the ordering should be during resume. If ACPI evaluation (which I assume talks to the BIOS), might change device state, it seems that ACPI code should execute before resuming devices not after. But changing the order here seems drastic.

An alternate solution would be to have two pm_ops, one for early_resume and another for late, and split the ACPI work.

kernel/power/main.c +0 -0

@@ -132,12 +132,12 @@
 
 static void suspend_finish(suspend_state_t state)
 {

kernel/power/main.c line 142

Quoted 10 lines

kernel/power/main.c +2 -2

+	if (pm_ops && pm_ops->finish)
+		pm_ops->finish(state);
 	device_resume();
 	resume_console();
 	thaw_processes();
 	enable_nonboot_cpus();
-	if (pm_ops && pm_ops->finish)
-		pm_ops->finish(state);
 	pm_restore_console();
 }

Yes, I agree that _WAK needs to come before device_resume().
Need to let any BIOS nasties happen and get over with before we restore device drivers. This is consistent with the wording in ACPI 3.0b (section 7.4) that says
11. _WAK is run
12. OSPM notifies all native device drivefrs of the return from the sleep state transition

However, commit 1a38416cea8ac801ae8f261074721f35317613dc says that _WAK must follow INIT -- ie finish() must come after enable_nonboot_cpus(), and this patch as it stands would violate that.

So it looks like we need this sequence:

Quoted 4 lines

enable_nonboot_cpus() /* INIT */
finish() /* _WAK */
device_resume()

Do you want to do this, or shall I? send off a patch. I can test on about 5 machines first.