[SRU] [Bionic] [PATCH 0/5] Gracefully handle ACPI battery status on Asus laptops

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU] [Bionic] [PATCH 0/5] Gracefully handle ACPI battery status on Asus laptops

Kai-Heng Feng
BugLink: https://bugs.launchpad.net/bugs/1745032

[Impact]
My original fix for LP: #1745032 got placed by a more general solution,
so replace the old one with the new one.

1) The DMI based solution got reverted, so we won't be able to SRU any
new affected models from upstream.
2) Many Asus laptops are affected by this bug, DMI quirk table will be really
unmaintainable.

[Test]
Along with fixes in UPower, Gnome Settings Daemon and Gnome Shell, the
user confirm the kernel fix works.

[Fix]
Reports the power status as POWER_SUPPLY_STATUS_NOT_CHARGING, the
userspace will handle the rest.

[Regression Potential]
Low. It covers a corner case when "power_supply_is_system_supplied()
&& battery->rate_now == 0", other cases are unaffected.

Daniel Drake (1):
  Revert "ACPI / battery: Add quirk for Asus GL502VSK and UX305LA"

Hans de Goede (4):
  ACPI / AC: Remove initializer for unused ident dmi_system_id
  ACPI / battery: Remove initializer for unused ident dmi_system_id
  ACPI / battery: Add handling for devices which wrongly report
    discharging state
  ACPI / battery: Ignore AC state in handle_discharging on systems where
    it is broken

 drivers/acpi/ac.c      |  2 +-
 drivers/acpi/battery.c | 69 ++++++++++++++++++------------------------
 2 files changed, 31 insertions(+), 40 deletions(-)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/5] Revert "ACPI / battery: Add quirk for Asus GL502VSK and UX305LA"

Kai-Heng Feng
From: Daniel Drake <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1745032

Revert 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, and the quirk
approach taken is inadequate and more thought is needed first.
Specifically:

 1. It only quirks discharging state, not 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.

 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).

    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.

    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.

 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.

Revert this quirk before it gets included in a release, while we look
for better approaches.

Signed-off-by: Daniel Drake <[hidden email]>
Acked-by: Kai-Heng Feng <[hidden email]>
Signed-off-by: Rafael J. Wysocki <[hidden email]>
(cherry picked from commit 82bf43b291888599b4079244d12195d214086fa4)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/acpi/battery.c | 48 +++---------------------------------------
 1 file changed, 3 insertions(+), 45 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index a4fd1d8b4fe9..13e7b56e33ae 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -70,7 +70,6 @@ static async_cookie_t async_cookie;
 static bool battery_driver_registered;
 static int battery_bix_broken_package;
 static int battery_notification_delay_ms;
-static int battery_full_discharging;
 static unsigned int cache_time = 1000;
 module_param(cache_time, uint, 0644);
 MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
@@ -215,12 +214,9 @@ static int acpi_battery_get_property(struct power_supply *psy,
  return -ENODEV;
  switch (psp) {
  case POWER_SUPPLY_PROP_STATUS:
- if (battery->state & ACPI_BATTERY_STATE_DISCHARGING) {
- if (battery_full_discharging && battery->rate_now == 0)
- val->intval = POWER_SUPPLY_STATUS_FULL;
- else
- val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
- } else if (battery->state & ACPI_BATTERY_STATE_CHARGING)
+ if (battery->state & ACPI_BATTERY_STATE_DISCHARGING)
+ val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+ else if (battery->state & ACPI_BATTERY_STATE_CHARGING)
  val->intval = POWER_SUPPLY_STATUS_CHARGING;
  else if (acpi_battery_is_charged(battery))
  val->intval = POWER_SUPPLY_STATUS_FULL;
@@ -1170,12 +1166,6 @@ battery_notification_delay_quirk(const struct dmi_system_id *d)
  return 0;
 }
 
-static int __init battery_full_discharging_quirk(const struct dmi_system_id *d)
-{
- battery_full_discharging = 1;
- return 0;
-}
-
 static const struct dmi_system_id bat_dmi_table[] __initconst = {
  {
  .callback = battery_bix_broken_package_quirk,
@@ -1193,38 +1183,6 @@ static const struct dmi_system_id bat_dmi_table[] __initconst = {
  DMI_MATCH(DMI_PRODUCT_NAME, "Aspire V5-573G"),
  },
  },
- {
- .callback = battery_full_discharging_quirk,
- .ident = "ASUS GL502VSK",
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
- DMI_MATCH(DMI_PRODUCT_NAME, "GL502VSK"),
- },
- },
- {
- .callback = battery_full_discharging_quirk,
- .ident = "ASUS UX305LA",
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
- DMI_MATCH(DMI_PRODUCT_NAME, "UX305LA"),
- },
- },
- {
- .callback = battery_full_discharging_quirk,
- .ident = "ASUS UX360UA",
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
- DMI_MATCH(DMI_PRODUCT_NAME, "UX360UA"),
- },
- },
- {
- .callback = battery_full_discharging_quirk,
- .ident = "ASUS UX410UAK",
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
- DMI_MATCH(DMI_PRODUCT_NAME, "UX410UAK"),
- },
- },
  {},
 };
 
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/5] ACPI / AC: Remove initializer for unused ident dmi_system_id

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Hans de Goede <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1745032

The ac.c code does not use the dmi_system_id ident member, so there is
no need to initialize it. This saves us storing the unused "thinkpad e530"
string as const data.

Signed-off-by: Hans de Goede <[hidden email]>
Signed-off-by: Rafael J. Wysocki <[hidden email]>
(cherry picked from commit 6605e3423f37ba4d24771a65b850d8a900830610)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/acpi/ac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index 47a7ed557bd6..1b60a6db8e53 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -318,8 +318,8 @@ static int thinkpad_e530_quirk(const struct dmi_system_id *d)
 
 static const struct dmi_system_id ac_dmi_table[] = {
  {
+ /* Thinkpad e530 */
  .callback = thinkpad_e530_quirk,
- .ident = "thinkpad e530",
  .matches = {
  DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
  DMI_MATCH(DMI_PRODUCT_NAME, "32597CG"),
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/5] ACPI / battery: Remove initializer for unused ident dmi_system_id

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Hans de Goede <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1745032

The battery code does not use the dmi_system_id ident member, so there is
no need to initialize it. This saves us storing the unused strings as
as const data.

Signed-off-by: Hans de Goede <[hidden email]>
Signed-off-by: Rafael J. Wysocki <[hidden email]>
(cherry picked from commit 91afa07664a8d26f51fb59b13fd5fa3592b728bc)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/acpi/battery.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 13e7b56e33ae..6fe6b2e37334 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -1168,16 +1168,16 @@ battery_notification_delay_quirk(const struct dmi_system_id *d)
 
 static const struct dmi_system_id bat_dmi_table[] __initconst = {
  {
+ /* NEC LZ750/LS */
  .callback = battery_bix_broken_package_quirk,
- .ident = "NEC LZ750/LS",
  .matches = {
  DMI_MATCH(DMI_SYS_VENDOR, "NEC"),
  DMI_MATCH(DMI_PRODUCT_NAME, "PC-LZ750LS"),
  },
  },
  {
+ /* Acer Aspire V5-573G */
  .callback = battery_notification_delay_quirk,
- .ident = "Acer Aspire V5-573G",
  .matches = {
  DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
  DMI_MATCH(DMI_PRODUCT_NAME, "Aspire V5-573G"),
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 4/5] ACPI / battery: Add handling for devices which wrongly report discharging state

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Hans de Goede <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1745032

On quite a few devices 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.

This causes the wrong status to be reported for the battery in the
following quite common scenario:

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 wrongly translate this to "discharging" and ends
up setting the ACPI_BATTERY_STATE_DISCHARGING bit in its state field.

Reporting this "not charging" state as discharging is confusing for users,
making the user think his adapter/power-brick is broken or not properly
plugged in.

This commit adds a helper for handling the ACPI_BATTERY_STATE_DISCHARGING
state. This helper checks if we're an AC and the current going out of the
battery is 0 and in that case reports a status of "not charging" to
userspace rather then "discharging".

This replaces commit c68f0676ef7d ("ACPI / battery: Add quirk for Asus
GL502VSK and UX305LA"), a previous fix for this which was reverted.

Signed-off-by: Hans de Goede <[hidden email]>
Reviewed-by: Daniel Drake <[hidden email]>
Reviewed-by: Sebastian Reichel <[hidden email]>
Signed-off-by: Rafael J. Wysocki <[hidden email]>
(backported from commit 19fffc8450d4378580a8f019b195c4617083176f)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/acpi/battery.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 6fe6b2e37334..5b4e0a49ddc4 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -200,6 +200,19 @@ static int acpi_battery_is_charged(struct acpi_battery *battery)
  return 0;
 }
 
+static int acpi_battery_handle_discharging(struct acpi_battery *battery)
+{
+ /*
+ * 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)
+ return POWER_SUPPLY_STATUS_NOT_CHARGING;
+
+ return POWER_SUPPLY_STATUS_DISCHARGING;
+}
+
 static int acpi_battery_get_property(struct power_supply *psy,
      enum power_supply_property psp,
      union power_supply_propval *val)
@@ -215,7 +228,7 @@ static int acpi_battery_get_property(struct power_supply *psy,
  switch (psp) {
  case POWER_SUPPLY_PROP_STATUS:
  if (battery->state & ACPI_BATTERY_STATE_DISCHARGING)
- val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+ val->intval = acpi_battery_handle_discharging(battery);
  else if (battery->state & ACPI_BATTERY_STATE_CHARGING)
  val->intval = POWER_SUPPLY_STATUS_CHARGING;
  else if (acpi_battery_is_charged(battery))
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 5/5] ACPI / battery: Ignore AC state in handle_discharging on systems where it is broken

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Hans de Goede <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1745032

On some devices the "AC" interface ACPI AML code uses the exact same broken
logic which is causing the battery code to wrongly report discharging to
determine the "AC" state. Specifically the ACPI AML code is checking the
charging status bits of the charger-IC rather then the vbus present or
power-good status bits.

This makes our workaround for devices which wrongly report discharging when
plugged into AC while the charge is above the start charging threshold not
work on these devices.

This commit adds a battery_ac_is_broken flag and when that is set it skips
the power_supply_is_system_supplied() check in the workaround fixing this.

This flag gets set by a DMI quirk selected by systems where we know the AC
AML code is broken in this way *and* the rate_now value can be trusted.

Signed-off-by: Hans de Goede <[hidden email]>
Signed-off-by: Rafael J. Wysocki <[hidden email]>
(cherry picked from commit 1b799c5cf031c2b615f4b21150eafde3ff227788)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/acpi/battery.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 5b4e0a49ddc4..f6ac6cc8c905 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -70,6 +70,7 @@ static async_cookie_t async_cookie;
 static bool battery_driver_registered;
 static int battery_bix_broken_package;
 static int battery_notification_delay_ms;
+static int battery_ac_is_broken;
 static unsigned int cache_time = 1000;
 module_param(cache_time, uint, 0644);
 MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
@@ -207,7 +208,8 @@ static int acpi_battery_handle_discharging(struct acpi_battery *battery)
  * 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)
+ if ((battery_ac_is_broken || power_supply_is_system_supplied()) &&
+    battery->rate_now == 0)
  return POWER_SUPPLY_STATUS_NOT_CHARGING;
 
  return POWER_SUPPLY_STATUS_DISCHARGING;
@@ -1179,6 +1181,13 @@ battery_notification_delay_quirk(const struct dmi_system_id *d)
  return 0;
 }
 
+static int __init
+battery_ac_is_broken_quirk(const struct dmi_system_id *d)
+{
+ battery_ac_is_broken = 1;
+ return 0;
+}
+
 static const struct dmi_system_id bat_dmi_table[] __initconst = {
  {
  /* NEC LZ750/LS */
@@ -1196,6 +1205,17 @@ static const struct dmi_system_id bat_dmi_table[] __initconst = {
  DMI_MATCH(DMI_PRODUCT_NAME, "Aspire V5-573G"),
  },
  },
+ {
+ /* Point of View mobii wintab p800w */
+ .callback = battery_ac_is_broken_quirk,
+ .matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "AMI Corporation"),
+ DMI_MATCH(DMI_BOARD_NAME, "Aptio CRB"),
+ DMI_MATCH(DMI_BIOS_VERSION, "3BAIR1013"),
+ /* Above matches are too generic, add bios-date match */
+ DMI_MATCH(DMI_BIOS_DATE, "08/22/2014"),
+ },
+ },
  {},
 };
 
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK/Cmnt: [SRU] [Bionic] [PATCH 0/5] Gracefully handle ACPI battery status on Asus laptops

Stefan Bader-2
In reply to this post by Kai-Heng Feng
On 02.02.19 16:37, Kai-Heng Feng wrote:

> BugLink: https://bugs.launchpad.net/bugs/1745032
>
> [Impact]
> My original fix for LP: #1745032 got placed by a more general solution,
> so replace the old one with the new one.
>
> 1) The DMI based solution got reverted, so we won't be able to SRU any
> new affected models from upstream.
> 2) Many Asus laptops are affected by this bug, DMI quirk table will be really
> unmaintainable.
I believe you say it with different words here but it might help to clarify
things if you mentioned that this set drops the DMI quirks of those laptops and
that is replaced by generic code which does not need quirks.
Additionally it adds a quirk for the AC status (makes me wonder whether that is
not as painful to keep up-to-date as the battery quirk), maybe to get back the
same coverage as before.

About the risk: past experience showed that any modification within ACPI space
might have some odd side effects but at least reporting the wrong charging state
is not as critical as say the backlight.

Acked-by: Stefan Bader <[hidden email]>

>
> [Test]
> Along with fixes in UPower, Gnome Settings Daemon and Gnome Shell, the
> user confirm the kernel fix works.
>
> [Fix]
> Reports the power status as POWER_SUPPLY_STATUS_NOT_CHARGING, the
> userspace will handle the rest.
>
> [Regression Potential]
> Low. It covers a corner case when "power_supply_is_system_supplied()
> && battery->rate_now == 0", other cases are unaffected.
>
> Daniel Drake (1):
>   Revert "ACPI / battery: Add quirk for Asus GL502VSK and UX305LA"
>
> Hans de Goede (4):
>   ACPI / AC: Remove initializer for unused ident dmi_system_id
>   ACPI / battery: Remove initializer for unused ident dmi_system_id
>   ACPI / battery: Add handling for devices which wrongly report
>     discharging state
>   ACPI / battery: Ignore AC state in handle_discharging on systems where
>     it is broken
>
>  drivers/acpi/ac.c      |  2 +-
>  drivers/acpi/battery.c | 69 ++++++++++++++++++------------------------
>  2 files changed, 31 insertions(+), 40 deletions(-)
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment