Re: [PATCH v1 1/2] thermal: hwmon: Register one hwmon device for each thermal zone

[sashiko-bot@kernel.org]

Sashiko AI review found 5 potential issue(s):
- [Critical] UAPI Breakage by changing existing hwmon device names
- [High] Buffer truncation during hwmon name construction defeats disambiguation
- [High] Kernel panic during thermal zone removal due to conditionally re-evaluating hardware-dependent state
- [Medium] Subsystem Guideline Violation: Mixing goto and scoped_guard
- [Low] Unused dead code (hwmon_node) left in struct thermal_hwmon_temp
--

commit 7f75c80d7fc0d5195ae6453505545af64be540ea
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: thermal: hwmon: Register one hwmon device for each thermal zone

The commit reworks the thermal hwmon code to register one hwmon device for each
thermal zone. It appends the thermal zone ID to the hwmon device name to
disambiguate identical names.

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 b624892bc6d66..46c40c0852804 100644
> --- a/drivers/thermal/thermal_hwmon.c
> +++ b/drivers/thermal/thermal_hwmon.c
[ ... ]
> @@ -136,21 +95,17 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
[ ... ]
>  	hwmon = kzalloc_obj(*hwmon);
>  	if (!hwmon)
>  		return -ENOMEM;
>  
> -	INIT_LIST_HEAD(&hwmon->tz_list);
> -	strscpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
> +	/*
> +	 * Append the thermal zone ID preceded by an underline character to the
> +	 * type to disambiguate the sensors command output.
> +	 */
> +	scnprintf(hwmon->type, THERMAL_NAME_LENGTH, "%s_%d", tz->type, tz->id);

Could this truncation defeat the disambiguation? hwmon->type is defined as
char type[THERMAL_NAME_LENGTH]. If tz->type is already near
THERMAL_NAME_LENGTH characters long, the appended _%d suffix would be
truncated to fit the buffer, potentially resulting in identical names for
different zones.

Can appending a suffix to the name break the userspace ABI? Existing
user-space utilities and configurations that match exactly on the original
zone type string might no longer work when _0 is appended to the name.

>  	strreplace(hwmon->type, '-', '_');
>  	hwmon->device = hwmon_device_register_for_thermal(&tz->device,
>  							  hwmon->type, hwmon);
[ ... ]
> @@ -193,21 +137,17 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
>  			goto unregister_input;
>  	}
>  
> -	mutex_lock(&thermal_hwmon_list_lock);
> -	if (new_hwmon_device)
> +	/* The list is needed for hwmon lookup during removal. */
> +	scoped_guard(mutex, &thermal_hwmon_list_lock) {
>  		list_add_tail(&hwmon->node, &thermal_hwmon_list);
> -	list_add_tail(&temp->hwmon_node, &hwmon->tz_list);

Since hwmon->tz_list is removed, does temp->hwmon_node still need to exist
in struct thermal_hwmon_temp? It looks like it is no longer used anywhere.

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

Does this code mix goto-based error handling and scope-based cleanup helpers?
The cleanup subsystem guidelines expect that usage of goto and cleanup
helpers are never mixed in the same function.

This was noted as being fixed later in the series, but is included here
for completeness.

>  
> @@ -215,39 +155,37 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
[ ... ]
>  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 (thermal_zone_crit_temp_valid(tz))
>  		device_remove_file(hwmon->device, &temp->temp_crit.attr);

Is there a potential panic during removal if the hardware state changes?

If thermal_zone_crit_temp_valid() fails during thermal_add_hwmon_sysfs()
initialization, the temp_crit attribute is never created and its name
remains NULL.

If the hardware state later recovers and thermal_zone_crit_temp_valid()
returns true during thermal_remove_hwmon_sysfs(), device_remove_file()
will be called with a NULL name, which can cause a panic in the sysfs core.

This issue was also noted as being fixed later in the series.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5100596.31r3eYUQgx@rafael.j.wysocki?part=1