[5/9] thermal/core: Introduce locked version of thermal_zone_device_update

  In thermal_zone_device_set_mode(), the thermal zone mutex is released only
to be reacquired in the subsequent call to thermal_zone_device_update().

Introduce __thermal_zone_device_update() as locked version of
thermal_zone_device_update() and call it from
thermal_zone_device_set_mode() without releasing the lock to avoid
the extra release/acuire sequence.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/thermal/thermal_core.c | 57 ++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 26 deletions(-)
  

On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> In thermal_zone_device_set_mode(), the thermal zone mutex is released only
> to be reacquired in the subsequent call to thermal_zone_device_update().
>
> Introduce __thermal_zone_device_update() as locked version of

Did you mean "unlocked"?

> thermal_zone_device_update() and call it from
> thermal_zone_device_set_mode() without releasing the lock to avoid
> the extra release/acuire sequence.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/thermal/thermal_core.c | 57 ++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 562ece8d16aa..9facd9c5b70f 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -403,6 +403,34 @@ static void thermal_zone_device_init(struct thermal_zone_device *tz)
>                 pos->initialized = false;
>  }
>
> +static void __thermal_zone_device_update(struct thermal_zone_device *tz,
> +                                        enum thermal_notify_event event)
> +{
> +       int count;
> +
> +       if (atomic_read(&in_suspend))
> +               return;
> +
> +       if (WARN_ONCE(!tz->ops->get_temp,
> +                     "'%s' must not be called without 'get_temp' ops set\n",
> +                     __func__))
> +               return;
> +
> +       if (!thermal_zone_device_is_enabled(tz))
> +               return;
> +
> +       update_temperature(tz);
> +
> +       __thermal_zone_set_trips(tz);
> +
> +       tz->notify_event = event;
> +
> +       for (count = 0; count < tz->num_trips; count++)
> +               handle_thermal_trip(tz, count);
> +
> +       monitor_thermal_zone(tz);
> +}
> +
>  static int thermal_zone_device_set_mode(struct thermal_zone_device *tz,
>                                         enum thermal_device_mode mode)
>  {
> @@ -423,9 +451,9 @@ static int thermal_zone_device_set_mode(struct thermal_zone_device *tz,
>         if (!ret)
>                 tz->mode = mode;
>
> -       mutex_unlock(&tz->lock);
> +       __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
>
> -       thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> +       mutex_unlock(&tz->lock);
>
>         if (mode == THERMAL_DEVICE_ENABLED)
>                 thermal_notify_tz_enable(tz->id);
> @@ -457,31 +485,8 @@ int thermal_zone_device_is_enabled(struct thermal_zone_device *tz)
>  void thermal_zone_device_update(struct thermal_zone_device *tz,
>                                 enum thermal_notify_event event)
>  {
> -       int count;
> -
> -       if (atomic_read(&in_suspend))
> -               return;
> -
> -       if (WARN_ONCE(!tz->ops->get_temp, "'%s' must not be called without "
> -                     "'get_temp' ops set\n", __func__))
> -               return;
> -
>         mutex_lock(&tz->lock);
> -
> -       if (!thermal_zone_device_is_enabled(tz))
> -               goto out;
> -
> -       update_temperature(tz);
> -
> -       __thermal_zone_set_trips(tz);
> -
> -       tz->notify_event = event;
> -
> -       for (count = 0; count < tz->num_trips; count++)
> -               handle_thermal_trip(tz, count);
> -
> -       monitor_thermal_zone(tz);
> -out:
> +       __thermal_zone_device_update(tz, event);
>         mutex_unlock(&tz->lock);
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_device_update);
> --
> 2.36.2
>
  

On Wed, Nov 09, 2022 at 08:15:17PM +0100, Rafael J. Wysocki wrote:
> On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > In thermal_zone_device_set_mode(), the thermal zone mutex is released only
> > to be reacquired in the subsequent call to thermal_zone_device_update().
> >
> > Introduce __thermal_zone_device_update() as locked version of
> 
> Did you mean "unlocked"?
> 
No, I did mean "locked", as in "must be called with thermal zone device
mutex acquired".

locked:

void __thermal_zone_device_update(struct thermal_zone_device *tz,
                                  enum thermal_notify_event event)
{
	...
}

unlocked:

void thermal_zone_device_update(struct thermal_zone_device *tz,
                                enum thermal_notify_event event)
{
        mutex_lock(&tz->lock);
        if (device_is_registered(&tz->device))
                __thermal_zone_device_update(tz, event);
        mutex_unlock(&tz->lock);
}

Should I phrase or explain it differently ?

Thanks,
Guenter
  

On Thu, Nov 10, 2022 at 1:25 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Nov 09, 2022 at 08:15:17PM +0100, Rafael J. Wysocki wrote:
> > On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > In thermal_zone_device_set_mode(), the thermal zone mutex is released only
> > > to be reacquired in the subsequent call to thermal_zone_device_update().
> > >
> > > Introduce __thermal_zone_device_update() as locked version of
> >
> > Did you mean "unlocked"?
> >
> No, I did mean "locked", as in "must be called with thermal zone device
> mutex acquired".
>
> locked:
>
> void __thermal_zone_device_update(struct thermal_zone_device *tz,
>                                   enum thermal_notify_event event)
> {
>         ...
> }
>
> unlocked:
>
> void thermal_zone_device_update(struct thermal_zone_device *tz,
>                                 enum thermal_notify_event event)
> {
>         mutex_lock(&tz->lock);
>         if (device_is_registered(&tz->device))
>                 __thermal_zone_device_update(tz, event);
>         mutex_unlock(&tz->lock);
> }

Thanks for the explanation.

> Should I phrase or explain it differently ?

I would rather say "bare" or something like that so it is all clear to
people like me, but it is your call.
  

On Thu, Nov 10, 2022 at 02:01:49PM +0100, Rafael J. Wysocki wrote:
> On Thu, Nov 10, 2022 at 1:25 AM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Wed, Nov 09, 2022 at 08:15:17PM +0100, Rafael J. Wysocki wrote:
> > > On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > >
> > > > In thermal_zone_device_set_mode(), the thermal zone mutex is released only
> > > > to be reacquired in the subsequent call to thermal_zone_device_update().
> > > >
> > > > Introduce __thermal_zone_device_update() as locked version of
> > >
> > > Did you mean "unlocked"?
> > >
> > No, I did mean "locked", as in "must be called with thermal zone device
> > mutex acquired".
> >
> > locked:
> >
> > void __thermal_zone_device_update(struct thermal_zone_device *tz,
> >                                   enum thermal_notify_event event)
> > {
> >         ...
> > }
> >
> > unlocked:
> >
> > void thermal_zone_device_update(struct thermal_zone_device *tz,
> >                                 enum thermal_notify_event event)
> > {
> >         mutex_lock(&tz->lock);
> >         if (device_is_registered(&tz->device))
> >                 __thermal_zone_device_update(tz, event);
> >         mutex_unlock(&tz->lock);
> > }
> 
> Thanks for the explanation.
> 
> > Should I phrase or explain it differently ?
> 
> I would rather say "bare" or something like that so it is all clear to
> people like me, but it is your call.

I updated the commit description to use "must be called with thermal
device mutex held". I kept 'locked' in the subject; I don't think using
'bare' there would add any clarity. Hope that is ok.

Thanks,
Guenter
  

On Thu, Nov 10, 2022 at 3:12 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Nov 10, 2022 at 02:01:49PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Nov 10, 2022 at 1:25 AM Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On Wed, Nov 09, 2022 at 08:15:17PM +0100, Rafael J. Wysocki wrote:
> > > > On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > > >
> > > > > In thermal_zone_device_set_mode(), the thermal zone mutex is released only
> > > > > to be reacquired in the subsequent call to thermal_zone_device_update().
> > > > >
> > > > > Introduce __thermal_zone_device_update() as locked version of
> > > >
> > > > Did you mean "unlocked"?
> > > >
> > > No, I did mean "locked", as in "must be called with thermal zone device
> > > mutex acquired".
> > >
> > > locked:
> > >
> > > void __thermal_zone_device_update(struct thermal_zone_device *tz,
> > >                                   enum thermal_notify_event event)
> > > {
> > >         ...
> > > }
> > >
> > > unlocked:
> > >
> > > void thermal_zone_device_update(struct thermal_zone_device *tz,
> > >                                 enum thermal_notify_event event)
> > > {
> > >         mutex_lock(&tz->lock);
> > >         if (device_is_registered(&tz->device))
> > >                 __thermal_zone_device_update(tz, event);
> > >         mutex_unlock(&tz->lock);
> > > }
> >
> > Thanks for the explanation.
> >
> > > Should I phrase or explain it differently ?
> >
> > I would rather say "bare" or something like that so it is all clear to
> > people like me, but it is your call.
>
> I updated the commit description to use "must be called with thermal
> device mutex held". I kept 'locked' in the subject; I don't think using
> 'bare' there would add any clarity. Hope that is ok.

It is.