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

On 5/5/26 07:40, Rafael J. Wysocki wrote:
> On Tue, May 5, 2026 at 2:41 PM <[email protected]> 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 <[email protected]>
>>
>> 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/[email protected]/
>>
>>> 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