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: Fri, 7 Jun 2024 17:59:59 +0200
From: Armin Wolf <W_Armin@....de>
To: Guenter Roeck <linux@...ck-us.net>, 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

Am 04.06.24 um 16:30 schrieb Guenter Roeck:

> 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.
>
I understand.

> 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
>
Ok, then i am ok with with this patch.

Tested-by: Armin Wolf <W_Armin@....de>