linux-kernel - Re: [PATCH v2 1/3] nvmem: core: introduce cells parser

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 for Android: free password hash cracker in your pocket

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

Message-ID: <c19ba35d-68b3-5044-bca2-92627c810efa@linaro.org>
Date:   Tue, 28 Sep 2021 15:39:56 +0100
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Vadym Kochan <vadym.kochan@...ision.eu>
Cc:     John Thomson <john@...nthomson.fastmail.com.au>,
        Rob Herring <robh+dt@...nel.org>, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, Robert Marko <robert.marko@...tura.hr>
Subject: Re: [PATCH v2 1/3] nvmem: core: introduce cells parser



On 28/09/2021 15:11, Vadym Kochan wrote:
> Srinivas Kandagatla<srinivas.kandagatla@...aro.org>  writes:
> 
>> On 28/09/2021 14:31, Vadym Kochan wrote:
>>>>>> Can I note here that I would like to parse
>>>>>> TLV data from an SPI-NOR device to NVMEM cells.
>>>>>> The same general use case (getting mac-address from OEM data).
>>>>>>
>>>>>> Was planning to base my work on this series, as well as
>>>>>> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/
>>>>>> (thanks for pointing that out Srinivas)
>>>>>>
>>>>>> Cheers,
>>>>> What about at least to have just one call in core.c to make it a bit
>>>>> de-coupled, like:
>>>> Why do you want to decouple this? the provider driver should be very
>>>> well aware of the format the data layout.
>>>>
>>> In my understanding nvmem device should not aware about the data layout
>>> (in case it does not rely on device's specific characteristics). Same
>>> cells layout (TLV, etc) might exist on other nvmem devices.
>>>
>> How would provider driver parse this without even knowing data layout?
>>
>>
>>>> Its fine to an extent to adding parse_cells() callback in nvmem_config.
>>>>
>>> OK, in that case it will require small change in the core.
>>>
>>>>> core.c
>>>>>
>>>>> struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>>>> {
>>>>> ...
>>>>>             rval = nvmem_add_cells_from_table(nvmem);
>>>>>             if (rval)
>>>>>                     goto err_remove_cells;
>>>>>
>>>>> +        rval = nvmem_parse_cells(nvmem, of);
>>>>> +        if (rval) {
>>>>> +        /* err handling */
>>>>> +        }
>>>>> +
>>>>>             rval = nvmem_add_cells_from_of(nvmem);
>>>>>             if (rval)
>>>>>                     goto err_remove_cells;
>>>>>
>>>>>             blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
>>>>>
>>>>>             return nvmem;
>>>>>
>>>>> ...
>>>>>
>>>>> }
>>>>>
>>>>> somewhere in nvmem-parser.c:
>>>> However this is totally over kill.
>>>>
>>>>> /* retreive parser name from of_node and call appropriate function to parse
>>>>>       non-fixed cells and update via of_update */
>>>> This is completely provider drivers job, nothing nvmem core should worry
>>>> about.
>>>>
>>>> If you have concern of having code duplicated then we could make some of
>>>> the common functions as library functions, But it still is within the
>>>> scope of provider drivers.
>>>>
>>> Do I understand correctly that this parser function should be exported
>>> from at24.c (in case of ONIE) and not from a separate C module ? Or
>>> it just means that if there will be more users of this parsing function
>>> then it might be moved to separate C module ?
>> yes.
>> For now am not really sure how many users are for such parsing function.
>>
>>>> --srini
>>>>
>>> BTW, what if such change will be declined by particular nvmem driver
>>> maintainer ?
>> You would need some changes to provider driver to be able to flag that
>> there is some kind of parsing required anyway.
>>
> It might be some new property in device-tree which can be used also
> for the other providers.

But this new binding will still belong to the provider driver that you 
want to support parsing.


--srini
>