[PATCH] preserve correct battery state through suspend/resume cycles

[PATCH] preserve correct battery state through suspend/resume cycles

[Jiri Kosina]

There is a problem in th following scenario(s):

boot -> suspend -> (un)plug battery -> resume

The problem arises in both cases - i.e. suspend with battery plugged in, 
and resume with battery unplugged, or vice versa.

After resume, when the battery status has changed (plugged in -> unplegged 
or unplugged -> plugged in) during the time when the system was sleeping, 
the /proc/acpi/battery/*/* is wrong (showing the state before suspend, not 
the current state).

The following patch adds ->resume method to the ACPI battery handler, which
has the only aim - to check whether the battery state has changed during sleep, 
and if so, update the ACPI internal data structures, so that information 
published through /proc/acpi/battery/*/* is correct even after suspend/resume
cycle, during which the battery was removed/inserted.

The patch is against current ACPI git tree, but applies cleanly also 
against -mm and probably other trees. Please apply.

Signed-off-by: Jiri Kosina <jikos@jikos.cz>

 drivers/acpi/battery.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 9810e2a..82a0f5b 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -64,6 +64,7 @@ extern void *acpi_unlock_battery_dir(str
 
 static int acpi_battery_add(struct acpi_device *device);
 static int acpi_battery_remove(struct acpi_device *device, int type);
+static int acpi_battery_resume(struct acpi_device *device, int status);
 
 static struct acpi_driver acpi_battery_driver = {
 	.name = ACPI_BATTERY_DRIVER_NAME,
@@ -71,6 +72,7 @@ static struct acpi_driver acpi_battery_d
 	.ids = ACPI_BATTERY_HID,
 	.ops = {
 		.add = acpi_battery_add,
+		.resume = acpi_battery_resume,
 		.remove = acpi_battery_remove,
 		},
 };
@@ -753,6 +755,19 @@ static int acpi_battery_remove(struct ac
 	return 0;
 }
 
+/* this is needed to learn about changes made in suspended state */
+static int acpi_battery_resume(struct acpi_device *device, int state)
+{
+	struct acpi_battery *battery;
+	
+	if (!device)
+		return -EINVAL;
+	
+	battery = (struct acpi_battery *)device->driver_data;
+	return acpi_battery_check(battery);
+	
+}
+
 static int __init acpi_battery_init(void)
 {
 	int result;

-- 
Jiri Kosina

Re: [PATCH] preserve correct battery state through suspend/resume cycles

[Stefan Seyfried]

On Thu, Sep 28, 2006 at 04:50:49AM +0200, Jiri Kosina wrote:
> There is a problem in th following scenario(s):
> 
> boot -> suspend -> (un)plug battery -> resume
> 
> The problem arises in both cases - i.e. suspend with battery plugged in, 
> and resume with battery unplugged, or vice versa.
> 
> After resume, when the battery status has changed (plugged in -> unplegged 
> or unplugged -> plugged in) during the time when the system was sleeping, 
> the /proc/acpi/battery/*/* is wrong (showing the state before suspend, not 
> the current state).

Is this also needed if you use "platform" method? Also with suspend-to-RAM?

> The following patch adds ->resume method to the ACPI battery handler, which
> has the only aim - to check whether the battery state has changed during sleep, 
> and if so, update the ACPI internal data structures, so that information 
> published through /proc/acpi/battery/*/* is correct even after suspend/resume
> cycle, during which the battery was removed/inserted.

Although it generally is a good idea to add suspend and resume methods to
all ACPI drivers, it would be interesting to know if you still need this
when using the correct method (platform) instead of the incorrect default
method (shutdown).

echo "platform" > /sys/power/disk
echo "disk" > /sys/power/state
-- 
Stefan Seyfried                  \ "I didn't want to write for pay. I
QA / R&D Team Mobile Devices      \ wanted to be paid for what I write."
SUSE LINUX Products GmbH, Nürnberg \                    -- Leonard Cohen

Re: [PATCH] preserve correct battery state through suspend/resume cycles

[Pavel Machek]

Hi!

> > boot -> suspend -> (un)plug battery -> resume
> > 
> > The problem arises in both cases - i.e. suspend with battery plugged in, 
> > and resume with battery unplugged, or vice versa.
> > 
> > After resume, when the battery status has changed (plugged in -> unplegged 
> > or unplugged -> plugged in) during the time when the system was sleeping, 
> > the /proc/acpi/battery/*/* is wrong (showing the state before suspend, not 
> > the current state).
> 
> Is this also needed if you use "platform" method? Also with suspend-to-RAM?
> 
> > The following patch adds ->resume method to the ACPI battery handler, which
> > has the only aim - to check whether the battery state has changed during sleep, 
> > and if so, update the ACPI internal data structures, so that information 
> > published through /proc/acpi/battery/*/* is correct even after suspend/resume
> > cycle, during which the battery was removed/inserted.
> 
> Although it generally is a good idea to add suspend and resume methods to
> all ACPI drivers, it would be interesting to know if you still need this
> when using the correct method (platform) instead of the incorrect default
> method (shutdown).
> 
> echo "platform" > /sys/power/disk
> echo "disk" > /sys/power/state

Maybe we should change the default in 2.6.20 or so?

-- 
Thanks for all the (sleeping) penguins.

Re: [PATCH] preserve correct battery state through suspend/resume cycles

[Rafael J. Wysocki]

On Sunday, 8 October 2006 20:42, Pavel Machek wrote:
> Hi!
> 
> > > boot -> suspend -> (un)plug battery -> resume
> > > 
> > > The problem arises in both cases - i.e. suspend with battery plugged in, 
> > > and resume with battery unplugged, or vice versa.
> > > 
> > > After resume, when the battery status has changed (plugged in -> unplegged 
> > > or unplugged -> plugged in) during the time when the system was sleeping, 
> > > the /proc/acpi/battery/*/* is wrong (showing the state before suspend, not 
> > > the current state).
> > 
> > Is this also needed if you use "platform" method? Also with suspend-to-RAM?
> > 
> > > The following patch adds ->resume method to the ACPI battery handler, which
> > > has the only aim - to check whether the battery state has changed during sleep, 
> > > and if so, update the ACPI internal data structures, so that information 
> > > published through /proc/acpi/battery/*/* is correct even after suspend/resume
> > > cycle, during which the battery was removed/inserted.
> > 
> > Although it generally is a good idea to add suspend and resume methods to
> > all ACPI drivers, it would be interesting to know if you still need this
> > when using the correct method (platform) instead of the incorrect default
> > method (shutdown).
> > 
> > echo "platform" > /sys/power/disk
> > echo "disk" > /sys/power/state
> 
> Maybe we should change the default in 2.6.20 or so?

Well, I think swsusp should work with "shutdown" just as well.  If it doesn't,
that means there are some bugs in the ACPI code which should be fixed.
By using "platform" as the default method we'll be hiding those bugs IMHO.

OTOH that may be desirable. ;-)

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller

Re: [PATCH] preserve correct battery state through suspend/resume cycles

[Pavel Machek]

On Tue 2006-10-10 00:52:09, Rafael J. Wysocki wrote:
> On Sunday, 8 October 2006 20:42, Pavel Machek wrote:
> > Hi!
> > 
> > > > boot -> suspend -> (un)plug battery -> resume
> > > > 
> > > > The problem arises in both cases - i.e. suspend with battery plugged in, 
> > > > and resume with battery unplugged, or vice versa.
> > > > 
> > > > After resume, when the battery status has changed (plugged in -> unplegged 
> > > > or unplugged -> plugged in) during the time when the system was sleeping, 
> > > > the /proc/acpi/battery/*/* is wrong (showing the state before suspend, not 
> > > > the current state).
> > > 
> > > Is this also needed if you use "platform" method? Also with suspend-to-RAM?
> > > 
> > > > The following patch adds ->resume method to the ACPI battery handler, which
> > > > has the only aim - to check whether the battery state has changed during sleep, 
> > > > and if so, update the ACPI internal data structures, so that information 
> > > > published through /proc/acpi/battery/*/* is correct even after suspend/resume
> > > > cycle, during which the battery was removed/inserted.
> > > 
> > > Although it generally is a good idea to add suspend and resume methods to
> > > all ACPI drivers, it would be interesting to know if you still need this
> > > when using the correct method (platform) instead of the incorrect default
> > > method (shutdown).
> > > 
> > > echo "platform" > /sys/power/disk
> > > echo "disk" > /sys/power/state
> > 
> > Maybe we should change the default in 2.6.20 or so?
> 
> Well, I think swsusp should work with "shutdown" just as well.  If it doesn't,
> that means there are some bugs in the ACPI code which should be fixed.
> By using "platform" as the default method we'll be hiding those bugs IMHO.
> 
> OTOH that may be desirable. ;-)

You are right. We probably want both: suspend/resume method in battery
driver _and_ platform mode used by default.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] preserve correct battery state through suspend/resume cycles

[Stefan Seyfried]

On Tue, Oct 10, 2006 at 12:52:09AM +0200, Rafael J. Wysocki wrote:
> On Sunday, 8 October 2006 20:42, Pavel Machek wrote:
 
> > > echo "platform" > /sys/power/disk
> > > echo "disk" > /sys/power/state
> > 
> > Maybe we should change the default in 2.6.20 or so?
> 
> Well, I think swsusp should work with "shutdown" just as well.  If it doesn't,
> that means there are some bugs in the ACPI code which should be fixed.
> By using "platform" as the default method we'll be hiding those bugs IMHO.

I'm not really intimately familiar with the ACPI spec, but IIRC those AML
methods executed by pm_ops->prepare and pm_ops->finish are mandatory for
suspending ACPI enabled machines. So using "platform" as a default seems
reasonable (assuming that on non-ACPI machines, pm_ops->{prepare,finish} will
be a noop anyway)
-- 
Stefan Seyfried
QA / R&D Team Mobile Devices        |              "Any ideas, John?"
SUSE LINUX Products GmbH, Nürnberg  | "Well, surrounding them's out." 

Re: [PATCH] preserve correct battery state through suspend/resume cycles

[Rafael J. Wysocki]

On Tuesday, 10 October 2006 14:10, Stefan Seyfried wrote:
> On Tue, Oct 10, 2006 at 12:52:09AM +0200, Rafael J. Wysocki wrote:
> > On Sunday, 8 October 2006 20:42, Pavel Machek wrote:
>  
> > > > echo "platform" > /sys/power/disk
> > > > echo "disk" > /sys/power/state
> > > 
> > > Maybe we should change the default in 2.6.20 or so?
> > 
> > Well, I think swsusp should work with "shutdown" just as well.  If it doesn't,
> > that means there are some bugs in the ACPI code which should be fixed.
> > By using "platform" as the default method we'll be hiding those bugs IMHO.
> 
> I'm not really intimately familiar with the ACPI spec, but IIRC those AML
> methods executed by pm_ops->prepare and pm_ops->finish are mandatory for
> suspending ACPI enabled machines. So using "platform" as a default seems
> reasonable (assuming that on non-ACPI machines, pm_ops->{prepare,finish} will
> be a noop anyway)

Well, what swsusp does is not really a suspend operation.  It is, roughly, a
"save the contents of memory and power off" thing.  During the "resume" we do
something like "restore the contents of memory and use it as the initial
data", but the state of devices (ie. hardware) is not expected to be saved.

Moreover, I'm starting to think that it's actually wrong to assume that the
hardware state will be saved and the drivers that make such an assumption
need fixing.


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller

Re: [PATCH] preserve correct battery state through suspend/resume cycles

[Pavel Machek]

Hi!

> > > > echo "platform" > /sys/power/disk
> > > > echo "disk" > /sys/power/state
> > > 
> > > Maybe we should change the default in 2.6.20 or so?
> > 
> > Well, I think swsusp should work with "shutdown" just as well.  If it doesn't,
> > that means there are some bugs in the ACPI code which should be fixed.
> > By using "platform" as the default method we'll be hiding those bugs IMHO.
> 
> I'm not really intimately familiar with the ACPI spec, but IIRC those AML
> methods executed by pm_ops->prepare and pm_ops->finish are mandatory for
> suspending ACPI enabled machines. So using "platform" as a default
> seems

With "shutdown" we are not *suspending* machine. We are just saving
its state, and then restoring it. ACPI BIOS should not need to know.

We want both "platform" _and_ "shutdown" to work.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] preserve correct battery state through suspend/resume cycles

[Pavel Machek]

Hi!

> There is a problem in th following scenario(s):
> 
> boot -> suspend -> (un)plug battery -> resume
> 
> The problem arises in both cases - i.e. suspend with battery plugged in, 
> and resume with battery unplugged, or vice versa.
> 
> After resume, when the battery status has changed (plugged in -> unplegged 
> or unplugged -> plugged in) during the time when the system was sleeping, 
> the /proc/acpi/battery/*/* is wrong (showing the state before suspend, not 
> the current state).
> 
> The following patch adds ->resume method to the ACPI battery handler, which
> has the only aim - to check whether the battery state has changed during sleep, 
> and if so, update the ACPI internal data structures, so that information 
> published through /proc/acpi/battery/*/* is correct even after suspend/resume
> cycle, during which the battery was removed/inserted.
> 
> The patch is against current ACPI git tree, but applies cleanly also 
> against -mm and probably other trees. Please apply.
> 
> Signed-off-by: Jiri Kosina <jikos@jikos.cz>

Looks okay to me.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html