linux-kernel - Re: [PATCH 2/3] hwmon: Add support for SPD5118 compliant temperature sensors

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: <b8aa563a-5ca1-4624-a1c7-25744f15cfa9@roeck-us.net>
Date: Thu, 30 May 2024 10:33:55 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Armin Wolf <W_Armin@....de>, linux-hwmon@...r.kernel.org
Cc: Hristo Venev <hristo@...ev.name>, René Rebe
 <rene@...ctcode.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, Radu Sabau <radu.sabau@...log.com>
Subject: Re: [PATCH 2/3] hwmon: Add support for SPD5118 compliant temperature
 sensors

On 5/30/24 10:03, Armin Wolf wrote:
> Am 29.05.24 um 22:52 schrieb Guenter Roeck:
> 
[ ... ]

>> +
>> +temp1_lcrit_alarm    Temperature low critical alarm
>> +temp1_min_alarm        Temperature low alarm
>> +temp1_max_alarm        Temperature high alarm
>> +temp1_crit_alarm    Temperature critical alarm
> 
> Maybe it would be a good idea to tell users that the alarm attributes are sticky.
> 

The driver auto-clears them after read, so they are only sticky in the sense
that they will remain active until read. This is quite common for hardware
monitoring devices. However, sure, I'll add a note.

[ ... ]

>> +static int spd5118_write_enable(struct regmap *regmap, u32 attr, long val)
>> +{
>> +    if (val && val != 1)
>> +        return -EINVAL;
>> +
>> +    return regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG,
>> +                  SPD5118_TS_DISABLE,
>> +                  val ? 0 : SPD5118_TS_DISABLE);
> 
> The spd5118 spec says that we have to wait 10ms after enabling the sensors before
> we start reading temperature values, maybe we need a delay + locking here?
> 

I don't think that would add much if any value but a lot of complexity
for little gain. I find it acceptable that the sensor returns 0 for a few ms
after enabling it. Pretty much all chips have the same problem, so I am
really not concerned about it.

>> +
>> +static struct i2c_driver spd5118_driver = {
>> +    .class        = I2C_CLASS_HWMON,
>> +    .driver = {
>> +        .name    = "spd5118",
>> +        .of_match_table = spd5118_of_ids,
> 
> The driver is missing suspend support, without it hibernation/S4 sleep will cause the
> limit and config registers to be out of sync with the regmap cache.
> 

Good point. Do you have a means to test this if I add suspend support ?
I have not been able to figure out how to put my system into suspend.

Thanks,
Guenter