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

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?