Received-SPF: pass (google.com: domain of linux-kernel+bounces-27158-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <d824d351-b1b1-46e3-86ac-f4a6b42c89fc@linaro.org>
Date: Tue, 16 Jan 2024 10:14:18 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 26/26] thermal: Introduce thermal zones names
Content-Language: en-US
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: rafael@kernel.org, rui.zhang@intel.com, lukasz.luba@arm.com,
 linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, kernel@collabora.com
References: <20231221124825.149141-1-angelogioacchino.delregno@collabora.com>
 <20231221124825.149141-27-angelogioacchino.delregno@collabora.com>
From: Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <20231221124825.149141-27-angelogioacchino.delregno@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 21/12/2023 13:48, AngeloGioacchino Del Regno wrote:
> Currently thermal zones have a "type" but this is often used, and
> referenced, as a name instead in multiple kernel drivers that are
> either registering a zone or grabbing a thermal zone handle and
> unfortunately this is a kind of abuse/misuse of the thermal zone
> concept of "type".
> 
> In order to disambiguate name<->type and to actually provide an
> accepted way of giving a specific name to a thermal zone for both
> platform drivers and devicetree-defined zones, add a new "name"
> member in the main thermal_zone_device structure, and also to the
> thermal_zone_device_params structure which is used to register a
> thermal zone device.
> 
> This will enforce the following constraints:
>   - Multiple thermal zones may be of the same "type" (no change);
>   - A thermal zone may have a *unique* name: trying to register
>     a new zone with the same name as an already present one will
>     produce a failure;
> ---
>   drivers/thermal/thermal_core.c  | 34 ++++++++++++++++++++++++++++++---
>   drivers/thermal/thermal_of.c    |  1 +
>   drivers/thermal/thermal_sysfs.c |  9 +++++++++
>   drivers/thermal/thermal_trace.h | 17 +++++++++++------
>   include/linux/thermal.h         |  4 ++++
>   5 files changed, 56 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 9eb0200a85ff..adf2ac8113e1 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1238,8 +1238,8 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>   struct thermal_zone_device *thermal_zone_device_register(struct thermal_zone_device_params *tzdp)
>   {
>   	struct thermal_zone_device *tz;
> -	int id;
> -	int result;
> +	int id, tz_name_len;
> +	int result = 0;
>   	struct thermal_governor *governor;
>   
>   	if (!tzdp->type || strlen(tzdp->type) == 0) {
> @@ -1248,11 +1248,36 @@ struct thermal_zone_device *thermal_zone_device_register(struct thermal_zone_dev
>   	}
>   
>   	if (strlen(tzdp->type) >= THERMAL_NAME_LENGTH) {
> -		pr_err("Thermal zone name (%s) too long, should be under %d chars\n",
> +		pr_err("Thermal zone type (%s) too long, should be under %d chars\n",
>   		       tzdp->type, THERMAL_NAME_LENGTH);
>   		return ERR_PTR(-EINVAL);

I would keep that as is and do second round of changes to clarify the 
usage of ->type

>   	}
>   
> +	tz_name_len = tzdp->name ? strlen(tzdp->name) : 0;

I suggest to change to a const char * and no longer limit to 
THERMAL_NAME_LENGTH.

> +	if (tz_name_len) {
> +		struct thermal_zone_device *pos;
> +
> +		if (tz_name_len >= THERMAL_NAME_LENGTH) {
> +			pr_err("Thermal zone name (%s) too long, should be under %d chars\n",
> +			       tzdp->name, THERMAL_NAME_LENGTH);
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		mutex_lock(&thermal_list_lock);
> +		list_for_each_entry(pos, &thermal_tz_list, node)
> +			if (!strncasecmp(tzdp->name, pos->name, THERMAL_NAME_LENGTH)) {
> +				result = -EEXIST;
> +				break;
> +			}
> +		mutex_unlock(&thermal_list_lock);
> +
> +		if (result) {
> +			pr_err("Thermal zone name (%s) already exists and must be unique\n",
> +			       tzdp->name);
> +			return ERR_PTR(result);
> +		}

Perhaps a lookup function would be more adequate. What about reusing 
thermal_zone_get_by_name() and search with tz->name if it is !NULL, 
tz->type otherwise ?

> +	}
> +
>   	/*
>   	 * Max trip count can't exceed 31 as the "mask >> num_trips" condition.
>   	 * For example, shifting by 32 will result in compiler warning:
> @@ -1307,6 +1332,9 @@ struct thermal_zone_device *thermal_zone_device_register(struct thermal_zone_dev
>   	tz->id = id;
>   	strscpy(tz->type, tzdp->type, sizeof(tz->type));
>   
> +	if (tz_name_len)
> +		strscpy(tz->name, tzdp->name, sizeof(tzdp->name));
> +
>   	if (!tzdp->ops->critical)
>   		tzdp->ops->critical = thermal_zone_device_critical;
>   
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 62a903ad649f..eaacc140abeb 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -486,6 +486,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
>   		ret = PTR_ERR(np);
>   		goto out_kfree_of_ops;
>   	}
> +	tzdp.name = np->name;
>   	tzdp.type = np->name;
>   
>   	tzdp.trips = thermal_of_trips_init(np, &tzdp.num_trips);
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index f52af8a3b4b5..f4002fa6caa2 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -23,6 +23,14 @@
>   
>   /* sys I/F for thermal zone */
>   
> +static ssize_t
> +name_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct thermal_zone_device *tz = to_thermal_zone(dev);
> +
> +	return sprintf(buf, "%s\n", tz->name);
> +}
> +
>   static ssize_t
>   type_show(struct device *dev, struct device_attribute *attr, char *buf)
>   {
> @@ -341,6 +349,7 @@ create_s32_tzp_attr(offset);
>    * All the attributes created for tzp (create_s32_tzp_attr) also are always
>    * present on the sysfs interface.
>    */
> +static DEVICE_ATTR_RO(name);
>   static DEVICE_ATTR_RO(type);
>   static DEVICE_ATTR_RO(temp);
>   static DEVICE_ATTR_RW(policy);
> diff --git a/drivers/thermal/thermal_trace.h b/drivers/thermal/thermal_trace.h
> index 459c8ce6cf3b..c9dbae1e3b9e 100644
> --- a/drivers/thermal/thermal_trace.h
> +++ b/drivers/thermal/thermal_trace.h
> @@ -28,6 +28,7 @@ TRACE_EVENT(thermal_temperature,
>   	TP_ARGS(tz),
>   
>   	TP_STRUCT__entry(
> +		__string(thermal_zone_name, tz->name)
>   		__string(thermal_zone, tz->type)
>   		__field(int, id)
>   		__field(int, temp_prev)
> @@ -35,15 +36,16 @@ TRACE_EVENT(thermal_temperature,
>   	),
>   
>   	TP_fast_assign(
> +		__assign_str(thermal_zone_name, tz->name);
>   		__assign_str(thermal_zone, tz->type);
>   		__entry->id = tz->id;
>   		__entry->temp_prev = tz->last_temperature;
>   		__entry->temp = tz->temperature;
>   	),
>   
> -	TP_printk("thermal_zone=%s id=%d temp_prev=%d temp=%d",
> -		__get_str(thermal_zone), __entry->id, __entry->temp_prev,
> -		__entry->temp)
> +	TP_printk("thermal_zone=%s name=%s id=%d temp_prev=%d temp=%d",
> +		  __get_str(thermal_zone), __get_str(thermal_zone_name),
> +		  __entry->id, __entry->temp_prev, __entry->temp)
>   );
>   
>   TRACE_EVENT(cdev_update,
> @@ -73,6 +75,7 @@ TRACE_EVENT(thermal_zone_trip,
>   	TP_ARGS(tz, trip, trip_type),
>   
>   	TP_STRUCT__entry(
> +		__string(thermal_zone_name, tz->name)
>   		__string(thermal_zone, tz->type)
>   		__field(int, id)
>   		__field(int, trip)
> @@ -80,15 +83,17 @@ TRACE_EVENT(thermal_zone_trip,
>   	),
>   
>   	TP_fast_assign(
> +		__assign_str(thermal_zone_name, tz->name);
>   		__assign_str(thermal_zone, tz->type);
>   		__entry->id = tz->id;
>   		__entry->trip = trip;
>   		__entry->trip_type = trip_type;
>   	),
>   
> -	TP_printk("thermal_zone=%s id=%d trip=%d trip_type=%s",
> -		__get_str(thermal_zone), __entry->id, __entry->trip,
> -		show_tzt_type(__entry->trip_type))
> +	TP_printk("thermal_zone=%s name=%s id=%d trip=%d trip_type=%s",
> +		  __get_str(thermal_zone), __get_str(thermal_zone_name),
> +		  __entry->id, __entry->trip,
> +		  show_tzt_type(__entry->trip_type))
>   );

For now, I think we can keep the traces as they are and keep passing the 
tz->type. Then we can replace tz->type by tz->name without changing the 
trace format.

>   #ifdef CONFIG_CPU_THERMAL
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 84b62575d93a..a06521eda162 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -115,6 +115,7 @@ struct thermal_cooling_device {
>   /**
>    * struct thermal_zone_device - structure for a thermal zone
>    * @id:		unique id number for each thermal zone
> + * @name:       name of the thermal zone device
>    * @type:	the thermal zone device type
>    * @device:	&struct device for this thermal zone
>    * @removal:	removal completion
> @@ -155,6 +156,7 @@ struct thermal_cooling_device {
>    */
>   struct thermal_zone_device {
>   	int id;
> +	char name[THERMAL_NAME_LENGTH];
>   	char type[THERMAL_NAME_LENGTH];
>   	struct device device;
>   	struct completion removal;
> @@ -260,6 +262,7 @@ struct thermal_zone_params {
>   
>   /**
>    * struct thermal_zone_device_params - parameters for a thermal zone device
> + * @name:		name of the thermal zone device
>    * @type:		the thermal zone device type
>    * @tzp:		thermal zone platform parameters
>    * @ops:		standard thermal zone device callbacks
> @@ -274,6 +277,7 @@ struct thermal_zone_params {
>    *			driven systems)
>    */
>   struct thermal_zone_device_params {
> +	const char *name;
>   	const char *type;
>   	struct thermal_zone_params tzp;
>   	struct thermal_zone_device_ops *ops;

-- 
<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