Current implementation inserts nodes at the head of the list, resulting
in sibling order being reversed from the .dts file. Some drivers care
about the order and do not work properly. These changes add nodes at the
end of the list instead, preversing sibling order.

Signed-off-by: Stephen Gordon <gordoste@iinet.net.au>
---

I ran across this issue using the ASoC audio_graph_card2 driver. Prior
to the fix, I needed to reverse sibling order in the .dts to make things
work. After the fix, it all works as expected.

Also, I noticed that drivers/of/fdt.c line 325-330 fix the same problem
for flattened device trees.


 drivers/of/dynamic.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 0aba760f7577..57bea2d4af30 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -222,8 +222,15 @@ static void __of_attach_node(struct device_node *np)
 	}
 
 	np->child = NULL;
-	np->sibling = np->parent->child;
-	np->parent->child = np;
+	np->sibling = NULL;
+	struct device_node *last_child = np->parent->child;
+	if (!last_child)
+		np->parent->child = np;
+	else {
+		while (last_child->sibling)
+			last_child = last_child->sibling;
+		last_child->sibling = np;
+	}
 	of_node_clear_flag(np, OF_DETACHED);
 	np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
 
-- 
2.39.5

On Mon, Dec 30, 2024 at 6:03 AM Stephen Gordon <gordoste@iinet.net.au> wrote:
>
> Current implementation inserts nodes at the head of the list, resulting
> in sibling order being reversed from the .dts file. Some drivers care
> about the order and do not work properly. These changes add nodes at the
> end of the list instead, preversing sibling order.

s/preversing/preserving/

>
> Signed-off-by: Stephen Gordon <gordoste@iinet.net.au>
> ---
>
> I ran across this issue using the ASoC audio_graph_card2 driver. Prior
> to the fix, I needed to reverse sibling order in the .dts to make things
> work. After the fix, it all works as expected.

The order should not be significant. What are the nodes where the order matters?

If the order matters, we create yet another problem with overlays
because if an overlay adds a child node where does it go WRT existing
child nodes? There is no way for the overlay to express that.

> Also, I noticed that drivers/of/fdt.c line 325-330 fix the same problem
> for flattened device trees.

Obviously some platforms cared at some point. Who knows if those still
exist or not. I'd rather not create more unknown cases. Though it's
probably not possible to enumerate the exceptions here. (I'm trying to
reduce the number of unconditional work-arounds for bad DTs in the DT
code and make them explicit so that we don't see new cases and can
remove work-arounds if platforms are removed.)

Rob

On 3/01/2025 3:46 am, Rob Herring wrote:

> The order should not be significant. What are the nodes where the order matters?

The devicetree spec certainly doesn't make any guarantees of node order, 
so this is a reasonable question.

The driver in question is using the of_graph_* functions with a tree like:

ports {
	p0: port@0 { reg = <0>; endpoint { }; };
	p1: port@1 { reg = <1>; endpoint { }; };
	p2: port@2 { reg = <2>; endpoint { }; };
};

The driver navigates to p0 using various graph functions, because it 
needs to process that before any other endpoint. It then uses 
of_graph_get_next_port_endpoint to iterate through the remaining 
endpoints. However, when the DT is created by dynamic.c, p0 is last and 
no further endpoints are returned.  If the tree is created by fdt.c or 
pdt.c, everything works fine, because the list of siblings is in the 
order they were specified in the file.

> If the order matters, we create yet another problem with overlays
> because if an overlay adds a child node where does it go WRT existing
> child nodes? There is no way for the overlay to express that.
> 

AFAICS, this is the only example where child nodes are not inserted at 
the end, so a different order is guaranteed when the tree is loaded by 
this code (versus fdt.c/pdt.c).

For example, code using a phandle to access the "first" child of a node 
and then calling of_get_next_child() to attempt to iterate through the 
siblings will work fine - except on trees loaded by dynamic.c.

In the end, it's up to you. If this can't be changed, then we can fairly 
easily work around it. I just thought it was worth letting you know that 
the various tree-building mechanisms behave differently in this regard.

Happy New Year!

Regards
Stephen

On Thu, Jan 2, 2025 at 8:08 PM Stephen Gordon <gordoste@iinet.net.au> wrote:
>
> On 3/01/2025 3:46 am, Rob Herring wrote:
>
> > The order should not be significant. What are the nodes where the order matters?
>
> The devicetree spec certainly doesn't make any guarantees of node order,
> so this is a reasonable question.
>
> The driver in question is using the of_graph_* functions with a tree like:
>
> ports {
>         p0: port@0 { reg = <0>; endpoint { }; };
>         p1: port@1 { reg = <1>; endpoint { }; };
>         p2: port@2 { reg = <2>; endpoint { }; };
> };

Well, this should be easy to fix. We have addresses, so we should
iterate in address order.

> The driver navigates to p0 using various graph functions, because it
> needs to process that before any other endpoint. It then uses
> of_graph_get_next_port_endpoint to iterate through the remaining
> endpoints. However, when the DT is created by dynamic.c, p0 is last and
> no further endpoints are returned.  If the tree is created by fdt.c or
> pdt.c, everything works fine, because the list of siblings is in the
> order they were specified in the file.

Can you just iterate by the index instead? We have functions which
return the specified port and/or endpoint number.

We can't really wholesale sort the child list to address order. While
that is the preferred source order as well, I'm sure there's something
somewhere depending on the order. The order can affect probe order
just like linking order does. However, I think we can safely change
the graph functions to guarantee address order. That means we need a
version of of_get_next_child() which guarantees it goes in address
order and then use that in the graph functions. It won't be efficient
since each iteration has to walk the whole list of children, but
there's never a lot of ports/endpoints.

Rob