[PATCH v2 04/11] thermal: core: Manage thermal_list_lock using a mutex guard

Posted by Rafael J. Wysocki 1 year, 3 months ago

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

Switch over the thermal core to using a mutex guard for
thermal_list_lock management.

No intentional functional impact.

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

This is a resend of

https://lore.kernel.org/linux-pm/2018087.usQuhbGJ8B@rjwysocki.net/

---
 drivers/thermal/thermal_core.c |  148 ++++++++++++++++++-----------------------
 1 file changed, 68 insertions(+), 80 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -140,7 +140,7 @@ int thermal_register_governor(struct the
 			def_governor = governor;
 	}
 
-	mutex_lock(&thermal_list_lock);
+	guard(mutex)(&thermal_list_lock);
 
 	list_for_each_entry(pos, &thermal_tz_list, node) {
 		/*
@@ -163,7 +163,6 @@ int thermal_register_governor(struct the
 		}
 	}
 
-	mutex_unlock(&thermal_list_lock);
 	mutex_unlock(&thermal_governor_lock);
 
 	return err;
@@ -181,7 +180,9 @@ void thermal_unregister_governor(struct
 	if (!__find_governor(governor->name))
 		goto exit;
 
-	mutex_lock(&thermal_list_lock);
+	list_del(&governor->governor_list);
+
+	guard(mutex)(&thermal_list_lock);
 
 	list_for_each_entry(pos, &thermal_tz_list, node) {
 		if (!strncasecmp(pos->governor->name, governor->name,
@@ -189,8 +190,6 @@ void thermal_unregister_governor(struct
 			thermal_set_governor(pos, NULL);
 	}
 
-	mutex_unlock(&thermal_list_lock);
-	list_del(&governor->governor_list);
 exit:
 	mutex_unlock(&thermal_governor_lock);
 }
@@ -686,51 +685,52 @@ int for_each_thermal_cooling_device(int
 					      void *), void *data)
 {
 	struct thermal_cooling_device *cdev;
-	int ret = 0;
 
-	mutex_lock(&thermal_list_lock);
+	guard(mutex)(&thermal_list_lock);
+
 	list_for_each_entry(cdev, &thermal_cdev_list, node) {
+		int ret;
+
 		ret = cb(cdev, data);
 		if (ret)
-			break;
+			return ret;
 	}
-	mutex_unlock(&thermal_list_lock);
 
-	return ret;
+	return 0;
 }
 
 int for_each_thermal_zone(int (*cb)(struct thermal_zone_device *, void *),
 			  void *data)
 {
 	struct thermal_zone_device *tz;
-	int ret = 0;
 
-	mutex_lock(&thermal_list_lock);
+	guard(mutex)(&thermal_list_lock);
+
 	list_for_each_entry(tz, &thermal_tz_list, node) {
+		int ret;
+
 		ret = cb(tz, data);
 		if (ret)
-			break;
+			return ret;
 	}
-	mutex_unlock(&thermal_list_lock);
 
-	return ret;
+	return 0;
 }
 
 struct thermal_zone_device *thermal_zone_get_by_id(int id)
 {
-	struct thermal_zone_device *tz, *match = NULL;
+	struct thermal_zone_device *tz;
+
+	guard(mutex)(&thermal_list_lock);
 
-	mutex_lock(&thermal_list_lock);
 	list_for_each_entry(tz, &thermal_tz_list, node) {
 		if (tz->id == id) {
 			get_device(&tz->device);
-			match = tz;
-			break;
+			return tz;
 		}
 	}
-	mutex_unlock(&thermal_list_lock);
 
-	return match;
+	return NULL;
 }
 
 /*
@@ -969,14 +969,12 @@ static void thermal_cooling_device_init_
 {
 	struct thermal_zone_device *tz;
 
-	mutex_lock(&thermal_list_lock);
+	guard(mutex)(&thermal_list_lock);
 
 	list_add(&cdev->node, &thermal_cdev_list);
 
 	list_for_each_entry(tz, &thermal_tz_list, node)
 		thermal_zone_cdev_bind(tz, cdev);
-
-	mutex_unlock(&thermal_list_lock);
 }
 
 /**
@@ -1210,10 +1208,10 @@ void thermal_cooling_device_update(struc
 	 * Hold thermal_list_lock throughout the update to prevent the device
 	 * from going away while being updated.
 	 */
-	mutex_lock(&thermal_list_lock);
+	guard(mutex)(&thermal_list_lock);
 
 	if (!thermal_cooling_device_present(cdev))
-		goto unlock_list;
+		return;
 
 	/*
 	 * Update under the cdev lock to prevent the state from being set beyond
@@ -1255,9 +1253,6 @@ void thermal_cooling_device_update(struc
 
 unlock:
 	mutex_unlock(&cdev->lock);
-
-unlock_list:
-	mutex_unlock(&thermal_list_lock);
 }
 EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
 
@@ -1281,24 +1276,18 @@ static void thermal_zone_cdev_unbind(str
 static bool thermal_cooling_device_exit(struct thermal_cooling_device *cdev)
 {
 	struct thermal_zone_device *tz;
-	bool ret = true;
 
-	mutex_lock(&thermal_list_lock);
+	guard(mutex)(&thermal_list_lock);
 
-	if (!thermal_cooling_device_present(cdev)) {
-		ret = false;
-		goto unlock;
-	}
+	if (!thermal_cooling_device_present(cdev))
+		return false;
 
 	list_del(&cdev->node);
 
 	list_for_each_entry(tz, &thermal_tz_list, node)
 		thermal_zone_cdev_unbind(tz, cdev);
 
-unlock:
-	mutex_unlock(&thermal_list_lock);
-
-	return ret;
+	return true;
 }
 
 /**
@@ -1345,7 +1334,7 @@ static void thermal_zone_init_complete(s
 {
 	struct thermal_cooling_device *cdev;
 
-	mutex_lock(&thermal_list_lock);
+	guard(mutex)(&thermal_list_lock);
 
 	list_add_tail(&tz->node, &thermal_tz_list);
 
@@ -1365,8 +1354,6 @@ static void thermal_zone_init_complete(s
 		tz->state |= TZ_STATE_FLAG_SUSPENDED;
 
 	__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
-
-	mutex_unlock(&thermal_list_lock);
 }
 
 /**
@@ -1587,14 +1574,11 @@ EXPORT_SYMBOL_GPL(thermal_zone_device);
 static bool thermal_zone_exit(struct thermal_zone_device *tz)
 {
 	struct thermal_cooling_device *cdev;
-	bool ret = true;
 
-	mutex_lock(&thermal_list_lock);
+	guard(mutex)(&thermal_list_lock);
 
-	if (list_empty(&tz->node)) {
-		ret = false;
-		goto unlock;
-	}
+	if (list_empty(&tz->node))
+		return false;
 
 	guard(thermal_zone)(tz);
 
@@ -1605,10 +1589,7 @@ static bool thermal_zone_exit(struct the
 	list_for_each_entry(cdev, &thermal_cdev_list, node)
 		__thermal_zone_cdev_unbind(tz, cdev);
 
-unlock:
-	mutex_unlock(&thermal_list_lock);
-
-	return ret;
+	return true;
 }
 
 /**
@@ -1660,24 +1641,23 @@ struct thermal_zone_device *thermal_zone
 	unsigned int found = 0;
 
 	if (!name)
-		goto exit;
+		return ERR_PTR(-EINVAL);
+
+	guard(mutex)(&thermal_list_lock);
 
-	mutex_lock(&thermal_list_lock);
 	list_for_each_entry(pos, &thermal_tz_list, node)
 		if (!strncasecmp(name, pos->type, THERMAL_NAME_LENGTH)) {
 			found++;
 			ref = pos;
 		}
-	mutex_unlock(&thermal_list_lock);
 
-	/* nothing has been found, thus an error code for it */
-	if (found == 0)
-		ref = ERR_PTR(-ENODEV);
-	else if (found > 1)
-	/* Success only when an unique zone is found */
-		ref = ERR_PTR(-EEXIST);
+	if (!found)
+		return ERR_PTR(-ENODEV);
+
+	/* Success only when one zone is found. */
+	if (found > 1)
+		return ERR_PTR(-EEXIST);
 
-exit:
 	return ref;
 }
 EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
@@ -1718,6 +1698,18 @@ static void thermal_zone_pm_prepare(stru
 	tz->state |= TZ_STATE_FLAG_SUSPENDED;
 }
 
+static void thermal_pm_notify_prepare(void)
+{
+	struct thermal_zone_device *tz;
+
+	guard(mutex)(&thermal_list_lock);
+
+	thermal_pm_suspended = true;
+
+	list_for_each_entry(tz, &thermal_tz_list, node)
+		thermal_zone_pm_prepare(tz);
+}
+
 static void thermal_zone_pm_complete(struct thermal_zone_device *tz)
 {
 	guard(thermal_zone)(tz);
@@ -1736,35 +1728,31 @@ static void thermal_zone_pm_complete(str
 	mod_delayed_work(system_freezable_power_efficient_wq, &tz->poll_queue, 0);
 }
 
-static int thermal_pm_notify(struct notifier_block *nb,
-			     unsigned long mode, void *_unused)
+static void thermal_pm_notify_complete(void)
 {
 	struct thermal_zone_device *tz;
 
+	guard(mutex)(&thermal_list_lock);
+
+	thermal_pm_suspended = false;
+
+	list_for_each_entry(tz, &thermal_tz_list, node)
+		thermal_zone_pm_complete(tz);
+}
+
+static int thermal_pm_notify(struct notifier_block *nb,
+			     unsigned long mode, void *_unused)
+{
 	switch (mode) {
 	case PM_HIBERNATION_PREPARE:
 	case PM_RESTORE_PREPARE:
 	case PM_SUSPEND_PREPARE:
-		mutex_lock(&thermal_list_lock);
-
-		thermal_pm_suspended = true;
-
-		list_for_each_entry(tz, &thermal_tz_list, node)
-			thermal_zone_pm_prepare(tz);
-
-		mutex_unlock(&thermal_list_lock);
+		thermal_pm_notify_prepare();
 		break;
 	case PM_POST_HIBERNATION:
 	case PM_POST_RESTORE:
 	case PM_POST_SUSPEND:
-		mutex_lock(&thermal_list_lock);
-
-		thermal_pm_suspended = false;
-
-		list_for_each_entry(tz, &thermal_tz_list, node)
-			thermal_zone_pm_complete(tz);
-
-		mutex_unlock(&thermal_list_lock);
+		thermal_pm_notify_complete();
 		break;
 	default:
 		break;

Re: [PATCH v2 04/11] thermal: core: Manage thermal_list_lock using a mutex guard

Posted by Lukasz Luba 1 year, 3 months ago


On 10/10/24 23:10, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Switch over the thermal core to using a mutex guard for
> thermal_list_lock management.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> This is a resend of
> 
> https://lore.kernel.org/linux-pm/2018087.usQuhbGJ8B@rjwysocki.net/
> 
> ---
>   drivers/thermal/thermal_core.c |  148 ++++++++++++++++++-----------------------
>   1 file changed, 68 insertions(+), 80 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -140,7 +140,7 @@ int thermal_register_governor(struct the
>   			def_governor = governor;
>   	}
>   
> -	mutex_lock(&thermal_list_lock);
> +	guard(mutex)(&thermal_list_lock);
>   
>   	list_for_each_entry(pos, &thermal_tz_list, node) {
>   		/*
> @@ -163,7 +163,6 @@ int thermal_register_governor(struct the
>   		}
>   	}
>   
> -	mutex_unlock(&thermal_list_lock);
>   	mutex_unlock(&thermal_governor_lock);
>   
>   	return err;
> @@ -181,7 +180,9 @@ void thermal_unregister_governor(struct
>   	if (!__find_governor(governor->name))
>   		goto exit;
>   
> -	mutex_lock(&thermal_list_lock);
> +	list_del(&governor->governor_list);
> +
> +	guard(mutex)(&thermal_list_lock);
>   
>   	list_for_each_entry(pos, &thermal_tz_list, node) {
>   		if (!strncasecmp(pos->governor->name, governor->name,
> @@ -189,8 +190,6 @@ void thermal_unregister_governor(struct
>   			thermal_set_governor(pos, NULL);
>   	}
>   
> -	mutex_unlock(&thermal_list_lock);
> -	list_del(&governor->governor_list);
>   exit:
>   	mutex_unlock(&thermal_governor_lock);
>   }
> @@ -686,51 +685,52 @@ int for_each_thermal_cooling_device(int
>   					      void *), void *data)
>   {
>   	struct thermal_cooling_device *cdev;
> -	int ret = 0;
>   
> -	mutex_lock(&thermal_list_lock);
> +	guard(mutex)(&thermal_list_lock);
> +
>   	list_for_each_entry(cdev, &thermal_cdev_list, node) {
> +		int ret;
> +
>   		ret = cb(cdev, data);
>   		if (ret)
> -			break;
> +			return ret;
>   	}
> -	mutex_unlock(&thermal_list_lock);
>   
> -	return ret;
> +	return 0;
>   }
>   
>   int for_each_thermal_zone(int (*cb)(struct thermal_zone_device *, void *),
>   			  void *data)
>   {
>   	struct thermal_zone_device *tz;
> -	int ret = 0;
>   
> -	mutex_lock(&thermal_list_lock);
> +	guard(mutex)(&thermal_list_lock);
> +
>   	list_for_each_entry(tz, &thermal_tz_list, node) {
> +		int ret;
> +
>   		ret = cb(tz, data);
>   		if (ret)
> -			break;
> +			return ret;
>   	}
> -	mutex_unlock(&thermal_list_lock);
>   
> -	return ret;
> +	return 0;
>   }
>   
>   struct thermal_zone_device *thermal_zone_get_by_id(int id)
>   {
> -	struct thermal_zone_device *tz, *match = NULL;
> +	struct thermal_zone_device *tz;
> +
> +	guard(mutex)(&thermal_list_lock);
>   
> -	mutex_lock(&thermal_list_lock);
>   	list_for_each_entry(tz, &thermal_tz_list, node) {
>   		if (tz->id == id) {
>   			get_device(&tz->device);
> -			match = tz;
> -			break;
> +			return tz;
>   		}
>   	}
> -	mutex_unlock(&thermal_list_lock);
>   
> -	return match;
> +	return NULL;
>   }
>   
>   /*
> @@ -969,14 +969,12 @@ static void thermal_cooling_device_init_
>   {
>   	struct thermal_zone_device *tz;
>   
> -	mutex_lock(&thermal_list_lock);
> +	guard(mutex)(&thermal_list_lock);
>   
>   	list_add(&cdev->node, &thermal_cdev_list);
>   
>   	list_for_each_entry(tz, &thermal_tz_list, node)
>   		thermal_zone_cdev_bind(tz, cdev);
> -
> -	mutex_unlock(&thermal_list_lock);
>   }
>   
>   /**
> @@ -1210,10 +1208,10 @@ void thermal_cooling_device_update(struc
>   	 * Hold thermal_list_lock throughout the update to prevent the device
>   	 * from going away while being updated.
>   	 */
> -	mutex_lock(&thermal_list_lock);
> +	guard(mutex)(&thermal_list_lock);
>   
>   	if (!thermal_cooling_device_present(cdev))
> -		goto unlock_list;
> +		return;
>   
>   	/*
>   	 * Update under the cdev lock to prevent the state from being set beyond
> @@ -1255,9 +1253,6 @@ void thermal_cooling_device_update(struc
>   
>   unlock:
>   	mutex_unlock(&cdev->lock);
> -
> -unlock_list:
> -	mutex_unlock(&thermal_list_lock);
>   }
>   EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
>   
> @@ -1281,24 +1276,18 @@ static void thermal_zone_cdev_unbind(str
>   static bool thermal_cooling_device_exit(struct thermal_cooling_device *cdev)
>   {
>   	struct thermal_zone_device *tz;
> -	bool ret = true;
>   
> -	mutex_lock(&thermal_list_lock);
> +	guard(mutex)(&thermal_list_lock);
>   
> -	if (!thermal_cooling_device_present(cdev)) {
> -		ret = false;
> -		goto unlock;
> -	}
> +	if (!thermal_cooling_device_present(cdev))
> +		return false;
>   
>   	list_del(&cdev->node);
>   
>   	list_for_each_entry(tz, &thermal_tz_list, node)
>   		thermal_zone_cdev_unbind(tz, cdev);
>   
> -unlock:
> -	mutex_unlock(&thermal_list_lock);
> -
> -	return ret;
> +	return true;
>   }
>   
>   /**
> @@ -1345,7 +1334,7 @@ static void thermal_zone_init_complete(s
>   {
>   	struct thermal_cooling_device *cdev;
>   
> -	mutex_lock(&thermal_list_lock);
> +	guard(mutex)(&thermal_list_lock);
>   
>   	list_add_tail(&tz->node, &thermal_tz_list);
>   
> @@ -1365,8 +1354,6 @@ static void thermal_zone_init_complete(s
>   		tz->state |= TZ_STATE_FLAG_SUSPENDED;
>   
>   	__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> -
> -	mutex_unlock(&thermal_list_lock);
>   }
>   
>   /**
> @@ -1587,14 +1574,11 @@ EXPORT_SYMBOL_GPL(thermal_zone_device);
>   static bool thermal_zone_exit(struct thermal_zone_device *tz)
>   {
>   	struct thermal_cooling_device *cdev;
> -	bool ret = true;
>   
> -	mutex_lock(&thermal_list_lock);
> +	guard(mutex)(&thermal_list_lock);
>   
> -	if (list_empty(&tz->node)) {
> -		ret = false;
> -		goto unlock;
> -	}
> +	if (list_empty(&tz->node))
> +		return false;
>   
>   	guard(thermal_zone)(tz);
>   
> @@ -1605,10 +1589,7 @@ static bool thermal_zone_exit(struct the
>   	list_for_each_entry(cdev, &thermal_cdev_list, node)
>   		__thermal_zone_cdev_unbind(tz, cdev);
>   
> -unlock:
> -	mutex_unlock(&thermal_list_lock);
> -
> -	return ret;
> +	return true;
>   }
>   
>   /**
> @@ -1660,24 +1641,23 @@ struct thermal_zone_device *thermal_zone
>   	unsigned int found = 0;
>   
>   	if (!name)
> -		goto exit;
> +		return ERR_PTR(-EINVAL);
> +
> +	guard(mutex)(&thermal_list_lock);
>   
> -	mutex_lock(&thermal_list_lock);
>   	list_for_each_entry(pos, &thermal_tz_list, node)
>   		if (!strncasecmp(name, pos->type, THERMAL_NAME_LENGTH)) {
>   			found++;
>   			ref = pos;
>   		}
> -	mutex_unlock(&thermal_list_lock);
>   
> -	/* nothing has been found, thus an error code for it */
> -	if (found == 0)
> -		ref = ERR_PTR(-ENODEV);
> -	else if (found > 1)
> -	/* Success only when an unique zone is found */
> -		ref = ERR_PTR(-EEXIST);
> +	if (!found)
> +		return ERR_PTR(-ENODEV);
> +
> +	/* Success only when one zone is found. */
> +	if (found > 1)
> +		return ERR_PTR(-EEXIST);
>   
> -exit:
>   	return ref;
>   }
>   EXPORT_SYMBOL_GPL(thermal_zone_get_zone_by_name);
> @@ -1718,6 +1698,18 @@ static void thermal_zone_pm_prepare(stru
>   	tz->state |= TZ_STATE_FLAG_SUSPENDED;
>   }
>   
> +static void thermal_pm_notify_prepare(void)
> +{
> +	struct thermal_zone_device *tz;
> +
> +	guard(mutex)(&thermal_list_lock);
> +
> +	thermal_pm_suspended = true;
> +
> +	list_for_each_entry(tz, &thermal_tz_list, node)
> +		thermal_zone_pm_prepare(tz);
> +}
> +
>   static void thermal_zone_pm_complete(struct thermal_zone_device *tz)
>   {
>   	guard(thermal_zone)(tz);
> @@ -1736,35 +1728,31 @@ static void thermal_zone_pm_complete(str
>   	mod_delayed_work(system_freezable_power_efficient_wq, &tz->poll_queue, 0);
>   }
>   
> -static int thermal_pm_notify(struct notifier_block *nb,
> -			     unsigned long mode, void *_unused)
> +static void thermal_pm_notify_complete(void)
>   {
>   	struct thermal_zone_device *tz;
>   
> +	guard(mutex)(&thermal_list_lock);
> +
> +	thermal_pm_suspended = false;
> +
> +	list_for_each_entry(tz, &thermal_tz_list, node)
> +		thermal_zone_pm_complete(tz);
> +}
> +
> +static int thermal_pm_notify(struct notifier_block *nb,
> +			     unsigned long mode, void *_unused)
> +{
>   	switch (mode) {
>   	case PM_HIBERNATION_PREPARE:
>   	case PM_RESTORE_PREPARE:
>   	case PM_SUSPEND_PREPARE:
> -		mutex_lock(&thermal_list_lock);
> -
> -		thermal_pm_suspended = true;
> -
> -		list_for_each_entry(tz, &thermal_tz_list, node)
> -			thermal_zone_pm_prepare(tz);
> -
> -		mutex_unlock(&thermal_list_lock);
> +		thermal_pm_notify_prepare();
>   		break;
>   	case PM_POST_HIBERNATION:
>   	case PM_POST_RESTORE:
>   	case PM_POST_SUSPEND:
> -		mutex_lock(&thermal_list_lock);
> -
> -		thermal_pm_suspended = false;
> -
> -		list_for_each_entry(tz, &thermal_tz_list, node)
> -			thermal_zone_pm_complete(tz);
> -
> -		mutex_unlock(&thermal_list_lock);
> +		thermal_pm_notify_complete();
>   		break;
>   	default:
>   		break;
> 
> 
> 

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