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

[Alexander Graf <graf@amazon.com>]

Hi Krzysztof!

Thanks a lot for the fast review!

On 23.12.23 15:30, Krzysztof Kozlowski wrote:
> 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.


Ah, this is about directly CC'ing maintainers? I was slightly picky on 
CCs since the CC list is already a bit long for this patch set, so I 
limited the CC list to mailing lists and people that I know were 
directly interested. Happy to CC you next time.


>
> 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.


Happy to fix up for v3 :)


>
>> 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.


Alrighty, I'll add descriptions and make the commit message stand on its 
own.

For quick reference: KHO is a new mechanism this patch set introduces 
which allows Linux to pass arbitrary memory and metadata between kernels 
on kexec. I'm reusing FDTs to implement the hand over protocol, as 
Linux-to-Linux boot communication holds very similar properties to 
firmware-to-Linux boot communication. So this binding is not about 
hardware; it's about preserving Linux subsystem state across kexec.

For more details, please refer to the KHO documentation which is part of 
patch 7 of this patch set: 
https://lore.kernel.org/lkml/20231222195144.24532-2-graf@amazon.com/


>
>> +maintainers:
>> +  - Alexander Graf <graf@amazon.com>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ftrace,array-v1
>> +
>> +  trace_flags:
> Underscores are not allowed. Does not look like generic property.


Let me make it "trace-flags" to not have underscores. Could you please 
elaborate on what you mean by 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:


Ok, making it false and adding pattern matches instead for subnodes.


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


Ok :)


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


I don't understand what you're referring to as redundant spaces? Double 
checking, I believe indentation is off for every line below "ftrace {". 
Is that what you're referring to? Fixing :)


>
>> +            mem = < 0x101000000ULL 0x38ULL 0x101000100ULL 0x1000ULL 0x101000038ULL 0x38ULL 0x101002000ULL 0x1000ULL>;
> ? Do you see any of such syntax in DTS?


I was trying to make it easy to reason to readers about 64bit numbers - 
and then potentially extend dtc to consume that new syntax. KHO DTs are 
native/little endian, so dtc already has some difficulties interpreting 
it which I'll need to fix up with patches to it eventually :). I'll 
change it to something that looks more 32bit'y for now.


Alex




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879