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

On 02/08/2026 11:01, [email protected] wrote:
> 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 <[email protected]>
> 
> 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

Not valid DTS

> spaces like "/  {", is_root would evaluate to False. Since

Handled elsewhere, but I can use pattern here

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

False positives, top-level in overlays is properly handled.


Best regards,
Krzysztof