linux-kernel - Re: [PATCH v4 4/6] hwmon: (spd5118) Add support for reading SPD data

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]

Date: Tue, 4 Jun 2024 07:30:22 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Armin Wolf <W_Armin@....de>, linux-hwmon@...r.kernel.org
Cc: linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 René Rebe <rene@...ctcode.de>,
 Thomas Weißschuh <linux@...ssschuh.net>,
 Stephen Horvath <s.horvath@...look.com.au>
Subject: Re: [PATCH v4 4/6] hwmon: (spd5118) Add support for reading SPD data

On 6/4/24 04:58, Armin Wolf wrote:
> Am 04.06.24 um 06:02 schrieb Guenter Roeck:
> 
>> Add support for reading SPD NVMEM data from SPD5118 (Jedec JESD300)
>> compliant memory modules. NVMEM write operation is not supported.
>>
>> NVMEM support is optional. If CONFIG_NVMEM is disabled, the driver will
>> still instantiate but not provide NVMEM attribute files.
>>
>> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
>> ---
>> v4: Use NVMEM_DEVID_NONE instead of NVMEM_DEVID_AUTO
>>      Ignore nvmem registration failure if nvmem support is disabled
>>
>> v3: New patch
>>
>>   Documentation/hwmon/spd5118.rst |   8 ++
>>   drivers/hwmon/spd5118.c         | 147 +++++++++++++++++++++++++++++++-


[ ... ]

>> +static int spd5118_nvmem_init(struct device *dev, struct spd5118_data *data)
>> +{
>> +    struct nvmem_config nvmem_config = {
>> +        .type = NVMEM_TYPE_EEPROM,
>> +        .name = dev_name(dev),
>> +        .id = NVMEM_DEVID_NONE,
>> +        .dev = dev,
>> +        .base_dev = dev,
>> +        .read_only = true,
>> +        .root_only = false,
>> +        .owner = THIS_MODULE,
>> +        .compat = true,
> 
> Hi,
> 
> do we really need this setting here?
> 

The "eeprom" file is only created if both "base_dev" and "compat" are set.
decode-dimms depends on it. While decode-dimms has to be updated anyway,
I did not want to make that more complicated than necessary.

Another option would be not to use the nvmem subsystem in the first place,
similar to the ee1004 driver, but my understanding is that the use of the
nvmem subsystem is preferred.

[ ... ]

>> +
>> +    err = spd5118_nvmem_init(dev, data);
>> +    /* Ignore if NVMEM support is disabled */
>> +    if (err && err != -EOPNOTSUPP) {
> 
> Maybe we can use IS_REACHABLE(CONFIG_NVMEM) here?
> 

We could, but I prefer to avoid conditionals in the code if possible,
the dummy devm_nvmem_register() is there specifically to cover that
situation, and no other driver does that. Also, since the underlying
functions are dummy, the compiler should optimize it all away if
CONFIG_NVMEM=n.

Thanks,
Guenter