Bug 10699 - Wrong battery information on MSI S271 laptop with msi-laptop module loaded - 2.6.23 regression
Wrong battery information on MSI S271 laptop with msi-laptop module loaded - ...
Status: CLOSED UNREPRODUCIBLE
Product: ACPI
Classification: Unclassified
Component: EC
All Linux
: P1 low
Assigned To: Alexey Starikovskiy
:
: 11632 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-14 14:54 UTC by Mike Trim
Modified: 2014-01-17 16:15 UTC (History)
9 users (show)

See Also:
Kernel Version: 3.12.5
Tree: Mainline
Regression: Yes


Attachments
Output of acpidump (85.61 KB, text/plain)
2008-05-27 11:56 UTC, Mike Trim
Details
try the debug patch (2.04 KB, patch)
2008-05-29 00:15 UTC, ykzhao
Details | Diff
Output of dmesg after booting and modprobing msi-laptop (32.91 KB, text/plain)
2008-10-01 12:21 UTC, Mike Trim
Details
dmesg output with patch from bug #11549, force_poll=1 (119.65 KB, text/plain)
2008-10-02 13:53 UTC, Mike Trim
Details
dmesg output with patch from bug #11549, force_poll=0 (155.91 KB, text/plain)
2008-10-08 12:38 UTC, Mike Trim
Details
don't write auto_brightness bit if already set (429 bytes, patch)
2008-10-08 13:25 UTC, Alexey Starikovskiy
Details | Diff
Use burst mode everywhere (3.12 KB, patch)
2008-10-08 14:22 UTC, Alexey Starikovskiy
Details | Diff
dmesg output with patches from comment #19 and comment #20 (213.23 KB, text/plain)
2008-10-13 11:26 UTC, Mike Trim
Details
ec patch 4 (329 bytes, patch)
2009-02-14 11:56 UTC, Timo Tretter
Details | Diff
Console session log (1.36 KB, text/plain)
2009-03-31 19:14 UTC, Mike Trim
Details
dmesg output for above session (37.06 KB, text/plain)
2009-03-31 19:15 UTC, Mike Trim
Details
merge modes and disable burst (9.53 KB, patch)
2009-04-05 06:49 UTC, Alexey Starikovskiy
Details | Diff
Console session log with patch from comment #33 (5.59 KB, text/plain)
2009-04-06 18:48 UTC, Mike Trim
Details
dmesg output for above session (with patch from comment #33) (57.44 KB, text/plain)
2009-04-06 18:49 UTC, Mike Trim
Details
merge irq and poll modes #3 (11.65 KB, patch)
2009-04-06 19:01 UTC, Alexey Starikovskiy
Details | Diff
dmesg output with patch from comment #36 (37.12 KB, text/plain)
2009-04-07 19:05 UTC, Mike Trim
Details
dmesg output after booting with blacklisted msi_laptop.ko and loading it manually (54.04 KB, text/plain)
2014-01-11 01:13 UTC, Bernd Steinhauser
Details

Description Mike Trim 2008-05-14 14:54:41 UTC
Latest working kernel version: cfc94cdf8e0f14e692a5a40ef3cc10f464b2511b (after v2.6.22)
Earliest failing kernel version: 4f5c791a850e5305a5b1b48d0e4b4de248dc96f9 (before v2.6.23-rc1)
Distribution: Ubuntu 8.04 (Hardy Heron)
Hardware Environment: MSI S271 laptop (MS-1058)
Software Environment: clean install
Problem Description:

Ubuntu bug: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/218094

On an MSI S271 laptop (aka MS-1058), the battery state is reported incorrectly after the msi-laptop module is loaded.

e.g. (with battery fully charged) 
$ cat /proc/acpi/battery/BAT1/state

Correct behaviour:
remaining capacity: 3670 mAh

After msi-laptop loaded:
remaining capacity:      526 mAh  (reported values are not consistent)
or sometimes:
remaining capacity:      unknown

See the Ubuntu bug (https://bugs.launchpad.net/ubuntu/+source/linux/+bug/218094) for more detailed output.


I have run git bisect which tracked this down to:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=4f5c791a850e5305a5b1b48d0e4b4de248dc96f9
commit 4f5c791a850e5305a5b1b48d0e4b4de248dc96f9 
Author: Lennart Poettering <mzxreary@0pointer.de>
Date: Tue May 8 22:07:02 2007 +0200

    DMI-based module autoloading

This patch causes the msi-laptop module to be automatically loaded on laptops that are (supposed to be) supported by the msi-laptop module.  With this commit the msi-laptop module is automatically loaded as expected:
 msi-laptop: Identified laptop model 'MSI S271'.
 msi-laptop: driver 0.5 successfully loaded.


I have verified that blacklisting the msi-laptop module restores the correct battery status on a clean boot.  If the module is subsequently loaded then the battery status again reports incorrect values.  However, removing the module after it has been loaded does not restore the battery state to the correct values.


This behaviour occurs on all kernel versions after this commit (up to v2.6.26-rc2 tested), on both amd64 and i386.
Comment 1 Mike Trim 2008-05-15 13:20:47 UTC
The commit above only autoloads the module, so at Lennart's suggestion, I have done a further bisection with the msi-laptop module loaded manually.  This has narrowed the problem down to the following commit:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=00eb43a1897a8845d0edb198cec69ac5f1f299dd
commit 00eb43a1897a8845d0edb198cec69ac5f1f299dd
Author: Lennart Poettering <mzxreary@0pointer.de>
Date:   Fri May 4 14:16:19 2007 +0200

    acpi,msi-laptop: Fall back to EC polling mode for MSI laptop specific EC commands



Comment 2 ykzhao 2008-05-26 20:26:48 UTC
Hi, Mike 
    Will you please try the latest kernel and see whether the problem still exists? 
    Will you please confirm whether the 2.6.23-rc1 kernel can work well if the commit 00eb43a1897a8845d0edb198cec69ac5f1f299dd is reverted?
    Please attach the output of acpidump.
    Thanks.
Comment 3 Mike Trim 2008-05-27 11:55:58 UTC
The problem still occurs with the latest kernel (v.2.6.26-rc4).

I have done some more digging, and the problem specifically seems to occur after a call to any of get_lcd_level(), get_auto_brightness() and set_auto_brightness() functions in msi-laptop.c.

These functions each contain the following (lines 97, 109 and 123 in current revision of msi-laptop.c):
    result = ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, &wdata, 1, &rdata, 1, 1); 

When using the current value of 1 for force_poll in these calls (at least for get_lcd_level(), get_auto_brightness()), reading the corresponding values from sysfs (/sys/devices/platform/msi-laptop-pf/) always returns 0, and subsequent reads of the battery status from /proc/acpi/battery/ return garbage values.

Changing the force_poll argument in these calls to 0 (which effectively restores the pre-00eb43a1... behaviour) in each of these instances gives the correct behaviour (both sysfs info and battery info), however each call takes approximately 1 second to execute in this case (which was the original reason for the force_poll patch).

The other calls to ec_transaction() in msi-laptop.c all work correctly regardless of the force_poll argument.

The battery status is read correctly until one of the functions referenced above is called, usually by reading /sys/devices/platform/msi-laptop-pf/.  The battery state continues to be read incorrectly even if the module is unloaded.

By default, the module init function msi_init() calls set_auto_brightness() on load, causing the erroneous behaviour when the module is loaded.  If this is disabled with auto_brightness=2 then the battery info continues to be reported correctly until one of the offending functions is read via sysfs.
Comment 4 Mike Trim 2008-05-27 11:56:58 UTC
Created attachment 16298 [details]
Output of acpidump
Comment 5 ykzhao 2008-05-29 00:15:44 UTC
Created attachment 16312 [details]
try the debug patch

After the following patch is applied, EC polling mode is used for some EC commands. 
commit 00eb43a1897a8845d0edb198cec69ac5f1f299dd
Author: Lennart Poettering <mzxreary@0pointer.de>
Date:   Fri May 4 14:16:19 2007 +0200

    acpi,msi-laptop: Fall back to EC polling mode for MSI laptop specific EC commands

Will you please not use the EC polling mode in the MSI laptop driver and see whether the problem still exists?
Thanks.
Comment 6 Mike Trim 2008-05-29 06:42:15 UTC
ykzhao: I have already tested this, please see my comment #3 above.

Without EC polling mode, the problem does not occur, but each call takes about 1 second to execute.  I have also tested each ec_transaction call independently, and the problem occurs only with the calls at lines 97, 109 and 123 of msi-laptop.c.  The calls in question are reading from MSI_EC_COMMAND_LCD_LEVEL (writing seems to not be affected, nor is MSI_EC_COMMAND_WIRELESS).

When using EC polling mode for the three calls referenced above, the corresponding sysfs entry always returns 0 instead of the normal value.  The battery state is read correctly until one of those functions occurs and afterwards always returns invalid data, even if the msi-laptop module is unloaded.
Comment 7 ykzhao 2008-08-10 06:51:32 UTC
HI, Mike
   Will you please try the debug patch in
http://bugzilla.kernel.org/show_bug.cgi?id=9823#c47 
and see whether the problem still exists?
  Thanks.
Comment 8 Mike Trim 2008-08-11 11:53:13 UTC
Hi ykzhao,

I have tried the patch you referenced and the problem still occurs.
Comment 9 ykzhao 2008-08-14 00:56:37 UTC
Hi, Mike
    I check the log info in https://bugs.launchpad.net/ubuntu/+source/linux/+bug/218094 
and find some weird info related with the battery.
   > cat /proc/acpi/battery/BAT1/info
   > present: yes
   > design capacity: 17 mAh 
   > last full capacity: 12302 mAh (this is already larger than design capacity).
   > battery technology: rechargeable
   > design voltage: 22073 mV
   > design capacity warning: 0 mAh
   
   Will you please double check whether the battery info is always correct before MSI-laptop module is loaded? Please attach the output of dmesg.
   Will you please attach the output of dmesg after trying the patch in comment #7?
   Thanks.
Comment 10 ykzhao 2008-09-23 00:38:23 UTC
As there is no response for more than one month, the bug will be rejected. 
If the problem still exists, please do the test as required in comment #9 and reopen it.
Thanks.
Comment 11 Mike Trim 2008-10-01 12:21:26 UTC
Created attachment 18135 [details]
Output of dmesg after booting and modprobing msi-laptop
Comment 12 Mike Trim 2008-10-01 12:22:24 UTC
Tested again with v2.6.27-rc8, which includes the patch referenced in comment #7.

The battery info is always correct before the msi-laptop module is loaded.  After it is loaded, /proc/acpi/battery/BAT1/info remains correct (this seems to have changed since I originally filed the bug) but /proc/acpi/battery/BAT1/state is corrupted:
$ cat /proc/acpi/battery/BAT1/state
present:                 yes
capacity state:          ok
charging state:          charged
present rate:            unknown
remaining capacity:      unknown
present voltage:         10000 mV

Comment 13 Alexey Starikovskiy 2008-10-01 12:50:32 UTC
Hi Mike,
Could you please try the patch from bug #11549, uncomment DEBUG at the beginning of /drivers/acpi/ec.c, change force_poll to 0 and post a dmesg?
Comment 14 Mike Trim 2008-10-02 13:53:18 UTC
Created attachment 18140 [details]
dmesg output with patch from bug #11549, force_poll=1

This is the output of dmesg with the patch from bug #11549, and DEBUG enabled, using force_poll=1 (the default).  The first few seconds of the dmesg are lost due to the number of messages.

The msi-laptop driver is loaded at [  287.834007]; after that I ran cat /proc/acpi/battery/BAT1/info and /state a couple of times.  I still get the same erroneous output from /state as above.

I will try again with force_poll=0 tomorrow.
Comment 15 ykzhao 2008-10-06 19:45:01 UTC
hi, Mike
    Thanks for the confirmation.
    It seems that this issue is related with the following commit:
   > commit 00eb43a1897a8845d0edb198cec69ac5f1f299dd
   > Author: Lennart Poettering <mzxreary@0pointer.de>
   > Date:   Fri May 4 14:16:19 2007 +0200
      >acpi,msi-laptop: Fall back to EC polling mode for MSI laptop specific EC
commands

   The EC command used in msi-laptop driver is non-standard EC command. Maybe
it can be handled by Lennart Poettering <mzxreary@0pointer.de>.
   thanks.

Comment 16 ykzhao 2008-10-06 19:46:31 UTC
*** Bug 11632 has been marked as a duplicate of this bug. ***
Comment 17 Alexey Starikovskiy 2008-10-07 14:35:40 UTC
Mike, please provide force_poll=0 data.
Comment 18 Mike Trim 2008-10-08 12:38:40 UTC
Created attachment 18220 [details]
dmesg output with patch from bug #11549, force_poll=0

dmesg output when force_poll=0 is attached.  I still get the same results, i.e. before loading msi-laptop I get: 

$ cat /proc/acpi/battery/BAT1/state 
present:                 yes
capacity state:          ok
charging state:          charged
present rate:            0 mA
remaining capacity:      3336 mAh
present voltage:         16644 mV

after loading msi-laptop I get:

$ cat /proc/acpi/battery/BAT1/state 
present:                 yes
capacity state:          ok
charging state:          charged
present rate:            unknown
remaining capacity:      unknown
present voltage:         10000 mV

I have inserted some comments into the attachment (starting with #####) explaining what commands I was running at the time.

This behaviour is a bit different to what I expected as in comment #3 I found that using force_poll=0 got rid of the problem (but caused the msi-laptop functions to be very slow).  I'm not sure if this is because of other changes to the kernel since that time or if it is related to the patch.
Comment 19 Alexey Starikovskiy 2008-10-08 13:25:51 UTC
Created attachment 18223 [details]
don't write auto_brightness bit if already set

let's assume that force_poll does not break EC by itself.
Please add this patch to the mix, it should remove one of the two "special transactions" (last one).
Comment 20 Alexey Starikovskiy 2008-10-08 14:22:19 UTC
Created attachment 18224 [details]
Use burst mode everywhere

Looking at Lennarts' userspace tool, it appears that it used to switch to burst mode for transaction, so let's try to do it too. Please add on top of the mix...
Comment 21 Mike Trim 2008-10-13 11:26:26 UTC
Created attachment 18285 [details]
dmesg output with patches from comment #19 and comment #20

Attached is dmesg output with the above patches, annotated as before.

This time, loading msi-laptop does not cause the /proc/acpi/... output to be broken; however after accessing /sys/devices/platform/msi-laptop-pf/auto-brightness the problem occurs as before.
Comment 22 ykzhao 2008-11-03 07:56:48 UTC
Hi, Mike
    Will you please try the patch set in http://bugzilla.kernel.org/show_bug.cgi?id=10699#C1,2,3,4 on the latest kernel and see whether the problem still exists?
    Thanks.
Comment 23 ykzhao 2008-11-03 07:57:51 UTC
Hi, Mike
    Sorry that I give the incorrect address in comment #22.
    Will you please try the patch set in
http://bugzilla.kernel.org/show_bug.cgi?id=11896#C1,2,3,4 on the latest kernel
and see whether the problem still exists?
    Thanks.

    
Comment 24 Mike Trim 2008-11-08 07:40:48 UTC
Hi,

I tried the patches with v2.6.28-rc3, but I was unable to boot successfully (probably due to some unrelated bug) -- I will try again in a couple more rcs time.  Unfortunately the patches do not apply to v2.6.27 series kernel that I am running.
Comment 25 ykzhao 2008-11-10 00:54:50 UTC
Hi,Mike
   Will you please go to the bug 11969 and try the attached patch set(1-6) on the 2.6.27 stable kernel?
   After the test, please attach the output of dmesg to the bug 11969.
   Thanks.
Comment 26 Zhang Rui 2009-01-04 18:52:48 UTC
ping Mike
Comment 27 Mike Trim 2009-01-23 11:07:48 UTC
Hi,

Apologies, I have not had much time for testing recently.

I have tested the v2.6.28.1 kernel (unpatched) and the problem still exists there.  I did do some testing a while ago using the patches in bug 11969 and noticed some improvement, but the behaviour was a bit intermittent so I wasn't able to get any conclusive results, and I didn't have time to pursue it.

If someone would like me to test patches against a newer kernel please ask, however I may not able to spend a lot of time on this issue.
Comment 28 Timo Tretter 2009-02-14 11:56:47 UTC
Created attachment 20244 [details]
ec patch 4

this patch is tested for a full discharging/charging cycle on a MSI PR200.
Comment 29 Timo Tretter 2009-02-14 11:58:10 UTC
oh sorry, wrong bug
Comment 30 Zhang Rui 2009-03-30 07:38:50 UTC
Mike,
does the problem still exists in 2.6.29?
Comment 31 Mike Trim 2009-03-31 19:14:44 UTC
Created attachment 20763 [details]
Console session log

I have tried again with v2.6.29 and the problem still exists.  See attached output of `cat /proc/acpi/battery/BAT1/state` before and after loading msi-laptop.
Comment 32 Mike Trim 2009-03-31 19:15:57 UTC
Created attachment 20764 [details]
dmesg output for above session
Comment 33 Alexey Starikovskiy 2009-04-05 06:49:20 UTC
Created attachment 20807 [details]
merge modes and disable burst

Please check if this patch helps?
Comment 34 Mike Trim 2009-04-06 18:48:22 UTC
Created attachment 20833 [details]
Console session log with patch from comment #33

I have tested again with v2.6.29 + the patch from comment #33 above.

This patch does have an effect on the behaviour.  With just v2.6.29, all calls to `cat /proc/acpi/battery/BAT1/state` after loading msi-laptop returned 'unknown' state.  With v2.6.29+patch, sometimes the results is 'unknown' and other times some values are returned, however these values are incorrect and change randomly.  Somewhat different values are returned if the mains cable is disconnected also (see end of console log).

The patch results in a log of identical messages on dmesg (see next attachment):
ACPI: EC: non-query interrupt received, switching to interrupt mode

Also note the following messages in the dmesg output occur when the mains cable is plugged/unplugged:
atkbd.c: Unknown key pressed (translated set 2, code 0xf1 on isa0060/serio0).
Comment 35 Mike Trim 2009-04-06 18:49:36 UTC
Created attachment 20834 [details]
dmesg output for above session (with patch from comment #33)
Comment 36 Alexey Starikovskiy 2009-04-06 19:01:07 UTC
Created attachment 20835 [details]
merge irq and poll modes #3

Could you please check this patch?
Comment 37 Mike Trim 2009-04-07 19:05:51 UTC
Created attachment 20863 [details]
dmesg output with patch from comment #36

I have tested with v2.6.29 + the patch from comment #36.

It behaves exactly as with v2.6.29, i.e. /proc/acpi/battery/BAT1/state shows 'unknown' status all the time after loading msi-laptop.  The only difference I noticed in dmesg is that the message "ACPI: EC: missing confirmations, switch off interrupt mode." no longer appears.
Comment 38 Len Brown 2009-08-13 03:01:56 UTC
closing due to no activity in this bug report for 4 months.
please re-open if this is still an issue in 2.6.32.stable
Comment 39 Len Brown 2009-08-13 03:02:29 UTC
s/2.6.32.stable/latest stable kernel:-)
Comment 40 Bernd Steinhauser 2014-01-11 01:12:16 UTC
So finally, I got my laptop battery working again and even though this bug is really old, I can still reproduce it with kernel 3.12.5.
Comment 41 Bernd Steinhauser 2014-01-11 01:13:41 UTC
Created attachment 121621 [details]
dmesg output after booting with blacklisted msi_laptop.ko and loading it manually

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