Hi Rafael et al,
Rafael J. Wysocki wrote:
> A notification event 0x81 from an ACPI battery device requires us to
> re-read the battery information structure. Follow this requirement
> and remove and re-create the battery's attibutes in sysfs so that
> they reflect the reporting units used by the battery at the moment
> (those units may actually change sometimes at run time, which happens
> on some Thinkpads).
Ralf Jung was noticing his system tray power management icon in the
bottom-right corner *flickering* every 30 seconds with recent kernels.
Bisects to v2.6.38-rc1~68^2~4 ("ACPI / Battery: Update information on
info notification and resume"), which has the above description.
Some affected systems:
HP ProBook 4515s/3077, BIOS 68GPI Ver. F.03 (Adrian Fita)
HP ProBook 4510s (Paolo Scarabelli)
HP Compaq 615 (Ralf Jung)
Some symptoms:
KDE power management icon shows "no battery" for a fraction of a
second (upower is its backend)
upowerd makes CPU run at close to 100% CPU all the time (?)
/var/log/syslog gets lots of messages from laptop-mode, like this:
| Apr 20 10:52:14 zero laptop-mode: Laptop mode
| Apr 20 10:52:14 zero laptop-mode: enabled,
| Apr 20 10:52:14 zero laptop-mode: not active [unchanged]
| Apr 20 10:52:14 zero laptop-mode: Laptop mode
| Apr 20 10:52:14 zero laptop-mode: enabled,
| Apr 20 10:52:14 zero laptop-mode: not active [unchanged]
| Apr 20 10:52:33 zero laptop-mode: Laptop mode
| Apr 20 10:52:33 zero laptop-mode: enabled,
Known problems? Is there some way to handle notification events
without these side effects? Anything the submitters can do to help
track details down?
An acpidump from Adrian's system can be found at [1]. More background
at [2].
Thanks,
Jonathan
[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=54;filename=acpidump_HP_ProBook_4515s.dat.gz;att=1;bug=670958
[2] http://bugs.debian.org/670958
On 01/05/12 21:47, Jonathan Nieder wrote:
>
> [...]
>
> [...] More background
> at [2].
>
> [2] http://bugs.debian.org/670958
Also, searching on Google after "upowerd device
removed:/org/freedesktop/UPower/devices/battery_BAT0", reveals much more
bug reports with the exact issue.
Thanks,
--
Fita Adrian
(cc-ing Andy)
Adrian Fita wrote:
> Also, searching on Google after "upowerd device
> removed:/org/freedesktop/UPower/devices/battery_BAT0", reveals much
> more bug reports with the exact issue.
Thanks. That confirms the high CPU consumption in upowerd ---
see [1], for example.
[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/987807
Hi,
in addition to the constant flickering when running on AC, there is a more
"high frequency" flickering immediately after plugging in the AC: For some 5
to 10 seconds, the battery appears and disappears (according to upower) around
once per second. There's also a short moment without battery after plugging
out the AC.
All this is gone after going to a kernel version without this patch applied.
I did not notice unusual high CPU usage of upower on my system, however I
noticed disc activity - according to iotop, upower is writing several MiB of
data per minute to /var/lib/upower/ where it keeps some battery statistics. I
do not know whether this is out of the ordinary.
Kind regards,
Ralf
On Tuesday 01 May 2012 21:14:08 Jonathan Nieder wrote:
> (cc-ing Andy)
>
> Adrian Fita wrote:
> > Also, searching on Google after "upowerd device
> > removed:/org/freedesktop/UPower/devices/battery_BAT0", reveals much
> > more bug reports with the exact issue.
>
> Thanks. That confirms the high CPU consumption in upowerd ---
> see [1], for example.
>
> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/987807
Hi,
I have the same problem on kernel 3.2.0 (actually I have the same
problem with all kernels I tried since reporting the issue the first time).
I'm almost sure this is related to several Debian bugs open for upower:
#606414, #596721 and #619343.
Is it possible that the high cpu problem with upowerd is caused just to
the log size increasing too much when the laptop is left on for a long time?
In my laptop upowerd starts using a lot of cpu only when I leave it on
overnight.
Regards,
Paolo
On 05/02/2012 03:42 AM, Ralf Jung wrote:
> Hi,
>
> in addition to the constant flickering when running on AC, there is a more
> "high frequency" flickering immediately after plugging in the AC: For some 5
> to 10 seconds, the battery appears and disappears (according to upower) around
> once per second. There's also a short moment without battery after plugging
> out the AC.
> All this is gone after going to a kernel version without this patch applied.
>
> I did not notice unusual high CPU usage of upower on my system, however I
> noticed disc activity - according to iotop, upower is writing several MiB of
> data per minute to /var/lib/upower/ where it keeps some battery statistics. I
> do not know whether this is out of the ordinary.
>
> Kind regards,
> Ralf
>
>
> On Tuesday 01 May 2012 21:14:08 Jonathan Nieder wrote:
>> (cc-ing Andy)
>>
>> Adrian Fita wrote:
>>> Also, searching on Google after "upowerd device
>>> removed:/org/freedesktop/UPower/devices/battery_BAT0", reveals much
>>> more bug reports with the exact issue.
>>
>> Thanks. That confirms the high CPU consumption in upowerd ---
>> see [1], for example.
>>
>> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/987807
>
On Tue, May 01, 2012 at 02:14:08PM -0500, Jonathan Nieder wrote:
> (cc-ing Andy)
> Adrian Fita wrote:
>
> > Also, searching on Google after "upowerd device
> > removed:/org/freedesktop/UPower/devices/battery_BAT0", reveals much
> > more bug reports with the exact issue.
>
> Thanks. That confirms the high CPU consumption in upowerd ---
> see [1], for example.
>
> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/987807
It does seem somewhat heavyweight to be removing and reinstalling all of
the sysfs files every time we get one of these events. I am assuming
here that some BIOSs are using this interface to tell us the batery
capacity has changed and triggering the constant add/remove of the
devices and associated flickering.
>From the description of the change this is necessary because the
capacity units may change over time? Can we not use those to avoid this
update? I presume it is these two we are referring to?
int capacity_granularity_1;
int capacity_granularity_2;
If those are unchanged perhaps we can just skip the update? Something
like the below (completly untested, for discussion).
Thoughts?
-apw
commit d558d0c38e26e2c7eae68d19f4d2fa3ecd8e31f2
Author: Andy Whitcroft <[email protected]>
Date: Thu May 3 09:52:28 2012 +0100
battery: only refresh the sysfs files when pertinant information changes
We only need to regenerate the sysfs files when the capacity units
change, avoid the update otherwise.
Signed-off-by: Andy Whitcroft <[email protected]>
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index eb18c44..f8d37b4 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -643,10 +643,20 @@ static int acpi_battery_update(struct acpi_battery *battery)
static void acpi_battery_refresh(struct acpi_battery *battery)
{
+ int cg1, cg2;
+
if (!battery->bat.dev)
return;
+ cg1 = battery->capacity_granularity_1;
+ cg2 = battery->capacity_granularity_2;
+
acpi_battery_get_info(battery);
+
+ if (cg1 == battery->capacity_granularity_1 &&
+ cg2 == capacity_granularity_2)
+ return;
+
/* The battery may have changed its reporting units. */
sysfs_remove_battery(battery);
sysfs_add_battery(battery);
On Thu, May 03, 2012 at 09:54:58AM +0100, Andy Whitcroft wrote:
>
> From the description of the change this is necessary because the
> capacity units may change over time? Can we not use those to avoid this
> update? I presume it is these two we are referring to?
>
> int capacity_granularity_1;
> int capacity_granularity_2;
power_unit rather than capacity_granularity, but the idea seems solid.
--
Matthew Garrett | [email protected]
We only need to regenerate the sysfs files when the capacity units
change, avoid the update otherwise.
Signed-off-by: Andy Whitcroft <[email protected]>
---
drivers/acpi/battery.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Based on Matthew's feedback here is a version which optimises
based on the power_unit field as returned from the battery info.
Could someone who suffers from this issue please test this out
and report back. Thanks.
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 86933ca..7dd3f9f 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -643,11 +643,19 @@ static int acpi_battery_update(struct acpi_battery *battery)
static void acpi_battery_refresh(struct acpi_battery *battery)
{
+ int power_unit;
+
if (!battery->bat.dev)
return;
+ power_unit = battery->power_unit;
+
acpi_battery_get_info(battery);
- /* The battery may have changed its reporting units. */
+
+ if (power_unit == battery->power_unit)
+ return;
+
+ /* The battery has changed its reporting units. */
sysfs_remove_battery(battery);
sysfs_add_battery(battery);
}
--
1.7.9.5
Hi,
I applied this to 3.4-rc5, and it fixes the issue. Thanks a lot :)
> We only need to regenerate the sysfs files when the capacity units
> change, avoid the update otherwise.
>
> Signed-off-by: Andy Whitcroft <[email protected]>
Tested-by: Ralf Jung <[email protected]>
> ---
> drivers/acpi/battery.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> Based on Matthew's feedback here is a version which optimises
> based on the power_unit field as returned from the battery info.
> Could someone who suffers from this issue please test this out
> and report back. Thanks.
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 86933ca..7dd3f9f 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -643,11 +643,19 @@ static int acpi_battery_update(struct acpi_battery
> *battery)
>
> static void acpi_battery_refresh(struct acpi_battery *battery)
> {
> + int power_unit;
> +
> if (!battery->bat.dev)
> return;
>
> + power_unit = battery->power_unit;
> +
> acpi_battery_get_info(battery);
> - /* The battery may have changed its reporting units. */
> +
> + if (power_unit == battery->power_unit)
> + return;
> +
> + /* The battery has changed its reporting units. */
> sysfs_remove_battery(battery);
> sysfs_add_battery(battery);
> }
Kind regards,
Ralf Jung
On 04/05/12 16:29, Ralf Jung wrote:
> Hi,
>
> I applied this to 3.4-rc5, and it fixes the issue. Thanks a lot :)
[...]
Hi. I got to test the patch today against the 3.2.16 stable kernel and I
can confirm that it solved the issue. I no longer see "remove"/"add"
events when running "udevadm monitor --property"; I only see "change"
events.
Thanks alot!
Do you know if this patch will be backported to the 3.2 kernel, which
will be the stable kernel for many linux distributions for many years (I
know that the next Debian Stable/wheezy will be using the 3.2 kernel)?
PS: you misspelled "pertinent" in the patch's title. You wrote "pertinant".
Regards,
--
Fita Adrian
applied to ACPI next branch
thanks,
Len Brown, Intel Open Source Technology Center