Hi Zekun,

On Mon, Feb 10, 2025 at 02:47:28PM +0800, zhangzekun (A) wrote:

quoted

I think we all agree that of_find_node_by_name() is miused, and that it
shows the API isn't optimal. What we have different opinions on is how
to make the API less error-prone. I think adding a new
of_find_node_by_name_balanced() function works around the issue and
doesn't improve the situation much, I would argue it makes things even
more confusing.

We have only 20 calls to of_find_node_by_name() with a non-NULL first
argument in v6.14-rc1:

arch/powerpc/platforms/chrp/pci.c:      rtas = of_find_node_by_name (root, "rtas");

The 'root' variable here is the result of a call to
'of_find_node_by_path("/")', so I think we could pass a null pointer
instead to simplify things.

arch/powerpc/platforms/powermac/pic.c:          slave = of_find_node_by_name(master, "mac-io");

Here I believe of_find_node_by_name() is called to find a *child* node
of 'master'. of_find_node_by_name() is the wrong function for that.

arch/sparc/kernel/leon_kernel.c:        np = of_find_node_by_name(rootnp, "GAISLER_IRQMP");
arch/sparc/kernel/leon_kernel.c:                np = of_find_node_by_name(rootnp, "01_00d");
arch/sparc/kernel/leon_kernel.c:        np = of_find_node_by_name(nnp, "GAISLER_GPTIMER");
arch/sparc/kernel/leon_kernel.c:                np = of_find_node_by_name(nnp, "01_011");

Here too the code seems to be looking for child nodes only (but I
couldn't find a DT example or binding in-tree, so I'm not entirely
sure).

drivers/clk/ti/clk.c:   return of_find_node_by_name(from, tmp);

Usage here seems correct, the reference-count decrement is intended.

drivers/media/i2c/max9286.c:    i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
drivers/media/platform/qcom/venus/core.c:       enp = of_find_node_by_name(dev->of_node, node_name);
drivers/net/dsa/bcm_sf2.c:      ports = of_find_node_by_name(dn, "ports");
drivers/net/dsa/hirschmann/hellcreek_ptp.c:     leds = of_find_node_by_name(hellcreek->dev->of_node, "leds");
drivers/net/ethernet/broadcom/asp2/bcmasp.c:    ports_node = of_find_node_by_name(dev->of_node, "ethernet-ports");
drivers/net/ethernet/marvell/prestera/prestera_main.c:  ports = of_find_node_by_name(sw->np, "ports");
drivers/net/pse-pd/tps23881.c:  channels_node = of_find_node_by_name(priv->np, "channels");
drivers/regulator/scmi-regulator.c:     np = of_find_node_by_name(handle->dev->of_node, "regulators");
drivers/regulator/tps6594-regulator.c:          np = of_find_node_by_name(tps->dev->of_node, multi_regs[multi].supply_name);

Incorrect usage, as far as I understand all those drivers are looking
for child nodes only.

drivers/of/unittest.c:          found = of_find_node_by_name(nd->overlay, "test-unittest16");
drivers/of/unittest.c:          found = of_find_node_by_name(nd->overlay, "test-unittest17");
drivers/of/unittest.c:          found = of_find_node_by_name(nd->overlay, "test-unittest18");
drivers/of/unittest.c:          found = of_find_node_by_name(nd->overlay, "test-unittest19");

Here too I think only child nodes are meant to be considered.

of_find_node_by_name() is very much misused as most callers want to find
child nodes, while of_find_node_by_name() will walk the whole DT from a
given starting point.

I think the right fix here is to

- Replace of_find_node_by_name(root, ...) with
   of_find_node_by_name(NULL, ...) in arch/powerpc/platforms/chrp/pci.c
   (if my understanding of the code is correct).

For arch/powerpc/platforms/chrp/pci.c, noticing that there is a comment 
in setup_peg2():
  /* keep the reference to the root node */

It might can not be convert to of_find_node_by_name(NULL, ...), and the 
origin use of of_find_node_by_name() put the ref count which want to be 
kept.

But the reference is dropped by of_find_node_by_name(). Unless I'm
missing something, dropping the lien

	struct device_node *root = of_find_node_by_path("/");

and changing

	rtas = of_find_node_by_name (root, "rtas");

to

	rtas = of_find_node_by_name (NULL, "rtas");

will not change the behaviour of the code.

quoted

- Replace of_find_node_by_name() with of_get_child_by_name() in callers
   that need to search immediate children only (I expected that to be the
   majority of the above call sites)

Since there is no enough information about these DT nodes, it would take 
time to prove if it is OK to make such convert.

It will take a bit of time, yes. I'm afraid time is needed to improve
things :-) In most cases, as DT bindings are available, it shouldn't be
very difficult.

quoted

- If there are other callers that need to find indirect children,
   introduce a new of_get_child_by_name_recursive() function.

At that point, the only remaining caller of of_find_node_by_name()
(beside its usage in the for_each_node_by_name() macro) will be
drivers/clk/ti/clk.c, which uses the function correctly.

I'm tempted to then rename of_find_node_by_name() to
__of_find_node_by_name() to indicate it's an internal function not meant
to be called except in special cases. It could all be renamed to
__of_find_next_node_by_name() to make its behaviour clearer.

The actual code logic of of_find_node_by_name() is more suitable to be 
used in a loop.So,rename of_find_node_by_name() to 
__of_find_next_node_by_name() seems to be a good idea.

-- 
Regards,

Laurent Pinchart