Re: [PATCH v2.1 09/11] thermal: core: Add and use cooling device guard

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

On 10/14/24 15:59, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Add and use a special guard for cooling devices.
> 
> This allows quite a few error code paths to be simplified among
> other things and brings in code size reduction for a good measure.
> 
> 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/1890654.atdPhlSkOF@rjwysocki.net/
> 
> v2 -> v2.1: Add missing hunk in trans_table_show()
> 
> v1 -> v2: Rearrange cur_state_store()
> 
> ---
>   drivers/thermal/gov_power_allocator.c |   21 ++++++--------
>   drivers/thermal/gov_step_wise.c       |    6 ++--
>   drivers/thermal/thermal_core.c        |   17 +++--------
>   drivers/thermal/thermal_debugfs.c     |   25 ++++++++++------
>   drivers/thermal/thermal_helpers.c     |   19 +++---------
>   drivers/thermal/thermal_sysfs.c       |   51 ++++++++++++----------------------
>   include/linux/thermal.h               |    3 ++
>   7 files changed, 59 insertions(+), 83 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -758,12 +758,10 @@ static int thermal_instance_add(struct t
>   
>   	list_add_tail(&new_instance->trip_node, &td->thermal_instances);
>   
> -	mutex_lock(&cdev->lock);
> +	guard(cooling_dev)(cdev);
>   
>   	list_add_tail(&new_instance->cdev_node, &cdev->thermal_instances);
>   
> -	mutex_unlock(&cdev->lock);
> -
>   	return 0;
>   }
>   
> @@ -872,11 +870,9 @@ static void thermal_instance_delete(stru
>   {
>   	list_del(&instance->trip_node);
>   
> -	mutex_lock(&instance->cdev->lock);
> +	guard(cooling_dev)(instance->cdev);
>   
>   	list_del(&instance->cdev_node);
> -
> -	mutex_unlock(&instance->cdev->lock);
>   }
>   
>   /**
> @@ -1239,10 +1235,10 @@ void thermal_cooling_device_update(struc
>   	 * Update under the cdev lock to prevent the state from being set beyond
>   	 * the new limit concurrently.
>   	 */
> -	mutex_lock(&cdev->lock);
> +	guard(cooling_dev)(cdev);
>   
>   	if (cdev->ops->get_max_state(cdev, &cdev->max_state))
> -		goto unlock;
> +		return;
>   
>   	thermal_cooling_device_stats_reinit(cdev);
>   
> @@ -1269,12 +1265,9 @@ void thermal_cooling_device_update(struc
>   	}
>   
>   	if (cdev->ops->get_cur_state(cdev, &state) || state > cdev->max_state)
> -		goto unlock;
> +		return;
>   
>   	thermal_cooling_device_stats_update(cdev, state);
> -
> -unlock:
> -	mutex_unlock(&cdev->lock);
>   }
>   EXPORT_SYMBOL_GPL(thermal_cooling_device_update);
>   
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -140,6 +140,9 @@ struct thermal_cooling_device {
>   #endif
>   };
>   
> +DEFINE_GUARD(cooling_dev, struct thermal_cooling_device *, mutex_lock(&_T->lock),
> +	     mutex_unlock(&_T->lock))
> +
>   /* Structure to define Thermal Zone parameters */
>   struct thermal_zone_params {
>   	const char *governor_name;
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -544,14 +544,15 @@ cur_state_store(struct device *dev, stru
>   	if (state > cdev->max_state)
>   		return -EINVAL;
>   
> -	mutex_lock(&cdev->lock);
> +	guard(cooling_dev)(cdev);
>   
>   	result = cdev->ops->set_cur_state(cdev, state);
> -	if (!result)
> -		thermal_cooling_device_stats_update(cdev, state);
> +	if (result)
> +		return result;
>   
> -	mutex_unlock(&cdev->lock);
> -	return result ? result : count;
> +	thermal_cooling_device_stats_update(cdev, state);
> +
> +	return count;
>   }
>   
>   static struct device_attribute
> @@ -625,21 +626,18 @@ static ssize_t total_trans_show(struct d
>   {
>   	struct thermal_cooling_device *cdev = to_cooling_device(dev);
>   	struct cooling_dev_stats *stats;
> -	int ret = 0;
> +	int ret;
>   
> -	mutex_lock(&cdev->lock);
> +	guard(cooling_dev)(cdev);
>   
>   	stats = cdev->stats;
>   	if (!stats)
> -		goto unlock;
> +		return 0;
>   
>   	spin_lock(&stats->lock);
>   	ret = sprintf(buf, "%u\n", stats->total_trans);
>   	spin_unlock(&stats->lock);
>   
> -unlock:
> -	mutex_unlock(&cdev->lock);
> -
>   	return ret;
>   }
>   
> @@ -652,11 +650,11 @@ time_in_state_ms_show(struct device *dev
>   	ssize_t len = 0;
>   	int i;
>   
> -	mutex_lock(&cdev->lock);
> +	guard(cooling_dev)(cdev);
>   
>   	stats = cdev->stats;
>   	if (!stats)
> -		goto unlock;
> +		return 0;
>   
>   	spin_lock(&stats->lock);
>   
> @@ -668,9 +666,6 @@ time_in_state_ms_show(struct device *dev
>   	}
>   	spin_unlock(&stats->lock);
>   
> -unlock:
> -	mutex_unlock(&cdev->lock);
> -
>   	return len;
>   }
>   
> @@ -682,11 +677,11 @@ reset_store(struct device *dev, struct d
>   	struct cooling_dev_stats *stats;
>   	int i, states;
>   
> -	mutex_lock(&cdev->lock);
> +	guard(cooling_dev)(cdev);
>   
>   	stats = cdev->stats;
>   	if (!stats)
> -		goto unlock;
> +		return count;
>   
>   	states = cdev->max_state + 1;
>   
> @@ -702,9 +697,6 @@ reset_store(struct device *dev, struct d
>   
>   	spin_unlock(&stats->lock);
>   
> -unlock:
> -	mutex_unlock(&cdev->lock);
> -
>   	return count;
>   }
>   
> @@ -716,13 +708,11 @@ static ssize_t trans_table_show(struct d
>   	ssize_t len = 0;
>   	int i, j;
>   
> -	mutex_lock(&cdev->lock);
> +	guard(cooling_dev)(cdev);
>   
>   	stats = cdev->stats;
> -	if (!stats) {
> -		len = -ENODATA;
> -		goto unlock;
> -	}
> +	if (!stats)
> +		return -ENODATA;
>   
>   	len += snprintf(buf + len, PAGE_SIZE - len, " From  :    To\n");
>   	len += snprintf(buf + len, PAGE_SIZE - len, "       : ");
> @@ -731,10 +721,8 @@ static ssize_t trans_table_show(struct d
>   			break;
>   		len += snprintf(buf + len, PAGE_SIZE - len, "state%2u  ", i);
>   	}
> -	if (len >= PAGE_SIZE) {
> -		len = PAGE_SIZE;
> -		goto unlock;
> -	}
> +	if (len >= PAGE_SIZE)
> +		return PAGE_SIZE;
>   
>   	len += snprintf(buf + len, PAGE_SIZE - len, "\n");
>   
> @@ -760,9 +748,6 @@ static ssize_t trans_table_show(struct d
>   		len = -EFBIG;
>   	}
>   
> -unlock:
> -	mutex_unlock(&cdev->lock);
> -
>   	return len;
>   }
>   
> Index: linux-pm/drivers/thermal/thermal_helpers.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_helpers.c
> +++ linux-pm/drivers/thermal/thermal_helpers.c
> @@ -58,17 +58,10 @@ bool thermal_trip_is_bound_to_cdev(struc
>   				   const struct thermal_trip *trip,
>   				   struct thermal_cooling_device *cdev)
>   {
> -	bool ret;
> -
>   	guard(thermal_zone)(tz);
> +	guard(cooling_dev)(cdev);
>   
> -	mutex_lock(&cdev->lock);
> -
> -	ret = thermal_instance_present(tz, cdev, trip);
> -
> -	mutex_unlock(&cdev->lock);
> -
> -	return ret;
> +	return thermal_instance_present(tz, cdev, trip);
>   }
>   EXPORT_SYMBOL_GPL(thermal_trip_is_bound_to_cdev);
>   
> @@ -197,12 +190,12 @@ void __thermal_cdev_update(struct therma
>    */
>   void thermal_cdev_update(struct thermal_cooling_device *cdev)
>   {
> -	mutex_lock(&cdev->lock);
> +	guard(cooling_dev)(cdev);
> +
>   	if (!cdev->updated) {
>   		__thermal_cdev_update(cdev);
>   		cdev->updated = true;
>   	}
> -	mutex_unlock(&cdev->lock);
>   }
>   
>   /**
> @@ -211,11 +204,9 @@ void thermal_cdev_update(struct thermal_
>    */
>   void thermal_cdev_update_nocheck(struct thermal_cooling_device *cdev)
>   {
> -	mutex_lock(&cdev->lock);
> +	guard(cooling_dev)(cdev);
>   
>   	__thermal_cdev_update(cdev);
> -
> -	mutex_unlock(&cdev->lock);
>   }
>   
>   /**
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -516,6 +516,19 @@ void thermal_debug_cdev_add(struct therm
>   	cdev->debugfs = thermal_dbg;
>   }
>   
> +static struct thermal_debugfs *thermal_debug_cdev_clear(struct thermal_cooling_device *cdev)
> +{
> +	struct thermal_debugfs *thermal_dbg;
> +
> +	guard(cooling_dev)(cdev);
> +
> +	thermal_dbg = cdev->debugfs;
> +	if (thermal_dbg)
> +		cdev->debugfs = NULL;
> +
> +	return thermal_dbg;
> +}
> +
>   /**
>    * thermal_debug_cdev_remove - Remove a cooling device debugfs entry
>    *
> @@ -527,17 +540,9 @@ void thermal_debug_cdev_remove(struct th
>   {
>   	struct thermal_debugfs *thermal_dbg;
>   
> -	mutex_lock(&cdev->lock);
> -
> -	thermal_dbg = cdev->debugfs;
> -	if (!thermal_dbg) {
> -		mutex_unlock(&cdev->lock);
> +	thermal_dbg = thermal_debug_cdev_clear(cdev);
> +	if (!thermal_dbg)
>   		return;
> -	}
> -
> -	cdev->debugfs = NULL;
> -
> -	mutex_unlock(&cdev->lock);
>   
>   	mutex_lock(&thermal_dbg->lock);
>   
> Index: linux-pm/drivers/thermal/gov_power_allocator.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_power_allocator.c
> +++ linux-pm/drivers/thermal/gov_power_allocator.c
> @@ -549,18 +549,17 @@ static void allow_maximum_power(struct t
>   		cdev = instance->cdev;
>   
>   		instance->target = 0;
> -		mutex_lock(&cdev->lock);
> -		/*
> -		 * Call for updating the cooling devices local stats and avoid
> -		 * periods of dozen of seconds when those have not been
> -		 * maintained.
> -		 */
> -		cdev->ops->get_requested_power(cdev, &req_power);
> +		scoped_guard(cooling_dev, cdev) {
> +			/*
> +			 * Call for updating the cooling devices local stats and
> +			 * avoid periods of dozen of seconds when those have not
> +			 * been maintained.
> +			 */
> +			cdev->ops->get_requested_power(cdev, &req_power);
>   
> -		if (params->update_cdevs)
> -			__thermal_cdev_update(cdev);
> -
> -		mutex_unlock(&cdev->lock);
> +			if (params->update_cdevs)
> +				__thermal_cdev_update(cdev);
> +		}
>   	}
>   }
>   
> Index: linux-pm/drivers/thermal/gov_step_wise.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/gov_step_wise.c
> +++ linux-pm/drivers/thermal/gov_step_wise.c
> @@ -97,9 +97,9 @@ static void thermal_zone_trip_update(str
>   
>   		instance->initialized = true;
>   
> -		mutex_lock(&instance->cdev->lock);
> -		instance->cdev->updated = false; /* cdev needs update */
> -		mutex_unlock(&instance->cdev->lock);
> +		scoped_guard(cooling_dev, instance->cdev) {
> +			instance->cdev->updated = false; /* cdev needs update */
> +		}
>   	}
>   }
>   
> 
> 
> 

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