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

On Tue, May 5, 2026 at 4:59 PM Guenter Roeck <[email protected]> wrote:
>
> 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.

Right.