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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
---
  .../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@xxxxxxxxxx/



+maintainers:
+  - Alexander Graf <graf@xxxxxxxxxx>
+
+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






[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux