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

[Rafael J. Wysocki]

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