| Message ID | 3391226.KRKnzuvfpg@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: thermal: Do not call acpi_thermal_check() directly | expand |
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; > } > > >
On Fri, 22 Jan 2021 17:23:36 +0100 "Rafael J. Wysocki" <rafael@kernel.org> wrote: > 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? 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 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 <bigeasy@linutronix.de> 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" <rafael@kernel.org> wrote: > On Fri, Jan 22, 2021 at 5:39 PM Stephen Berman <stephen.berman@gmx.net> wrote: >> >> On Fri, 22 Jan 2021 17:23:36 +0100 "Rafael J. Wysocki" <rafael@kernel.org> wrote: >> >> > 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? >> >> 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
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; }