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

[Lukasz Luba <lukasz.luba@arm.com>]

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>