Re: [PATCH 1/23] Add missing device_suspsend(PMSG_FREEZE) calls.

Re: [PATCH 1/23] Add missing device_suspsend(PMSG_FREEZE) calls.

[Nigel Cunningham]

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

Hi.

Could you please send PMSG_* related patches to linux-pm at
lists.osdl.org as well?

Thanks!

Nigel

On Wed, 2005-07-27 at 03:21, Eric W. Biederman wrote:
> In the recent addition of device_suspend calls into
> sys_reboot two code paths were missed.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
> 
>  kernel/sys.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> 5f0fb00783b94248b5a76c161f1c30a033fce4d3
> diff --git a/kernel/sys.c b/kernel/sys.c
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -391,6 +391,7 @@ asmlinkage long sys_reboot(int magic1, i
>  	case LINUX_REBOOT_CMD_RESTART:
>  		notifier_call_chain(&reboot_notifier_list, SYS_RESTART, NULL);
>  		system_state = SYSTEM_RESTART;
> +		device_suspend(PMSG_FREEZE);
>  		device_shutdown();
>  		printk(KERN_EMERG "Restarting system.\n");
>  		machine_restart(NULL);
> @@ -452,6 +453,7 @@ asmlinkage long sys_reboot(int magic1, i
>  		}
>  		notifier_call_chain(&reboot_notifier_list, SYS_RESTART, NULL);
>  		system_state = SYSTEM_RESTART;
> +		device_suspend(PMSG_FREEZE);
>  		device_shutdown();
>  		printk(KERN_EMERG "Starting new kernel\n");
>  		machine_shutdown();
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-- 
Evolution.
Enumerate the requirements.
Consider the interdependencies.
Calculate the probabilities.


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

Re: [PATCH 1/23] Add missing device_suspsend(PMSG_FREEZE) calls.

[Eric W. Biederman]

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

Nigel Cunningham <ncunningham@cyclades.com> writes:

> Hi.
>
> Could you please send PMSG_* related patches to linux-pm at
> lists.osdl.org as well?

I'll try.  My goal was not to add or change not functionality but to
make what the kernel was already doing be consistent.

It turns out the device_suspend(PMSG_FREEZE) is a major pain
sitting in the reboot path and I will be submitting a patch to
remove it from the reboot path in 2.6.13 completely.

At the very least the ide driver breaks, and the e1000 driver
is affected.

And there is of course the puzzle of why there exists simultaneously
driver shutdown() and suspend(PMSG_FREEZE) methods as I believed they
are defined to do exactly the same thing.

Eric


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

Re: Re: [PATCH 1/23] Add missing device_suspsend(PMSG_FREEZE) calls.

[david-b]

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

> And there is of course the puzzle of why there exists simultaneously
> driver shutdown() and suspend(PMSG_FREEZE) methods as I believed they
> are defined to do exactly the same thing.

No puzzle; that's not how they're defined.  Both of them cause the
device to quiesce that device's activity.  But:

 - shutdown() is a "dirty shutdown OK" heads-up for some level
   of restart/reboot; hardware will be completely re-initialized
   later, normally with hardware level resets and OS rebooting.

 - freeze() is a "clean shutdown only" that normally sees hardware
   state preserved, and is followed by suspend() or resume().

Or so I had understood.  That does suggest why having FREEZE in the
reboot path could be trouble:  you could be rebooting because that
hardware won't do a clean shutdown!

- Dave


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

Re: Re: [PATCH 1/23] Add missing device_suspsend(PMSG_FREEZE) calls.

[Shaohua Li]

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

On Wed, 2005-07-27 at 19:12 -0600, Eric W. Biederman wrote:
> Nigel Cunningham <ncunningham@cyclades.com> writes:
> 
> > Hi.
> >
> > Could you please send PMSG_* related patches to linux-pm at
> > lists.osdl.org as well?
> 
> I'll try.  My goal was not to add or change not functionality but to
> make what the kernel was already doing be consistent.
> 
> It turns out the device_suspend(PMSG_FREEZE) is a major pain
> sitting in the reboot path and I will be submitting a patch to
> remove it from the reboot path in 2.6.13 completely.
> 
> At the very least the ide driver breaks, and the e1000 driver
> is affected.
> 
> And there is of course the puzzle of why there exists simultaneously
> driver shutdown() and suspend(PMSG_FREEZE) methods as I believed they
> are defined to do exactly the same thing.
I would expect more driver breakage and for the shutdown either. In
current stage, suspend(PMSG_FREEZE) might put devices into D3 state. How
can a shutdown() be done again?

Thanks,
Shaohua


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

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

[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 --]

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

[Pavel Machek]

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

Hi!

> 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.

Code is not ready now => it can never be fixed? Thats quite a strange
conclusion to make.
				Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         


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

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

[Eric W. Biederman]

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

Pavel Machek <pavel@suse.cz> writes:

> Hi!
>
>> 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.
>
> Code is not ready now => it can never be fixed? Thats quite a strange
> conclusion to make.

It seems there is an fundamental incompatibility with ACPI power off.
As best as I can tell the normal case of device_suspend(PMSG_SUSPEND)
works reasonably well in 2.6.x.

>From what I can tell there are some fairly fundamental semantic
differences, on that code path.  The most peculiar problem I tracked
is someone had a machine that would go into power off state and then
wake right back up because of the device_suspend(PMSG_SUSPEND) change.
If acpi power off doesn't expect the hardware to be suspended I don't
see how you can make device_suspend(PMSG_SUSPEND) a default in 2.6.x.

I won't call it impossible to resolve the problems, but the people
doing it must be extremely sensitive to the subtle semantic
differences that exist and the burden of proof for showing things work
better need to be extremely high.  And the developer who reintroduces
it gets to own all of the reboot/halt/power off problems in the stable
tree when it gets merged.

Pavel the fact that you did not articulate a possibility that your
change could have caused most of the problems that were seen with
it is what scares me the most.  The fairly trivial and obvious
problems were not addressed when the patch was merged much less the
subtle ones.  Your initial patch did not even touch all of the code
paths necessary to achieve what it was trying to do.

So yes without a darn good argument as to why it should work.  I will
go with the experimental evidence that it fails miserably and
trivially because of semantic incompatibility and can therefore
never be fixed.

Eric

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

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

[Pavel Machek]

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

Hi!

> >> 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.
> >
> > Code is not ready now => it can never be fixed? Thats quite a strange
> > conclusion to make.
> 
> It seems there is an fundamental incompatibility with ACPI power off.
> As best as I can tell the normal case of device_suspend(PMSG_SUSPEND)
> works reasonably well in 2.6.x.

Powerdown is going to have the same problems as the powerdown at the
end of suspend-to-disk. Can you ask people reporting broken shutdown
to try suspend-to-disk?

> >From what I can tell there are some fairly fundamental semantic
> differences, on that code path.  The most peculiar problem I tracked
> is someone had a machine that would go into power off state and then
> wake right back up because of the device_suspend(PMSG_SUSPEND)
> change.

So something is wrong with ACPI wakeup GPEs. It would hurt in
suspend-to-disk case, too.

> I won't call it impossible to resolve the problems, but the people

Good.

> So yes without a darn good argument as to why it should work.  I will
> go with the experimental evidence that it fails miserably and
> trivially because of semantic incompatibility and can therefore
> never be fixed.

I do not think any "semantic" issues exist. We need to pass detailed
info down to the drivers that care, and we need to fix all the bugs in
the drivers. That should be pretty much it.
								Pavel
-- 
if you have sharp zaurus hardware you don't need... you know my address

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

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

[Eric W. Biederman]

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

Pavel Machek <pavel@ucw.cz> writes:

> Hi!
>
>> >> 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.
>> >
>> > Code is not ready now => it can never be fixed? Thats quite a strange
>> > conclusion to make.
>> 
>> It seems there is an fundamental incompatibility with ACPI power off.
>> As best as I can tell the normal case of device_suspend(PMSG_SUSPEND)
>> works reasonably well in 2.6.x.
>
> Powerdown is going to have the same problems as the powerdown at the
> end of suspend-to-disk. Can you ask people reporting broken shutdown
> to try suspend-to-disk?

Everyone I know of who is affected has been copied on this thread.
However your request is just nonsense.  There is a device_resume in
the code before we get to the device_shutdown so there should be no
effect at all.  Are we looking at the same kernel?

>> >From what I can tell there are some fairly fundamental semantic
>> differences, on that code path.  The most peculiar problem I tracked
>> is someone had a machine that would go into power off state and then
>> wake right back up because of the device_suspend(PMSG_SUSPEND)
>> change.
>
> So something is wrong with ACPI wakeup GPEs. It would hurt in
> suspend-to-disk case, too.

Something was wrong.  I can't possibly see how the suspend-to-disk
case would be affected.

>> I won't call it impossible to resolve the problems, but the people
>
> Good.

Nope.  Now that I have read the code I would just call it nonsense.

>> So yes without a darn good argument as to why it should work.  I will
>> go with the experimental evidence that it fails miserably and
>> trivially because of semantic incompatibility and can therefore
>> never be fixed.
>
> I do not think any "semantic" issues exist. We need to pass detailed
> info down to the drivers that care, and we need to fix all the bugs in
> the drivers. That should be pretty much it.

Given that acpi and other platform firmware is involved there are
pieces we cannot fix.  We either match the spec or we are incorrect.

I haven't a clue how suspend/resume is expected to interact with
things in suspend to disk scenario.  Reading through the code
the power message is PMSG_FREEZE not PMSG_SUSPEND (as you
implemented).  All of the hardware is actually resumed before
we device_shutdown() is called.

I want to see the correlation between device_suspend(PMSG_FREEZE) and
the code in device_shutdown(), but I don't see it.
device_suspend(...) is all about allowing the state of a device to be
preserved.  device_shutdown() is really about stopping it.  These are
really quite different operations. 

With the pm_suspend_disk calling kernel_power_off it appears that we
currently have complete code reuse of the relevant code on that path.

Currently I see no true redundancy between the two cases at all.
The methods do different things for different purposes.  Which is
about the largest semantic difference I can think of.  The fact
that the methods at first glance look like they do the same
thing is probably the real surprise.

Calling device_suspend(...) from kernel_power_off, kernel_halt,
kernel_kexec, or kernel_restart seems pointless, useless and silly.

Eric

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

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

[Nigel Cunningham]

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

Hi.

On Wed, 2005-08-10 at 03:25, Eric W. Biederman wrote:
> Pavel Machek <pavel@ucw.cz> writes:
> 
> > Hi!
> >
> >> >> 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.
> >> >
> >> > Code is not ready now => it can never be fixed? Thats quite a strange
> >> > conclusion to make.
> >> 
> >> It seems there is an fundamental incompatibility with ACPI power off.
> >> As best as I can tell the normal case of device_suspend(PMSG_SUSPEND)
> >> works reasonably well in 2.6.x.
> >
> > Powerdown is going to have the same problems as the powerdown at the
> > end of suspend-to-disk. Can you ask people reporting broken shutdown
> > to try suspend-to-disk?
> 
> Everyone I know of who is affected has been copied on this thread.
> However your request is just nonsense.  There is a device_resume in
> the code before we get to the device_shutdown so there should be no
> effect at all.  Are we looking at the same kernel?

My poweroff after suspend-to-disk was broken during 2.6.13-rcs, and came
right in rc6.

> >> >From what I can tell there are some fairly fundamental semantic
> >> differences, on that code path.  The most peculiar problem I tracked
> >> is someone had a machine that would go into power off state and then
> >> wake right back up because of the device_suspend(PMSG_SUSPEND)
> >> change.
> >
> > So something is wrong with ACPI wakeup GPEs. It would hurt in
> > suspend-to-disk case, too.
> 
> Something was wrong.  I can't possibly see how the suspend-to-disk
> case would be affected.
> 
> >> I won't call it impossible to resolve the problems, but the people
> >
> > Good.
> 
> Nope.  Now that I have read the code I would just call it nonsense.
> 
> >> So yes without a darn good argument as to why it should work.  I will
> >> go with the experimental evidence that it fails miserably and
> >> trivially because of semantic incompatibility and can therefore
> >> never be fixed.
> >
> > I do not think any "semantic" issues exist. We need to pass detailed
> > info down to the drivers that care, and we need to fix all the bugs in
> > the drivers. That should be pretty much it.
> 
> Given that acpi and other platform firmware is involved there are
> pieces we cannot fix.  We either match the spec or we are incorrect.
> 
> I haven't a clue how suspend/resume is expected to interact with
> things in suspend to disk scenario.  Reading through the code
> the power message is PMSG_FREEZE not PMSG_SUSPEND (as you
> implemented).  All of the hardware is actually resumed before
> we device_shutdown() is called.
> 
> I want to see the correlation between device_suspend(PMSG_FREEZE) and
> the code in device_shutdown(), but I don't see it.
> device_suspend(...) is all about allowing the state of a device to be
> preserved.  device_shutdown() is really about stopping it.  These are
> really quite different operations. 

Agreed here.

> With the pm_suspend_disk calling kernel_power_off it appears that we
> currently have complete code reuse of the relevant code on that path.
> 
> Currently I see no true redundancy between the two cases at all.
> The methods do different things for different purposes.  Which is
> about the largest semantic difference I can think of.  The fact
> that the methods at first glance look like they do the same
> thing is probably the real surprise.

If the suspend to disk code called kernel_power_off, it should be
exactly what it sounds like. We've already written the image and we now
went to simply power down the machine. Just as with a 'normal'
powerdown, we should do everything necessary to ensure all data
submitted to hard drives is really flushed and that emergency head
parking isn't done, and then power down (or reboot). This should, so far
as I can see, be exactly the same in both cases.

Regards,

Nigel

> Calling device_suspend(...) from kernel_power_off, kernel_halt,
> kernel_kexec, or kernel_restart seems pointless, useless and silly.
> 
> Eric
-- 
Evolution.
Enumerate the requirements.
Consider the interdependencies.
Calculate the probabilities.


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

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

[Pavel Machek]

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

Hi!

> >> > Code is not ready now => it can never be fixed? Thats quite a strange
> >> > conclusion to make.
> >> 
> >> It seems there is an fundamental incompatibility with ACPI power off.
> >> As best as I can tell the normal case of device_suspend(PMSG_SUSPEND)
> >> works reasonably well in 2.6.x.
> >
> > Powerdown is going to have the same problems as the powerdown at the
> > end of suspend-to-disk. Can you ask people reporting broken shutdown
> > to try suspend-to-disk?
> 
> Everyone I know of who is affected has been copied on this thread.
> However your request is just nonsense.  There is a device_resume in
> the code before we get to the device_shutdown so there should be no
> effect at all.  Are we looking at the same kernel?

Sorry, my fault. kernel/power/disk.c:power_down(): it calls
device_shutdown even in PM_DISK_PLATFORM case. I thought it would do
device_suspend() to enable drivers doing something more clever.

So only missing piece of puzzle seems to be making sure disks are
properly spinned down in device_shutdown... and even that seems to be
there, not sure why it was broken before.
								Pavel
-- 
if you have sharp zaurus hardware you don't need... you know my address

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