* [PATCH 2/3] ACPI / battery: Use design-cap for capacity calculations if full-cap is not available
2019-12-10 9:57 [PATCH 1/3] ACPI / battery: Deal with design or full capacity being reported as -1 Hans de Goede
@ 2019-12-10 9:57 ` Hans de Goede
2019-12-10 9:57 ` [PATCH 3/3] ACPI / battery: Deal better with neither design nor full capacity not being reported Hans de Goede
2019-12-16 10:18 ` [PATCH 1/3] ACPI / battery: Deal with design or full capacity being reported as -1 Rafael J. Wysocki
2 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2019-12-10 9:57 UTC (permalink / raw)
To: Rafael J . Wysocki, Len Brown; +Cc: Hans de Goede, linux-acpi
The ThunderSoft TS178 tablet's _BIX implementation reports design_capacity
but not full_charge_capacity.
Before this commit this would cause us to return -ENODEV for the capacity
attribute, which userspace does not like. Specifically upower does this:
if (sysfs_file_exists (native_path, "capacity")) {
percentage = sysfs_get_double (native_path, "capacity");
Where the sysfs_get_double() helper returns 0 when we return -ENODEV,
so the battery always reads 0% if we return -ENODEV.
This commit fixes this by using the design-capacity instead of the
full-charge-capacity when the full-charge-capacity is not available.
Fixes: b41901a2cf06 ("ACPI / battery: Do not export energy_full[_design] on devices without full_charge_capacity")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/acpi/battery.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 9c0d7c577cb9..6132401f27d7 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -217,7 +217,7 @@ static int acpi_battery_get_property(struct power_supply *psy,
enum power_supply_property psp,
union power_supply_propval *val)
{
- int ret = 0;
+ int full_capacity = ACPI_BATTERY_VALUE_UNKNOWN, ret = 0;
struct acpi_battery *battery = to_acpi_battery(psy);
if (acpi_battery_present(battery)) {
@@ -286,12 +286,17 @@ static int acpi_battery_get_property(struct power_supply *psy,
val->intval = battery->capacity_now * 1000;
break;
case POWER_SUPPLY_PROP_CAPACITY:
+ if (ACPI_BATTERY_CAPACITY_VALID(battery->full_charge_capacity))
+ full_capacity = battery->full_charge_capacity;
+ else if (ACPI_BATTERY_CAPACITY_VALID(battery->design_capacity))
+ full_capacity = battery->design_capacity;
+
if (battery->capacity_now == ACPI_BATTERY_VALUE_UNKNOWN ||
- !ACPI_BATTERY_CAPACITY_VALID(battery->full_charge_capacity))
+ full_capacity == ACPI_BATTERY_VALUE_UNKNOWN)
ret = -ENODEV;
else
val->intval = battery->capacity_now * 100/
- battery->full_charge_capacity;
+ full_capacity;
break;
case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
if (battery->state & ACPI_BATTERY_STATE_CRITICAL)
--
2.23.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 3/3] ACPI / battery: Deal better with neither design nor full capacity not being reported
2019-12-10 9:57 [PATCH 1/3] ACPI / battery: Deal with design or full capacity being reported as -1 Hans de Goede
2019-12-10 9:57 ` [PATCH 2/3] ACPI / battery: Use design-cap for capacity calculations if full-cap is not available Hans de Goede
@ 2019-12-10 9:57 ` Hans de Goede
2019-12-16 10:18 ` [PATCH 1/3] ACPI / battery: Deal with design or full capacity being reported as -1 Rafael J. Wysocki
2 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2019-12-10 9:57 UTC (permalink / raw)
To: Rafael J . Wysocki, Len Brown; +Cc: Hans de Goede, linux-acpi
Commit b41901a2cf06 ("ACPI / battery: Do not export energy_full[_design] on
devices without full_charge_capacity") added support for some (broken)
devices which always report 0 for both design_capacity and
full_charge_capacity.
Since the device that commit was written as a fix for is not reporting any
form of "full" capacity we cannot calculate the value for the
POWER_SUPPLY_PROP_CAPACITY, this is worked around by using an alternative
array of available properties which does not contain this property.
This is necessary because userspace (upower) treats us returning -ENODEV
as 0 and then typically will trigger an emergency shutdown because of that.
Userspace does not do this if the capacity sysfs attribute is not present
at all.
There are two potential problems with that commit:
1) It assumes that both full_charge- and design-capacity are broken at the
same time and only checks if full_charge- is broken.
2) It assumes that this only ever happens for devices which report energy
units rather then charge units.
This commit fixes both issues by only using the alternative
array of available properties if both full_charge- and design-capacity are
broken and by also adding an alternative array of available properties for
devices using mA units.
Fixes: b41901a2cf06 ("ACPI / battery: Do not export energy_full[_design] on devices without full_charge_capacity")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/acpi/battery.c | 51 ++++++++++++++++++++++++++++++++----------
1 file changed, 39 insertions(+), 12 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 6132401f27d7..254a7d98b9d4 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -342,6 +342,20 @@ static enum power_supply_property charge_battery_props[] = {
POWER_SUPPLY_PROP_SERIAL_NUMBER,
};
+static enum power_supply_property charge_battery_full_cap_broken_props[] = {
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_PRESENT,
+ POWER_SUPPLY_PROP_TECHNOLOGY,
+ POWER_SUPPLY_PROP_CYCLE_COUNT,
+ POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
+ POWER_SUPPLY_PROP_VOLTAGE_NOW,
+ POWER_SUPPLY_PROP_CURRENT_NOW,
+ POWER_SUPPLY_PROP_CHARGE_NOW,
+ POWER_SUPPLY_PROP_MODEL_NAME,
+ POWER_SUPPLY_PROP_MANUFACTURER,
+ POWER_SUPPLY_PROP_SERIAL_NUMBER,
+};
+
static enum power_supply_property energy_battery_props[] = {
POWER_SUPPLY_PROP_STATUS,
POWER_SUPPLY_PROP_PRESENT,
@@ -803,21 +817,34 @@ static void __exit battery_hook_exit(void)
static int sysfs_add_battery(struct acpi_battery *battery)
{
struct power_supply_config psy_cfg = { .drv_data = battery, };
+ bool full_cap_broken = false;
+
+ if (!ACPI_BATTERY_CAPACITY_VALID(battery->full_charge_capacity) &&
+ !ACPI_BATTERY_CAPACITY_VALID(battery->design_capacity))
+ full_cap_broken = true;
if (battery->power_unit == ACPI_BATTERY_POWER_UNIT_MA) {
- battery->bat_desc.properties = charge_battery_props;
- battery->bat_desc.num_properties =
- ARRAY_SIZE(charge_battery_props);
- } else if (!ACPI_BATTERY_CAPACITY_VALID(
- battery->full_charge_capacity)) {
- battery->bat_desc.properties =
- energy_battery_full_cap_broken_props;
- battery->bat_desc.num_properties =
- ARRAY_SIZE(energy_battery_full_cap_broken_props);
+ if (full_cap_broken) {
+ battery->bat_desc.properties =
+ charge_battery_full_cap_broken_props;
+ battery->bat_desc.num_properties =
+ ARRAY_SIZE(charge_battery_full_cap_broken_props);
+ } else {
+ battery->bat_desc.properties = charge_battery_props;
+ battery->bat_desc.num_properties =
+ ARRAY_SIZE(charge_battery_props);
+ }
} else {
- battery->bat_desc.properties = energy_battery_props;
- battery->bat_desc.num_properties =
- ARRAY_SIZE(energy_battery_props);
+ if (full_cap_broken) {
+ battery->bat_desc.properties =
+ energy_battery_full_cap_broken_props;
+ battery->bat_desc.num_properties =
+ ARRAY_SIZE(energy_battery_full_cap_broken_props);
+ } else {
+ battery->bat_desc.properties = energy_battery_props;
+ battery->bat_desc.num_properties =
+ ARRAY_SIZE(energy_battery_props);
+ }
}
battery->bat_desc.name = acpi_device_bid(battery->device);
--
2.23.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/3] ACPI / battery: Deal with design or full capacity being reported as -1
2019-12-10 9:57 [PATCH 1/3] ACPI / battery: Deal with design or full capacity being reported as -1 Hans de Goede
2019-12-10 9:57 ` [PATCH 2/3] ACPI / battery: Use design-cap for capacity calculations if full-cap is not available Hans de Goede
2019-12-10 9:57 ` [PATCH 3/3] ACPI / battery: Deal better with neither design nor full capacity not being reported Hans de Goede
@ 2019-12-16 10:18 ` Rafael J. Wysocki
2 siblings, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2019-12-16 10:18 UTC (permalink / raw)
To: Hans de Goede; +Cc: Rafael J . Wysocki, Len Brown, ACPI Devel Maling List
On Tue, Dec 10, 2019 at 10:57 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Commit b41901a2cf06 ("ACPI / battery: Do not export energy_full[_design] on
> devices without full_charge_capacity") added support for some (broken)
> devices which always report 0 for both design- and full_charge-capacity.
>
> This assumes that if the capacity is not being reported it is 0. The
> ThunderSoft TS178 tablet's _BIX implementation falsifies this assumption.
> It reports ACPI_BATTERY_VALUE_UNKNOWN (-1) as full_charge_capacity, which
> we treat as a valid value which causes several problems.
>
> This commit fixes this by adding a new ACPI_BATTERY_CAPACITY_VALID() helper
> which checks that the value is not 0 and not -1; and using this whenever we
> need to test if either design_capacity or full_charge_capacity is valid.
>
> Fixes: b41901a2cf06 ("ACPI / battery: Do not export energy_full[_design] on devices without full_charge_capacity")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Applying along with the [2-3/3] as 5.6 material, thanks!
> ---
> drivers/acpi/battery.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 558fedf8a7a1..9c0d7c577cb9 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -38,6 +38,8 @@
> #define PREFIX "ACPI: "
>
> #define ACPI_BATTERY_VALUE_UNKNOWN 0xFFFFFFFF
> +#define ACPI_BATTERY_CAPACITY_VALID(capacity) \
> + ((capacity) != 0 && (capacity) != ACPI_BATTERY_VALUE_UNKNOWN)
>
> #define ACPI_BATTERY_DEVICE_NAME "Battery"
>
> @@ -192,7 +194,8 @@ static int acpi_battery_is_charged(struct acpi_battery *battery)
>
> static bool acpi_battery_is_degraded(struct acpi_battery *battery)
> {
> - return battery->full_charge_capacity && battery->design_capacity &&
> + return ACPI_BATTERY_CAPACITY_VALID(battery->full_charge_capacity) &&
> + ACPI_BATTERY_CAPACITY_VALID(battery->design_capacity) &&
> battery->full_charge_capacity < battery->design_capacity;
> }
>
> @@ -263,14 +266,14 @@ static int acpi_battery_get_property(struct power_supply *psy,
> break;
> case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
> - if (battery->design_capacity == ACPI_BATTERY_VALUE_UNKNOWN)
> + if (!ACPI_BATTERY_CAPACITY_VALID(battery->design_capacity))
> ret = -ENODEV;
> else
> val->intval = battery->design_capacity * 1000;
> break;
> case POWER_SUPPLY_PROP_CHARGE_FULL:
> case POWER_SUPPLY_PROP_ENERGY_FULL:
> - if (battery->full_charge_capacity == ACPI_BATTERY_VALUE_UNKNOWN)
> + if (!ACPI_BATTERY_CAPACITY_VALID(battery->full_charge_capacity))
> ret = -ENODEV;
> else
> val->intval = battery->full_charge_capacity * 1000;
> @@ -283,11 +286,12 @@ static int acpi_battery_get_property(struct power_supply *psy,
> val->intval = battery->capacity_now * 1000;
> break;
> case POWER_SUPPLY_PROP_CAPACITY:
> - if (battery->capacity_now && battery->full_charge_capacity)
> + if (battery->capacity_now == ACPI_BATTERY_VALUE_UNKNOWN ||
> + !ACPI_BATTERY_CAPACITY_VALID(battery->full_charge_capacity))
> + ret = -ENODEV;
> + else
> val->intval = battery->capacity_now * 100/
> battery->full_charge_capacity;
> - else
> - val->intval = 0;
> break;
> case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
> if (battery->state & ACPI_BATTERY_STATE_CRITICAL)
> @@ -799,7 +803,8 @@ static int sysfs_add_battery(struct acpi_battery *battery)
> battery->bat_desc.properties = charge_battery_props;
> battery->bat_desc.num_properties =
> ARRAY_SIZE(charge_battery_props);
> - } else if (battery->full_charge_capacity == 0) {
> + } else if (!ACPI_BATTERY_CAPACITY_VALID(
> + battery->full_charge_capacity)) {
> battery->bat_desc.properties =
> energy_battery_full_cap_broken_props;
> battery->bat_desc.num_properties =
> --
> 2.23.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread