On Fri, Jun 5, 2020 at 5:30 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote: > > On Thu, Jun 4, 2020 at 9:57 PM Mathew King <mathewk@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > --- > > 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; > > --
