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

[sashiko-bot@kernel.org]

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