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

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

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>