linux-kernel - Re: [PATCH 1/6] dt-bindings: edac: altera: Document additional ECC instances

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]

Message-ID: <20251029-adventurous-russet-jackal-64e3e5@kuoka>
Date: Wed, 29 Oct 2025 07:50:02 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: niravkumarlaxmidas.rabara@...era.com
Cc: dinguyen@...nel.org, matthew.gerlach@...era.com, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, bp@...en8.de, tony.luck@...el.com, 
	linux-edac@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] dt-bindings: edac: altera: Document additional ECC
 instances

On Tue, Oct 28, 2025 at 05:22:27PM +0800, niravkumarlaxmidas.rabara@...era.com wrote:
> From: Niravkumar L Rabara <niravkumarlaxmidas.rabara@...era.com>
> 
> Add support for Secure Device Manager(SDM) QSPI ECC, IO96B memory
> controller ECC and Configuration RAM(CRAM) Single Event Upset(SEU).
> 
> Add interrupt-names property and increase interrupts maxItems from 2 to 7
> to accommodate additional interrupts.
> 
> Signed-off-by: Niravkumar L Rabara <niravkumarlaxmidas.rabara@...era.com>
> ---
>  .../edac/altr,socfpga-ecc-manager.yaml        | 77 ++++++++++++++++++-
>  1 file changed, 76 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/edac/altr,socfpga-ecc-manager.yaml b/Documentation/devicetree/bindings/edac/altr,socfpga-ecc-manager.yaml
> index 3d787dea0f14..5e0c08a15ab9 100644
> --- a/Documentation/devicetree/bindings/edac/altr,socfpga-ecc-manager.yaml
> +++ b/Documentation/devicetree/bindings/edac/altr,socfpga-ecc-manager.yaml
> @@ -33,7 +33,13 @@ properties:
>  
>    interrupts:
>      minItems: 1
> -    maxItems: 2
> +    maxItems: 7

No, list the interrupts instead. Your commit msg must clearly explain
why exception of not-fixed length/entries is justified.

See writing bindings.

> +
> +  interrupt-names:
> +    items:
> +      enum: [global_sbe, global_dbe, io96b0, io96b1, sdm_qspi_sbe, sdm_qspi_dbe, sdm_seu]

Nope, list the items instead. Please do not come up with some custom
syntax.

> +    minItems: 1
> +    maxItems: 7
>  
>    interrupt-controller: true
>  
> @@ -70,6 +76,41 @@ properties:
>        - interrupts
>        - altr,sdr-syscon
>  
> +  cram-seu:

Missing description, so difficult to say what is here.
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
If you cannot find a name matching your device, please check in kernel
sources for similar cases or you can grow the spec (via pull request to
DT spec repo).

> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      compatible:
> +        items:
> +          - const: altr,socfpga-cram-seu

Why do you need compatible?

> +
> +      reg:
> +        maxItems: 1

So you created child node only for reg? No, fold it into parent.

You also forgot to update the example.

> +
> +      altr,seu-safe-inject-ce-msb:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: MSB of error injection command for Correctable Error
> +
> +      altr,seu-safe-inject-ce-lsb:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: LSB of error injection command for Correctable Error
> +
> +      altr,seu-safe-inject-ue-msb:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: MSB of error injection command for Uncorrectable Error
> +
> +      altr,seu-safe-inject-ue-lsb:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: LSB of error injection command for Uncorrectable Error

How are these board-level properties?

> +
> +    required:
> +      - compatible
> +      - altr,seu-safe-inject-ce-msb
> +      - altr,seu-safe-inject-ce-lsb
> +      - altr,seu-safe-inject-ue-msb
> +      - altr,seu-safe-inject-ue-lsb
> +
>  patternProperties:
>    "^ocram-ecc@[a-f0-9]+$":
>      type: object
> @@ -191,6 +232,40 @@ patternProperties:
>        - interrupts
>        - altr,ecc-parent
>  
> +  "^sdm-qspi-ecc@[a-f0-9]+$":
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      compatible:
> +        items:
> +          - const: altr,socfpga-sdm-qspi-ecc

No, drop.

> +
> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg

No point for empty children. One reg is not justification for having a
child.

> +
> +  "^io96b[0-9]-ecc@[a-f0-9]+$":

You need to stop coming with random node names. Nothing explains why you
need children, why these are not part of parent node.

> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      compatible:
> +        items:
> +          - enum:
> +              - altr,socfpga-io96b0-ecc
> +              - altr,socfpga-io96b1-ecc

Plus all your compatibles have WRONG format. See writing bindings and
numerouse presentations - you always must use SoC specific compatible.

Best regards,
Krzysztof