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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 21 Sep 2021 05:50:36 +0000
From:   "John Thomson" <john@...nthomson.fastmail.com.au>
To:     "Srinivas Kandagatla" <srinivas.kandagatla@...aro.org>,
        "Vadym Kochan" <vadym.kochan@...ision.eu>
Cc:     "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 Mon, 20 Sep 2021, at 13:40, Srinivas Kandagatla wrote:
> On 20/09/2021 14:29, Vadym Kochan wrote:
>> 
>> Srinivas Kandagatla <srinivas.kandagatla@...aro.org> writes:
>> 
>>> On 20/09/2021 13:29, Vadym Kochan wrote:
>>>>
>>>> Srinivas Kandagatla <srinivas.kandagatla@...aro.org> writes:
>>>>
>>>>> On 20/09/2021 12:25, Vadym Kochan wrote:
>>>>>>>> Or, treat cells with length "0" in a special way and allow to update
>>>>>>>> cell info later.you can update irrespective of the length, as long as this is done
>>>>>>> before register.
>>>>>>>
>>>>>>>
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> some_dev_node {
>>>>>>>>> 	compatible = "xxx";
>>>>>>>>> 	nvmem-cells = <&mac_address>;
>>>>>>>>> 	nvmem-cell-names = "mac-address";
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> == CODE ==
>>>>>>>>> ret = of_get_mac_address(dev->of_node, base_mac);
>>>>>>>>> ==========
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If you notice the mac_address node reg is 0.
>>>>>>>>> This node "reg" property should be updated ( using of_update_property())
>>>>>>>>> by nvmem-provider driver while tlv parsing and by matching the node name
>>>>>>>>> with tlv name.
>>>>>>>>>
>>>>>>>> I assume parser driver can just invoke add_cell_table (with may be
>>>>>>>> by adding 'bool update' field to the cell_info struct) and core.c will just
>>>>>>>> update existing cells parsed from OF.
>>>>>>>>
>>>>>>> Lets keep the core code clean for now, I would expect the provider
>>>>>>> driver along with parser function to do update the nodes.
>>>>>>>
>>>>>>> --srini
>>>>>>>
>>>>>> core.c sequence:
>>>>>>
>>>>>> 1) after cells parsed from OF:
>>>>>>
>>>>>> 2) lookup the parser
>>>>>>
>>>>>> 3) parser->cells_parse(ctx, table)
>>>>>>
>>>>>> 3.a) update existing cells matched by name from table
>>>>>>
>>>>>> 4) parser->cells_clean(ctx, table)
>>>>>> /* to free cell's_info allocated by the parser driver */
>>>>>>
>>>>>> as alternative way, I think it would be more generic to
>>>>>> provide nvmem-provider.h API to update the existing cell info,
>>>>>> however it makes sense only when cells were parsed
>>>>>> by the cell parser, in the other situations the
>>>>>> cell should be well defined.
>>>>>>
>>>>>> with this approach the parser driver will be just called
>>>>>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update()
>>>>>> for each parsed cells.
>>>>>
>>>>> TBH, This is an over kill.
>>>>>
>>>>> In nvmem provider driver probe you can parse the tlv data and update the
>>>>> dt nodes before nvmem register.
>>>>>
>>>>> rest of the code should just work as it is.
>>>>>
>>>>> --srini
>>>>
>>>> You mean to parse TLV in the particular nvmem provider driver (for
>>>> example in at24 driver) ? If so, then it will not allow to parse
>>>> this TLV format from the other kinds of nvmem.
>>>
>>> Why not?
>>>
>> 
>> Well, I think that nvmem driver and TLV parsing should somehow be
>> de-coupled from each other like block devices and FS does. BUT,
>> in case this TLV data will be used only on at24 devices then
>> may be it is OK.
>> 
>
> It has to be decoupled yes, which could be at any level with simple 
> library function to a infrastructure level..
>
> In this case with few or single users, doing with an additional 
> infrastructure is a bit of over kill IMO.
>
>
> --srini
>>> Can you also tell us which other nvmem providers are you going to test
>>> this on?
>>>
>> 
>> Currently I can test only on at24 devices. From the:
>> 
>> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html
>> 
>> "
>> Each ONIE system must include non-volatile storage which contains vital
>> product data assigned by the manufacturer. The non-volatile storage
>> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash
>> sector or any other non-volatile storage medium.
>> "
>> 
>> I am not aware about other nvmem devices which are used for existing
>> ONIE supported boards.
>> 
>>> As long as you represent this parsing function as some library function,
>>> I do not see any issue.
>>> If any exported symbol is available for this then any nvmem provider
>>> could use it.
>>>
>>> --srini
>>>

Hi Srinivas,

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,
-- 
  John Thomson