Re: [PATCH v2 17/17] devicetree: Add bindings for ftrace KHO

[Krzysztof Kozlowski <krzk@kernel.org>]

On 22/12/2023 20:51, Alexander Graf wrote:
> With ftrace in KHO, we are creating an ABI between old kernel and new
> kernel about the state that they transfer. To ensure that we document
> that state and catch any breaking change, let's add its schema to the
> common devicetree bindings. This way, we can quickly reason about the
> state that gets passed.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

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.

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.

> 
> Signed-off-by: Alexander Graf <graf@amazon.com>
> ---
>  .../bindings/kho/ftrace/ftrace-array.yaml     | 46 +++++++++++++++
>  .../bindings/kho/ftrace/ftrace-cpu.yaml       | 56 +++++++++++++++++++
>  .../bindings/kho/ftrace/ftrace.yaml           | 48 ++++++++++++++++
>  3 files changed, 150 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml
>  create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace-cpu.yaml
>  create mode 100644 Documentation/devicetree/bindings/kho/ftrace/ftrace.yaml
> 
> diff --git a/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml
> new file mode 100644
> index 000000000000..9960fefc292d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/kho/ftrace/ftrace-array.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/kho/ftrace/ftrace-array.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ftrace trace array
> +

Missing description. Commit msg also does not tell me much. This must
stand on its own and must describe the hardware. Whatever you have in
cover letter, does not matter, especially that you did not Cc us on it.

> +maintainers:
> +  - Alexander Graf <graf@amazon.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ftrace,array-v1
> +
> +  trace_flags:

Underscores are not allowed. Does not look like generic property.


> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Bitmap of all the trace flags that were enabled in the trace array at the
> +      point of serialization.
> +
> +# Subnodes will be of type "ftrace,cpu-v1", one each per CPU

> +additionalProperties: true

No, this must be false. And it goes after required:


> +
> +required:
> +  - compatible
> +  - trace_flags
> +
> +examples:
> +  - |
> +    ftrace {
> +        compatible = "ftrace-v1";
> +        events = <1 1 2 2 3 3>;
> +
> +        global_trace {

Again, no underscores.

> +          compatible = "ftrace,array-v1";
> +          trace_flags = < 0x3354601 >;
> +
> +          cpu0 {
> +            compatible = "ftrace,cpu-v1";
> +            cpu = < 0x00 >;

Drop redundant spaces.

> +            mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>;

? Do you see any of such syntax in DTS?

Best regards,
Krzysztof