On Thu, Jan 14, 2021 at 7:35 PM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > Calling acpi_thermal_check() from acpi_thermal_notify() directly > is problematic if _TMP triggers Notify () on the thermal zone for > which it has been evaluated (which happens on some systems), because > it causes a new acpi_thermal_notify() invocation to be queued up > every time and if that takes place too often, an indefinite number of > pending work items may accumulate in kacpi_notify_wq over time. > > Besides, it is not really useful to queue up a new invocation of > acpi_thermal_check() if one of them is pending already. > > For these reasons, rework acpi_thermal_notify() to queue up a thermal > check instead of calling acpi_thermal_check() directly and only allow > one thermal check to be pending at a time. Moreover, only allow one > acpi_thermal_check_fn() instance at a time to run > thermal_zone_device_update() for one thermal zone and make it return > early if it sees other instances running for the same thermal zone. > > While at it, fold acpi_thermal_check() into acpi_thermal_check_fn(), > as it is only called from there after the other changes made here. > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208877 > Reported-by: Stephen Berman <stephen.berman@xxxxxxx> > Diagnosed-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> Well, it's been over a week since this was posted. Does anyone have any comments? > --- > drivers/acpi/thermal.c | 46 +++++++++++++++++++++++++++++++++------------- > 1 file changed, 33 insertions(+), 13 deletions(-) > > Index: linux-pm/drivers/acpi/thermal.c > =================================================================== > --- linux-pm.orig/drivers/acpi/thermal.c > +++ linux-pm/drivers/acpi/thermal.c > @@ -174,6 +174,8 @@ struct acpi_thermal { > struct thermal_zone_device *thermal_zone; > int kelvin_offset; /* in millidegrees */ > struct work_struct thermal_check_work; > + struct mutex thermal_check_lock; > + refcount_t thermal_check_count; > }; > > /* -------------------------------------------------------------------------- > @@ -495,14 +497,6 @@ static int acpi_thermal_get_trip_points( > return 0; > } > > -static void acpi_thermal_check(void *data) > -{ > - struct acpi_thermal *tz = data; > - > - thermal_zone_device_update(tz->thermal_zone, > - THERMAL_EVENT_UNSPECIFIED); > -} > - > /* sys I/F for generic thermal sysfs support */ > > static int thermal_get_temp(struct thermal_zone_device *thermal, int *temp) > @@ -900,6 +894,12 @@ static void acpi_thermal_unregister_ther > Driver Interface > -------------------------------------------------------------------------- */ > > +static void acpi_queue_thermal_check(struct acpi_thermal *tz) > +{ > + if (!work_pending(&tz->thermal_check_work)) > + queue_work(acpi_thermal_pm_queue, &tz->thermal_check_work); > +} > + > static void acpi_thermal_notify(struct acpi_device *device, u32 event) > { > struct acpi_thermal *tz = acpi_driver_data(device); > @@ -910,17 +910,17 @@ static void acpi_thermal_notify(struct a > > switch (event) { > case ACPI_THERMAL_NOTIFY_TEMPERATURE: > - acpi_thermal_check(tz); > + acpi_queue_thermal_check(tz); > break; > case ACPI_THERMAL_NOTIFY_THRESHOLDS: > acpi_thermal_trips_update(tz, ACPI_TRIPS_REFRESH_THRESHOLDS); > - acpi_thermal_check(tz); > + acpi_queue_thermal_check(tz); > acpi_bus_generate_netlink_event(device->pnp.device_class, > dev_name(&device->dev), event, 0); > break; > case ACPI_THERMAL_NOTIFY_DEVICES: > acpi_thermal_trips_update(tz, ACPI_TRIPS_REFRESH_DEVICES); > - acpi_thermal_check(tz); > + acpi_queue_thermal_check(tz); > acpi_bus_generate_netlink_event(device->pnp.device_class, > dev_name(&device->dev), event, 0); > break; > @@ -1020,7 +1020,25 @@ static void acpi_thermal_check_fn(struct > { > struct acpi_thermal *tz = container_of(work, struct acpi_thermal, > thermal_check_work); > - acpi_thermal_check(tz); > + > + /* > + * In general, it is not sufficient to check the pending bit, because > + * subsequent instances of this function may be queued after one of them > + * has started running (e.g. if _TMP sleeps). Avoid bailing out if just > + * one of them is running, though, because it may have done the actual > + * check some time ago, so allow at least one of them to block on the > + * mutex while another one is running the update. > + */ > + if (!refcount_dec_not_one(&tz->thermal_check_count)) > + return; > + > + mutex_lock(&tz->thermal_check_lock); > + > + thermal_zone_device_update(tz->thermal_zone, THERMAL_EVENT_UNSPECIFIED); > + > + refcount_inc(&tz->thermal_check_count); > + > + mutex_unlock(&tz->thermal_check_lock); > } > > static int acpi_thermal_add(struct acpi_device *device) > @@ -1052,6 +1070,8 @@ static int acpi_thermal_add(struct acpi_ > if (result) > goto free_memory; > > + refcount_set(&tz->thermal_check_count, 3); > + mutex_init(&tz->thermal_check_lock); > INIT_WORK(&tz->thermal_check_work, acpi_thermal_check_fn); > > pr_info(PREFIX "%s [%s] (%ld C)\n", acpi_device_name(device), > @@ -1117,7 +1137,7 @@ static int acpi_thermal_resume(struct de > tz->state.active |= tz->trips.active[i].flags.enabled; > } > > - queue_work(acpi_thermal_pm_queue, &tz->thermal_check_work); > + acpi_queue_thermal_check(tz); > > return AE_OK; > } > > >
