Re: [PATCH 4/9] dtc: dt-check-style: Handle sorting of top-level nodes and properties

On 26/07/2026 17:57, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] Tree hierarchy tracking in `_walk_bodies()` is broken, incorrectly assigning closed sibling nodes as parents to subsequent siblings.
> - [Medium] `_property_bucket_root()` collapses standard and vendor properties into a single bucket, breaking canonical property ordering at the root node.
> - [Medium] Top-level addressed nodes are exempted from address sorting but are never subjected to the node name sorting claimed in the commit message.
> --
> 
> --- Patch [4]: [PATCH 4/9] dtc: dt-check-style: Handle sorting of top-level nodes and properties ---
> commit d752014f34dc3d4a2a9f9617748ec741da0cb1c5
> Author: Krzysztof Kozlowski <[email protected]>
> 
> dtc: dt-check-style: Handle sorting of top-level nodes and properties
> 
> This commit updates the DTS styling rules for top-level nodes and properties.
> It skips unit-address sorting for root nodes since they mix unit and non-unit
> addresses. It also adds a special case for the 'model' property to sort before
> 'compatible' at the root node.
> 
>> diff --git a/scripts/dtc/dt-check-style b/scripts/dtc/dt-check-style
>> index 1937c6feb57ec..db4451e30dfbc 100755
>> --- a/scripts/dtc/dt-check-style
>> +++ b/scripts/dtc/dt-check-style
> 
> [ ... ]
> 
>> @@ -496,15 +497,21 @@ def _walk_bodies(lines):
>>      in the input. Skips ref-nodes (&label) since those don't have an
>>      intrinsic ordering."""
>>      body_stack = [[]]
>> +    node_stack = [[]]
>> +    parent_dl = None
>>      for dl in lines:
>>          if dl.linetype == LineType.NODE_OPEN:
>> +            dl.parent = parent_dl
>> +            node_stack.append(dl)
>>              body_stack[-1].append(dl)
>>              body_stack.append([])
>> +            parent_dl = dl
>>              continue
>>          if dl.linetype == LineType.NODE_CLOSE:
>>              if len(body_stack) <= 1:
>>                  # Unbalanced; ignore to avoid crashing on malformed input
>>                  continue
>> +            parent_dl = node_stack.pop()
> 
> [Severity: Medium]
> Does this pop the node that just closed instead of the actual parent?
> 
> When node_stack.pop() is called here, it returns the node that just closed
> because that was the last one appended. Does this cause subsequent siblings

That's intended, since we go up in the hierarchy. Current parent should
be now what we left on the stack.

> to incorrectly inherit their preceding closed sibling as their parent instead
> of the actual parent, corrupting the tree state for checks?
> 
> [ ... ]
> 
>> @@ -525,12 +532,18 @@ def _natural_sort_key(s):
>>  
>>  def check_child_address_order(ctx):
>>      """Addressed siblings (foo@N) must appear in ascending address
>> -    order within their parent node body."""
>> +    order within their parent node body.
>> +    Exception: Top-level in DTS follows name order, regardless of unit address
>> +    in memory@N and soc@N nodes
>> +    """
>>      for children in _walk_bodies(ctx.lines):
>>          addressed = []
>>          for c in children:
>>              if c.node_addr is None:
>>                  continue
>> +            if c.parent and c.parent.is_root:
>> +                # Top-level does not use unit address sorting usually
>> +                continue
> 
> [Severity: Medium]
> The commit message mentions that the top-level part usually chooses sorting
> by node name, not the unit address. However, looking at
> check_child_name_order() elsewhere in the file, it unconditionally skips

Valid point.

> nodes with a unit address:
> 
>     if c.node_addr is not None:
>         continue
> 
> By skipping addressed nodes here in check_child_address_order() and not
> updating check_child_name_order() to handle them, are top-level addressed
> nodes now completely excluded from any ordering enforcement?


> 
> [ ... ]
> 
>> @@ -597,6 +610,22 @@ def _property_bucket(name):
>>      return (5 if ',' in stripped else 4, None)
>>  
>>  
>> +def _property_bucket_root(name):
>> +    """Return the canonical bucket index for a property:
>> +       0 model (for root nodes only)
>> +       1 compatible
>> +    Plus a sub-key inside the bucket for fixed slots (device_type, compatible,
>> +    reg, reg-names, ranges, status). 'standard' and 'vendor' return None for
>> +    the sub-key, signalling that the within-bucket key is computed by
>> +    the pairing rules."""
>> +    stripped = name.lstrip('#')
>> +    if name == 'model':
>> +        return (0, 0)
>> +    if name == 'compatible':
>> +        return (1, 0)
>> +    return (2, None)
> 
> [Severity: Medium]
> Does returning (2, None) here for all unhandled properties override the
> standard bucketing rules for the root node?
> 
> The original _property_bucket() sorts properties into buckets (like 4 for
> standard and 5 for vendor). Because this hardcodes (2, None) for everything
> else, standard and vendor properties at the root node might now be grouped
> together and sorted alphabetically against each other, overriding the standard
> before vendor rule.

ack

Best regards,
Krzysztof