Re: [PATCH v2 08/12] thermal: core: Consolidate thermal zone locking in the exit path

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

On 10/4/24 20:30, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In analogy with a previous change in the thermal zone initialization
> path, to avoid acquiring the thermal zone lock and releasing it multiple
> times back and forth unnecessarily, move all of the code running under
> thermal_list_lock in thermal_zone_device_unregister() into a new
> function called thermal_zone_exit() and make the latter acquire the
> thermal zone lock only once and release it along with thermal_list_lock.
> 
> For this purpose, provide an "unlocked" variant of
> thermal_zone_cdev_unbind() to be called by thermal_zone_exit() under the
> thermal zone lock.
> 
> 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/7729094.EvYhyI6sBW@rjwysocki.net/
> 
> v1 -> v2: Rebase, use list_del_init() for removing thermal zones from the
>            global list so that the new list_emty() check doesn't blow up.
> 
> ---
>   drivers/thermal/thermal_core.c |   66 ++++++++++++++++++++++++-----------------
>   1 file changed, 39 insertions(+), 27 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1266,15 +1266,21 @@ unlock_list:
>   }
>   EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
>   
> -static void thermal_zone_cdev_unbind(struct thermal_zone_device *tz,
> -				     struct thermal_cooling_device *cdev)
> +static void __thermal_zone_cdev_unbind(struct thermal_zone_device *tz,
> +				       struct thermal_cooling_device *cdev)
>   {
>   	struct thermal_trip_desc *td;
>   
> -	mutex_lock(&tz->lock);
> -
>   	for_each_trip_desc(tz, td)
>   		thermal_unbind_cdev_from_trip(tz, &td->trip, cdev);
> +}
> +
> +static void thermal_zone_cdev_unbind(struct thermal_zone_device *tz,
> +				     struct thermal_cooling_device *cdev)
> +{
> +	mutex_lock(&tz->lock);
> +
> +	__thermal_zone_cdev_unbind(tz, cdev);
>   
>   	mutex_unlock(&tz->lock);
>   }
> @@ -1588,43 +1594,49 @@ struct device *thermal_zone_device(struc
>   }
>   EXPORT_SYMBOL_GPL(thermal_zone_device);
>   
> -/**
> - * thermal_zone_device_unregister - removes the registered thermal zone device
> - * @tz: the thermal zone device to remove
> - */
> -void thermal_zone_device_unregister(struct thermal_zone_device *tz)
> +static bool thermal_zone_exit(struct thermal_zone_device *tz)
>   {
>   	struct thermal_cooling_device *cdev;
> -	struct thermal_zone_device *pos = NULL;
> -
> -	if (!tz)
> -		return;
> -
> -	thermal_debug_tz_remove(tz);
> +	bool ret = true;
>   
>   	mutex_lock(&thermal_list_lock);
> -	list_for_each_entry(pos, &thermal_tz_list, node)
> -		if (pos == tz)
> -			break;
> -	if (pos != tz) {
> -		/* thermal zone device not found */
> -		mutex_unlock(&thermal_list_lock);
> -		return;
> +
> +	if (list_empty(&tz->node)) {
> +		ret = false;
> +		goto unlock;
>   	}
>   
>   	mutex_lock(&tz->lock);
>   
>   	tz->state |= TZ_STATE_FLAG_EXIT;
> -	list_del(&tz->node);
> -
> -	mutex_unlock(&tz->lock);
> +	list_del_init(&tz->node);
>   
> -	/* Unbind all cdevs associated with 'this' thermal zone */
> +	/* Unbind all cdevs associated with this thermal zone. */
>   	list_for_each_entry(cdev, &thermal_cdev_list, node)
> -		thermal_zone_cdev_unbind(tz, cdev);
> +		__thermal_zone_cdev_unbind(tz, cdev);
> +
> +	mutex_unlock(&tz->lock);
>   
> +unlock:
>   	mutex_unlock(&thermal_list_lock);
>   
> +	return ret;
> +}
> +
> +/**
> + * thermal_zone_device_unregister - removes the registered thermal zone device
> + * @tz: the thermal zone device to remove
> + */
> +void thermal_zone_device_unregister(struct thermal_zone_device *tz)
> +{
> +	if (!tz)
> +		return;
> +
> +	thermal_debug_tz_remove(tz);
> +
> +	if (!thermal_zone_exit(tz))
> +		return;
> +
>   	cancel_delayed_work_sync(&tz->poll_queue);
>   
>   	thermal_set_governor(tz, NULL);
> 
> 
> 

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