Re: [PATCH v2 2/9] thermal: core: Add flags to struct thermal_trip

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 12/02/2024 19:31, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In order to allow thermal zone creators to specify the writability of
> trip point temperature and hysteresis on a per-trip basis, add a flags
> field to struct thermal_trip and define flags to represent the desired
> trip properties.
> 
> Also make thermal_zone_device_register_with_trips() set the
> THERMAL_TRIP_FLAG_RW_TEMP flag for all trips covered by the writable
> trips mask passed to it and modify the thermal sysfs code to look at
> the trip flags instead of using the writable trips mask directly or
> checking the presence of the .set_trip_hyst() zone callback.
> 
> Additionally, make trip_point_temp_store() and trip_point_hyst_store()
> fail with an error code if the trip passed to one of them has
> THERMAL_TRIP_FLAG_RW_TEMP or THERMAL_TRIP_FLAG_RW_HYST,
> respectively, clear in its flags.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v1 -> v2:
>     * Rename trip flags (Stanislaw).
> 
> ---
>   drivers/thermal/thermal_core.c  |   12 +++++++++++-
>   drivers/thermal/thermal_core.h  |    2 +-
>   drivers/thermal/thermal_sysfs.c |   28 +++++++++++++++++++---------
>   include/linux/thermal.h         |    7 +++++++
>   4 files changed, 38 insertions(+), 11 deletions(-)
> 
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -64,15 +64,23 @@ enum thermal_notify_event {
>    * @threshold: trip crossing notification threshold miliCelsius
>    * @type: trip point type
>    * @priv: pointer to driver data associated with this trip
> + * @flags: flags representing binary properties of the trip
>    */
>   struct thermal_trip {
>   	int temperature;
>   	int hysteresis;
>   	int threshold;
>   	enum thermal_trip_type type;
> +	u8 flags;
>   	void *priv;
>   };
>   
> +#define THERMAL_TRIP_FLAG_RW_TEMP	BIT(0)
> +#define THERMAL_TRIP_FLAG_RW_HYST	BIT(1)
> +
> +#define THERMAL_TRIP_FLAG_MASK_RW	(THERMAL_TRIP_FLAG_RW_TEMP | \
> +					 THERMAL_TRIP_FLAG_RW_HYST)

What about THERMAL_TRIP_FLAG_RW instead ?

>   struct thermal_zone_device_ops {
>   	int (*bind) (struct thermal_zone_device *,
>   		     struct thermal_cooling_device *);
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1356,13 +1356,23 @@ thermal_zone_device_register_with_trips(
>   	tz->devdata = devdata;
>   	tz->trips = trips;
>   	tz->num_trips = num_trips;
> +	if (num_trips > 0) {

Is this check really necessary? for_each_trip() should exit immediately 
if there is no trip points.

> +		struct thermal_trip *trip;
> +
> +		for_each_trip(tz, trip) {
> +			if (mask & 1)
> +				trip->flags |= THERMAL_TRIP_FLAG_RW_TEMP;
> +
> +			mask >>= 1;
> +		}
> +	}
>   
>   	thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
>   	thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
>   
>   	/* sys I/F */
>   	/* Add nodes that are always present via .groups */
> -	result = thermal_zone_create_device_groups(tz, mask);
> +	result = thermal_zone_create_device_groups(tz);
>   	if (result)
>   		goto remove_id;
>   
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -131,7 +131,7 @@ void thermal_zone_trip_updated(struct th
>   int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
>   
>   /* sysfs I/F */
> -int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
> +int thermal_zone_create_device_groups(struct thermal_zone_device *tz);
>   void thermal_zone_destroy_device_groups(struct thermal_zone_device *);
>   void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
>   void thermal_cooling_device_destroy_sysfs(struct thermal_cooling_device *cdev);
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -122,6 +122,11 @@ trip_point_temp_store(struct device *dev
>   
>   	trip = &tz->trips[trip_id];
>   
> +	if (!(trip->flags & THERMAL_TRIP_FLAG_RW_TEMP)) {
> +		ret = -EPERM;
> +		goto unlock;
> +	}

Does it really happen?

If the sysfs file is created with the right permission regarding the 
trip->flags then this condition can never be true.

>   	if (temp != trip->temperature) {
>   		if (tz->ops->set_trip_temp) {
>   			ret = tz->ops->set_trip_temp(tz, trip_id, temp);
> @@ -173,6 +178,11 @@ trip_point_hyst_store(struct device *dev
>   
>   	trip = &tz->trips[trip_id];
>   
> +	if (!(trip->flags & THERMAL_TRIP_FLAG_RW_HYST)) {
> +		ret = -EPERM;
> +		goto unlock;
> +	}

Ditto

>   	if (hyst != trip->hysteresis) {
>   		if (tz->ops->set_trip_hyst) {
>   			ret = tz->ops->set_trip_hyst(tz, trip_id, hyst);
> @@ -392,17 +402,16 @@ static const struct attribute_group *the
>   /**
>    * create_trip_attrs() - create attributes for trip points
>    * @tz:		the thermal zone device
> - * @mask:	Writeable trip point bitmap.
>    *
>    * helper function to instantiate sysfs entries for every trip
>    * point and its properties of a struct thermal_zone_device.
>    *
>    * Return: 0 on success, the proper error value otherwise.
>    */
> -static int create_trip_attrs(struct thermal_zone_device *tz, int mask)
> +static int create_trip_attrs(struct thermal_zone_device *tz)
>   {
> +	const struct thermal_trip *trip;
>   	struct attribute **attrs;
> -	int indx;
>   
>   	/* This function works only for zones with at least one trip */
>   	if (tz->num_trips <= 0)
> @@ -437,7 +446,9 @@ static int create_trip_attrs(struct ther
>   		return -ENOMEM;
>   	}
>   
> -	for (indx = 0; indx < tz->num_trips; indx++) {
> +	for_each_trip(tz, trip) {
> +		int indx = thermal_zone_trip_id(tz, trip);
> +
>   		/* create trip type attribute */
>   		snprintf(tz->trip_type_attrs[indx].name, THERMAL_NAME_LENGTH,
>   			 "trip_point_%d_type", indx);
> @@ -458,7 +469,7 @@ static int create_trip_attrs(struct ther
>   						tz->trip_temp_attrs[indx].name;
>   		tz->trip_temp_attrs[indx].attr.attr.mode = S_IRUGO;
>   		tz->trip_temp_attrs[indx].attr.show = trip_point_temp_show;
> -		if (mask & (1 << indx)) {
> +		if (trip->flags & THERMAL_TRIP_FLAG_RW_TEMP) {
>   			tz->trip_temp_attrs[indx].attr.attr.mode |= S_IWUSR;
>   			tz->trip_temp_attrs[indx].attr.store =
>   							trip_point_temp_store;
> @@ -473,7 +484,7 @@ static int create_trip_attrs(struct ther
>   					tz->trip_hyst_attrs[indx].name;
>   		tz->trip_hyst_attrs[indx].attr.attr.mode = S_IRUGO;
>   		tz->trip_hyst_attrs[indx].attr.show = trip_point_hyst_show;
> -		if (tz->ops->set_trip_hyst) {
> +		if (trip->flags & THERMAL_TRIP_FLAG_RW_HYST) {
>   			tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
>   			tz->trip_hyst_attrs[indx].attr.store =
>   					trip_point_hyst_store;
> @@ -505,8 +516,7 @@ static void destroy_trip_attrs(struct th
>   	kfree(tz->trips_attribute_group.attrs);
>   }
>   
> -int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
> -				      int mask)
> +int thermal_zone_create_device_groups(struct thermal_zone_device *tz)
>   {
>   	const struct attribute_group **groups;
>   	int i, size, result;
> @@ -522,7 +532,7 @@ int thermal_zone_create_device_groups(st
>   		groups[i] = thermal_zone_attribute_groups[i];
>   
>   	if (tz->num_trips) {
> -		result = create_trip_attrs(tz, mask);
> +		result = create_trip_attrs(tz);
>   		if (result) {
>   			kfree(groups);
>   
> 
> 
> 

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog