On Fri, Jan 22, 2021 at 5:39 PM Stephen Berman <stephen.berman@xxxxxxx> wrote: > > On Fri, 22 Jan 2021 17:23:36 +0100 "Rafael J. Wysocki" <rafael@xxxxxxxxxx> wrote: > > > 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? > > Sorry, I haven't been able to make time to test the patch yet, but I'll > try to do so this weekend. Is it just the patch below that I should > apply, ignoring the previous patches you sent? Yes. > And can I apply it to the current mainline kernel? Yes, it should be applicable to the current mainline (at least as of 5.11-rc4). Thanks! > >> --- > >> 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; > >> } > >> > >> > >>
