ACPI: Always return valid 'status' from acpi_battery_get_property()

ACPI: Always return valid 'status' from acpi_battery_get_property()

[Roland Dreier]

If a battery is at a critical charge level and not being charged or
discharged, then the ACPI _BST method will return a state of 4, and
the current acpi_battery_get_property() code will not set any property
value for POWER_SUPPLY_PROP_STATUS.  This will cause an oops in
power_supply_show_property() when it reads off the end of the
status_text array.  This actually was causing a 100% reproducible
crash on boot on my laptop with two batteries, when one battery was
completely drained and the laptop was not plugged in.

Fix this by making sure acpi_battery_get_property() returns
POWER_SUPPLY_STATUS_UNKNOWN for any battery state it doesn't already
handle explicitly.  There doesn't seem to be any status enum value
defined that makes more sense than 'unknown' for a battery at a
critical charge level.

Signed-off-by: Roland Dreier <roland@digitalvampire.org>
---
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index c2ce0ad..cbb27b4 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -152,6 +152,8 @@ static int acpi_battery_get_property(struct power_supply *psy,
 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
 		else if (battery->state == 0)
 			val->intval = POWER_SUPPLY_STATUS_FULL;
+		else
+			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
 		break;
 	case POWER_SUPPLY_PROP_PRESENT:
 		val->intval = acpi_battery_present(battery);

Re: ACPI: Always return valid 'status' from acpi_battery_get_property()

[Alexey Starikovskiy]

Thanks,
Alex.

Roland Dreier wrote:
> If a battery is at a critical charge level and not being charged or
> discharged, then the ACPI _BST method will return a state of 4, and
> the current acpi_battery_get_property() code will not set any property
> value for POWER_SUPPLY_PROP_STATUS.  This will cause an oops in
> power_supply_show_property() when it reads off the end of the
> status_text array.  This actually was causing a 100% reproducible
> crash on boot on my laptop with two batteries, when one battery was
> completely drained and the laptop was not plugged in.
> 
> Fix this by making sure acpi_battery_get_property() returns
> POWER_SUPPLY_STATUS_UNKNOWN for any battery state it doesn't already
> handle explicitly.  There doesn't seem to be any status enum value
> defined that makes more sense than 'unknown' for a battery at a
> critical charge level.
> 
> Signed-off-by: Roland Dreier <roland@digitalvampire.org>
Acked-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index c2ce0ad..cbb27b4 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -152,6 +152,8 @@ static int acpi_battery_get_property(struct power_supply *psy,
>  			val->intval = POWER_SUPPLY_STATUS_CHARGING;
>  		else if (battery->state == 0)
>  			val->intval = POWER_SUPPLY_STATUS_FULL;
> +		else
> +			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
>  		break;
>  	case POWER_SUPPLY_PROP_PRESENT:
>  		val->intval = acpi_battery_present(battery);

Re: ACPI: Always return valid 'status' from acpi_battery_get_property()

[Henrique de Moraes Holschuh]

On Thu, 08 Nov 2007, Alexey Starikovskiy wrote:
>> handle explicitly.  There doesn't seem to be any status enum value
>> defined that makes more sense than 'unknown' for a battery at a
>> critical charge level.

Maybe one should be added?

-- 
  "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

Re: ACPI: Always return valid 'status' from acpi_battery_get_property()

[Alexey Starikovskiy]

[-- Attachment #1: Type: text/plain, Size: 476 bytes --]

Henrique de Moraes Holschuh wrote:
> On Thu, 08 Nov 2007, Alexey Starikovskiy wrote:
>   
>>> handle explicitly.  There doesn't seem to be any status enum value
>>> defined that makes more sense than 'unknown' for a battery at a
>>> critical charge level.
>>>       
>
> Maybe one should be added?
>
>   
There is already "Critical" field for capacity.
It seems that the state should be limited to only 3 options:
    charging, discharging, and not charging.

Regards,
Alex.


[-- Attachment #2: always_return_valid_status.patch --]
[-- Type: text/x-patch, Size: 860 bytes --]

ACPI: Battery: Always return valid 'status' from get_property

From: Alexey Starikovskiy <astarikovskiy@suse.de>

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/battery.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 192c244..064d80b 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -151,8 +151,8 @@ static int acpi_battery_get_property(struct power_supply *psy,
 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
 		else if (battery->state & 0x02)
 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
-		else if (battery->state == 0)
-			val->intval = POWER_SUPPLY_STATUS_FULL;
+		else
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
 		break;
 	case POWER_SUPPLY_PROP_PRESENT:
 		val->intval = acpi_battery_present(battery);

Re: ACPI: Always return valid 'status' from acpi_battery_get_property()

[Henrique de Moraes Holschuh]

On Mon, 19 Nov 2007, Alexey Starikovskiy wrote:
> Henrique de Moraes Holschuh wrote:
>> On Thu, 08 Nov 2007, Alexey Starikovskiy wrote:
>>>> handle explicitly.  There doesn't seem to be any status enum value
>>>> defined that makes more sense than 'unknown' for a battery at a
>>>> critical charge level.
>>
>> Maybe one should be added?
>>   
> There is already "Critical" field for capacity.

Ah, that's ok, then.

> It seems that the state should be limited to only 3 options:
>    charging, discharging, and not charging.

I'd call "not charging", "idle" instead.  After all, discharging IS not
charging as well...

-- 
  "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

Re: ACPI: Always return valid 'status' from acpi_battery_get_property()

[Alexey Starikovskiy]

Henrique de Moraes Holschuh wrote:
> On Mon, 19 Nov 2007, Alexey Starikovskiy wrote:
>> Henrique de Moraes Holschuh wrote:
>>> On Thu, 08 Nov 2007, Alexey Starikovskiy wrote:
>>>>> handle explicitly.  There doesn't seem to be any status enum value
>>>>> defined that makes more sense than 'unknown' for a battery at a
>>>>> critical charge level.
>>> Maybe one should be added?
>>>   
>> There is already "Critical" field for capacity.
> 
> Ah, that's ok, then.
> 
>> It seems that the state should be limited to only 3 options:
>>    charging, discharging, and not charging.
> 
> I'd call "not charging", "idle" instead.  After all, discharging IS not
> charging as well...
> 
There is power_supply interface, and authors already declared this "not charging" 
state, also they already failed to invent "idle".
So, you might send them patch, renaming "not charging" to "idle"...

Regards,
Alex.