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

[sashiko-bot@kernel.org]

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 <krzysztof.kozlowski@oss.qualcomm.com>

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

Is this the intended behavior?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726-n-dts-style-checker-continued-v1-0-b05b8f897670@oss.qualcomm.com?part=4