Hi Daniel,
I was actually somewhat happy to see the original patch which this
reverts as I'm seeing similar problems on various devices.
With that said I agree with you that a dmi based quirk is not
the solution because that will turn in a game of whack-a-mole.
On 14-03-18 09:42, Daniel Drake wrote:
This reverts commit c68f0676ef7d ("ACPI / battery: Add quirk for Asus
GL502VSK and UX305LA") and commit 4446823e2573 ("ACPI / battery: Add quirk
for Asus UX360UA and UX410UAK").
On many many Asus products, the battery is sometimes reported as charging
or discharging even when it is full and you are on AC power. This
change quirked the kernel to avoid advertising the discharging state
when this happens on 4 laptop models, under the belief that this was
incorrect information. I presume it originates from user reports who are
confused that their battery status icon says that it is discharging.
However, the reported information is indeed correct,
That is not true, at least not for the troublesome devices which I have.
Here is what I see happening:
a) The battery code in the ACPI tables is buggy and first checks the
charging status bits of the charger-IC, and if those report not charging
it will report discharging, without looking at the presence of AC power
or at the battery dis(charge) current from the fuel-gauge.
b) The bug reproduces after doing the following:
1) Plug in charger while battery is say half full, battery starts
charging, charging state bits indicate: pre-charge or fast-charge,
ACPI reported battery status is ok
2) When fully charged charging state bits indicate: end-of-charge,
ACPI reported battery status is ok
3) unplug the charger, wait 1 minute, replug. Now the battery voltage
is still above the start-charging threshold, so the charger will not
start charging to avoid wrecking the battery by repeatedly recharging
the last 1% capacity. The charger IC charging state bits now are all 0
("not charging") and the broken ACPI code translate this to "discharging".
So we end up in a state where the ACPI code reports "discharging" where
it should be reporting "not charging", which is a huge difference and
indeed confuses users. TL;DR: the reported information is actually
incorrect. Note I'm seeing this on various Asus models: T100TA, T100CHI
(same generation) and on the T100HA (newer generation) too.
Note the reverted patch checked for current_now == 0, so it would
only report FULL if now current was being drawn from the battery, which
is what the ACPI code really should be doing (combined with checking for
AC presence).
and the quirk approach
taken is inadequate and more thought is needed first. Specifically:
1. It only quirks discharging state, not charging
See above, the problem is falsely reporting discharging, not
falsely reporting charging.
2. There are so many different Asus products and DMI naming variants
within those product families that behave this way; Linux could grow to
quirk hundreds of products and still not even be close at "winning"
this battle.
I agree we need a generic solution rather then relying on quirks.
3. Asus previously clarified that this behaviour is intentional. The
platform will periodically do a partial discharge/charge cycle when
the battery is full, because this is one way to extend the lifetime of
the battery (leaving a battery at 100% charge and unused will decrease
its usable capacity over time).
Pardon my French: but this is bullshit, the behavior I'm describing can be
observed every single charge cycle. I've written multiple full-gauge and
charger-ic drivers by now and none actively discharge the battery.
What they do is they do not *start* *charging* the battery when it is above
a certain threshold, so they charge to 100%, and if you then disconnect it
only shortly, so it drops to 99% and then plug in again they do not start
a new charge cycle. That is a very different thing from actively discharging
the battery and reporting this "not charging" state as discharging to the
user will make the user think his adapter/power-brick is broken or not
properly plugged in.
My understanding is that any decent consumer product will have this
behaviour, but it appears that Asus is different in that they expose
this info through ACPI.
Any decent consumer product will have a start-charging threshold below which
the charge needs to drop before a new charge cycle is started yes. Active
discharging is typically only done by things like smart battery-chargers
where you remove the battery-cells from the device using them and directly
plug them (e.g. an AA battery) into the charger and even then you typically
need to explicitly enable this option in their menus before starting the
charge cycle.
Devices with sealed in batteries (or phones or laptops with custom battery
packs) doing active discharging is something I've never heard of.
However, the behaviour seems correct. The ACPI spec does not suggest in
that the platform should hide the truth. It lets you report that
the battery is full of charge, and discharging, and with external power
connected; and Asus does this.
Accept that as the current_now == 0 check triggering shows (otherwise the
reverted patch would not work) there is no discharging happening.
So Asus is simply not telling the entire story here and their ACPI implementation
really is buggy.
4. In terms of not confusing the user, this seems like something that
could/should be handled by userspace, which can also detect these
same (accurate) conditions in the general case.
I disagree, upower which most desktop-environments use to get battery status
already has heuristics to deal with the kernel reporting an "unknown" status.
The "unknown" status e.g. happens on the T200TA when it gets into a "not charging"
state which correctly sets neither the "discharging" nor the "charging" bits
in the battery state field when it is on AC power and the battery charge was
above the start threshold when the AC adapter got plugged in. The kernel ends
up reporting "unknown" in this case due to the ACPI battery spec not having a
"not charging" state bit. This is the behavior which I would expect on the
other Asus devices which I have too, but their ACPI code is broken.
When the kernel reports a state other then "unknown" upower trusts the kernel
and that seems like a reasonable thing to do. The kernel is there to abstract
hardware. In the past (e.g. with wireless drivers) we have always told the
userspace folks that they should not do kludges to work around device specific
quirks, but rather that they should file a bug with the kernel and get things
fixed there. This IMHO is the same.
TL;DR: the kernel reporting a status of "discharging" when the battery is not
discharging is a bug (caused by broken ACPI tables) and something which we
need to work around at the component which is interpreting those ACPI tables,
which is the kernel.
###
So with the above hopefully explaining why I believe that we do need a kernel
level fix for this, lets discuss how we can fix this, here is what I propose:
1) Add a new acpi_battery_handle_discharging_state() helper which gets
called when ACPI_BATTERY_STATE_DISCHARGING is set instead of always reporting
POWER_SUPPLY_STATUS_DISCHARGING in this case
2) In this new helper do the following:
/*
* Some devices wrongly report discharging if the battery's charge level
* was above the device's start charging threshold atm the AC adapter
* was plugged in and the device thus did not start a new charge cycle.
*/
if (power_supply_is_system_supplied() && battery->rate_now == 0) {
/*
* 20180409: Ideally we would use STATUS_NOT_CHARGING here, but
* that trips a bug in userspace, so for now we use FULL, see:
* https://bugs.freedesktop.org/attachment.cgi?id=138070
* Note that the fix for that bug translates not-charging to
* full, so it does not matter much anyways.
*/
val->intval = POWER_SUPPLY_STATUS_FULL;
} else {
val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
}
So what do you think about this as a solution?
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
