Re: (bug report) HWMON & Thermal interactions #forregzbot

[Thorsten Leemhuis]

[Note: this mail is primarily send for documentation purposes and/or for
regzbot, my Linux kernel regression tracking bot. That's why I removed
most or all folks from the list of recipients, but left any that looked
like a mailing lists. These mails usually contain '#forregzbot' in the
subject, to make them easy to spot and filter out.]

[TLDR: I'm adding this regression report to the list of tracked
regressions; all text from me you find below is based on a few templates
paragraphs you might have encountered already already in similar form.]

Hi, this is your Linux kernel regression tracker. CCing the regression
mailing list, as it should be in the loop for all regressions, as
explained here:
https://www.kernel.org/doc/html/latest/admin-guide/reporting-issues.html

On 23.10.22 20:27, Cristian Marussi wrote:
> 
> Starting with v6.1-rc1 the SCMI HWMON driver failed probing on my JUNO due
> to the fact that no trip points were (ever !) defined in the DT; bisecting it
> looks like that after:

Thanks for the report. To be sure below issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, my Linux kernel regression
tracking bot:

#regzbot ^introduced e51813313
#regzbot title SCMI HWMON driver failed probing on JUNO
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply -- ideally with also
telling regzbot about it, as explained here:
https://linux-regtracking.leemhuis.info/tracked-regression/

Reminder for developers: When fixing the issue, add 'Link:' tags
pointing to the report (the mail this one replies to), as explained for
in the Linux kernel's documentation; above webpage explains why this is
important for tracked regressions.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

P.S.: As the Linux kernel's regression tracker I deal with a lot of
reports and sometimes miss something important when writing mails like
this. If that's the case here, don't hesitate to tell me in a public
reply, it's in everyone's interest to set the public record straight.

> https://lore.kernel.org/all/20220804224349.1926752-28-daniel.lezcano@xxxxxxxxxx/
> 
> the presence of the mandatory trips node within thermal zones is now
> enforced.
> 
> So, this is NOT what this bug report is about (I'll post soon patches for
> the JUNO DT missing trips) BUT once this problem was solved in the DT,
> another issue appeared:
> 
> [    1.921929] hwmon hwmon0: temp2_input not attached to any thermal zone
> 
> that despite having now a goodi/valid DT describing 2 sensors and 2 thermal zones
> embedding that sensors, only the first one is found as belonging to one ThermZ.
> (this happens ALSO with v6.0 once I added the trips...)
> 
> Digging deep into this, it turned out that inside the call chain
> 
> devm_hwmon_device_register_with_info
>   hwmon_device_register_with_info
>     __hwmon_device_register
> 	hwmon_thermal_register_sensors(dev)
> 		--> hwmon_thermal_add_sensor(dev, j)
> 			--> devm_thermal_of_zone_register(dev, sensor_id, tdata, )
> 
> the HWMON channel index j is passed to the Thermal framework in order to
> search and bind sensors with defined thermal zone, but this lead to the
> assumption that sequential HWMON channel indexes corresponds one-to-one to the
> underlying real sensor IDs that the ThermalFramework uses for matching
> within the DT.
> 
> On a system like my SCMI-based DT where I have 2 temp-sensors bound to 2
> thermal zones like:
> 
> thernal_zones {
> 	pmic {
> 		...
> 		thermal-sensors = <&scmi_sensors0 0>;
> 		...
> 		trips {
> 			...
> 		}
> 	soc {
> 		...
> 		thermal-sensors = <&scmi_sensors0 3>;
> 		...
> 		trips {
> 			...
> 		}
> 	}
> }
> 
> This works fine by chance for the pmic (j=0, sensor_id=0) BUT cannot work for
> the soc where J=1 BUT the real sensor ID is 3.
> 
> Note that there can be a number of sensors, not all of them of a type handled
> by HWMON, and enumerated by SCMI in different ways depending on the
> platform.
> 
> I suppose this is not an SCMI-only related issue, but maybe in non-SCMI
> context, where sensors are purely defined in the DT, the solution can be
> more easily attained (i.e. renumber the sensors).
> 
> At first I tried to solve this inside scmi-hwmon.c BUT I could not find
> a way to present to the HWMON subsystem the list of sensors preserving
> the above index/sensor_id matching (not even with a hack like passing
> down dummy sensors to the HWMON subsystem to fill the 'holes' in the
> numbering)
> 
> My tentative solution, which works fine for me in my context, was to add
> an optional HWMON hwops, so that the core hwmon can retrieve if needed the
> real sensor ID if different from the channel index (using an optional hwops
> instead of some static hwinfo var let me avoid to have to patch all the
> existent hwmon drivers that happens to just work fine as of today...but
> maybe it is not necessarily the proper final solution...)
> 
> i.e.
> 
> ----8<----
> 
> Author: Cristian Marussi <cristian.marussi@xxxxxxx>
> Date:   Fri Oct 21 17:24:04 2022 +0100
> 
>     hwmon: Add new .get_sensor_id hwops
>     
>     Add a new optional helper which can be defined to allow an hwmon chip to
>     provide the logic to map hwmon indexes to the real underlying sensor IDs.
>     
>     Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 4218750d5a66..45d3d5070cde 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -213,7 +213,8 @@ static void hwmon_thermal_remove_sensor(void *data)
>         list_del(data);
>  }
>  
> -static int hwmon_thermal_add_sensor(struct device *dev, int index)
> +static int hwmon_thermal_add_sensor(struct device *dev, int index,
> 7+                                   unsigned int sensor_id)
>  {
>         struct hwmon_device *hwdev = to_hwmon_device(dev);
>         struct hwmon_thermal_data *tdata;
> @@ -227,7 +228,7 @@ static int hwmon_thermal_add_sensor(struct device *dev, int index)
>         tdata->dev = dev;
>         tdata->index = index;
>  
> -       tzd = devm_thermal_of_zone_register(dev, index, tdata,
> +       tzd = devm_thermal_of_zone_register(dev, sensor_id, tdata,
>                                             &hwmon_thermal_ops);
>         if (IS_ERR(tzd)) {
>                 if (PTR_ERR(tzd) != -ENODEV)
> @@ -264,13 +265,18 @@ static int hwmon_thermal_register_sensors(struct device *dev)
>  
>                 for (j = 0; info[i]->config[j]; j++) {
>                         int err;
> +                       unsigned int id;
>  
>                         if (!(info[i]->config[j] & HWMON_T_INPUT) ||
>                             !chip->ops->is_visible(drvdata, hwmon_temp,
>                                                    hwmon_temp_input, j))
>                                 continue;
>  
> -                       err = hwmon_thermal_add_sensor(dev, j);
> +                       id = !chip->ops->get_sensor_id ? j :
> +                               chip->ops->get_sensor_id(drvdata,
> +                                                        hwmon_temp, j);
> +
> +                       err = hwmon_thermal_add_sensor(dev, j, id);
>                         if (err)
>                                 return err;
>                 }
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index 14325f93c6b2..e5dbab83f4d1 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -396,6 +396,9 @@ enum hwmon_intrusion_attributes {
>  struct hwmon_ops {
>         umode_t (*is_visible)(const void *drvdata, enum hwmon_sensor_types type,
>                               u32 attr, int channel);
> +       unsigned int (*get_sensor_id)(const void *drvdata,
> +                                     enum hwmon_sensor_types type,
> +                                     int channel);
>         int (*read)(struct device *dev, enum hwmon_sensor_types type,
>                     u32 attr, int channel, long *val);
>         int (*read_string)(struct device *dev, enum hwmon_sensor_types type,
> 
> 
> ----->8----
> 
> 
> ... plus obviously the related scmi-hwmon.c patch to make use of this.
> 
> Any thought ? Am I missing something ?
> (not really an expert on both subsystems really ... :P)
> 
> Thanks,
> Cristian
>