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
===================================================================
--- 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;
}
On Fri, Jan 22, 2021 at 5:39 PM Stephen Berman <[email protected]> wrote:
>
> On Fri, 22 Jan 2021 17:23:36 +0100 "Rafael J. Wysocki" <[email protected]> wrote:
>
> > On Thu, Jan 14, 2021 at 7:35 PM Rafael J. Wysocki <[email protected]> wrote:
> >>
> >> 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]>
> >
> > 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;
> >> }
> >>
> >>
> >>
On Fri, 22 Jan 2021 17:23:36 +0100 "Rafael J. Wysocki" <[email protected]> wrote:
> On Thu, Jan 14, 2021 at 7:35 PM Rafael J. Wysocki <[email protected]> wrote:
>>
>> 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]>
>
> 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?
Thanks,
Steve Berman
>> ---
>> 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;
>> }
>>
>>
>>
On Thu, Jan 14, 2021 at 7:35 PM Rafael J. Wysocki <[email protected]> wrote:
>
> 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]>
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;
> }
>
>
>
On 2021-01-22 17:23:36 [+0100], Rafael J. Wysocki wrote:
>
> Well, it's been over a week since this was posted.
Thank you for this ;)
> Does anyone have any comments?
I looked over it and it makes sense, so
Reviewed-by: Sebastian Andrzej Siewior <[email protected]>
I didn't comment on it since a testing-by would be better ;)
Could you please add a stable tag? I've seen a few "comments" in forums
suggesting what I suggested to Stephen as a work around while I was
searching for his motherboard so they are more people affected by the
shutdown problem.
Sebastian
On Fri, 22 Jan 2021 17:42:59 +0100 "Rafael J. Wysocki" <[email protected]> wrote:
> On Fri, Jan 22, 2021 at 5:39 PM Stephen Berman <[email protected]> wrote:
>>
>> On Fri, 22 Jan 2021 17:23:36 +0100 "Rafael J. Wysocki" <[email protected]> wrote:
>>
>> > On Thu, Jan 14, 2021 at 7:35 PM Rafael J. Wysocki <[email protected]> wrote:
>> >>
>> >> 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]>
>> >
>> > 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!
I've now updated my local repo to 5.11.0-rc4+, installed your patch,
rebuilt and installed the kernel, rebooted (without adding
'thermal.tzp=300' to the kernel command line), did some normal activity,
then ran 'shutdown -h now', and the machine did just that. So your
patch seems to have fixed the problem I reported. Many thanks!
Steve Berman