Message ID | 20200604195658.66201-1-mathewk@chromium.org (mailing list archive) |
---|---|
State | Under Review |
Headers | show |
Series | acpi: battery: Always read fresh battery state on update | expand |
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; > --
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; > > --
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;
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(+)