diff mbox series

[RFC,26/26] thermal: Introduce thermal zones names

Message ID 20231221124825.149141-27-angelogioacchino.delregno@collabora.com (mailing list archive)
State RFC, archived
Headers show
Series Add thermal zones names and new registration func | expand

Commit Message

AngeloGioacchino Del Regno Dec. 21, 2023, 12:48 p.m. UTC
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(-)

Comments

Daniel Lezcano Jan. 16, 2024, 9:14 a.m. UTC | #1
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;
AngeloGioacchino Del Regno Jan. 16, 2024, 9:45 a.m. UTC | #2
Il 16/01/24 10:14, Daniel Lezcano ha scritto:
> 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
> 

Problem is, if we keep this one as-is, then we'll have ambiguous error messages in
this series... unless we stop limiting the tz type string to THERMAL_NAME_LENGTH
as well as not limit the tz name to that...

>>       }
>> +    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.

...and to be completely honest, I actually like the THERMAL_NAME_LENGTH limitation,
because this both limits the length of the related sysfs file and avoids prolonging
error messages with very-very-very long type-strings and name-strings.

What I have in my head is:

imagine having a thermal zone named "cpu-little-bottom-right-core0" and a driver
named "qualcomm-power-controller" (which doesn't exist, but there are drivers with
pretty long names in the kernel anyway).

Kernel logging *may* have very long strings, decreasing readability:
[ 0.0000100 ] qualcomm-power-controller: cpu-little-bottom-right-core0: Failed to 
read thermal zone temperature -22

And sysfs would have something like
cpu-little-top-right-core0 cpu-little-top-left-core0 cpu-little-bottom-right-core0

While sysfs could be ok, are we sure that allowing such long names is a good idea?

It's a genuine question - I don't really have more than just cosmetic reasons and
those are just only personal perspectives....

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

Okay yes that makes a lot of sense, and also breaks some of my brain loops around
making the migration a bit less painful.

Nice one! Will do :-D

>> +    }
>> +
>>       /*
>>        * 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.
> 

We can but, as a personal consideration, this looks "more complete" - as in - we
are not dropping the thermal zone *type* concept anyway (even though we're doing
"something else" with it), so including both type and name in tracing is useful
to whoever is trying to debug something.

If you have strong opinions against, though, it literally takes 30 seconds for me
to just remove that part and there's no problem in doing so!

Cheers,
Angelo

>>   #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;
>
Daniel Lezcano Jan. 16, 2024, 11:30 a.m. UTC | #3
On 16/01/2024 10:45, AngeloGioacchino Del Regno wrote:
> Il 16/01/24 10:14, Daniel Lezcano ha scritto:
>> 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
>>
> 
> Problem is, if we keep this one as-is, then we'll have ambiguous error 
> messages in
> this series... unless we stop limiting the tz type string to 
> THERMAL_NAME_LENGTH
> as well as not limit the tz name to that...

I'm missing the point, how can it be more ambiguous if the message is 
unchanged ?

>>>       }
>>> +    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.
> 
> ...and to be completely honest, I actually like the THERMAL_NAME_LENGTH 
> limitation,
> because this both limits the length of the related sysfs file and avoids 
> prolonging
> error messages with very-very-very long type-strings and name-strings.
> 
> What I have in my head is:
> 
> imagine having a thermal zone named "cpu-little-bottom-right-core0" and 
> a driver
> named "qualcomm-power-controller" (which doesn't exist, but there are 
> drivers with
> pretty long names in the kernel anyway).
> 
> Kernel logging *may* have very long strings, decreasing readability:
> [ 0.0000100 ] qualcomm-power-controller: cpu-little-bottom-right-core0: 
> Failed to read thermal zone temperature -22
> 
> And sysfs would have something like
> cpu-little-top-right-core0 cpu-little-top-left-core0 
> cpu-little-bottom-right-core0
> 
> While sysfs could be ok, are we sure that allowing such long names is a 
> good idea?
> 
> It's a genuine question - I don't really have more than just cosmetic 
> reasons and
> those are just only personal perspectives....

IMO, it is up to the programmer to choose a convenient name.

If the traces are giving unreadable output, then someone will send a 
patch to change the name to something more readable.

In addition, having array in structure to be passed as parameter is not 
good because of the limited stack size in the kernel.


>>> +    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 ?
>>
> 
> Okay yes that makes a lot of sense, and also breaks some of my brain 
> loops around
> making the migration a bit less painful.
> 
> Nice one! Will do :-D
> 
>>> +    }
>>> +
>>>       /*
>>>        * 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.
>>
> 
> We can but, as a personal consideration, this looks "more complete" - as 
> in - we
> are not dropping the thermal zone *type* concept anyway (even though 
> we're doing
> "something else" with it), so including both type and name in tracing is 
> useful
> to whoever is trying to debug something.
> 
> If you have strong opinions against, though, it literally takes 30 
> seconds for me
> to just remove that part and there's no problem in doing so!

Yes, just drop it for now. We will sort it out after.
AngeloGioacchino Del Regno Jan. 16, 2024, 11:37 a.m. UTC | #4
Il 16/01/24 12:30, Daniel Lezcano ha scritto:
> On 16/01/2024 10:45, AngeloGioacchino Del Regno wrote:
>> Il 16/01/24 10:14, Daniel Lezcano ha scritto:
>>> 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
>>>
>>
>> Problem is, if we keep this one as-is, then we'll have ambiguous error messages in
>> this series... unless we stop limiting the tz type string to THERMAL_NAME_LENGTH
>> as well as not limit the tz name to that...
> 
> I'm missing the point, how can it be more ambiguous if the message is unchanged ?
> 

Because:

        if (strlen(tzdp->type) >= THERMAL_NAME_LENGTH) {
         pr_err("Thermal zone name (%s) too long, should be under %d chars\n",

.......

  +        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);
  +        }

...but anyway, since we're removing the THERMAL_NAME_LENGTH limit, those messages
are going away with it - this means that this is not a problem anymore.

Nevermind :-)

>>>>       }
>>>> +    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.
>>
>> ...and to be completely honest, I actually like the THERMAL_NAME_LENGTH limitation,
>> because this both limits the length of the related sysfs file and avoids prolonging
>> error messages with very-very-very long type-strings and name-strings.
>>
>> What I have in my head is:
>>
>> imagine having a thermal zone named "cpu-little-bottom-right-core0" and a driver
>> named "qualcomm-power-controller" (which doesn't exist, but there are drivers with
>> pretty long names in the kernel anyway).
>>
>> Kernel logging *may* have very long strings, decreasing readability:
>> [ 0.0000100 ] qualcomm-power-controller: cpu-little-bottom-right-core0: Failed to 
>> read thermal zone temperature -22
>>
>> And sysfs would have something like
>> cpu-little-top-right-core0 cpu-little-top-left-core0 cpu-little-bottom-right-core0
>>
>> While sysfs could be ok, are we sure that allowing such long names is a good idea?
>>
>> It's a genuine question - I don't really have more than just cosmetic reasons and
>> those are just only personal perspectives....
> 
> IMO, it is up to the programmer to choose a convenient name.
> 
> If the traces are giving unreadable output, then someone will send a patch to 
> change the name to something more readable.
> 
> In addition, having array in structure to be passed as parameter is not good 
> because of the limited stack size in the kernel.
> 
> 

Okay, will do then.

>>>> +    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 ?
>>>
>>
>> Okay yes that makes a lot of sense, and also breaks some of my brain loops around
>> making the migration a bit less painful.
>>
>> Nice one! Will do :-D
>>
>>>> +    }
>>>> +
>>>>       /*
>>>>        * 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.
>>>
>>
>> We can but, as a personal consideration, this looks "more complete" - as in - we
>> are not dropping the thermal zone *type* concept anyway (even though we're doing
>> "something else" with it), so including both type and name in tracing is useful
>> to whoever is trying to debug something.
>>
>> If you have strong opinions against, though, it literally takes 30 seconds for me
>> to just remove that part and there's no problem in doing so!
> 
> Yes, just drop it for now. We will sort it out after.
> 

Ok I'll drop this part!
diff mbox series

Patch

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);
 	}
 
+	tz_name_len = tzdp->name ? strlen(tzdp->name) : 0;
+	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);
+		}
+	}
+
 	/*
 	 * 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))
 );
 
 #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;