Bug 9494

Summary: Battery shows up twice in kpowersave
Product: ACPI Reporter: Rolf Eike Beer (eike-kernel)
Component: Power-BatteryAssignee: Alexey Starikovskiy (astarikovskiy)
Severity: normal CC: acpi-bugzilla, arvidjaar, astarikovskiy, fabio.comolli, lenb, marcus
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: d7380965752505951668e85de59c128d1d6fd21f Subsystem:
Regression: Yes Bisected commit-id:
Bug Depends on:    
Bug Blocks: 9243    
Attachments: acpidump
Update state on access
Update state on access #2
Make sysfs interface to ACPI power devices optional
Make sysfs interface to ACPI power devices optional #2

Description Rolf Eike Beer 2007-12-03 12:06:32 UTC
Most recent kernel where this bug did not occur: aa650bbdcb94bde4292eabc44490970825c98669
Distribution: openSuSE 10.3
Hardware Environment: Toshiba Satellite A110-178
Software Environment: KDE 3.5.7
Problem Description: Battery shows up twice in kpowersave. This leads to bad behaviour if the battery is empty as the laptop is not suspended automatically.

Bisect log:

# good: [aa650bbdcb94bde4292eabc44490970825c98669] ACPI: Battery: Misc clean-ups, no functional changes
git-bisect good aa650bbdcb94bde4292eabc44490970825c98669
# bad: [00a2b433557f10736e8a02de619b3e9052556c12] Pull acpica into test branch
git-bisect bad 00a2b433557f10736e8a02de619b3e9052556c12
# good: [4ecbca8554d0f643351ee07d3284138a5e85ba81] Remove unnecessary cast in prefetch()
git-bisect good 4ecbca8554d0f643351ee07d3284138a5e85ba81
# good: [6cffd46651b881a11791a7ef4d99f2f20853fcd9] ACPI: clean up acpi_enter_sleep_state_prep
git-bisect good 6cffd46651b881a11791a7ef4d99f2f20853fcd9
# good: [73a855f70d9b3571fa878727cbc568d9774c6dce] Pull video into release branch
git-bisect good 73a855f70d9b3571fa878727cbc568d9774c6dce
# bad: [d5b4a3d0efa36de31b86d5677dad6c36cb8735d7] ACPI: AC: Add sysfs interface
git-bisect bad d5b4a3d0efa36de31b86d5677dad6c36cb8735d7
# bad: [91087dfa51a29b3c190e99339c4c32eb13646c51] ACPI: SBS: Split host controller (ACPI0001) from SBS driver (ACPI0002)
git-bisect bad 91087dfa51a29b3c190e99339c4c32eb13646c51
# bad: [3e58ea0d31659b22ba5753f7abf3d7db346eab81] ACPI: Battery: add sysfs alarm
git-bisect bad 3e58ea0d31659b22ba5753f7abf3d7db346eab81
# bad: [d7380965752505951668e85de59c128d1d6fd21f] ACPI: Battery: Add sysfs support
git-bisect bad d7380965752505951668e85de59c128d1d6fd21f
Comment 1 Alexey Starikovskiy 2007-12-03 12:56:23 UTC
Could you please append acpidump and .config?
also, please do a 'ls /proc/acpi/battery/*' and /sys/class/power*/
Comment 2 Rolf Eike Beer 2007-12-03 13:48:03 UTC
donald:~ # ls /proc/acpi/battery/*
alarm  info  state
donald:~ # ls /sys/class/power*/
donald:~ # ls /sys/class/power_supply/BAT1/
charge_full         charge_now   device        model_name  present  subsystem   type    voltage_min_design
charge_full_design  current_now  manufacturer  power       status   technology  uevent  voltage_now
Comment 3 Rolf Eike Beer 2007-12-03 13:49:14 UTC
Created attachment 13840 [details]
Comment 4 Rolf Eike Beer 2007-12-03 13:49:57 UTC
Created attachment 13841 [details]
Comment 5 Alexey Starikovskiy 2007-12-03 22:58:41 UTC
could you please check the latest git tree? 2.6.24-rc4 is good starting point.
Please try to set CONFIG_ACPI_PROCFS_POWER to No, if it does not work.
Comment 6 Rolf Eike Beer 2007-12-04 10:36:01 UTC
Comment 7 Andrey Borzenkov 2007-12-07 22:39:11 UTC
Danny works on it. But yes, this *is* user space breakage after all. Personally I think we need conditional compiling of power_supply and automatically disable procfs interface if power_supply is compiled in.

It would be interesting to see what does not work as expected in sysfs code.

On Samstag, 27. Oktober 2007, Danny Kukawka wrote:
> On Samstag, 27. Oktober 2007, Andrey Borzenkov wrote:
> > In kernel 2.6.24-rc1 if I turn on ACPI_PROCFS number of batteries in HAL
> > is doubled; it gets them once via sysfs and once via /proc. Because there
> > may be legal reasons to enable ACPI_PROCFS for compatibility and because
> > there is no way to disable sysfs entries it would be nice if HAL could
> > detect those duplicates. What I am not exactly sure, how. After all we
> > may have non-ACPI batteries in sysfs and ACPI ones in procfs ...
> >
> > -andrey
> Okay, I take a look at this. Could you send me your lshal output directly?

Update: found the problems with 2.6.24 and wrote a first patch to fix it, but 
this need much more changes in the acpi addon and on several other places, 
since the sysfs battery/AC code don't work as expected atm. I send a patch as 
soon as it work.
Comment 8 Alexey Starikovskiy 2007-12-08 02:54:39 UTC
all entries in /sys/class/power_supply have parent, which would be some acpi_device in case you mention.
Comment 9 Rolf Eike Beer 2007-12-10 03:36:14 UTC
It looks like charge_now in sysfs does not decrease when laptop is on battery. At least I doubt it will stay on the same value for half an hour when I was using the laptop. After plugging power in it decreased by a big amount (one third or something like that).
Comment 10 Alexey Starikovskiy 2007-12-10 05:22:51 UTC
Created attachment 13942 [details]
Update state on access

Please check if this patch helps.
Comment 11 Rolf Eike Beer 2007-12-11 03:15:09 UTC
Does not change anything
Comment 12 Alexey Starikovskiy 2007-12-11 03:39:24 UTC
Created attachment 13970 [details]
Update state on access #2

Please check this one...
Comment 13 Rolf Eike Beer 2007-12-11 04:01:32 UTC
I think it's the completely wrong place to fix as I don't have SBS enabled at all.
Comment 14 Alexey Starikovskiy 2007-12-11 04:49:59 UTC
right... I was confused by your bisect log, and the fact that rc4 has patch for CM battery to update on read in sysfs... 
Comment 15 Alexey Starikovskiy 2007-12-11 12:55:41 UTC
Rolf, could you add printk to acpi_battery_get_state() near the evaluate_object and check that you see it in dmesg after read of current from /sys?
Comment 16 Rolf Eike Beer 2007-12-12 02:31:57 UTC
evaluate_object() returns 0. I see the message only when I cat that file by hand so hal is not looking at it at all. That means that it became pretty useless now. hal can't even detect if I remove the battery now.
Comment 17 Alexey Starikovskiy 2007-12-12 02:39:56 UTC
so hal looks at sysfs battery to enumerate it and then ignores it completely?
Comment 18 Rolf Eike Beer 2007-12-12 02:47:37 UTC
seems to be that way.
Comment 19 Rolf Eike Beer 2007-12-12 06:17:27 UTC
It looks into charge_now once every time you plug or unplug the AC connector.
Comment 20 Alexey Starikovskiy 2007-12-12 07:36:54 UTC
this means hal looks at it only then gets notify from power_supply...
Comment 21 Andrey Borzenkov 2007-12-18 00:14:46 UTC
(In reply to comment #20)
> this means hal looks at it only then gets notify from power_supply...

that's correct; when using power_supply HAL reacts only on explicit KOBJ_{ADD,REMOVE,CHANGE} events. I do not have charge_now at all so I cannot test.
Comment 22 Alexey Starikovskiy 2007-12-19 15:22:01 UTC
Created attachment 14131 [details]
Make sysfs interface to ACPI power devices optional

Please check if this patch (and disabling ACPI_SYSFS_POWER) helps...
Comment 23 Fabio Comolli 2007-12-20 13:07:10 UTC
Hi. I also experience this problem, although with gnome-power-manager instead of kpowersave

I just applied this last patch to 2.6.24-rc5-gda8cadb3 and it partially helps.

I don't have the two batteries anymore, only one is detected. The problem is that now /proc/acpi/battery/BAT0/info looks like this:

present:                 yes
design capacity:         0 mWh
last full capacity:      0 mWh
battery technology:      non-rechargeable
design voltage:          0 mV
design capacity warning: 0 mWh
design capacity low:     0 mWh
capacity granularity 1:  0 mWh
capacity granularity 2:  0 mWh
model number:            
serial number:           
battery type:            
OEM info: 

whilst /proc/acpi/battery/BAT0/state looks fine.

/sys/class/power_supply is now empty (as expected, I think)

Please let me know if you need more details.
Comment 24 Rolf Eike Beer 2007-12-20 13:27:56 UTC
Same here. This is probably just HAL getting confused by what the kernel prints out.
Comment 25 Rolf Eike Beer 2007-12-20 13:29:08 UTC
Oh, by the way, the "state" file is just fine:

# cat /proc/acpi/battery/BAT1/state
present:                 yes
capacity state:          ok
charging state:          discharging
present rate:            0 mW
remaining capacity:      3526 mWh
present voltage:         11100 mV
Comment 26 Alexey Starikovskiy 2007-12-20 14:37:03 UTC
Created attachment 14137 [details]
Make sysfs interface to ACPI power devices optional #2

Please check this patch
Comment 27 Rolf Eike Beer 2007-12-20 14:58:34 UTC
Yes, that one helps.
Comment 28 Fabio Comolli 2007-12-21 12:31:09 UTC
Confirmed. This patch fixes the issue for me too. Many thanks.
Comment 29 Fabio Comolli 2007-12-24 03:18:41 UTC
Is this going to be pushed for .24-final?
Comment 30 Alexey Starikovskiy 2007-12-24 05:36:35 UTC
Hope so.
Adding Len to be sure.

bugme-daemon@bugzilla.kernel.org wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=9494
> ------- Comment #29 from fabio.comolli@gmail.com  2007-12-24 03:18 -------
> Is this going to be pushed for .24-final?
Comment 31 Len Brown 2008-01-01 11:31:41 UTC
patch in comment #26 added to acpi test branch

it is unfortunate to require yet another kernel build option
it isn't clear from the Kconfig test when to say Y and when N
(has the "Future" arrived yet?)
it is also unfortunate to need all those ifdefs in the driver --
will we ever be able to get rid of them?
Comment 32 Alexey Starikovskiy 2008-01-01 13:04:39 UTC
With ACPI_PROCFS being deprecated we could remove all these ifdefs with all other ACPI procfs entries.
Comment 33 Len Brown 2008-01-13 23:11:26 UTC
patch in comment #26
shipped in linux-2.6.24-rc7-git5

Comment 34 Andrey Borzenkov 2008-01-16 10:18:49 UTC
It is not clear how it fixes this regression - whoever updates to 2.6.24 still gets both old and new interfaces.

Is it possible to express XOR in Kconfig? Either PROCFS_POWER or SYSFS_POWER?
Comment 35 Alexey Starikovskiy 2008-01-16 10:28:58 UTC
Having battery exported by both interfaces does not brake every setup, so there is no need to have them exclude each other.