FYI: device_suspend(...) in kernel_power_off().

[ebiederm@xmission.com (Eric W. Biederman)]

[-- Attachment #1: Type: text/plain, Size: 2494 bytes --]


Early in the 2.6.13 process my kexec related patches were introduced
into the reboot path, and under the rule you touch it you fix it
it I have been involved in tracking quite a few regressions
on the reboot path.

Recently with Benjamin Herrenschmidt's removal of
device_suppend(PMSG_SUPPEND) from kernel_power_off(),
it seems the last of the issues went away.  I asked
for confirmation that reintroducing the patch below 
would break systems and I received it.

The experimental evidence then is that calling 
device_suspend(PMSG_SUSPEND) in kernel_power_off
when performing an acpi_power_off is actively harmful.

There as been a fair amount of consensus that calling
device_suspend(...) in the reboot path was inappropriate now, because
the device suspend code was too immature.   With this latest
piece of evidence it seems to me that introducing device_suspend(...)
in kernel_power_off, kernel_halt, kernel_reboot, or kernel_kexec
can never be appropriate.  I am including as many people as
I can on this so we in our collective wisdom don't forget this
lesson.

What lead us to this situation was a real problem, and a desire
to fix it.  Currently linux has a proliferation of interfaces
to place devices in different states.  The reboot notifiers,
device_suspend(...), device_shutdown+system_state, remove_one,
module_exit, and probably a few I'm not aware of.  Each interface
introduced by a different author wanting to solve a different problem.
Just writing this list of interfaces is confusing.  The implementation
task for device driver writers appears even harder as simultaneously
implementing all of these interfaces correctly seems not to happen.

The lesson: fixing this problem is heavy lifting, and that
device_suspend() in the reboot path is not the answer.

Eric


This is the patch that subtly and mysterously broke things.
> diff --git a/kernel/sys.c b/kernel/sys.c
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -404,6 +404,7 @@ void kernel_halt(void)
>  {
>  	notifier_call_chain(&reboot_notifier_list, SYS_HALT, NULL);
>  	system_state = SYSTEM_HALT;
> +	device_suspend(PMSG_SUSPEND);
>  	device_shutdown();
>  	printk(KERN_EMERG "System halted.\n");
>  	machine_halt();
> @@ -414,6 +415,7 @@ void kernel_power_off(void)
>  {
>  	notifier_call_chain(&reboot_notifier_list, SYS_POWER_OFF, NULL);
>  	system_state = SYSTEM_POWER_OFF;
> +	device_suspend(PMSG_SUSPEND);
>  	device_shutdown();
>  	printk(KERN_EMERG "Power down.\n");
>  	machine_power_off();
> 
> 







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