lkml.org 
[lkml]   [2022]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: (bug report) HWMON & Thermal interactions #forregzbot
From
[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@linexp.org/
>
> 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@arm.com>
> 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@arm.com>
>
> 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
>

\
 
 \ /
  Last update: 2022-10-24 12:19    [W:0.073 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site