[PATCH v2 0/3] thermal: hwmon: Rework of automatic hwmon device registration

[PATCH v2 0/3] thermal: hwmon: Rework of automatic hwmon device registration

[Rafael J. Wysocki]

Hi All,

This is an update of

https://lore.kernel.org/linux-pm/5100596.31r3eYUQgx@rafael.j.wysocki/

sent mosty to address feedback from sashiko.dev:

https://sashiko.dev/#/patchset/5100596.31r3eYUQgx%40rafael.j.wysocki

There is an extra patch in this revision (the first one) that addresses
a possible thermal hwmon removal issue pointed out by sashiko.dev.

The second patch in the series reworks the automatic registration of hwmon
devices for thermal zones so that one hwmon device is registered for each of
them.  This is done to address a thermal zone removal deadlock related to the
sharing of a hwmon device with other thermal zones of the same type (see the
changelog of patch [2/3] for details).

The last patch simplifies the thermal hwmon code further by using the
canonical mechanism for registering extra sysfs attributes of hwmon devices
instead of manually adding files to sysfs.

The patches are targeted at 7.2.

Thanks!

[PATCH v2 1/3] thermal: hwmon: Fix critical temperature attribute removal

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since the return value of thermal_zone_crit_temp_valid() depends on
the behavior of the thermal zone .get_crit_temp() callback which
may change over time in theory, thermal_remove_hwmon_sysfs() may
attempt to remove a critical temperature attribute that has not
been created, passing a pointer to an uninitialized attribute
structure to device_remove_file().

To avoid that, set a flag in struct thermal_hwmon_temp after creating
a critical temperature attribute and use the value of that flag to
decide whether or not the attribute needs to be removed.

Fixes: e8db5d6736a7 ("thermal: hwmon: Make the check for critical temp valid consistent")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2: New patch (due to sashiko.dev feedback)

---
 drivers/thermal/thermal_hwmon.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- a/drivers/thermal/thermal_hwmon.c
+++ b/drivers/thermal/thermal_hwmon.c
@@ -40,6 +40,7 @@ struct thermal_hwmon_temp {
 	struct thermal_zone_device *tz;
 	struct thermal_hwmon_attr temp_input;	/* hwmon sys attr */
 	struct thermal_hwmon_attr temp_crit;	/* hwmon sys attr */
+	bool temp_crit_present;
 };
 
 static LIST_HEAD(thermal_hwmon_list);
@@ -191,6 +192,8 @@ int thermal_add_hwmon_sysfs(struct therm
 					    &temp->temp_crit.attr);
 		if (result)
 			goto unregister_input;
+
+		temp->temp_crit_present = true;
 	}
 
 	mutex_lock(&thermal_hwmon_list_lock);
@@ -235,7 +238,7 @@ void thermal_remove_hwmon_sysfs(struct t
 	}
 
 	device_remove_file(hwmon->device, &temp->temp_input.attr);
-	if (thermal_zone_crit_temp_valid(tz))
+	if (temp->temp_crit_present)
 		device_remove_file(hwmon->device, &temp->temp_crit.attr);
 
 	mutex_lock(&thermal_hwmon_list_lock);

[PATCH v2 2/3] thermal: hwmon: Register a hwmon device for each thermal zone

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The current code creates one hwmon device per thermal zone type and that
device is registered under the first thermal zone of the given type.

That turns out to be problematic when the thermal zone holding the
hwmon device is removed.

For example, say that there are two ACPI thermal zones on a system

/sys/devices/virtual/thermal/thermal_zone0/
/sys/devices/virtual/thermal/thermal_zone1/

The current code registers a hwmon class device for thermal_zone0 only:

/sys/devices/virtual/thermal/thermal_zone0/hwmon0/

because the type is "acpitz" for both of them, but it adds a sysfs
attribute that belongs to thermal_zone1 under it:

/sys/devices/virtual/thermal/thermal_zone0/hwmon0/temp2_input

There is also

/sys/devices/virtual/thermal/thermal_zone0/hwmon0/temp1_input

which belongs to thermal_zone0.

When thermal_zone0 is removed, say because the ACPI thermal driver is
unbound from the underlying platform device, the removal code skips the
removal of hwmon0 because of the temp2_input attribute belonging to
thermal_zone1 which effectively prevents thermal_zone0 removal from
making progress.

To address this problem, rework the thermal hwmon code to register one
hwmon device for each thermal zone, but since user space utilities
produce confusing output in some cases when there are multiple hwmon
devices with the same name attribute value present under thermal zones
of the same type, append the thermal zone ID preceded by an underline
character to the name of the hwmon device registered for that thermal
zone.

Link: https://lore.kernel.org/linux-pm/20260402021828.16556-1-liujia6264@gmail.com/
Fixes: f6b6b52ef7a5 ("thermal_hwmon: Pass the originating device down to hwmon_device_register_with_info")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
   * Rebase on top of [1/3]
   * Subject update
   * Introduce THERMAL_HWMON_NAME_LENGTH (for hwmon device name attribute
     values) and avoid the truncation of thermal zone type when creating
     those attributes (sashiko.dev)
   * Drop hwmon_node (not necessary any more) from struct thermal_hwmon_temp
     (sashiko.dev)
   * Avoid mixing scoped_guard() with gotos in thermal_add_hwmon_sysfs()
     (sashiko.dev)

---

The reason why I have decided to take this approach is because it allows
the code to be simplified subsequently by using regular extra_groups for
hwmon instead of adding device attributes directly (see the next patch)
and going back to the state before commit f6b6b52ef7a5 is seriously
unattractive.  However, if it breaks user space for people, there really
won't be much choice.

---
 drivers/thermal/thermal_hwmon.c |  151 ++++++++++++----------------------------
 1 file changed, 47 insertions(+), 104 deletions(-)

--- a/drivers/thermal/thermal_hwmon.c
+++ b/drivers/thermal/thermal_hwmon.c
@@ -19,30 +19,33 @@
 #include "thermal_hwmon.h"
 #include "thermal_core.h"
 
-/* hwmon sys I/F */
-/* thermal zone devices with the same type share one hwmon device */
-struct thermal_hwmon_device {
-	char type[THERMAL_NAME_LENGTH];
-	struct device *device;
-	int count;
-	struct list_head tz_list;
-	struct list_head node;
-};
+/*
+ * Needs to be large enough to hold a thermal zone type string followed by an
+ * underline character and a 32-bit integer in decimal representation.
+ */
+#define THERMAL_HWMON_NAME_LENGTH (THERMAL_NAME_LENGTH + 11)
 
 struct thermal_hwmon_attr {
 	struct device_attribute attr;
-	char name[16];
 };
 
 /* one temperature input for each thermal zone */
 struct thermal_hwmon_temp {
-	struct list_head hwmon_node;
 	struct thermal_zone_device *tz;
 	struct thermal_hwmon_attr temp_input;	/* hwmon sys attr */
 	struct thermal_hwmon_attr temp_crit;	/* hwmon sys attr */
 	bool temp_crit_present;
 };
 
+/* hwmon sys I/F */
+/* thermal zone devices with the same type share one hwmon device */
+struct thermal_hwmon_device {
+	char name[THERMAL_HWMON_NAME_LENGTH];
+	struct device *device;
+	struct list_head node;
+	struct thermal_hwmon_temp tz_temp;
+};
+
 static LIST_HEAD(thermal_hwmon_list);
 
 static DEFINE_MUTEX(thermal_hwmon_list_lock);
@@ -88,45 +91,6 @@ temp_crit_show(struct device *dev, struc
 	return sysfs_emit(buf, "%d\n", temperature);
 }
 
-
-static struct thermal_hwmon_device *
-thermal_hwmon_lookup_by_type(const struct thermal_zone_device *tz)
-{
-	struct thermal_hwmon_device *hwmon;
-	char type[THERMAL_NAME_LENGTH];
-
-	mutex_lock(&thermal_hwmon_list_lock);
-	list_for_each_entry(hwmon, &thermal_hwmon_list, node) {
-		strscpy(type, tz->type);
-		strreplace(type, '-', '_');
-		if (!strcmp(hwmon->type, type)) {
-			mutex_unlock(&thermal_hwmon_list_lock);
-			return hwmon;
-		}
-	}
-	mutex_unlock(&thermal_hwmon_list_lock);
-
-	return NULL;
-}
-
-/* Find the temperature input matching a given thermal zone */
-static struct thermal_hwmon_temp *
-thermal_hwmon_lookup_temp(const struct thermal_hwmon_device *hwmon,
-			  const struct thermal_zone_device *tz)
-{
-	struct thermal_hwmon_temp *temp;
-
-	mutex_lock(&thermal_hwmon_list_lock);
-	list_for_each_entry(temp, &hwmon->tz_list, hwmon_node)
-		if (temp->tz == tz) {
-			mutex_unlock(&thermal_hwmon_list_lock);
-			return temp;
-		}
-	mutex_unlock(&thermal_hwmon_list_lock);
-
-	return NULL;
-}
-
 static bool thermal_zone_crit_temp_valid(struct thermal_zone_device *tz)
 {
 	int temp;
@@ -137,54 +101,39 @@ int thermal_add_hwmon_sysfs(struct therm
 {
 	struct thermal_hwmon_device *hwmon;
 	struct thermal_hwmon_temp *temp;
-	int new_hwmon_device = 1;
 	int result;
 
-	hwmon = thermal_hwmon_lookup_by_type(tz);
-	if (hwmon) {
-		new_hwmon_device = 0;
-		goto register_sys_interface;
-	}
-
 	hwmon = kzalloc_obj(*hwmon);
 	if (!hwmon)
 		return -ENOMEM;
 
-	INIT_LIST_HEAD(&hwmon->tz_list);
-	strscpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
-	strreplace(hwmon->type, '-', '_');
+	/*
+	 * Append the thermal zone ID preceded by an underline character to the
+	 * type to disambiguate the sensors command output.
+	 */
+	scnprintf(hwmon->name, THERMAL_HWMON_NAME_LENGTH, "%s_%d", tz->type, tz->id);
+	strreplace(hwmon->name, '-', '_');
 	hwmon->device = hwmon_device_register_for_thermal(&tz->device,
-							  hwmon->type, hwmon);
+							  hwmon->name, hwmon);
 	if (IS_ERR(hwmon->device)) {
 		result = PTR_ERR(hwmon->device);
 		goto free_mem;
 	}
 
- register_sys_interface:
-	temp = kzalloc_obj(*temp);
-	if (!temp) {
-		result = -ENOMEM;
-		goto unregister_name;
-	}
+	temp = &hwmon->tz_temp;
 
 	temp->tz = tz;
-	hwmon->count++;
 
-	snprintf(temp->temp_input.name, sizeof(temp->temp_input.name),
-		 "temp%d_input", hwmon->count);
-	temp->temp_input.attr.attr.name = temp->temp_input.name;
+	temp->temp_input.attr.attr.name = "temp1_input";
 	temp->temp_input.attr.attr.mode = 0444;
 	temp->temp_input.attr.show = temp_input_show;
 	sysfs_attr_init(&temp->temp_input.attr.attr);
 	result = device_create_file(hwmon->device, &temp->temp_input.attr);
 	if (result)
-		goto free_temp_mem;
+		goto unregister_name;
 
 	if (thermal_zone_crit_temp_valid(tz)) {
-		snprintf(temp->temp_crit.name,
-				sizeof(temp->temp_crit.name),
-				"temp%d_crit", hwmon->count);
-		temp->temp_crit.attr.attr.name = temp->temp_crit.name;
+		temp->temp_crit.attr.attr.name = "temp1_crit";
 		temp->temp_crit.attr.attr.mode = 0444;
 		temp->temp_crit.attr.show = temp_crit_show;
 		sysfs_attr_init(&temp->temp_crit.attr.attr);
@@ -196,21 +145,17 @@ int thermal_add_hwmon_sysfs(struct therm
 		temp->temp_crit_present = true;
 	}
 
+	/* The list is needed for hwmon lookup during removal. */
 	mutex_lock(&thermal_hwmon_list_lock);
-	if (new_hwmon_device)
-		list_add_tail(&hwmon->node, &thermal_hwmon_list);
-	list_add_tail(&temp->hwmon_node, &hwmon->tz_list);
+	list_add_tail(&hwmon->node, &thermal_hwmon_list);
 	mutex_unlock(&thermal_hwmon_list_lock);
 
 	return 0;
 
  unregister_input:
 	device_remove_file(hwmon->device, &temp->temp_input.attr);
- free_temp_mem:
-	kfree(temp);
  unregister_name:
-	if (new_hwmon_device)
-		hwmon_device_unregister(hwmon->device);
+	hwmon_device_unregister(hwmon->device);
  free_mem:
 	kfree(hwmon);
 
@@ -218,39 +163,37 @@ int thermal_add_hwmon_sysfs(struct therm
 }
 EXPORT_SYMBOL_GPL(thermal_add_hwmon_sysfs);
 
+static struct thermal_hwmon_device *
+thermal_hwmon_lookup(const struct thermal_zone_device *tz)
+{
+	struct thermal_hwmon_device *hwmon;
+
+	list_for_each_entry(hwmon, &thermal_hwmon_list, node) {
+		if (hwmon->tz_temp.tz == tz)
+			return hwmon;
+	}
+	return NULL;
+}
+
 void thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
 {
 	struct thermal_hwmon_device *hwmon;
 	struct thermal_hwmon_temp *temp;
 
-	hwmon = thermal_hwmon_lookup_by_type(tz);
-	if (unlikely(!hwmon)) {
-		/* Should never happen... */
-		dev_dbg(&tz->device, "hwmon device lookup failed!\n");
-		return;
-	}
+	scoped_guard(mutex, &thermal_hwmon_list_lock) {
+		hwmon = thermal_hwmon_lookup(tz);
+		if (!hwmon)
+			return;
 
-	temp = thermal_hwmon_lookup_temp(hwmon, tz);
-	if (unlikely(!temp)) {
-		/* Should never happen... */
-		dev_dbg(&tz->device, "temperature input lookup failed!\n");
-		return;
+		list_del(&hwmon->node);
 	}
 
+	temp = &hwmon->tz_temp;
+
 	device_remove_file(hwmon->device, &temp->temp_input.attr);
 	if (temp->temp_crit_present)
 		device_remove_file(hwmon->device, &temp->temp_crit.attr);
 
-	mutex_lock(&thermal_hwmon_list_lock);
-	list_del(&temp->hwmon_node);
-	kfree(temp);
-	if (!list_empty(&hwmon->tz_list)) {
-		mutex_unlock(&thermal_hwmon_list_lock);
-		return;
-	}
-	list_del(&hwmon->node);
-	mutex_unlock(&thermal_hwmon_list_lock);
-
 	hwmon_device_unregister(hwmon->device);
 	kfree(hwmon);
 }

Re: [PATCH v2 2/3] thermal: hwmon: Register a hwmon device for each thermal zone

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Changing the hwmon device name and splitting aggregate devices breaks userspace hwmon tools and configurations.
--

commit ef04f4845b76df7ab7442699654ee8214cc3a4ea
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

thermal: hwmon: Register a hwmon device for each thermal zone

The current code creates one hwmon device per thermal zone type and registers
it under the first thermal zone of that type. This commit addresses removal
issues by registering one hwmon device for each thermal zone individually,
appending the zone ID to the hwmon name, and giving each its own temp1_input.

Link: https://lore.kernel.org/linux-pm/20260402021828.16556-1-liujia6264@gmail.com/

> diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
> index 597c33c8a5550..223ae1571655b 100644
> --- a/drivers/thermal/thermal_hwmon.c
> +++ b/drivers/thermal/thermal_hwmon.c

[ ... ]

> @@ -137,54 +101,39 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
>  {
>  	struct thermal_hwmon_device *hwmon;
>  	struct thermal_hwmon_temp *temp;
> -	int new_hwmon_device = 1;
>  	int result;
>  
> -	hwmon = thermal_hwmon_lookup_by_type(tz);
> -	if (hwmon) {
> -		new_hwmon_device = 0;
> -		goto register_sys_interface;
> -	}
> -
>  	hwmon = kzalloc_obj(*hwmon);
>  	if (!hwmon)
>  		return -ENOMEM;
>  
> -	INIT_LIST_HEAD(&hwmon->tz_list);
> -	strscpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
> -	strreplace(hwmon->type, '-', '_');
> +	/*
> +	 * Append the thermal zone ID preceded by an underline character to the
> +	 * type to disambiguate the sensors command output.
> +	 */
> +	scnprintf(hwmon->name, THERMAL_HWMON_NAME_LENGTH, "%s_%d", tz->type, tz->id);
> +	strreplace(hwmon->name, '-', '_');

Could this change to the hwmon name attribute cause a regression for userspace
tools?

Tools and configuration files, such as sensors.conf, typically use the name
attribute for matching devices (for example, matching acpitz-*). Appending an
instance-specific ID to the name might prevent these configurations from
applying correctly.

>  	hwmon->device = hwmon_device_register_for_thermal(&tz->device,
> -							  hwmon->type, hwmon);
> +							  hwmon->name, hwmon);
>  	if (IS_ERR(hwmon->device)) {
>  		result = PTR_ERR(hwmon->device);
>  		goto free_mem;
>  	}
>  
> - register_sys_interface:
> -	temp = kzalloc_obj(*temp);
> -	if (!temp) {
> -		result = -ENOMEM;
> -		goto unregister_name;
> -	}
> +	temp = &hwmon->tz_temp;
>  
>  	temp->tz = tz;
> -	hwmon->count++;
>  
> -	snprintf(temp->temp_input.name, sizeof(temp->temp_input.name),
> -		 "temp%d_input", hwmon->count);
> -	temp->temp_input.attr.attr.name = temp->temp_input.name;
> +	temp->temp_input.attr.attr.name = "temp1_input";

Does splitting aggregate devices break existing sysfs access patterns?

Previously, a single hwmon device might have contained multiple temperature
inputs, like temp1_input and temp2_input. Since each zone now gets its own
hwmon device with only a temp1_input, scripts or tools that explicitly read
temp2_input from the old aggregate device path will likely fail with a missing
file error.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6017595.DvuYhMxLoT@rafael.j.wysocki?part=2

Re: [PATCH v2 2/3] thermal: hwmon: Register a hwmon device for each thermal zone

[Rafael J. Wysocki]

On Tue, May 5, 2026 at 2:41 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] Changing the hwmon device name and splitting aggregate devices breaks userspace hwmon tools and configurations.
> --
>
> commit ef04f4845b76df7ab7442699654ee8214cc3a4ea
> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> thermal: hwmon: Register a hwmon device for each thermal zone
>
> The current code creates one hwmon device per thermal zone type and registers
> it under the first thermal zone of that type. This commit addresses removal
> issues by registering one hwmon device for each thermal zone individually,
> appending the zone ID to the hwmon name, and giving each its own temp1_input.
>
> Link: https://lore.kernel.org/linux-pm/20260402021828.16556-1-liujia6264@gmail.com/
>
> > diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
> > index 597c33c8a5550..223ae1571655b 100644
> > --- a/drivers/thermal/thermal_hwmon.c
> > +++ b/drivers/thermal/thermal_hwmon.c
>
> [ ... ]
>
> > @@ -137,54 +101,39 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
> >  {
> >       struct thermal_hwmon_device *hwmon;
> >       struct thermal_hwmon_temp *temp;
> > -     int new_hwmon_device = 1;
> >       int result;
> >
> > -     hwmon = thermal_hwmon_lookup_by_type(tz);
> > -     if (hwmon) {
> > -             new_hwmon_device = 0;
> > -             goto register_sys_interface;
> > -     }
> > -
> >       hwmon = kzalloc_obj(*hwmon);
> >       if (!hwmon)
> >               return -ENOMEM;
> >
> > -     INIT_LIST_HEAD(&hwmon->tz_list);
> > -     strscpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
> > -     strreplace(hwmon->type, '-', '_');
> > +     /*
> > +      * Append the thermal zone ID preceded by an underline character to the
> > +      * type to disambiguate the sensors command output.
> > +      */
> > +     scnprintf(hwmon->name, THERMAL_HWMON_NAME_LENGTH, "%s_%d", tz->type, tz->id);
> > +     strreplace(hwmon->name, '-', '_');
>
> Could this change to the hwmon name attribute cause a regression for userspace
> tools?
>
> Tools and configuration files, such as sensors.conf, typically use the name
> attribute for matching devices (for example, matching acpitz-*). Appending an
> instance-specific ID to the name might prevent these configurations from
> applying correctly.

Or not.

> >       hwmon->device = hwmon_device_register_for_thermal(&tz->device,
> > -                                                       hwmon->type, hwmon);
> > +                                                       hwmon->name, hwmon);
> >       if (IS_ERR(hwmon->device)) {
> >               result = PTR_ERR(hwmon->device);
> >               goto free_mem;
> >       }
> >
> > - register_sys_interface:
> > -     temp = kzalloc_obj(*temp);
> > -     if (!temp) {
> > -             result = -ENOMEM;
> > -             goto unregister_name;
> > -     }
> > +     temp = &hwmon->tz_temp;
> >
> >       temp->tz = tz;
> > -     hwmon->count++;
> >
> > -     snprintf(temp->temp_input.name, sizeof(temp->temp_input.name),
> > -              "temp%d_input", hwmon->count);
> > -     temp->temp_input.attr.attr.name = temp->temp_input.name;
> > +     temp->temp_input.attr.attr.name = "temp1_input";
>
> Does splitting aggregate devices break existing sysfs access patterns?
>
> Previously, a single hwmon device might have contained multiple temperature
> inputs, like temp1_input and temp2_input. Since each zone now gets its own
> hwmon device with only a temp1_input, scripts or tools that explicitly read
> temp2_input from the old aggregate device path will likely fail with a missing
> file error.

Does this happen in practice though?

Re: [PATCH v2 2/3] thermal: hwmon: Register a hwmon device for each thermal zone

[Guenter Roeck]

On 5/5/26 07:40, Rafael J. Wysocki wrote:
> On Tue, May 5, 2026 at 2:41 PM <sashiko-bot@kernel.org> wrote:
>>
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [Critical] Changing the hwmon device name and splitting aggregate devices breaks userspace hwmon tools and configurations.
>> --
>>
>> commit ef04f4845b76df7ab7442699654ee8214cc3a4ea
>> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> thermal: hwmon: Register a hwmon device for each thermal zone
>>
>> The current code creates one hwmon device per thermal zone type and registers
>> it under the first thermal zone of that type. This commit addresses removal
>> issues by registering one hwmon device for each thermal zone individually,
>> appending the zone ID to the hwmon name, and giving each its own temp1_input.
>>
>> Link: https://lore.kernel.org/linux-pm/20260402021828.16556-1-liujia6264@gmail.com/
>>
>>> diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
>>> index 597c33c8a5550..223ae1571655b 100644
>>> --- a/drivers/thermal/thermal_hwmon.c
>>> +++ b/drivers/thermal/thermal_hwmon.c
>>
>> [ ... ]
>>
>>> @@ -137,54 +101,39 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
>>>   {
>>>        struct thermal_hwmon_device *hwmon;
>>>        struct thermal_hwmon_temp *temp;
>>> -     int new_hwmon_device = 1;
>>>        int result;
>>>
>>> -     hwmon = thermal_hwmon_lookup_by_type(tz);
>>> -     if (hwmon) {
>>> -             new_hwmon_device = 0;
>>> -             goto register_sys_interface;
>>> -     }
>>> -
>>>        hwmon = kzalloc_obj(*hwmon);
>>>        if (!hwmon)
>>>                return -ENOMEM;
>>>
>>> -     INIT_LIST_HEAD(&hwmon->tz_list);
>>> -     strscpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
>>> -     strreplace(hwmon->type, '-', '_');
>>> +     /*
>>> +      * Append the thermal zone ID preceded by an underline character to the
>>> +      * type to disambiguate the sensors command output.
>>> +      */
>>> +     scnprintf(hwmon->name, THERMAL_HWMON_NAME_LENGTH, "%s_%d", tz->type, tz->id);
>>> +     strreplace(hwmon->name, '-', '_');
>>
>> Could this change to the hwmon name attribute cause a regression for userspace
>> tools?
>>
>> Tools and configuration files, such as sensors.conf, typically use the name
>> attribute for matching devices (for example, matching acpitz-*). Appending an
>> instance-specific ID to the name might prevent these configurations from
>> applying correctly.
> 
> Or not.
> 
>>>        hwmon->device = hwmon_device_register_for_thermal(&tz->device,
>>> -                                                       hwmon->type, hwmon);
>>> +                                                       hwmon->name, hwmon);
>>>        if (IS_ERR(hwmon->device)) {
>>>                result = PTR_ERR(hwmon->device);
>>>                goto free_mem;
>>>        }
>>>
>>> - register_sys_interface:
>>> -     temp = kzalloc_obj(*temp);
>>> -     if (!temp) {
>>> -             result = -ENOMEM;
>>> -             goto unregister_name;
>>> -     }
>>> +     temp = &hwmon->tz_temp;
>>>
>>>        temp->tz = tz;
>>> -     hwmon->count++;
>>>
>>> -     snprintf(temp->temp_input.name, sizeof(temp->temp_input.name),
>>> -              "temp%d_input", hwmon->count);
>>> -     temp->temp_input.attr.attr.name = temp->temp_input.name;
>>> +     temp->temp_input.attr.attr.name = "temp1_input";
>>
>> Does splitting aggregate devices break existing sysfs access patterns?
>>
>> Previously, a single hwmon device might have contained multiple temperature
>> inputs, like temp1_input and temp2_input. Since each zone now gets its own
>> hwmon device with only a temp1_input, scripts or tools that explicitly read
>> temp2_input from the old aggregate device path will likely fail with a missing
>> file error.
> 
> Does this happen in practice though?
> 
I guess we'll see.

Guenter

Re: [PATCH v2 2/3] thermal: hwmon: Register a hwmon device for each thermal zone

[Rafael J. Wysocki]

On Tue, May 5, 2026 at 4:59 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 5/5/26 07:40, Rafael J. Wysocki wrote:
> > On Tue, May 5, 2026 at 2:41 PM <sashiko-bot@kernel.org> wrote:
> >>
> >> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> >> - [Critical] Changing the hwmon device name and splitting aggregate devices breaks userspace hwmon tools and configurations.
> >> --
> >>
> >> commit ef04f4845b76df7ab7442699654ee8214cc3a4ea
> >> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> thermal: hwmon: Register a hwmon device for each thermal zone
> >>
> >> The current code creates one hwmon device per thermal zone type and registers
> >> it under the first thermal zone of that type. This commit addresses removal
> >> issues by registering one hwmon device for each thermal zone individually,
> >> appending the zone ID to the hwmon name, and giving each its own temp1_input.
> >>
> >> Link: https://lore.kernel.org/linux-pm/20260402021828.16556-1-liujia6264@gmail.com/
> >>
> >>> diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
> >>> index 597c33c8a5550..223ae1571655b 100644
> >>> --- a/drivers/thermal/thermal_hwmon.c
> >>> +++ b/drivers/thermal/thermal_hwmon.c
> >>
> >> [ ... ]
> >>
> >>> @@ -137,54 +101,39 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
> >>>   {
> >>>        struct thermal_hwmon_device *hwmon;
> >>>        struct thermal_hwmon_temp *temp;
> >>> -     int new_hwmon_device = 1;
> >>>        int result;
> >>>
> >>> -     hwmon = thermal_hwmon_lookup_by_type(tz);
> >>> -     if (hwmon) {
> >>> -             new_hwmon_device = 0;
> >>> -             goto register_sys_interface;
> >>> -     }
> >>> -
> >>>        hwmon = kzalloc_obj(*hwmon);
> >>>        if (!hwmon)
> >>>                return -ENOMEM;
> >>>
> >>> -     INIT_LIST_HEAD(&hwmon->tz_list);
> >>> -     strscpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
> >>> -     strreplace(hwmon->type, '-', '_');
> >>> +     /*
> >>> +      * Append the thermal zone ID preceded by an underline character to the
> >>> +      * type to disambiguate the sensors command output.
> >>> +      */
> >>> +     scnprintf(hwmon->name, THERMAL_HWMON_NAME_LENGTH, "%s_%d", tz->type, tz->id);
> >>> +     strreplace(hwmon->name, '-', '_');
> >>
> >> Could this change to the hwmon name attribute cause a regression for userspace
> >> tools?
> >>
> >> Tools and configuration files, such as sensors.conf, typically use the name
> >> attribute for matching devices (for example, matching acpitz-*). Appending an
> >> instance-specific ID to the name might prevent these configurations from
> >> applying correctly.
> >
> > Or not.
> >
> >>>        hwmon->device = hwmon_device_register_for_thermal(&tz->device,
> >>> -                                                       hwmon->type, hwmon);
> >>> +                                                       hwmon->name, hwmon);
> >>>        if (IS_ERR(hwmon->device)) {
> >>>                result = PTR_ERR(hwmon->device);
> >>>                goto free_mem;
> >>>        }
> >>>
> >>> - register_sys_interface:
> >>> -     temp = kzalloc_obj(*temp);
> >>> -     if (!temp) {
> >>> -             result = -ENOMEM;
> >>> -             goto unregister_name;
> >>> -     }
> >>> +     temp = &hwmon->tz_temp;
> >>>
> >>>        temp->tz = tz;
> >>> -     hwmon->count++;
> >>>
> >>> -     snprintf(temp->temp_input.name, sizeof(temp->temp_input.name),
> >>> -              "temp%d_input", hwmon->count);
> >>> -     temp->temp_input.attr.attr.name = temp->temp_input.name;
> >>> +     temp->temp_input.attr.attr.name = "temp1_input";
> >>
> >> Does splitting aggregate devices break existing sysfs access patterns?
> >>
> >> Previously, a single hwmon device might have contained multiple temperature
> >> inputs, like temp1_input and temp2_input. Since each zone now gets its own
> >> hwmon device with only a temp1_input, scripts or tools that explicitly read
> >> temp2_input from the old aggregate device path will likely fail with a missing
> >> file error.
> >
> > Does this happen in practice though?
> >
> I guess we'll see.

Right.

[PATCH v2 3/3] thermal: hwmon: Use extra_groups for adding temperature attributes

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Instead of passing NULL as the last argument to __hwmon_device_register()
in hwmon_device_register_for_thermal() and then adding each temperature
sysfs attribute to the hwmon device via device_create_file(), redefine
hwmon_device_register_for_thermal() to take an extra_groups argument
that will be passed to __hwmon_device_register(), define an attribute
group with a proper .is_visible() callback for the temperature
attributes and a related attribute groups pointer, and pass the latter
to hwmon_device_register_for_thermal().

This causes the code to be way more straightforward and closer to
what the other users of the hwmon subsystem do.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
   * Rebase on top of [1-2/3]
   * Drop struct thermal_hwmon_temp (sashiko.dev)
   * 

---
 drivers/hwmon/hwmon.c           |    6 +
 drivers/thermal/thermal_hwmon.c |  122 ++++++++++++++--------------------------
 include/linux/hwmon.h           |    3 
 3 files changed, 51 insertions(+), 80 deletions(-)

--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -1082,6 +1082,7 @@ EXPORT_SYMBOL_GPL(hwmon_device_register_
  * @dev: the parent device
  * @name: hwmon name attribute
  * @drvdata: driver data to attach to created device
+ * @extra_groups: pointer to list of additional non-standard attribute groups
  *
  * The use of this function is restricted. It is provided for legacy reasons
  * and must only be called from the thermal subsystem.
@@ -1093,12 +1094,13 @@ EXPORT_SYMBOL_GPL(hwmon_device_register_
  */
 struct device *
 hwmon_device_register_for_thermal(struct device *dev, const char *name,
-				  void *drvdata)
+				  void *drvdata,
+				  const struct attribute_group **extra_groups)
 {
 	if (!name || !dev)
 		return ERR_PTR(-EINVAL);
 
-	return __hwmon_device_register(dev, name, drvdata, NULL, NULL);
+	return __hwmon_device_register(dev, name, drvdata, NULL, extra_groups);
 }
 EXPORT_SYMBOL_NS_GPL(hwmon_device_register_for_thermal, "HWMON_THERMAL");
 
--- a/drivers/thermal/thermal_hwmon.c
+++ b/drivers/thermal/thermal_hwmon.c
@@ -25,25 +25,13 @@
  */
 #define THERMAL_HWMON_NAME_LENGTH (THERMAL_NAME_LENGTH + 11)
 
-struct thermal_hwmon_attr {
-	struct device_attribute attr;
-};
-
-/* one temperature input for each thermal zone */
-struct thermal_hwmon_temp {
-	struct thermal_zone_device *tz;
-	struct thermal_hwmon_attr temp_input;	/* hwmon sys attr */
-	struct thermal_hwmon_attr temp_crit;	/* hwmon sys attr */
-	bool temp_crit_present;
-};
-
 /* hwmon sys I/F */
 /* thermal zone devices with the same type share one hwmon device */
 struct thermal_hwmon_device {
 	char name[THERMAL_HWMON_NAME_LENGTH];
 	struct device *device;
 	struct list_head node;
-	struct thermal_hwmon_temp tz_temp;
+	struct thermal_zone_device *tz;
 };
 
 static LIST_HEAD(thermal_hwmon_list);
@@ -51,19 +39,14 @@ static LIST_HEAD(thermal_hwmon_list);
 static DEFINE_MUTEX(thermal_hwmon_list_lock);
 
 static ssize_t
-temp_input_show(struct device *dev, struct device_attribute *attr, char *buf)
+temp1_input_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
+	struct thermal_hwmon_device *hwmon = dev_get_drvdata(dev);
+	struct thermal_zone_device *tz = hwmon->tz;
 	int temperature;
 	int ret;
-	struct thermal_hwmon_attr *hwmon_attr
-			= container_of(attr, struct thermal_hwmon_attr, attr);
-	struct thermal_hwmon_temp *temp
-			= container_of(hwmon_attr, struct thermal_hwmon_temp,
-				       temp_input);
-	struct thermal_zone_device *tz = temp->tz;
 
 	ret = thermal_zone_get_temp(tz, &temperature);
-
 	if (ret)
 		return ret;
 
@@ -71,14 +54,10 @@ temp_input_show(struct device *dev, stru
 }
 
 static ssize_t
-temp_crit_show(struct device *dev, struct device_attribute *attr, char *buf)
+temp1_crit_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	struct thermal_hwmon_attr *hwmon_attr
-			= container_of(attr, struct thermal_hwmon_attr, attr);
-	struct thermal_hwmon_temp *temp
-			= container_of(hwmon_attr, struct thermal_hwmon_temp,
-				       temp_crit);
-	struct thermal_zone_device *tz = temp->tz;
+	struct thermal_hwmon_device *hwmon = dev_get_drvdata(dev);
+	struct thermal_zone_device *tz = hwmon->tz;
 	int temperature;
 	int ret;
 
@@ -91,22 +70,49 @@ temp_crit_show(struct device *dev, struc
 	return sysfs_emit(buf, "%d\n", temperature);
 }
 
-static bool thermal_zone_crit_temp_valid(struct thermal_zone_device *tz)
+static DEVICE_ATTR_RO(temp1_input);
+static DEVICE_ATTR_RO(temp1_crit);
+
+static struct attribute *thermal_hwmon_attrs[] = {
+	&dev_attr_temp1_input.attr,
+	&dev_attr_temp1_crit.attr,
+	NULL,
+};
+
+static umode_t thermal_hwmon_attr_is_visible(struct kobject *kobj,
+					     struct attribute *a, int n)
 {
-	int temp;
-	return tz->ops.get_crit_temp && !tz->ops.get_crit_temp(tz, &temp);
+	if (a == &dev_attr_temp1_input.attr)
+		return a->mode;
+
+	if (a == &dev_attr_temp1_crit.attr) {
+		struct thermal_hwmon_device *hwmon = dev_get_drvdata(kobj_to_dev(kobj));
+		struct thermal_zone_device *tz = hwmon->tz;
+		int dummy;
+
+		if (tz->ops.get_crit_temp && !tz->ops.get_crit_temp(tz, &dummy))
+			return a->mode;
+	}
+
+	return 0;
 }
 
+static const struct attribute_group thermal_hwmon_group = {
+	.attrs	= thermal_hwmon_attrs,
+	.is_visible = thermal_hwmon_attr_is_visible,
+};
+
+__ATTRIBUTE_GROUPS(thermal_hwmon);
+
 int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
 {
 	struct thermal_hwmon_device *hwmon;
-	struct thermal_hwmon_temp *temp;
-	int result;
 
 	hwmon = kzalloc_obj(*hwmon);
 	if (!hwmon)
 		return -ENOMEM;
 
+	hwmon->tz = tz;
 	/*
 	 * Append the thermal zone ID preceded by an underline character to the
 	 * type to disambiguate the sensors command output.
@@ -114,35 +120,13 @@ int thermal_add_hwmon_sysfs(struct therm
 	scnprintf(hwmon->name, THERMAL_HWMON_NAME_LENGTH, "%s_%d", tz->type, tz->id);
 	strreplace(hwmon->name, '-', '_');
 	hwmon->device = hwmon_device_register_for_thermal(&tz->device,
-							  hwmon->name, hwmon);
+							  hwmon->name, hwmon,
+							  thermal_hwmon_groups);
 	if (IS_ERR(hwmon->device)) {
-		result = PTR_ERR(hwmon->device);
-		goto free_mem;
-	}
-
-	temp = &hwmon->tz_temp;
-
-	temp->tz = tz;
+		int result = PTR_ERR(hwmon->device);
 
-	temp->temp_input.attr.attr.name = "temp1_input";
-	temp->temp_input.attr.attr.mode = 0444;
-	temp->temp_input.attr.show = temp_input_show;
-	sysfs_attr_init(&temp->temp_input.attr.attr);
-	result = device_create_file(hwmon->device, &temp->temp_input.attr);
-	if (result)
-		goto unregister_name;
-
-	if (thermal_zone_crit_temp_valid(tz)) {
-		temp->temp_crit.attr.attr.name = "temp1_crit";
-		temp->temp_crit.attr.attr.mode = 0444;
-		temp->temp_crit.attr.show = temp_crit_show;
-		sysfs_attr_init(&temp->temp_crit.attr.attr);
-		result = device_create_file(hwmon->device,
-					    &temp->temp_crit.attr);
-		if (result)
-			goto unregister_input;
-
-		temp->temp_crit_present = true;
+		kfree(hwmon);
+		return result;
 	}
 
 	/* The list is needed for hwmon lookup during removal. */
@@ -151,15 +135,6 @@ int thermal_add_hwmon_sysfs(struct therm
 	mutex_unlock(&thermal_hwmon_list_lock);
 
 	return 0;
-
- unregister_input:
-	device_remove_file(hwmon->device, &temp->temp_input.attr);
- unregister_name:
-	hwmon_device_unregister(hwmon->device);
- free_mem:
-	kfree(hwmon);
-
-	return result;
 }
 EXPORT_SYMBOL_GPL(thermal_add_hwmon_sysfs);
 
@@ -169,7 +144,7 @@ thermal_hwmon_lookup(const struct therma
 	struct thermal_hwmon_device *hwmon;
 
 	list_for_each_entry(hwmon, &thermal_hwmon_list, node) {
-		if (hwmon->tz_temp.tz == tz)
+		if (hwmon->tz == tz)
 			return hwmon;
 	}
 	return NULL;
@@ -178,7 +153,6 @@ thermal_hwmon_lookup(const struct therma
 void thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
 {
 	struct thermal_hwmon_device *hwmon;
-	struct thermal_hwmon_temp *temp;
 
 	scoped_guard(mutex, &thermal_hwmon_list_lock) {
 		hwmon = thermal_hwmon_lookup(tz);
@@ -188,12 +162,6 @@ void thermal_remove_hwmon_sysfs(struct t
 		list_del(&hwmon->node);
 	}
 
-	temp = &hwmon->tz_temp;
-
-	device_remove_file(hwmon->device, &temp->temp_input.attr);
-	if (temp->temp_crit_present)
-		device_remove_file(hwmon->device, &temp->temp_crit.attr);
-
 	hwmon_device_unregister(hwmon->device);
 	kfree(hwmon);
 }
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -477,7 +477,8 @@ hwmon_device_register_with_info(struct d
 				const struct attribute_group **extra_groups);
 struct device *
 hwmon_device_register_for_thermal(struct device *dev, const char *name,
-				  void *drvdata);
+				  void *drvdata,
+				  const struct attribute_group **extra_groups);
 struct device *
 devm_hwmon_device_register_with_info(struct device *dev,
 				const char *name, void *drvdata,