linux-kernel - Re: [PATCH v2 1/4] dt-bindings: nvmem: Add bootcount-nvmem

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]

Date:   Fri, 30 Apr 2021 11:21:00 +0300
From:   Nandor Han <nandor.han@...sala.com>
To:     Rob Herring <robh@...nel.org>
Cc:     srinivas.kandagatla@...aro.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/4] dt-bindings: nvmem: Add bootcount-nvmem

Hi,

Thanks for your feedback.

<snip>

>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - linux,bootcount-nvmem
> 
> What makes this Linux specific? IIRC, u-boot has boot counting function
> too.
> 

U-Boot has indeed the counterpart functionality of bootcount feature, 
however, in this particularly case is not called `u-boot,bootcount-nvmem`.
If you have any suggestions I'm happy to change it.
Should I remove the `linux` prefix?

<snip>

>> +
>> +  linux,bootcount-magic:
>> +    description: Override default mask value.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> I don't understand what this is. Is it magic or a mask?

It's the magic value. Seems to be a mistake in the description.
I'll correct this.

> 
>> +
>> +dependencies:
>> +  nvmem-cell-names: [ nvmem-cells ]
> 
> Core schema takes care of this.
> 

Ok. I will remove it in that case.

<snip>

>> +examples:
>> +  # example with 16 bit nvram cell:
>> +  - |
>> +    bootcount {
>> +        compatible = "linux,bootcount-nvmem";
>> +        nvmem-cells = <&bootcount_regs>;
>> +        nvmem-cell-names = "bootcount-regs";
>> +    };
>> +
>> +    rtc: rtc@68 {
>> +        bootcount_regs: bootcount_nvmem_regs@e {
>> +            reg = <0x0e 0x2>;
> 
> It would be simpler to just add a compatible here and get rid of the
> 'bootcount' node here.
> 

This is the configuration for NVMEM cell provider and is done according 
to bindings/nvmem.yaml document.
Is here something that I'm missing?

At least this method of declaring NVMEM cells providers are decoupling 
the code from hardware declaration resulting in a more robust code.

Regards,
    Nandor