Bug 28192 - battery confuses mW and mA reporting units => wrong rate/energy
Summary: battery confuses mW and mA reporting units => wrong rate/energy
Status: CLOSED CODE_FIX
Alias: None
Product: ACPI
Classification: Unclassified
Component: Power-Battery (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Lan Tianyu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-04 18:49 UTC by Björn Schließmann
Modified: 2011-08-30 01:14 UTC (History)
3 users (show)

See Also:
Kernel Version: v2.6.38
Subsystem:
Regression: No
Bisected commit-id:


Attachments
Patch that solves the issue for me (857 bytes, patch)
2011-04-03 22:32 UTC, Björn Schließmann
Details | Diff
Refresh sysfs in acpi_battery_update(), too (1001 bytes, patch)
2011-04-06 21:07 UTC, Björn Schließmann
Details | Diff
Output of acpidump (294.59 KB, text/plain)
2011-05-31 22:33 UTC, Björn Schließmann
Details
Output of acpidump -b (63.50 KB, application/octet-stream)
2011-05-31 22:34 UTC, Björn Schließmann
Details
Contents of sysfs battery directory, everything in order. (2.10 KB, text/plain)
2011-06-13 11:18 UTC, Björn Schließmann
Details
Same as above, but now the values are wrong. (2.10 KB, text/plain)
2011-06-13 11:24 UTC, Björn Schließmann
Details
Situation after file 61802; reloaded module battery (2.11 KB, text/plain)
2011-06-13 11:25 UTC, Björn Schließmann
Details
debug patch (532 bytes, patch)
2011-06-14 07:02 UTC, Lan Tianyu
Details | Diff
add_hibernation_and_restore.patch (558 bytes, patch)
2011-06-14 07:07 UTC, Lan Tianyu
Details | Diff

Description Björn Schließmann 2011-02-04 18:49:44 UTC
On Thinkpad X201, often either of these erroneous power supply data (from /sys/class/power_supply/BAT0) are visible in user space apps:

- energy and rate too low by factor of ~10
- energy and rate too high by factor of ~10

This happens mostly after resuming from hibernate or after fresh boot. I've compiled battery as module -- reloading the module corrects the data most of the time.

Using ACPI tracing, I checked the data returned by _BIF and _BST (it always checks out with the corresponding raw sysfs values).

My suspicion as to what is happening:

- _BIF, which among others returns the reporting unit of the battery data, is only called once at module load. Depending on the unit, either sysfs file current_now or power_now is set up -- after that, always containing the number in the second field of the package returned by _BST.

- Thinkpad X201 sometimes changes its mind which unit the data is in it reports to the periodically called _BST.

- But the module doesn't detect that and so there are two possibilities of miscalculations (data set used see at bottom):

 1. unit mW at module load, changed to mA:
    - power_now is present, but its value is a current, e.g. 1312000 (= 1.312 A)
    - so power is 1.312 "W", instead of voltage_now*current_now = 14.6 W
 2. unit mA at module load, changed to mW:
    - current_now is present, but its value is a power, e.g. 14740000
      (= 14.74 W)
    - so power is calculated as voltage_now*current now, yielding 163.7 "W" in 
      user space apps (like gnome-power-manager)

- reloading the module calls _BIF and _BST in short succession and the data  units and values fit again.

The approximate factor 10 can be identified as voltage_now, which is about 11 V.

Data set used as example follows. Here, _BIF must have reported mW initially, but _BST reports mA numbers. After reloading the module, the X201 changes its mind and reports mW again.
- fresh boot: 
  $ cat energy_now voltage_now power_now
  2049000
  11093000
  1312000
- after reloading battery module:
  $ cat energy_now voltage_now power_now 
  20360000
  11108000
  14740000

Please note I suspect this is a regression since v2.6.35 or so, but I'm having trouble bisecting because it's very time consuming. Most of the time, the error only appears after the X201 was turned off for a longer time period.

Please advise if I can help, though I don't feel competent (yet) for developing a fix.
Comment 1 Björn Schließmann 2011-04-03 22:30:20 UTC
After lengthy bisections and experiments I have gained two insights:

1. It's no regression. It happens from 7faa144a518c456e2057918f030f50100144ccc6 onward (in there, power_now was introduced).

2. The attached patch solves the issue for me. It was inspired by da8aeb92d4853f37e281f11fddf61f9c7d84c3cd; the latter does not solve the problem for me though. This one does.

How well are chances to get this patch into mainline? It applies cleanly on v2.6.38 and v2.6.39-rc1.
Comment 2 Björn Schließmann 2011-04-03 22:32:37 UTC
Created attachment 53352 [details]
Patch that solves the issue for me

tested to apply on v2.6.38 and v2.6.39-rc1 of git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
Comment 3 Björn Schließmann 2011-04-06 21:07:56 UTC
Created attachment 53692 [details]
Refresh sysfs in acpi_battery_update(), too

applies cleanly on v2.6.38, v2.6.39-rc1, v2.6.39-rc2 of git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
Comment 4 Lan Tianyu 2011-05-31 08:06:29 UTC
> - _BIF, which among others returns the reporting unit of the battery data, is
> only called once at module load

The _BIF will also be called when receiving the ACPI_BATTERY_NOTIFY_INF notification. This happens in the acpi_battery_refresh. This functions will do the same thing as your patch do. If the _BIF wasn't called, your patch would make no sense. After dealing with the ACPI_BATTERY_NOTIFY_INF notification, the _BST method will be called to update associated data.

The b23fffd778c312b8fb258d342051fcbdf6712128 mentions your issue. 

> Most of the time, the error only appears after the X201 was turned off for a 
> longer time period.

Does this problem happen very time?
This looks like lacking the ACPI_BATTERY_NOTIFY_INF notification during the situation.
Pls attach the output of acpidump.
Comment 5 Björn Schließmann 2011-05-31 22:30:07 UTC
> Does this problem happen very time?

I forgot to write it; later I could reproduce it practically always by

1. hibernating/suspending
2. plugging/unplugging from mains (toggle)
3. resuming

It's a while ago now so I'm not absolutely sure if it happened only with hibernating, suspending or with both. IIRC both. I could look into it again if it'd be useful.

> This looks like lacking the ACPI_BATTERY_NOTIFY_INF notification during the
> situation.
> Pls attach the output of acpidump.

I will.
Comment 6 Björn Schließmann 2011-05-31 22:33:43 UTC
Created attachment 60282 [details]
Output of acpidump
Comment 7 Björn Schließmann 2011-05-31 22:34:35 UTC
Created attachment 60292 [details]
Output of acpidump -b
Comment 8 Lan Tianyu 2011-06-01 03:35:01 UTC
Please attach the output of the following respectively in the normal situation and the abnormal situation.

cat /proc/acpi/battery/*/*
Comment 9 Lan Tianyu 2011-06-03 08:05:03 UTC
I find the patch 25be5821521640eb00b7eb219ffe59664510d073 have resolved your problem. Have you checked the problem on the 2.6.39-rc1 or up without your patch?

commit 25be5821521640eb00b7eb219ffe59664510d073
Author: Kyle McMartin <kyle@redhat.com>
Date:   Tue Mar 22 16:19:50 2011 -0400

    ACPI battery: fribble sysfs files from a resume notifier
    
    Commit da8aeb92 re-poked the battery on resume, but Linus reports that
    it broke his eee and partially reverted it in b23fffd7. Unfortunately
    this also results in my x201s giving crack values until the sysfs files
    are poked again. In the revert message, it was suggested that we poke it
    from a PM notifier, so let's do that.
    
    With this in place, I haven't noticed the units going nutty on my
    gnome-power-manager across a dozen suspends or so...
Comment 10 Björn Schließmann 2011-06-08 19:05:21 UTC
This bug is resolved? Cool, good to know. I didn't even have any time to test it yet. I will though during the next days.
Comment 11 Björn Schließmann 2011-06-13 11:16:46 UTC
I'm sorry to say the error appears again. A freshly compiled v3.0-rc2 without my patch produces the attached files (ad #8). (With my patch it works best.)

BTW, I have no /proc/acpi, I changed the command to 

for i in /sys/class/power_supply/*/*; do echo ==== ${i} ===; cat $i; echo; done &> file.log
Comment 12 Björn Schließmann 2011-06-13 11:18:35 UTC
Created attachment 61792 [details]
Contents of sysfs battery directory, everything in order.
Comment 13 Björn Schließmann 2011-06-13 11:24:13 UTC
Created attachment 61802 [details]
Same as above, but now the values are wrong.

Note that energy_*, power_now all differ by approx. factor 10.
Comment 14 Björn Schließmann 2011-06-13 11:25:35 UTC
Created attachment 61812 [details]
Situation after file 61802; reloaded module battery

After having reloaded module battery, the values are correct again.
Comment 15 Björn Schließmann 2011-06-13 11:29:52 UTC
Forgot to mention; I created att. 61802 after hibernating the computer in the evening yesterday and waking it up this noon, with an unpatched v3.0-rc2.
Comment 16 Lan Tianyu 2011-06-14 07:02:50 UTC
Created attachment 61922 [details]
debug patch

You can add CONFIG_ACPI_PROCFS_POWER and then will be able to visit /proc/acpi.
Please try add_hibernation.patch which add the process of the hibernation event.
If it doesn't work, try add_hibernation_and_restore.patch.
Comment 17 Lan Tianyu 2011-06-14 07:07:16 UTC
Created attachment 61932 [details]
add_hibernation_and_restore.patch
Comment 18 Björn Schließmann 2011-06-14 22:41:56 UTC
Thanks, I'll do so and report back.
Comment 19 Björn Schließmann 2011-06-19 17:12:04 UTC
Since my last message, I'm testing v3.0-rc3 with "add_hibernation.patch", and the error condition hasn't shown itself once. I'd consider this bug fixed. Thanks for your time :)
Comment 20 Lan Tianyu 2011-06-21 02:55:58 UTC
https://patchwork.kernel.org/patch/896172/
Comment 21 Len Brown 2011-07-31 18:10:52 UTC
patch above staged in acpi-test for 3.1
Comment 22 Florian Mickler 2011-08-08 08:21:41 UTC
A patch referencing this bug report has been merged in Linux v3.1-rc1:

commit d5a5911b3278bad6515a9958f7318f74d534ef64
Author: Lan Tianyu <tianyu.lan@intel.com>
Date:   Thu Jun 30 11:33:40 2011 +0800

    ACPI / Battery: Add the hibernation process in the battery_notify()

Note You need to log in before you can comment on or make changes to this bug.