ACPI: thermal: Do not call acpi_thermal_check() directly

From: Rafael J. Wysocki <[email protected]>

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 <[email protected]>

Diagnosed-by Sebastian Andrzej Siewior <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/thermal.c |   46 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/acpi/thermal.c
===================================================================

drivers/acpi/thermal.c +33 -13

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

Quoted 23 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

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.

Quoted 4 lines

BugLink https://bugzilla.kernel.org/show_bug.cgi?id=208877

Reported-by Stephen Berman <[email protected]>

Diagnosed-by Sebastian Andrzej Siewior <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

Well, it's been over a week since this was posted.

Does anyone have any comments?

Quoted 114 lines

---
 drivers/acpi/thermal.c |   46 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/acpi/thermal.c
===================================================================

drivers/acpi/thermal.c +33 -13

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

Quoted 29 lines · On Fri, 22 Jan 2021 17:23:36 +0100 "Rafael J. Wysocki" <[email protected]>…

Quoted 27 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

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 <[email protected]>

Diagnosed-by Sebastian Andrzej Siewior <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

Quoted 3 lines

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?

And can I apply it to the current mainline kernel?

Quoted 114 lines

Quoted 114 lines

---
 drivers/acpi/thermal.c |   46 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/acpi/thermal.c
===================================================================

drivers/acpi/thermal.c +33 -13

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

Quoted 32 lines · Stephen Berman

Quoted 29 lines · On Fri, 22 Jan 2021 17:23:36 +0100 "Rafael J. Wysocki" <[email protected]>…

Quoted 27 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

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 <[email protected]>

Diagnosed-by Sebastian Andrzej Siewior <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

Quoted 7 lines

Quoted 3 lines

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.

Quoted 1 line

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!

Well, it's been over a week since this was posted.

Does anyone have any comments?