Re: [PATCH v2 03/11] dtc: dt-check-style: Handle root node in overlays

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch has a meaningful code and commit message mismatch and introduces flawed parsing logic. It claims to fix property order checks for root nodes in overlays, but only modifies child name ordering logic. Furthermore, the new string-matching logic for `is_root` is brittle (causing valid root nodes with labels or extra spaces to bypass exclusion logic) and completely redundant for its intended target `&{/} {`.
--

--- Patch [3]: [PATCH v2 03/11] dtc: dt-check-style: Handle root node in overlays ---
commit caf9c0103adaa0c528c36d5ac2f2a91738b7dbea
Author: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

dtc: dt-check-style: Handle root node in overlays

Detection of overriding/extending a root node is updated to check for
expected syntax before creating DtsLine and store it as an 'is_root'
attribute to fix false positives for root nodes in overlays.

> This fixes false positives for property order for root nodes in overlays.

[Severity: Medium]
The commit message mentions fixing "property order", but the code modifies
check_child_name_order(). Is this a typo in the commit message, or was
check_property_order() intended to be modified instead?

> diff --git a/scripts/dtc/dt-check-style b/scripts/dtc/dt-check-style
> index 96deffc0d8a7c..1937c6feb57ec 100755
> --- a/scripts/dtc/dt-check-style
> +++ b/scripts/dtc/dt-check-style
[ ... ]
> @@ -228,7 +229,10 @@ def classify_lines(text):
>              continue
>  
>          if code.endswith('{'):
> -            dl = DtsLine(i, raw, LineType.NODE_OPEN, depth, indent_str, code)
> +            is_root = False
> +            if code == '&{/} {' or code == '/ {':
> +                is_root = True

[Severity: Medium]
Can this strict string comparison fail for valid root nodes?

For example, if a root node has a label like "label: / {" or contains extra
spaces like "/  {", is_root would evaluate to False. Since
"c.node_name in (None, '/')" is removed later in the patch, would these
valid nodes bypass the exclusion logic and be improperly evaluated for
child name ordering?

> +            dl = DtsLine(i, raw, LineType.NODE_OPEN, depth, indent_str, code, is_root=is_root)
>              parse_node_header(dl)
>              out.append(dl)
>              depth += 1
[ ... ]
> @@ -550,7 +554,9 @@ def check_child_name_order(ctx):
>          for c in children:
>              if c.node_addr is not None:
>                  continue
> -            if c.node_name in (None, '/'):
> +            if c.node_name is None:
> +                continue
> +            if c.is_root:
>                  continue

[Severity: Medium]
Is the c.is_root check redundant for "&{/} {" nodes?

Because "&{/} {" fails to match the regular expression in parse_node_header(),
its node_name evaluates to None. This means it would already be skipped by
the preceding "if c.node_name is None: continue" check.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802-n-dts-style-checker-continued-v2-0-6c5dbd1ccfe4@oss.qualcomm.com?part=3