diff mbox series

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

Message ID 3837835.kQq0lBPeGt@rjwysocki.net (mailing list archive)
State Mainlined, archived
Headers show
Series thermal: Rework binding cooling devices to trip points | expand

Commit Message

Rafael J. Wysocki Aug. 19, 2024, 3:58 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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@intel.com>
---

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(-)

Comments

Zhang Rui Aug. 20, 2024, 7:05 a.m. UTC | #1 
On Mon, 2024-08-19 at 17:58 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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@intel.com>

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

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
> 
> 
>
Huisong Li Aug. 20, 2024, 8:27 a.m. UTC | #2 
在 2024/8/19 23:58, Rafael J. Wysocki 写道:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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@intel.com>
> ---
>
> 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;
>   
<snip>
>   
> 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);
If so, it seems that the for_each_thermal_trip() can be removed or no 
need to export.
>   }
>   
>   static int
>
>
>
>
>
> .
Rafael J. Wysocki Aug. 20, 2024, 10:27 a.m. UTC | #3 
On Tue, Aug 20, 2024 at 10:27 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2024/8/19 23:58, Rafael J. Wysocki 写道:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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@intel.com>
> > ---
> >
> > 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;
> >
> <snip>
> >
> > 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);
> If so, it seems that the for_each_thermal_trip() can be removed or no
> need to export.

I beg to differ:

$ git grep for_each_thermal_trip | head -1
drivers/net/wireless/intel/iwlwifi/mvm/tt.c:
for_each_thermal_trip(mvm->tz_device.tzone, iwl_trip_temp_cb, &twd);
Huisong Li Aug. 21, 2024, 9:02 a.m. UTC | #4 
在 2024/8/20 18:27, Rafael J. Wysocki 写道:
> On Tue, Aug 20, 2024 at 10:27 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>>
>> 在 2024/8/19 23:58, Rafael J. Wysocki 写道:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> 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@intel.com>
>>> ---
>>>
>>> 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;
>>>
>> <snip>
>>> 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);
>> If so, it seems that the for_each_thermal_trip() can be removed or no
>> need to export.
> I beg to differ:
>
> $ git grep for_each_thermal_trip | head -1
> drivers/net/wireless/intel/iwlwifi/mvm/tt.c:
> for_each_thermal_trip(mvm->tz_device.tzone, iwl_trip_temp_cb, &twd);
Can we modify it for tt.c?
It doesn't seem to keep two interfaces. I'm a little confused for that.
> .
Daniel Lezcano Aug. 21, 2024, 9:46 a.m. UTC | #5 
On 19/08/2024 17:58, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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@intel.com>
> ---

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Rafael J. Wysocki Aug. 21, 2024, 10:30 a.m. UTC | #6 
On Wed, Aug 21, 2024 at 11:02 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2024/8/20 18:27, Rafael J. Wysocki 写道:
> > On Tue, Aug 20, 2024 at 10:27 AM lihuisong (C) <lihuisong@huawei.com> wrote:
> >>
> >> 在 2024/8/19 23:58, Rafael J. Wysocki 写道:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> 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@intel.com>
> >>> ---
> >>>
> >>> 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;
> >>>
> >> <snip>
> >>> 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);
> >> If so, it seems that the for_each_thermal_trip() can be removed or no
> >> need to export.
> > I beg to differ:
> >
> > $ git grep for_each_thermal_trip | head -1
> > drivers/net/wireless/intel/iwlwifi/mvm/tt.c:
> > for_each_thermal_trip(mvm->tz_device.tzone, iwl_trip_temp_cb, &twd);
> Can we modify it for tt.c?

Not really.

tt.c invokes this under the thermal zone lock, so it cannot use the
"locked" variant.

> It doesn't seem to keep two interfaces. I'm a little confused for that.

The difference between for_each_thermal_trip() and
thermal_zone_for_each_trip() is "unlocked" vs "locked", respectively.
It may just be a question of naming ...
diff mbox series

Patch

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