[PATCH] acpi: battery: Always read fresh battery state on update

[PATCH] acpi: battery: Always read fresh battery state on update

[Mathew King]

When the ACPI battery receives a notification event it should always
read the battery state fresh from the ACPI device and not use the cached
state. Currently the cached state stays valid and the new state may not
be read when a notification occurs. This can lead to a udev event
showing that the battery has changed but the sysfs state will still have
the cached state values. This change invalidates the update time forcing
the state to be updated before notifying the power_supply subsystem of
the change.

Signed-off-by: Mathew King <mathewk@chromium.org>
---
 drivers/acpi/battery.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 366c389175d8..ab7fa4879fbe 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -981,6 +981,7 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
 		acpi_battery_init_alarm(battery);
 	}
 
+	battery->update_time = 0;
 	result = acpi_battery_get_state(battery);
 	if (result)
 		return result;
-- 
2.27.0.rc2.251.g90737beb825-goog

Re: [PATCH] acpi: battery: Always read fresh battery state on update

[Rafael J. Wysocki]

On Thu, Jun 4, 2020 at 9:57 PM Mathew King <mathewk@chromium.org> wrote:
>
> When the ACPI battery receives a notification event it should always
> read the battery state fresh from the ACPI device and not use the cached
> state.

Why should it?

> Currently the cached state stays valid and the new state may not
> be read when a notification occurs. This can lead to a udev event
> showing that the battery has changed but the sysfs state will still have
> the cached state values.

Is there a bug entry or similar related to that which can be referred
to from this patch?

> This change invalidates the update time forcing
> the state to be updated before notifying the power_supply subsystem of
> the change.
>
> Signed-off-by: Mathew King <mathewk@chromium.org>
> ---
>  drivers/acpi/battery.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 366c389175d8..ab7fa4879fbe 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -981,6 +981,7 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
>                 acpi_battery_init_alarm(battery);
>         }
>
> +       battery->update_time = 0;

AFAICS this is equivalent to dropping battery->update_time altogether.
Isn't that a bit too excessive?

>         result = acpi_battery_get_state(battery);
>         if (result)
>                 return result;
> --

Re: [PATCH] acpi: battery: Always read fresh battery state on update

[Mat King]

On Fri, Jun 5, 2020 at 5:30 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Jun 4, 2020 at 9:57 PM Mathew King <mathewk@chromium.org> wrote:
> >
> > When the ACPI battery receives a notification event it should always
> > read the battery state fresh from the ACPI device and not use the cached
> > state.
>
> Why should it?

According to the ACPI Spec 10.2.1 Battery Events, "When the present
state of the battery has changed or when the trip point set by the
_BTP control method is reached or crossed, the hardware will assert a
general purpose event." So when this event is received we should
assume that the cached state of the battery is no longer valid

>
> > Currently the cached state stays valid and the new state may not
> > be read when a notification occurs. This can lead to a udev event
> > showing that the battery has changed but the sysfs state will still have
> > the cached state values.
>
> Is there a bug entry or similar related to that which can be referred
> to from this patch?

No, I discovered this issue while working on an internal issue where
it was observed that udev events generated when a battery changed did
not accurately reflect the state of the battery. I initially suspected
that the EC may not be updating its state before generating the ACPI
event, however after much debugging I discovered that the battery
driver was caching the state and the state is not always immediately
updated when the event is received. If there is a more formal process
to discuss the issue I will work through that process.

>
> > This change invalidates the update time forcing
> > the state to be updated before notifying the power_supply subsystem of
> > the change.
> >
> > Signed-off-by: Mathew King <mathewk@chromium.org>
> > ---
> >  drivers/acpi/battery.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> > index 366c389175d8..ab7fa4879fbe 100644
> > --- a/drivers/acpi/battery.c
> > +++ b/drivers/acpi/battery.c
> > @@ -981,6 +981,7 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
> >                 acpi_battery_init_alarm(battery);
> >         }
> >
> > +       battery->update_time = 0;
>
> AFAICS this is equivalent to dropping battery->update_time altogether.
> Isn't that a bit too excessive?

It is not the same as dropping the update_time. The cached state is
still used when acpi_battery_get_property() is called which happens
anytime userspace accesses the sysfs properties it is also what is
called by the power_supply subsystem when creating the environment for
the udev events. In those cases the cache still works and makes sense.
The acpi_battery_update() function is only called in a handful of
cases and in all of these cases reading the battery state fresh makes
sense to me. Those cases are:

1. When the battery is added with acpi_battery_add(), this case the
update_time is already cleared
2. On system resume with acpi_battery_resume(), in this case
update_time is cleared before calling acpi_battery_update() so that
static battery info is also updated by calling acpi_battery_get_info()
3. The acpi_battery_update() is called from procfs power functions
which should not be called a frequency where reading fresh battery
state from ACPI will have a performance impact
4. Finally it is called from acpi_battery_notify() when a battery
event is received from firmware that the state has changed

I considered clearing the update_time in acpi_battery_notify() before
acpi_battery_update() is called but if I did that by itself then
acpi_battery_get_info() would also get called and I wasn't sure that
behavior would be wanted. So invalidating the cache where I did seemed
to be the least disruptive way to fix the problem. I can see
opportunities to refactor this driver and I felt that this fix was
acceptable until a refactor could be done.

>
> >         result = acpi_battery_get_state(battery);
> >         if (result)
> >                 return result;
> > --