Re: 2.6.11-rc4-mm1: something is wrong with swsusp powerdown

Re: 2.6.11-rc4-mm1: something is wrong with swsusp powerdown

[Andrew Morton]

Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org> wrote:
>
> In `subj` kernel, machine no longer powers down at the end of
>  swsusp. 2.6.11-rc5-pavel works ok, as does 2.6.11-bk.

Binary searching indicates that this is due to
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.11-rc5/2.6.11-rc5-mm1/broken-out/acpi_power_off-bug-fix.patch.

I'll drop it.  That patch is pretty ugly-looking anyway (ACPI code in
drivers/base/power/?).

Perhaps someone who is hitting the problem which that patch addresses could
raise a bugzilla entry.

Oh.  It has one.  http://bugme.osdl.org/show_bug.cgi?id=4041

Anyway.  It needs more work.


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: 2.6.11-rc4-mm1: something is wrong with swsusp powerdown

[Pavel Machek]

Hi!

> > In `subj` kernel, machine no longer powers down at the end of
> >  swsusp. 2.6.11-rc5-pavel works ok, as does 2.6.11-bk.
> 
> Binary searching indicates that this is due to
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.11-rc5/2.6.11-rc5-mm1/broken-out/acpi_power_off-bug-fix.patch.
> 
> I'll drop it.  That patch is pretty ugly-looking anyway (ACPI code in
> drivers/base/power/?).
> 
> Perhaps someone who is hitting the problem which that patch addresses could
> raise a bugzilla entry.
> 
> Oh.  It has one.  http://bugme.osdl.org/show_bug.cgi?id=4041
> 
> Anyway.  It needs more work.

Yes, the patch is very ugly. If something like this needs to be done,
then perhaps acpi should properly register into driver model and do
the work there. This will also mean code will be called consistently.

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: 2.6.11-rc4-mm1: something is wrong with swsusp powerdown

[Eric W. Biederman]

Andrew Morton <akpm@osdl.org> writes:

> Pavel Machek <pavel@ucw.cz> wrote:
> >
> > In `subj` kernel, machine no longer powers down at the end of
> >  swsusp. 2.6.11-rc5-pavel works ok, as does 2.6.11-bk.
> 
> Binary searching indicates that this is due to
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.11-rc5/2.6.11-rc5-mm1/broken-out/acpi_power_off-bug-fix.patch.
> 
> 
> I'll drop it.  That patch is pretty ugly-looking anyway (ACPI code in
> drivers/base/power/?).
> 
> Perhaps someone who is hitting the problem which that patch addresses could
> raise a bugzilla entry.
> 
> Oh.  It has one.  http://bugme.osdl.org/show_bug.cgi?id=4041
> 
> Anyway.  It needs more work.

Agreed.

I threw it together to test a specific code path, and the fact it
fails in software suspend is actually almost confirmation that I am on
the right track.  This actually fixed the case I was testing.

In this case the failure is simply because system_state is
not set to SYSTEM_POWER_OFF before
kernel/power/disk.c:power_down() calls device_shutdown().
The appropriate reboot notifier is also not called..

So to fix this properly all of the places
that call machine_power_off now need to call a wrapper
that does all of the appropriate things and then calls
machine_power_off.

Likewise with the other reboot functions.

In addition a clean way to get device_shutdown() to 
call acpi_power_off_prepare() at roughly the location
I have it hard coded.  

The fundamental issue this patch was starting to address
before I ran out of steam, is that acpi_power_off_prepare()
must be called with interrupts enabled and after we have shut down
the system devices (i.e. the interrupt controllers) we can't
guarantee interrupts, are working.

I'm don't know how much earlier it is safe to
acpi_power_off_prepare().  But mostly I think we need to
throw in a fake device to attach acpi_power_off_prepare to.

Eric

Re: 2.6.11-rc4-mm1: something is wrong with swsusp powerdown

[Eric W. Biederman]

Pavel Machek <pavel-AlSwsSmVLrQ@public.gmane.org> writes:

> Yes, the patch is very ugly. If something like this needs to be done,
> then perhaps acpi should properly register into driver model and do
> the work there. This will also mean code will be called consistently.

I totally agree.  Do you have an example of how a non-device
can do this?

In particular something that gets as close to shutting down
the system devices as possible.  But gets called before that.

Or perhaps acpi should simply be setup to be the first system device?

Eric


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: 2.6.11-rc4-mm1: something is wrong with swsusp powerdown

[Pavel Machek]

Hi!

> > Yes, the patch is very ugly. If something like this needs to be done,
> > then perhaps acpi should properly register into driver model and do
> > the work there. This will also mean code will be called consistently.
> 
> I totally agree.  Do you have an example of how a non-device
> can do this?
> 
> In particular something that gets as close to shutting down
> the system devices as possible.  But gets called before that.
> 
> Or perhaps acpi should simply be setup to be the first system device?

I believe that's the prefered solution.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: 2.6.11-rc4-mm1: something is wrong with swsusp powerdown

[Pavel Machek]

Hi!

> > > In `subj` kernel, machine no longer powers down at the end of
> > >  swsusp. 2.6.11-rc5-pavel works ok, as does 2.6.11-bk.
> > 
> > Binary searching indicates that this is due to
> > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.11-rc5/2.6.11-rc5-mm1/broken-out/acpi_power_off-bug-fix.patch.
> > 
> > 
> > I'll drop it.  That patch is pretty ugly-looking anyway (ACPI code in
> > drivers/base/power/?).
> > 
> > Perhaps someone who is hitting the problem which that patch addresses could
> > raise a bugzilla entry.
> > 
> > Oh.  It has one.  http://bugme.osdl.org/show_bug.cgi?id=4041
> > 
> > Anyway.  It needs more work.
> 
> Agreed.
> 
> I threw it together to test a specific code path, and the fact it
> fails in software suspend is actually almost confirmation that I am on
> the right track.  This actually fixed the case I was testing.
> 
> In this case the failure is simply because system_state is
> not set to SYSTEM_POWER_OFF before
> kernel/power/disk.c:power_down() calls device_shutdown().
> The appropriate reboot notifier is also not called..

Can you suggest patch to do it right? Or perhaps there should be
just_plain_power_machine_down() that does all neccessary
trickery?
							Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: 2.6.11-rc4-mm1: something is wrong with swsusp powerdown

[Eric W. Biederman]

Pavel Machek <pavel@suse.cz> writes:

> > I threw it together to test a specific code path, and the fact it
> > fails in software suspend is actually almost confirmation that I am on
> > the right track.  This actually fixed the case I was testing.
> > 
> > In this case the failure is simply because system_state is
> > not set to SYSTEM_POWER_OFF before
> > kernel/power/disk.c:power_down() calls device_shutdown().
> > The appropriate reboot notifier is also not called..
> 
> Can you suggest patch to do it right? Or perhaps there should be
> just_plain_power_machine_down() that does all neccessary
> trickery?

I would call it kernel_power_down() and that
is what I am suggesting is the right fix.

We have it open coded in kernel/sys.c:sys_reboot()
in the switch case for: LINUX_REBOOT_CMD_POWER_OFF

So after the code gets factored out from there all
of the cases that call machine_power_off() and pm_power_off()
directly need to be updated.

There are similar cases for machine_restart() and machine_halt().
But the power off case seems to be the most acute.

My biggest problem with this is I get into the recursive code
cleanup problem.  Where I fix one piece and a bug is exposed somewhere
else.  And that then requires investigation and fixing.

Fixing the callers of machine_power_off() is about the fifth bug
fix down the chain triggered by disabling UP interrupts in
device_shutdown(), SMP interrupts have always been disabled.  With the
first bug fix was to create system devices in the device tree..

I haven't a clue where fixing this one will lead.  Recursive
code fixes are a hard thing to schedule :(

Eric