linux-kernel - Re: [RFC PATCH 2/2] dt-bindings: add device tree binding for silex multipk

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: <37707900-9162-43f2-b89b-3e1fec514daf@kernel.org>
Date: Wed, 12 Mar 2025 11:51:05 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Nipun Gupta <nipun.gupta@....com>, linux-kernel@...r.kernel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 derek.kiernan@....com, dragan.cvetic@....com, arnd@...db.de,
 gregkh@...uxfoundation.org
Cc: praveen.jain@....com, harpreet.anand@....com, nikhil.agarwal@....com,
 srivatsa@...il.mit.edu, code@...icks.com, ptsm@...ux.microsoft.com
Subject: Re: [RFC PATCH 2/2] dt-bindings: add device tree binding for silex
 multipk

On 12/03/2025 10:54, Nipun Gupta wrote:
> Add binding documentation for Silex multipk device node with compatible
> string as 'silex,mutlipk'.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> Signed-off-by: Nipun Gupta <nipun.gupta@....com>
> ---
>  .../bindings/misc/silex,multipk.yaml          | 50 +++++++++++++++++++

Bindings are before users.

>  MAINTAINERS                                   |  1 +
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/silex,multipk.yaml
> 
> diff --git a/Documentation/devicetree/bindings/misc/silex,multipk.yaml b/Documentation/devicetree/bindings/misc/silex,multipk.yaml
> new file mode 100644
> index 000000000000..6951886734ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/silex,multipk.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/silex,multipk.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Silex MultiPK driver

Drop "driver" and describe hardware.

> +
> +maintainers:
> +  - Nipun Gupta <nipun.gupta@....com>
> +  - Praveen Jain <praveen.jain@....com>
> +
> +description: |
> +  Silex Multipk device handles the Asymmetric crypto operations. The
> +  driver provides interface to user-space to directly interact with the
> +  Silex MultiPK device.

Why this isn't in crypto?

> +
> +properties:
> +  compatible:
> +    const: silex,mutlipk

Unknown vendor prefix

Device name part is weirdly generic. How is this device exactly called?
Where is it used? Where is datasheet?

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reg:
> +    items:
> +      - description: PKI Queues memory region
> +      - description: PKI TRNG memory region
> +      - description: PKI reset memory region

reset? Like reset controller? Why is this here instead of using existing
reset framework?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - iommus

You did not test your patches.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    multipk@...00000000 {
> +      compatible = "silex,multipk";
> +      interrupts = <0x00000000 0x0000009b IRQ_TYPE_LEVEL_HIGH>;

This is nowhere DTS coding style. See how other bindings do it.

> +      reg = <0x00000204 0x00000000 0x00000000 0x00010000>,
> +            <0x00000204 0x00020000 0x00000000 0x00000050>,
> +            <0x00000000 0xEC200340 0x00000000 0x00000004>;

lowercase hex, drop the padings of r0.

> +      iommus = <&smmu 0x25B>;

Lowercase hex

> +    };
Why is this patch a RFC? What is incomplete here?

RFC means patch is not ready so you will not get full review. Full
review will come once you send proper patch (and remember about
changelog and versioning - this is v1).

Best regards,
Krzysztof