linux-kernel - Re: [PATCH 1/2] hwmon: Add Congatec Board Controller monitoring driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <761490f4-b0b2-442b-b1b6-ced74e9b6300@roeck-us.net>
Date: Wed, 6 Nov 2024 07:14:22 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Thomas Richard <thomas.richard@...tlin.com>,
 Jean Delvare <jdelvare@...e.com>, Lee Jones <lee@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
 thomas.petazzoni@...tlin.com, blake.vermeer@...sight.com
Subject: Re: [PATCH 1/2] hwmon: Add Congatec Board Controller monitoring
 driver

On 11/6/24 05:46, Thomas Richard wrote:

>>> +
>>> +static struct cgbc_hwmon_sensor *cgbc_hwmon_find_sensor(struct
>>> cgbc_hwmon_data *hwmon,
>>> +                            enum hwmon_sensor_types type, int channel)
>>> +{
>>> +    struct cgbc_hwmon_sensor *sensor = NULL;
>>> +    int i, cnt = 0;
>>> +
>>> +    for (i = 0; i < hwmon->nb_sensors; i++) {
>>> +        if (hwmon->sensors[i].type == type && cnt++ == channel) {
>>
>> Isn't that a bit fragile ? It assumes that the nth reported sensor of a
>> given type
>> reflects a specific named sensor. If that is indeed the case, why bother
>> with
>> all the code in cgbc_hwmon_probe_sensors() ? The index to sensor
>> association
>> should be well defined, and the sensor type plus the channel index
>> should always
>> be a constant.
> 
> I'm not sure to get your comment.
> 
> I cannot assume that the sensor list returned by the Board Controller
> will be the same for all boards.
> I know the MFD driver only supports one board, but I think it's better
> if we can have a generic hwmon driver.
> 
> If I add some debug in cgbc_hwmon_probe_sensors() I can dump the sensor
> list returned by the Board Controller:
> 
> cgbc_hwmon_probe_sensors: index=0 type=1 id=5 label=Chipset Temperature
> cgbc_hwmon_probe_sensors: index=1 type=7 id=0 label=CPU Fan
> cgbc_hwmon_probe_sensors: index=4 type=1 id=3 label=Board Temperature
> cgbc_hwmon_probe_sensors: index=5 type=2 id=1 label=DC Runtime Voltage
> 
> It is the type and the id which give the name of the sensor.
> 
> I don't see how to do it in a different way if I cannot assume that the
> list above is not the same for all boards.
> If I assume that the list returned by the Board Controller is always the
> same for a board (which I not even sure, if for example a fan is
> plugged), I could have a static list for each different boards. But the
> driver will not be generic.
> 
> If I miss something, please let me know.
> 

My thought was to use the sensor ID as channel index. In general it
would be preferable to know that, say, in0 is always the CPU voltage
and that temp1 is always the CPU Temperature.

Guenter