Re: [PATCH v3 05/14] thermal: core: Move thermal zone locking out of bind/unbind functions

[Zhang, Rui]

On Mon, 2024-08-19 at 17:58 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
> Since thermal_bind_cdev_to_trip() and thermal_unbind_cdev_from_trip()
> acquire the thermal zone lock, the locking rules for their callers
> get
> complicated.  In particular, the thermal zone lock cannot be acquired
> in any code path leading to one of these functions even though it
> might
> be useful to do so.
> 
> To address this, remove the thermal zone locking from both these
> functions, add lockdep assertions for the thermal zone lock to both
> of them and make their callers acquire the thermal zone lock instead.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>

Reviewed-by: Zhang Rui <rui.zhang@xxxxxxxxx>

thanks,
rui

> ---
> 
> v2 -> v3: Rebase after dropping patches [04-05/17] from the series
> 
> v1 -> v2: No changes
> 
> ---
>  drivers/acpi/thermal.c         |    2 +-
>  drivers/thermal/thermal_core.c |   30 ++++++++++++++++++++++--------
>  2 files changed, 23 insertions(+), 9 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -785,6 +785,7 @@ int thermal_bind_cdev_to_trip(struct the
>         int result;
>  
>         lockdep_assert_held(&thermal_list_lock);
> +       lockdep_assert_held(&tz->lock);
>  
>         if (list_empty(&tz->node) || list_empty(&cdev->node))
>                 return -EINVAL;
> @@ -847,7 +848,6 @@ int thermal_bind_cdev_to_trip(struct the
>         if (result)
>                 goto remove_trip_file;
>  
> -       mutex_lock(&tz->lock);
>         mutex_lock(&cdev->lock);
>         list_for_each_entry(pos, &tz->thermal_instances, tz_node)
>                 if (pos->trip == trip && pos->cdev == cdev) {
> @@ -862,7 +862,6 @@ int thermal_bind_cdev_to_trip(struct the
>                 thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV);
>         }
>         mutex_unlock(&cdev->lock);
> -       mutex_unlock(&tz->lock);
>  
>         if (!result)
>                 return 0;
> @@ -886,11 +885,19 @@ int thermal_zone_bind_cooling_device(str
>                                      unsigned long upper, unsigned
> long lower,
>                                      unsigned int weight)
>  {
> +       int ret;
> +
>         if (trip_index < 0 || trip_index >= tz->num_trips)
>                 return -EINVAL;
>  
> -       return thermal_bind_cdev_to_trip(tz, &tz-
> >trips[trip_index].trip, cdev,
> -                                        upper, lower, weight);
> +       mutex_lock(&tz->lock);
> +
> +       ret = thermal_bind_cdev_to_trip(tz, &tz-
> >trips[trip_index].trip, cdev,
> +                                       upper, lower, weight);
> +
> +       mutex_unlock(&tz->lock);
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_bind_cooling_device);
>  
> @@ -912,7 +919,8 @@ int thermal_unbind_cdev_from_trip(struct
>  {
>         struct thermal_instance *pos, *next;
>  
> -       mutex_lock(&tz->lock);
> +       lockdep_assert_held(&tz->lock);
> +
>         mutex_lock(&cdev->lock);
>         list_for_each_entry_safe(pos, next, &tz->thermal_instances,
> tz_node) {
>                 if (pos->trip == trip && pos->cdev == cdev) {
> @@ -922,12 +930,10 @@ int thermal_unbind_cdev_from_trip(struct
>                         thermal_governor_update_tz(tz,
> THERMAL_TZ_UNBIND_CDEV);
>  
>                         mutex_unlock(&cdev->lock);
> -                       mutex_unlock(&tz->lock);
>                         goto unbind;
>                 }
>         }
>         mutex_unlock(&cdev->lock);
> -       mutex_unlock(&tz->lock);
>  
>         return -ENODEV;
>  
> @@ -945,10 +951,18 @@ int thermal_zone_unbind_cooling_device(s
>                                        int trip_index,
>                                        struct thermal_cooling_device
> *cdev)
>  {
> +       int ret;
> +
>         if (trip_index < 0 || trip_index >= tz->num_trips)
>                 return -EINVAL;
>  
> -       return thermal_unbind_cdev_from_trip(tz, &tz-
> >trips[trip_index].trip, cdev);
> +       mutex_lock(&tz->lock);
> +
> +       ret = thermal_unbind_cdev_from_trip(tz, &tz-
> >trips[trip_index].trip, cdev);
> +
> +       mutex_unlock(&tz->lock);
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_unbind_cooling_device);
>  
> Index: linux-pm/drivers/acpi/thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/thermal.c
> +++ linux-pm/drivers/acpi/thermal.c
> @@ -609,7 +609,7 @@ static int acpi_thermal_bind_unbind_cdev
>                 .thermal = thermal, .cdev = cdev, .bind = bind
>         };
>  
> -       return for_each_thermal_trip(thermal, bind_unbind_cdev_cb,
> &bd);
> +       return thermal_zone_for_each_trip(thermal,
> bind_unbind_cdev_cb, &bd);
>  }
>  
>  static int
> 
> 
>