[PATCH v2 06/12] thermal: core: Consolidate thermal zone locking during initialization

Posted by Rafael J. Wysocki 1 month, 3 weeks ago

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The part of thermal zone initialization carried out under
thermal_list_lock acquires the thermal zone lock and releases it
multiple times back and forth which is not really necessary.

Instead of doing this, make it acquire the thermal zone lock once after
acquiring thermal_list_lock and release it along with that lock.

For this purpose, move all of the code in question to
thermal_zone_init_complete() introduced previously and provide an
"unlocked" variant of thermal_zone_cdev_bind() to be invoked from
there.

Also notice that a thermal zone does not need to be added to
thermal_tz_list under its own lock, so make the new code acquire
the thermal zone lock after adding it to the list.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

This is a new iteration of

https://lore.kernel.org/linux-pm/10548633.nUPlyArG6x@rjwysocki.net/

v1 -> v2: Rebase, update the changelog.

---
 drivers/thermal/thermal_core.c |   39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -933,16 +933,14 @@ void print_bind_err_msg(struct thermal_z
 		cdev->type, thermal_zone_trip_id(tz, trip), ret);
 }
 
-static void thermal_zone_cdev_bind(struct thermal_zone_device *tz,
-				   struct thermal_cooling_device *cdev)
+static void __thermal_zone_cdev_bind(struct thermal_zone_device *tz,
+				     struct thermal_cooling_device *cdev)
 {
 	struct thermal_trip_desc *td;
 
 	if (!tz->ops.should_bind)
 		return;
 
-	mutex_lock(&tz->lock);
-
 	for_each_trip_desc(tz, td) {
 		struct thermal_trip *trip = &td->trip;
 		struct cooling_spec c = {
@@ -959,6 +957,14 @@ static void thermal_zone_cdev_bind(struc
 		if (ret)
 			print_bind_err_msg(tz, trip, cdev, ret);
 	}
+}
+
+static void thermal_zone_cdev_bind(struct thermal_zone_device *tz,
+				   struct thermal_cooling_device *cdev)
+{
+	mutex_lock(&tz->lock);
+
+	__thermal_zone_cdev_bind(tz, cdev);
 
 	mutex_unlock(&tz->lock);
 }
@@ -1336,8 +1342,18 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_
 
 static void thermal_zone_init_complete(struct thermal_zone_device *tz)
 {
+	struct thermal_cooling_device *cdev;
+
+	mutex_lock(&thermal_list_lock);
+
+	list_add_tail(&tz->node, &thermal_tz_list);
+
 	mutex_lock(&tz->lock);
 
+	/* Bind cooling devices for this zone. */
+	list_for_each_entry(cdev, &thermal_cdev_list, node)
+		__thermal_zone_cdev_bind(tz, cdev);
+
 	tz->state &= ~TZ_STATE_FLAG_INIT;
 	/*
 	 * If system suspend or resume is in progress at this point, the
@@ -1350,6 +1366,8 @@ static void thermal_zone_init_complete(s
 	__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
 
 	mutex_unlock(&tz->lock);
+
+	mutex_unlock(&thermal_list_lock);
 }
 
 /**
@@ -1386,7 +1404,6 @@ thermal_zone_device_register_with_trips(
 					unsigned int polling_delay)
 {
 	const struct thermal_trip *trip = trips;
-	struct thermal_cooling_device *cdev;
 	struct thermal_zone_device *tz;
 	struct thermal_trip_desc *td;
 	int id;
@@ -1514,20 +1531,8 @@ thermal_zone_device_register_with_trips(
 			goto unregister;
 	}
 
-	mutex_lock(&thermal_list_lock);
-
-	mutex_lock(&tz->lock);
-	list_add_tail(&tz->node, &thermal_tz_list);
-	mutex_unlock(&tz->lock);
-
-	/* Bind cooling devices for this zone */
-	list_for_each_entry(cdev, &thermal_cdev_list, node)
-		thermal_zone_cdev_bind(tz, cdev);
-
 	thermal_zone_init_complete(tz);
 
-	mutex_unlock(&thermal_list_lock);
-
 	thermal_notify_tz_create(tz);
 
 	thermal_debug_tz_add(tz);

Re: [PATCH v2 06/12] thermal: core: Consolidate thermal zone locking during initialization

Posted by Lukasz Luba 1 month, 1 week ago


On 10/4/24 20:23, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The part of thermal zone initialization carried out under
> thermal_list_lock acquires the thermal zone lock and releases it
> multiple times back and forth which is not really necessary.
> 
> Instead of doing this, make it acquire the thermal zone lock once after
> acquiring thermal_list_lock and release it along with that lock.
> 
> For this purpose, move all of the code in question to
> thermal_zone_init_complete() introduced previously and provide an
> "unlocked" variant of thermal_zone_cdev_bind() to be invoked from
> there.
> 
> Also notice that a thermal zone does not need to be added to
> thermal_tz_list under its own lock, so make the new code acquire
> the thermal zone lock after adding it to the list.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> This is a new iteration of
> 
> https://lore.kernel.org/linux-pm/10548633.nUPlyArG6x@rjwysocki.net/
> 
> v1 -> v2: Rebase, update the changelog.
> 
> ---
>   drivers/thermal/thermal_core.c |   39 ++++++++++++++++++++++-----------------
>   1 file changed, 22 insertions(+), 17 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -933,16 +933,14 @@ void print_bind_err_msg(struct thermal_z
>   		cdev->type, thermal_zone_trip_id(tz, trip), ret);
>   }
>   
> -static void thermal_zone_cdev_bind(struct thermal_zone_device *tz,
> -				   struct thermal_cooling_device *cdev)
> +static void __thermal_zone_cdev_bind(struct thermal_zone_device *tz,
> +				     struct thermal_cooling_device *cdev)
>   {
>   	struct thermal_trip_desc *td;
>   
>   	if (!tz->ops.should_bind)
>   		return;
>   
> -	mutex_lock(&tz->lock);
> -
>   	for_each_trip_desc(tz, td) {
>   		struct thermal_trip *trip = &td->trip;
>   		struct cooling_spec c = {
> @@ -959,6 +957,14 @@ static void thermal_zone_cdev_bind(struc
>   		if (ret)
>   			print_bind_err_msg(tz, trip, cdev, ret);
>   	}
> +}
> +
> +static void thermal_zone_cdev_bind(struct thermal_zone_device *tz,
> +				   struct thermal_cooling_device *cdev)
> +{
> +	mutex_lock(&tz->lock);
> +
> +	__thermal_zone_cdev_bind(tz, cdev);
>   
>   	mutex_unlock(&tz->lock);
>   }
> @@ -1336,8 +1342,18 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_
>   
>   static void thermal_zone_init_complete(struct thermal_zone_device *tz)
>   {
> +	struct thermal_cooling_device *cdev;
> +
> +	mutex_lock(&thermal_list_lock);
> +
> +	list_add_tail(&tz->node, &thermal_tz_list);
> +
>   	mutex_lock(&tz->lock);
>   
> +	/* Bind cooling devices for this zone. */
> +	list_for_each_entry(cdev, &thermal_cdev_list, node)
> +		__thermal_zone_cdev_bind(tz, cdev);
> +
>   	tz->state &= ~TZ_STATE_FLAG_INIT;
>   	/*
>   	 * If system suspend or resume is in progress at this point, the
> @@ -1350,6 +1366,8 @@ static void thermal_zone_init_complete(s
>   	__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
>   
>   	mutex_unlock(&tz->lock);
> +
> +	mutex_unlock(&thermal_list_lock);
>   }
>   
>   /**
> @@ -1386,7 +1404,6 @@ thermal_zone_device_register_with_trips(
>   					unsigned int polling_delay)
>   {
>   	const struct thermal_trip *trip = trips;
> -	struct thermal_cooling_device *cdev;
>   	struct thermal_zone_device *tz;
>   	struct thermal_trip_desc *td;
>   	int id;
> @@ -1514,20 +1531,8 @@ thermal_zone_device_register_with_trips(
>   			goto unregister;
>   	}
>   
> -	mutex_lock(&thermal_list_lock);
> -
> -	mutex_lock(&tz->lock);
> -	list_add_tail(&tz->node, &thermal_tz_list);
> -	mutex_unlock(&tz->lock);
> -
> -	/* Bind cooling devices for this zone */
> -	list_for_each_entry(cdev, &thermal_cdev_list, node)
> -		thermal_zone_cdev_bind(tz, cdev);
> -
>   	thermal_zone_init_complete(tz);
>   
> -	mutex_unlock(&thermal_list_lock);
> -
>   	thermal_notify_tz_create(tz);
>   
>   	thermal_debug_tz_add(tz);
> 
> 
> 

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>