Re: [PATCH] ACPI: thermal: Do not call acpi_thermal_check() directly

Rafael J. Wysocki
2021-01-22T17:07:34Z

On Thu, Jan 14, 2021 at 7:35 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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@gmx.net>
> Diagnosed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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;
> }
>
>
>