linux-kernel - Re: [PATCH] battery: Fix charge_now returned by broken batteries

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 4 Oct 2009 23:42:51 +0200
From:	Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:	Alexey Starikovskiy <astarikovskiy@...e.de>
Cc:	Henrique de Moraes Holschuh <hmh@....eng.br>,
	linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] battery: Fix charge_now returned by broken batteries

On Sun, Oct 4, 2009 at 8:57 PM, Alexey Starikovskiy
<astarikovskiy@...e.de> wrote:
> Hi Miguel,
>
> I am going to reject your patch on the basis, that the battery driver should
> report only
> information it gained from battery hardware, not interpret it in any way.
> As your patch fall into "interpret" category, it does not belong in the
> kernel and battery
> driver in particular. You may suggest it to any/all user space battery
> monitoring applications,
> this is the place for "interpretations".

I understand your point. However, there are other parts in the same
file that need to do "interpretation", for example:

http://lxr.linux.no/linux+*/drivers/acpi/battery.c#L157

 157        /* fallback to using design values for broken batteries */
 158        if (battery->design_capacity == battery->capacity_now)
 159                return 1;

I agree with Rafael, the kernel must interpret the hardware as best as
possible (it is its job, isn't it?). In countless places the kernel
has to "fix" (workaround) hardware bugs in order to present userspace
a unified interface, correct values, etc.

In this case my battery is reporting a obvious wrong value that causes
(apparently) correct userspace applications to misbehave.

Maybe the patch I proposed is not the correct solution; however, the
bug remains.

>
> Not-acknowledged-by: Alexey Starikovskiy
>
>
> Regards,
> Alex.
>
>
> Miguel Ojeda пишет:
>>
>> On Sun, Oct 4, 2009 at 6:45 PM, Henrique de Moraes Holschuh
>> <hmh@....eng.br> wrote:
>>>
>>> On Sun, 04 Oct 2009, Miguel Ojeda wrote:
>>>>
>>>> Some broken batteries like my DELL NR2227 or a friend's DELL GK4798
>>>> return
>>>> the design_capacity (charge_full_design) as capacity_now (charge_now)
>>>> when completely charged.
>>>>
>>>> I noticed this when looking at a battery plugin that reported "127%
>>>> charged".
>>>> Some of these plugins have already "fixed" this in userspace by coding
>>>> something like min(percentage, 100)).
>>>
>>> A battery can be charged above 100%.  It just depends what you call 100%,
>>> and the "I am full" level *varies* in a non-monotonic way during the
>>> battery
>>> lifetime...
>>>
>>> So, if you don't want to see > 100%, you have to clamp it to 100% and
>>> lose
>>> information (when your "100%" level is actually increasing as the thing
>>> keeps charging and you keep raising the baseline so that it doesn't go
>>> over
>>> 100%).
>>
>> If the 100% level increased, then full_charge_capacity (a.k.a. "_last_
>> full capacity" as seen in /proc) will increase as well, won't it? If
>> the battery went over that 100% that means there is a "new" 100%, why
>> are we losing information?.
>>
>> I am asking, I am not an expert on battery stuff.
>>
>>>> So I discovered that the battery wrongly returns charge_full_design when
>>>> completely charged instead of charge_full.
>>>
>>> Ick.
>>>
>>>> This patch fixes this by returning min(capacity_now,
>>>> full_charge_capacity)
>>>> on both procfs and sysfs.
>>>
>>> What will it cause on non-broken batteries?  Or during gauge reset, when
>>> any
>>> battery that updates full_charge_capacity only at the end of the cycle
>>> will
>>> really have capacity_now > full_charge_capacity ?
>>
>> Well, does it make sense to have capacity_now higher than
>> full_charge_capacity? Wouldn't that information be broken too?
>>
>> Again, I am just wondering.
>>
>>>> Now the userspace plugins report the correct 100% and their userspace
>>>> check
>>>> may not be needed (if this error is the only one producing >100%
>>>> results).
>>>
>>> Like I said, > 100% can happen, unless what you define to be 100% is very
>>> elastic (and gets updated all the time).
>>
>> I still think it does not make sense to have a battery charged over
>> its 100% capacity whatever the definition of 100% is. Maybe I do not
>> understand your point.
>>
>>> --
>>>  "One disk to rule them all, One disk to find them. One disk to bring
>>>  them all and in the darkness grind them. In the Land of Redmond
>>>  where the shadows lie." -- The Silicon Valley Tarot
>>>  Henrique Holschuh
>>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/