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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ