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