[PATCH 0/9] Add wrapper function of_find_node_by_name_balanced()

[PATCH 0/9] Add wrapper function of_find_node_by_name_balanced()

[Zhang Zekun]

Add wrapper function of_find_node_by_name_balanced() for drivers who
want to call of_find_node_by_name() and have to blance the ref count
by calling of_node_get(). For drivers who forget to call of_node_get(),
can also utilizing of_find_node_by_name_balanced() to fix a refcount
leak.

Zhang Zekun (9):
  of: Add warpper function of_find_node_by_name_balanced()
  net: bcmasp: Add missing of_node_get() before of_find_node_by_name()
  net: pse-pd: Add missing of_node_get() before of_find_node_by_name()
  media: max9286: Use of_find_node_by_name_balanced() to find
    device_node
  powerpc: Use of_find_node_by_name_balanced() to find device_node
  net: dsa: Use of_find_node_by_name_balanced() to find device_node
  net: dsa: hellcreek: Use of_find_node_by_name_balanced() to find
    device_node
  net: prestera: Use of_find_node_by_name_balanced() to find device_node
  regulator: scmi: Use of_find_node_by_name_balanced() to find
    device_node

 arch/powerpc/platforms/powermac/pic.c                 | 4 +---
 drivers/media/i2c/max9286.c                           | 4 +---
 drivers/net/dsa/bcm_sf2.c                             | 4 +---
 drivers/net/dsa/hirschmann/hellcreek_ptp.c            | 3 +--
 drivers/net/ethernet/broadcom/asp2/bcmasp.c           | 2 +-
 drivers/net/ethernet/marvell/prestera/prestera_main.c | 3 +--
 drivers/net/pse-pd/tps23881.c                         | 2 +-
 drivers/regulator/scmi-regulator.c                    | 3 +--
 include/linux/of.h                                    | 5 +++++
 9 files changed, 13 insertions(+), 17 deletions(-)

-- 
2.22.0

[PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Zhang Zekun]

There are many drivers use of_find_node_by_name() with a not-NULL
device_node pointer, and a number of callers would require a call to
of_node_get() before using it. There are also some drivers who forget
to call of_node_get() which would cause a ref count leak[1]. So, Add a
wraper function for of_find_node_by_name(), drivers may use this function
to call of_find_node_by_name() with the refcount already balanced.

[1] https://lore.kernel.org/all/20241024015909.58654-1-zhangzekun11@huawei.com/

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 include/linux/of.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index eaf0e2a2b75c..b7c6d7ff278c 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -268,6 +268,11 @@ static inline const char *of_node_full_name(const struct device_node *np)
 #define for_each_of_allnodes(dn) for_each_of_allnodes_from(NULL, dn)
 extern struct device_node *of_find_node_by_name(struct device_node *from,
 	const char *name);
+static inline struct device_node *of_find_node_by_name_balanced(struct device_node *from,
+								const char *name)
+{
+	return of_find_node_by_name(of_node_get(from), name);
+}
 extern struct device_node *of_find_node_by_type(struct device_node *from,
 	const char *type);
 extern struct device_node *of_find_compatible_node(struct device_node *from,
-- 
2.22.0

[PATCH 2/9] net: bcmasp: Add missing of_node_get() before of_find_node_by_name()

[Zhang Zekun]

of_find_node_by_name() will decrease the refcount of the device_node.
So, get the device_node before passing to it.

Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 drivers/net/ethernet/broadcom/asp2/bcmasp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
index a68fab1b05f0..093c8ea72af9 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
@@ -1367,7 +1367,7 @@ static int bcmasp_probe(struct platform_device *pdev)
 	bcmasp_core_init(priv);
 	bcmasp_core_init_filters(priv);
 
-	ports_node = of_find_node_by_name(dev->of_node, "ethernet-ports");
+	ports_node = of_find_node_by_name_balanced(dev->of_node, "ethernet-ports");
 	if (!ports_node) {
 		dev_warn(dev, "No ports found\n");
 		return -EINVAL;
-- 
2.22.0

[PATCH 3/9] net: pse-pd: Add missing of_node_get() before of_find_node_by_name()

[Zhang Zekun]

of_find_node_by_name() will decrease the refount of the device_node.
So, get the device_node before passing to it.

Fixes: 20e6d190ffe1 ("net: pse-pd: Add TI TPS23881 PSE controller driver")
Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 drivers/net/pse-pd/tps23881.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
index 5e9dda2c0eac..a595358ac60b 100644
--- a/drivers/net/pse-pd/tps23881.c
+++ b/drivers/net/pse-pd/tps23881.c
@@ -502,7 +502,7 @@ tps23881_get_of_channels(struct tps23881_priv *priv,
 	if (!priv->np)
 		return -EINVAL;
 
-	channels_node = of_find_node_by_name(priv->np, "channels");
+	channels_node = of_find_node_by_name_balanced(priv->np, "channels");
 	if (!channels_node)
 		return -EINVAL;
 
-- 
2.22.0

[PATCH 4/9] media: max9286: Use of_find_node_by_name_balanced() to find device_node

[Zhang Zekun]

Instead of directly using of_node_get() before of_find_node_by_name()
to balance the refcount of the device_node, using wraper function
of_find_node_by_name_balanced() to make code logic a bit simplier.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 drivers/media/i2c/max9286.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 9fc4e130a273..0299d08a7196 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1399,9 +1399,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	u32 i2c_clk_freq = 105000;
 	unsigned int i;
 
-	/* Balance the of_node_put() performed by of_find_node_by_name(). */
-	of_node_get(dev->of_node);
-	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
+	i2c_mux = of_find_node_by_name_balanced(dev->of_node, "i2c-mux");
 	if (!i2c_mux) {
 		dev_err(dev, "Failed to find i2c-mux node\n");
 		return -EINVAL;
-- 
2.22.0

[PATCH 5/9] powerpc: Use of_find_node_by_name_balanced() to find device_node

[Zhang Zekun]

Instead of directly using of_node_get() before of_find_node_by_name()
to balance the refcount of the device_node, using wraper function
of_find_node_by_name_balanced() to make code logic a bit simplier.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 arch/powerpc/platforms/powermac/pic.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/powermac/pic.c b/arch/powerpc/platforms/powermac/pic.c
index 2202bf77c7a3..0619334adf2a 100644
--- a/arch/powerpc/platforms/powermac/pic.c
+++ b/arch/powerpc/platforms/powermac/pic.c
@@ -308,9 +308,7 @@ static void __init pmac_pic_probe_oldstyle(void)
 
 		/* We might have a second cascaded heathrow */
 
-		/* Compensate for of_node_put() in of_find_node_by_name() */
-		of_node_get(master);
-		slave = of_find_node_by_name(master, "mac-io");
+		slave = of_find_node_by_name_balanced(master, "mac-io");
 
 		/* Check ordering of master & slave */
 		if (of_device_is_compatible(master, "gatwick")) {
-- 
2.22.0

[PATCH 6/9] net: dsa: Use of_find_node_by_name_balanced() to find device_node

[Zhang Zekun]

Instead of directly using of_node_get() before of_find_node_by_name()
to balance the refcount of the device_node, using wraper function
of_find_node_by_name_balanced() to make code logic a bit simplier.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 drivers/net/dsa/bcm_sf2.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index fa2bf3fa9019..7567686317f1 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -1435,9 +1435,7 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev)
 	set_bit(0, priv->cfp.used);
 	set_bit(0, priv->cfp.unique);
 
-	/* Balance of_node_put() done by of_find_node_by_name() */
-	of_node_get(dn);
-	ports = of_find_node_by_name(dn, "ports");
+	ports = of_find_node_by_name_balanced(dn, "ports");
 	if (ports) {
 		bcm_sf2_identify_ports(priv, ports);
 		of_node_put(ports);
-- 
2.22.0

[PATCH 7/9] net: dsa: hellcreek: Use of_find_node_by_name_balanced() to find device_node

[Zhang Zekun]

Instead of directly using of_node_get() before of_find_node_by_name()
to balance the refcount of the device_node, using wraper function
of_find_node_by_name_balanced() to make code logic a bit simplier.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 drivers/net/dsa/hirschmann/hellcreek_ptp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/dsa/hirschmann/hellcreek_ptp.c b/drivers/net/dsa/hirschmann/hellcreek_ptp.c
index bfe21f9f7dcd..360ceb6831ed 100644
--- a/drivers/net/dsa/hirschmann/hellcreek_ptp.c
+++ b/drivers/net/dsa/hirschmann/hellcreek_ptp.c
@@ -307,8 +307,7 @@ static int hellcreek_led_setup(struct hellcreek *hellcreek)
 	const char *label;
 	int ret = -EINVAL;
 
-	of_node_get(hellcreek->dev->of_node);
-	leds = of_find_node_by_name(hellcreek->dev->of_node, "leds");
+	leds = of_find_node_by_name_balanced(hellcreek->dev->of_node, "leds");
 	if (!leds) {
 		dev_err(hellcreek->dev, "No LEDs specified in device tree!\n");
 		return ret;
-- 
2.22.0

[PATCH 8/9] net: prestera: Use of_find_node_by_name_balanced() to find device_node

[Zhang Zekun]

Instead of directly using of_node_get() before of_find_node_by_name()
to balance the refcount of the device_node, using wraper function
of_find_node_by_name_balanced() to make code logic a bit simplier.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 drivers/net/ethernet/marvell/prestera/prestera_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
index 440a4c42b405..5d10031bfa32 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
@@ -378,8 +378,7 @@ static int prestera_port_sfp_bind(struct prestera_port *port)
 	if (!sw->np)
 		return 0;
 
-	of_node_get(sw->np);
-	ports = of_find_node_by_name(sw->np, "ports");
+	ports = of_find_node_by_name_balanced(sw->np, "ports");
 
 	for_each_child_of_node(ports, node) {
 		int num;
-- 
2.22.0

[PATCH 9/9] regulator: scmi: Use of_find_node_by_name_balanced() to find device_node

[Zhang Zekun]

Instead of directly using of_node_get() before of_find_node_by_name()
to balance the refcount of the device_node, using wraper function
of_find_node_by_name_balanced() to make code logic a bit simplier.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 drivers/regulator/scmi-regulator.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c
index 9df726f10ad1..11f75c13bdf0 100644
--- a/drivers/regulator/scmi-regulator.c
+++ b/drivers/regulator/scmi-regulator.c
@@ -339,8 +339,7 @@ static int scmi_regulator_probe(struct scmi_device *sdev)
 	 * plausible SCMI Voltage Domain number, all belonging to this SCMI
 	 * platform instance node (handle->dev->of_node).
 	 */
-	of_node_get(handle->dev->of_node);
-	np = of_find_node_by_name(handle->dev->of_node, "regulators");
+	np = of_find_node_by_name_balanced(handle->dev->of_node, "regulators");
 	for_each_child_of_node_scoped(np, child) {
 		ret = process_scmi_regulator_of_node(sdev, ph, child, rinfo);
 		/* abort on any mem issue */
-- 
2.22.0

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Oleksij Rempel]

On Fri, Feb 07, 2025 at 09:31:09AM +0800, Zhang Zekun wrote:
> There are many drivers use of_find_node_by_name() with a not-NULL
> device_node pointer, and a number of callers would require a call to
> of_node_get() before using it. There are also some drivers who forget
> to call of_node_get() which would cause a ref count leak[1]. So, Add a
> wraper function for of_find_node_by_name(), drivers may use this function
> to call of_find_node_by_name() with the refcount already balanced.
> 
> [1] https://lore.kernel.org/all/20241024015909.58654-1-zhangzekun11@huawei.com/

Hi Zhang Zekun,

thank you for working on this issue!

First of all, let's take a step back and analyze the initial problem.
Everything following is only my opinion...

The main issue I see is that the current API - of_find_node_by_name -
modifies the refcount of its input by calling of_node_put(from) as part
of its search. Typically, a "find" function is expected to treat its
input as read-only. That is, when you pass an object into such a
function, you expect its reference count to remain unchanged unless
ownership is explicitly transferred. In this case, lowering the refcount
on the input node is counterintuitive and already lead to unexpected
behavior and subtle bugs.

To address this, the workaround introduces a wrapper function,
of_find_node_by_name_balanced, which first increments the input’s
refcount (via of_node_get()) before calling the original function. While
this "balances" the refcount change, the naming remains problematic from
my perspective. The "_balanced" suffix isn’t part of our common naming
conventions (traditions? :)). Most drivers expect that a function
starting with "find" will not alter the reference count of its input.
The term "balanced" doesn’t clearly convey that the input's refcount is
being explicitly managed - it instead obscures the underlying behavior,
leaving many developers confused about what guarantees the API provides.

In my view, a more natural solution would be to redesign the API so that
it doesn’t modify the input object’s refcount at all. Instead, it should
solely increase the refcount of the returned node (if found) for safe
asynchronous usage. This approach would align with established
conventions where "find" implies no side effects on inputs or output,
and a "get" indicates that the output comes with an extra reference. For
example, a function named of_get_node_by_name would clearly signal that
only the returned node is subject to a refcount increase while leaving
the input intact.

Thus, while the current workaround "balances" the reference count, it
doesn't address the underlying design flaw. The naming still suggests a
"find" function that should leave the input untouched, which isn’t the
case here. A redesign of the API - with both the behavior and naming
aligned to common expectations - would be a clearer and more robust
solution.

Nevertheless, it is only my POV, and the final decision rests with the
OpenFirmware framework maintainers.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Laurent Pinchart]

Hi Oleksij,

On Fri, Feb 07, 2025 at 09:24:10AM +0100, Oleksij Rempel wrote:
> On Fri, Feb 07, 2025 at 09:31:09AM +0800, Zhang Zekun wrote:
> > There are many drivers use of_find_node_by_name() with a not-NULL
> > device_node pointer, and a number of callers would require a call to
> > of_node_get() before using it. There are also some drivers who forget
> > to call of_node_get() which would cause a ref count leak[1]. So, Add a
> > wraper function for of_find_node_by_name(), drivers may use this function
> > to call of_find_node_by_name() with the refcount already balanced.
> > 
> > [1] https://lore.kernel.org/all/20241024015909.58654-1-zhangzekun11@huawei.com/
> 
> Hi Zhang Zekun,
> 
> thank you for working on this issue!
> 
> First of all, let's take a step back and analyze the initial problem.
> Everything following is only my opinion...
> 
> The main issue I see is that the current API - of_find_node_by_name -
> modifies the refcount of its input by calling of_node_put(from) as part
> of its search. Typically, a "find" function is expected to treat its
> input as read-only. That is, when you pass an object into such a
> function, you expect its reference count to remain unchanged unless
> ownership is explicitly transferred. In this case, lowering the refcount
> on the input node is counterintuitive and already lead to unexpected
> behavior and subtle bugs.
> 
> To address this, the workaround introduces a wrapper function,
> of_find_node_by_name_balanced, which first increments the input’s
> refcount (via of_node_get()) before calling the original function. While
> this "balances" the refcount change, the naming remains problematic from
> my perspective. The "_balanced" suffix isn’t part of our common naming
> conventions (traditions? :)). Most drivers expect that a function
> starting with "find" will not alter the reference count of its input.
> The term "balanced" doesn’t clearly convey that the input's refcount is
> being explicitly managed - it instead obscures the underlying behavior,
> leaving many developers confused about what guarantees the API provides.
> 
> In my view, a more natural solution would be to redesign the API so that
> it doesn’t modify the input object’s refcount at all. Instead, it should
> solely increase the refcount of the returned node (if found) for safe
> asynchronous usage. This approach would align with established
> conventions where "find" implies no side effects on inputs or output,
> and a "get" indicates that the output comes with an extra reference. For
> example, a function named of_get_node_by_name would clearly signal that
> only the returned node is subject to a refcount increase while leaving
> the input intact.
> 
> Thus, while the current workaround "balances" the reference count, it
> doesn't address the underlying design flaw. The naming still suggests a
> "find" function that should leave the input untouched, which isn’t the
> case here. A redesign of the API - with both the behavior and naming
> aligned to common expectations - would be a clearer and more robust
> solution.
> 
> Nevertheless, it is only my POV, and the final decision rests with the
> OpenFirmware framework maintainers.

I agree overall that the naming is not optimal. Looking at the other
patches in the series, I think at least some of them misuse
of_find_node_by_name(). For instance, drivers/media/i2c/max9286.c calls
the function to find a *child* node of the device's of_node named
"i2c-mux", while of_find_node_by_name() looks at children first but will
then walk the *whole* DT to find a named node. I haven't checked all
patches, but other ones seem to suffer from the same misuse.

Assuming that the named node those drivers are looking for is a direct
child of the node passed as argument to of_find_node_by_name(), the
right fix would tbe to use of_get_child_by_name(). If it's not a direct
child, a recursive version of of_get_child_by_name() could be useful.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[zhangzekun (A)]

在 2025/2/7 16:24, Oleksij Rempel 写道:
> On Fri, Feb 07, 2025 at 09:31:09AM +0800, Zhang Zekun wrote:
>> There are many drivers use of_find_node_by_name() with a not-NULL
>> device_node pointer, and a number of callers would require a call to
>> of_node_get() before using it. There are also some drivers who forget
>> to call of_node_get() which would cause a ref count leak[1]. So, Add a
>> wraper function for of_find_node_by_name(), drivers may use this function
>> to call of_find_node_by_name() with the refcount already balanced.
>>
>> [1] https://lore.kernel.org/all/20241024015909.58654-1-zhangzekun11@huawei.com/
> 
> Hi Zhang Zekun,
> 
> thank you for working on this issue!
> 
> First of all, let's take a step back and analyze the initial problem.
> Everything following is only my opinion...
> 
> The main issue I see is that the current API - of_find_node_by_name -
> modifies the refcount of its input by calling of_node_put(from) as part
> of its search. Typically, a "find" function is expected to treat its
> input as read-only. That is, when you pass an object into such a
> function, you expect its reference count to remain unchanged unless
> ownership is explicitly transferred. In this case, lowering the refcount
> on the input node is counterintuitive and already lead to unexpected
> behavior and subtle bugs.
> 
> To address this, the workaround introduces a wrapper function,
> of_find_node_by_name_balanced, which first increments the input’s
> refcount (via of_node_get()) before calling the original function. While
> this "balances" the refcount change, the naming remains problematic from
> my perspective. The "_balanced" suffix isn’t part of our common naming
> conventions (traditions? :)). Most drivers expect that a function
> starting with "find" will not alter the reference count of its input.
> The term "balanced" doesn’t clearly convey that the input's refcount is
> being explicitly managed - it instead obscures the underlying behavior,
> leaving many developers confused about what guarantees the API provides.
> 
> In my view, a more natural solution would be to redesign the API so that
> it doesn’t modify the input object’s refcount at all. Instead, it should
> solely increase the refcount of the returned node (if found) for safe
> asynchronous usage. This approach would align with established
> conventions where "find" implies no side effects on inputs or output,
> and a "get" indicates that the output comes with an extra reference. For
> example, a function named of_get_node_by_name would clearly signal that
> only the returned node is subject to a refcount increase while leaving
> the input intact.
> 
> Thus, while the current workaround "balances" the reference count, it
> doesn't address the underlying design flaw. The naming still suggests a
> "find" function that should leave the input untouched, which isn’t the
> case here. A redesign of the API - with both the behavior and naming
> aligned to common expectations - would be a clearer and more robust
> solution.
> 
> Nevertheless, it is only my POV, and the final decision rests with the
> OpenFirmware framework maintainers.
> 
> Best Regards,
> Oleksij

Hi, Oleksij,

Thanks for your review. I think redesign the API would be a fundamental 
way to fix this issue, but it would cause impact for current users who 
knows the exact functionality of of_find_node_by_name(), which need to 
put the "from" before calling it (I can't make the assumption that all 
of drivers calling of_find_node_by_name() with a not-NULL "from" 
pointer, but not calling of_node_get() before is misuse). The basic idea 
for adding a new function is try not to impact current users. For users 
who are confused about the "_balanced" suffix,the comments of 
of_find_node_by_name() explains why this interface exists.

Thanks,
Zekun

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Laurent Pinchart]

Hi Zekun,

On Fri, Feb 07, 2025 at 07:28:23PM +0800, zhangzekun (A) wrote:
> 在 2025/2/7 16:24, Oleksij Rempel 写道:
> > On Fri, Feb 07, 2025 at 09:31:09AM +0800, Zhang Zekun wrote:
> >> There are many drivers use of_find_node_by_name() with a not-NULL
> >> device_node pointer, and a number of callers would require a call to
> >> of_node_get() before using it. There are also some drivers who forget
> >> to call of_node_get() which would cause a ref count leak[1]. So, Add a
> >> wraper function for of_find_node_by_name(), drivers may use this function
> >> to call of_find_node_by_name() with the refcount already balanced.
> >>
> >> [1] https://lore.kernel.org/all/20241024015909.58654-1-zhangzekun11@huawei.com/
> > 
> > Hi Zhang Zekun,
> > 
> > thank you for working on this issue!
> > 
> > First of all, let's take a step back and analyze the initial problem.
> > Everything following is only my opinion...
> > 
> > The main issue I see is that the current API - of_find_node_by_name -
> > modifies the refcount of its input by calling of_node_put(from) as part
> > of its search. Typically, a "find" function is expected to treat its
> > input as read-only. That is, when you pass an object into such a
> > function, you expect its reference count to remain unchanged unless
> > ownership is explicitly transferred. In this case, lowering the refcount
> > on the input node is counterintuitive and already lead to unexpected
> > behavior and subtle bugs.
> > 
> > To address this, the workaround introduces a wrapper function,
> > of_find_node_by_name_balanced, which first increments the input’s
> > refcount (via of_node_get()) before calling the original function. While
> > this "balances" the refcount change, the naming remains problematic from
> > my perspective. The "_balanced" suffix isn’t part of our common naming
> > conventions (traditions? :)). Most drivers expect that a function
> > starting with "find" will not alter the reference count of its input.
> > The term "balanced" doesn’t clearly convey that the input's refcount is
> > being explicitly managed - it instead obscures the underlying behavior,
> > leaving many developers confused about what guarantees the API provides.
> > 
> > In my view, a more natural solution would be to redesign the API so that
> > it doesn’t modify the input object’s refcount at all. Instead, it should
> > solely increase the refcount of the returned node (if found) for safe
> > asynchronous usage. This approach would align with established
> > conventions where "find" implies no side effects on inputs or output,
> > and a "get" indicates that the output comes with an extra reference. For
> > example, a function named of_get_node_by_name would clearly signal that
> > only the returned node is subject to a refcount increase while leaving
> > the input intact.
> > 
> > Thus, while the current workaround "balances" the reference count, it
> > doesn't address the underlying design flaw. The naming still suggests a
> > "find" function that should leave the input untouched, which isn’t the
> > case here. A redesign of the API - with both the behavior and naming
> > aligned to common expectations - would be a clearer and more robust
> > solution.
> > 
> > Nevertheless, it is only my POV, and the final decision rests with the
> > OpenFirmware framework maintainers.
> > 
> > Best Regards,
> > Oleksij
> 
> Hi, Oleksij,
> 
> Thanks for your review. I think redesign the API would be a fundamental 
> way to fix this issue, but it would cause impact for current users who 
> knows the exact functionality of of_find_node_by_name(), which need to 
> put the "from" before calling it (I can't make the assumption that all 
> of drivers calling of_find_node_by_name() with a not-NULL "from" 
> pointer, but not calling of_node_get() before is misuse). The basic idea 
> for adding a new function is try not to impact current users. For users 
> who are confused about the "_balanced" suffix,the comments of 
> of_find_node_by_name() explains why this interface exists.

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

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

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On Fri, Feb 07, 2025 at 05:37:22PM +0200, Laurent Pinchart wrote:
> 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.
> 

Adding "next" to the name would help a lot.

Joe Hattori was finding some of these bugs using his static checker.
We could easily write something really specific to find this sort of
bug using Smatch.  If you have ideas like this feel free to ask on
smatch@vger.kernel.org.  It doesn't find anything that your grep
didn't find but any new bugs will be detected when they're introduced.

regards,
dan carpenter

[-- Attachment #2: err-list --]
[-- Type: text/plain, Size: 426 bytes --]

drivers/net/ethernet/broadcom/asp2/bcmasp.c:1370 bcmasp_probe() warn: 'dev->of_node' was not incremented
drivers/net/pse-pd/tps23881.c:505 tps23881_get_of_channels() warn: 'priv->np' was not incremented
drivers/media/platform/qcom/venus/core.c:301 venus_add_video_core() warn: 'dev->of_node' was not incremented
drivers/regulator/tps6594-regulator.c:618 tps6594_regulator_probe() warn: 'tps->dev->of_node' was not incremented

[-- Attachment #3: check_of_find_node_by_name.c --]
[-- Type: text/x-csrc, Size: 1176 bytes --]

/*
 * Copyright 2025 Linaro Ltd.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"

static int my_id;

static void of_find_node_by_name(struct expression *expr, const char *name, struct symbol *sym, void *data)
{
	if (!refcount_was_inced_name_sym(name, sym, "->kobj.kref.refcount.refs.counter"))
		sm_warning("'%s' was not incremented", name);
}

void check_of_find_node_by_name(int id)
{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_function_param_key_hook_early("of_find_node_by_name", &of_find_node_by_name, 0, "$", NULL);
}

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[zhangzekun (A)]

Hi, Laurent,

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

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

Best regards,
Zekun

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Laurent Pinchart]

Hi Zekun,

On Mon, Feb 10, 2025 at 02:47:28PM +0800, zhangzekun (A) wrote:
> > 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.

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

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

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[zhangzekun (A)]

在 2025/2/10 18:03, Laurent Pinchart 写道:
> Hi Zekun,
> 
> On Mon, Feb 10, 2025 at 02:47:28PM +0800, zhangzekun (A) wrote:
>>> 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.
> 

Hi, Laurent,

I think that the original code try to keep the refcount get by 
of_find_node_by_path(), but leak it accidently by 
of_find_node_by_name(). I am not sure that what driver really wants to 
do and if it has a bug here.

Beset Regards,
Zekun

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Laurent Pinchart]

On Tue, Feb 11, 2025 at 07:26:18PM +0800, zhangzekun (A) wrote:
> 在 2025/2/10 18:03, Laurent Pinchart 写道:
> > On Mon, Feb 10, 2025 at 02:47:28PM +0800, zhangzekun (A) wrote:
> >>> 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.
> 
> Hi, Laurent,
> 
> I think that the original code try to keep the refcount get by 
> of_find_node_by_path(), but leak it accidently by 
> of_find_node_by_name(). I am not sure that what driver really wants to 
> do and if it has a bug here.

Looking at the git history, I don't think the code needs or tries to
keep a reference to the root node.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Rob Herring]

On Fri, Feb 7, 2025 at 9:37 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Zekun,
>
> On Fri, Feb 07, 2025 at 07:28:23PM +0800, zhangzekun (A) wrote:
> > 在 2025/2/7 16:24, Oleksij Rempel 写道:
> > > On Fri, Feb 07, 2025 at 09:31:09AM +0800, Zhang Zekun wrote:
> > >> There are many drivers use of_find_node_by_name() with a not-NULL
> > >> device_node pointer, and a number of callers would require a call to
> > >> of_node_get() before using it. There are also some drivers who forget
> > >> to call of_node_get() which would cause a ref count leak[1]. So, Add a
> > >> wraper function for of_find_node_by_name(), drivers may use this function
> > >> to call of_find_node_by_name() with the refcount already balanced.
> > >>
> > >> [1] https://lore.kernel.org/all/20241024015909.58654-1-zhangzekun11@huawei.com/
> > >
> > > Hi Zhang Zekun,
> > >
> > > thank you for working on this issue!
> > >
> > > First of all, let's take a step back and analyze the initial problem.
> > > Everything following is only my opinion...
> > >
> > > The main issue I see is that the current API - of_find_node_by_name -
> > > modifies the refcount of its input by calling of_node_put(from) as part
> > > of its search. Typically, a "find" function is expected to treat its
> > > input as read-only. That is, when you pass an object into such a
> > > function, you expect its reference count to remain unchanged unless
> > > ownership is explicitly transferred. In this case, lowering the refcount
> > > on the input node is counterintuitive and already lead to unexpected
> > > behavior and subtle bugs.
> > >
> > > To address this, the workaround introduces a wrapper function,
> > > of_find_node_by_name_balanced, which first increments the input’s
> > > refcount (via of_node_get()) before calling the original function. While
> > > this "balances" the refcount change, the naming remains problematic from
> > > my perspective. The "_balanced" suffix isn’t part of our common naming
> > > conventions (traditions? :)). Most drivers expect that a function
> > > starting with "find" will not alter the reference count of its input.
> > > The term "balanced" doesn’t clearly convey that the input's refcount is
> > > being explicitly managed - it instead obscures the underlying behavior,
> > > leaving many developers confused about what guarantees the API provides.
> > >
> > > In my view, a more natural solution would be to redesign the API so that
> > > it doesn’t modify the input object’s refcount at all. Instead, it should
> > > solely increase the refcount of the returned node (if found) for safe
> > > asynchronous usage. This approach would align with established
> > > conventions where "find" implies no side effects on inputs or output,
> > > and a "get" indicates that the output comes with an extra reference. For
> > > example, a function named of_get_node_by_name would clearly signal that
> > > only the returned node is subject to a refcount increase while leaving
> > > the input intact.
> > >
> > > Thus, while the current workaround "balances" the reference count, it
> > > doesn't address the underlying design flaw. The naming still suggests a
> > > "find" function that should leave the input untouched, which isn’t the
> > > case here. A redesign of the API - with both the behavior and naming
> > > aligned to common expectations - would be a clearer and more robust
> > > solution.
> > >
> > > Nevertheless, it is only my POV, and the final decision rests with the
> > > OpenFirmware framework maintainers.
> > >
> > > Best Regards,
> > > Oleksij
> >
> > Hi, Oleksij,
> >
> > Thanks for your review. I think redesign the API would be a fundamental
> > way to fix this issue, but it would cause impact for current users who
> > knows the exact functionality of of_find_node_by_name(), which need to
> > put the "from" before calling it (I can't make the assumption that all
> > of drivers calling of_find_node_by_name() with a not-NULL "from"
> > pointer, but not calling of_node_get() before is misuse). The basic idea
> > for adding a new function is try not to impact current users. For users
> > who are confused about the "_balanced" suffix,the comments of
> > of_find_node_by_name() explains why this interface exists.
>
> 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.

I think this could just be 'of_find_node_by_path("/rtas")' which does
occur elsewhere.

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

Yes, I think that's always a child of the PCI bus.

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

Sparc doesn't use OF_DYNAMIC, so who cares... But there are some Sparc
DT dumps here:

https://git.kernel.org/pub/scm/linux/kernel/git/davem/prtconfs.git/

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

Agreed. In general, it's preferred to look for nodes by compatible
rather than node name with child nodes being an exception.

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

+1

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

+1

There's a number of functions which I classify as "don't add new
users" which I keep meaning to document. It's generally older stuff
still using them. Anything returning pointers to raw DT data are
problematic for dynamic DT. Hence all the patches removing
of_find_property/of_get_property calls.

Rob

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Krzysztof Kozlowski]

On 07/02/2025 02:31, Zhang Zekun wrote:
> There are many drivers use of_find_node_by_name() with a not-NULL
> device_node pointer, and a number of callers would require a call to
> of_node_get() before using it. There are also some drivers who forget
> to call of_node_get() which would cause a ref count leak[1]. So, Add a
> wraper function for of_find_node_by_name(), drivers may use this function
> to call of_find_node_by_name() with the refcount already balanced.
> 
> [1] https://lore.kernel.org/all/20241024015909.58654-1-zhangzekun11@huawei.com/
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> ---
>  include/linux/of.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index eaf0e2a2b75c..b7c6d7ff278c 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -268,6 +268,11 @@ static inline const char *of_node_full_name(const struct device_node *np)
>  #define for_each_of_allnodes(dn) for_each_of_allnodes_from(NULL, dn)
>  extern struct device_node *of_find_node_by_name(struct device_node *from,
>  	const char *name);
> +static inline struct device_node *of_find_node_by_name_balanced(struct device_node *from,
> +								const char *name)
> +{
> +	return of_find_node_by_name(of_node_get(from), name);

I don't think that solution to people not reading API description is to
create more API with similar but a bit different behavior, especially
undocumented.

Best regards,
Krzysztof

Re: [PATCH 2/9] net: bcmasp: Add missing of_node_get() before of_find_node_by_name()

[Krzysztof Kozlowski]

On 07/02/2025 02:31, Zhang Zekun wrote:
> of_find_node_by_name() will decrease the refcount of the device_node.
> So, get the device_node before passing to it.
> 
> Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> ---
>  drivers/net/ethernet/broadcom/asp2/bcmasp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> index a68fab1b05f0..093c8ea72af9 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> @@ -1367,7 +1367,7 @@ static int bcmasp_probe(struct platform_device *pdev)
>  	bcmasp_core_init(priv);
>  	bcmasp_core_init_filters(priv);
>  
> -	ports_node = of_find_node_by_name(dev->of_node, "ethernet-ports");
> +	ports_node = of_find_node_by_name_balanced(dev->of_node, "ethernet-ports");

Why this cannot be of_get_child_by_name()?

Best regards,
Krzysztof

Re: [PATCH 2/9] net: bcmasp: Add missing of_node_get() before of_find_node_by_name()

[zhangzekun (A)]

在 2025/2/12 13:52, Krzysztof Kozlowski 写道:
> On 07/02/2025 02:31, Zhang Zekun wrote:
>> of_find_node_by_name() will decrease the refcount of the device_node.
>> So, get the device_node before passing to it.
>>
>> Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
>> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
>> ---
>>   drivers/net/ethernet/broadcom/asp2/bcmasp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
>> index a68fab1b05f0..093c8ea72af9 100644
>> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
>> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
>> @@ -1367,7 +1367,7 @@ static int bcmasp_probe(struct platform_device *pdev)
>>   	bcmasp_core_init(priv);
>>   	bcmasp_core_init_filters(priv);
>>   
>> -	ports_node = of_find_node_by_name(dev->of_node, "ethernet-ports");
>> +	ports_node = of_find_node_by_name_balanced(dev->of_node, "ethernet-ports");
> 
> Why this cannot be of_get_child_by_name()?
> 
> Best regards,
> Krzysztof

Thanks for point out. After looking into 
Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml, it should be 
of_get_child_by_name().

Best regards,
Zekun

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Dan Carpenter]

Whatever happened with this thread from Feb.
https://lore.kernel.org/all/20250207013117.104205-1-zhangzekun11@huawei.com/

The issue was that people weren't expecting of_find_node_by_name() to
drop the reference on the of_node.  The patchset introduced a wrapper
which basically worked as expected except no liked the naming.  Krzysztof
suggested that maybe the callers should be using of_get_child_by_name()
instead.

I created a Smatch warning for this and here are the four issues it
found.  The line numbers are from linux-next.

drivers/net/ethernet/broadcom/asp2/bcmasp.c:1370 bcmasp_probe() warn: 'dev->of_node' was not incremented
drivers/net/pse-pd/tps23881.c:505 tps23881_get_of_channels() warn: 'priv->np' was not incremented
drivers/media/platform/qcom/venus/core.c:301 venus_add_video_core() warn: 'dev->of_node' was not incremented
drivers/regulator/tps6594-regulator.c:618 tps6594_regulator_probe() warn: 'tps->dev->of_node' was not incremented

regards,
dan carpenter

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Laurent Pinchart]

On Fri, Apr 25, 2025 at 06:30:10PM +0300, Dan Carpenter wrote:
> Whatever happened with this thread from Feb.
> https://lore.kernel.org/all/20250207013117.104205-1-zhangzekun11@huawei.com/
> 
> The issue was that people weren't expecting of_find_node_by_name() to
> drop the reference on the of_node.  The patchset introduced a wrapper
> which basically worked as expected except no liked the naming.  Krzysztof
> suggested that maybe the callers should be using of_get_child_by_name()
> instead.

My conclusion is that most of the users of of_find_node_by_name() with a
non-NULL first argument are wrong, and should be replaced by
of_get_child_by_name(). We need a volunteer to do that work.

> I created a Smatch warning for this and here are the four issues it
> found.  The line numbers are from linux-next.
> 
> drivers/net/ethernet/broadcom/asp2/bcmasp.c:1370 bcmasp_probe() warn: 'dev->of_node' was not incremented
> drivers/net/pse-pd/tps23881.c:505 tps23881_get_of_channels() warn: 'priv->np' was not incremented
> drivers/media/platform/qcom/venus/core.c:301 venus_add_video_core() warn: 'dev->of_node' was not incremented
> drivers/regulator/tps6594-regulator.c:618 tps6594_regulator_probe() warn: 'tps->dev->of_node' was not incremented

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Andy Shevchenko]

On Fri, Apr 25, 2025 at 08:07:32PM +0300, Laurent Pinchart wrote:
> On Fri, Apr 25, 2025 at 06:30:10PM +0300, Dan Carpenter wrote:
> > Whatever happened with this thread from Feb.
> > https://lore.kernel.org/all/20250207013117.104205-1-zhangzekun11@huawei.com/
> > 
> > The issue was that people weren't expecting of_find_node_by_name() to
> > drop the reference on the of_node.  The patchset introduced a wrapper
> > which basically worked as expected except no liked the naming.  Krzysztof
> > suggested that maybe the callers should be using of_get_child_by_name()
> > instead.
> 
> My conclusion is that most of the users of of_find_node_by_name() with a
> non-NULL first argument are wrong, and should be replaced by
> of_get_child_by_name(). We need a volunteer to do that work.

Wouldn't be coccinelle a good worker for this job done?

> > I created a Smatch warning for this and here are the four issues it
> > found.  The line numbers are from linux-next.
> > 
> > drivers/net/ethernet/broadcom/asp2/bcmasp.c:1370 bcmasp_probe() warn: 'dev->of_node' was not incremented
> > drivers/net/pse-pd/tps23881.c:505 tps23881_get_of_channels() warn: 'priv->np' was not incremented
> > drivers/media/platform/qcom/venus/core.c:301 venus_add_video_core() warn: 'dev->of_node' was not incremented
> > drivers/regulator/tps6594-regulator.c:618 tps6594_regulator_probe() warn: 'tps->dev->of_node' was not incremented

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Laurent Pinchart]

On Tue, Jun 10, 2025 at 10:39:31PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 25, 2025 at 08:07:32PM +0300, Laurent Pinchart wrote:
> > On Fri, Apr 25, 2025 at 06:30:10PM +0300, Dan Carpenter wrote:
> > > Whatever happened with this thread from Feb.
> > > https://lore.kernel.org/all/20250207013117.104205-1-zhangzekun11@huawei.com/
> > > 
> > > The issue was that people weren't expecting of_find_node_by_name() to
> > > drop the reference on the of_node.  The patchset introduced a wrapper
> > > which basically worked as expected except no liked the naming.  Krzysztof
> > > suggested that maybe the callers should be using of_get_child_by_name()
> > > instead.
> > 
> > My conclusion is that most of the users of of_find_node_by_name() with a
> > non-NULL first argument are wrong, and should be replaced by
> > of_get_child_by_name(). We need a volunteer to do that work.
> 
> Wouldn't be coccinelle a good worker for this job done?

It's not mechanical work, every single user need to be audited manually.
Finding the call sites is the easy part, and we have them already.

> > > I created a Smatch warning for this and here are the four issues it
> > > found.  The line numbers are from linux-next.
> > > 
> > > drivers/net/ethernet/broadcom/asp2/bcmasp.c:1370 bcmasp_probe() warn: 'dev->of_node' was not incremented
> > > drivers/net/pse-pd/tps23881.c:505 tps23881_get_of_channels() warn: 'priv->np' was not incremented
> > > drivers/media/platform/qcom/venus/core.c:301 venus_add_video_core() warn: 'dev->of_node' was not incremented
> > > drivers/regulator/tps6594-regulator.c:618 tps6594_regulator_probe() warn: 'tps->dev->of_node' was not incremented

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/9] of: Add warpper function of_find_node_by_name_balanced()

[Andy Shevchenko]

On Tue, Jun 10, 2025 at 11:03:39PM +0300, Laurent Pinchart wrote:
> On Tue, Jun 10, 2025 at 10:39:31PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 25, 2025 at 08:07:32PM +0300, Laurent Pinchart wrote:
> > > On Fri, Apr 25, 2025 at 06:30:10PM +0300, Dan Carpenter wrote:
> > > > Whatever happened with this thread from Feb.
> > > > https://lore.kernel.org/all/20250207013117.104205-1-zhangzekun11@huawei.com/
> > > > 
> > > > The issue was that people weren't expecting of_find_node_by_name() to
> > > > drop the reference on the of_node.  The patchset introduced a wrapper
> > > > which basically worked as expected except no liked the naming.  Krzysztof
> > > > suggested that maybe the callers should be using of_get_child_by_name()
> > > > instead.
> > > 
> > > My conclusion is that most of the users of of_find_node_by_name() with a
> > > non-NULL first argument are wrong, and should be replaced by
> > > of_get_child_by_name(). We need a volunteer to do that work.
> > 
> > Wouldn't be coccinelle a good worker for this job done?
> 
> It's not mechanical work, every single user need to be audited manually.

Sure, but at least it can do some job that can be done automatically.

> Finding the call sites is the easy part, and we have them already.

> > > > I created a Smatch warning for this and here are the four issues it
> > > > found.  The line numbers are from linux-next.
> > > > 
> > > > drivers/net/ethernet/broadcom/asp2/bcmasp.c:1370 bcmasp_probe() warn: 'dev->of_node' was not incremented
> > > > drivers/net/pse-pd/tps23881.c:505 tps23881_get_of_channels() warn: 'priv->np' was not incremented
> > > > drivers/media/platform/qcom/venus/core.c:301 venus_add_video_core() warn: 'dev->of_node' was not incremented
> > > > drivers/regulator/tps6594-regulator.c:618 tps6594_regulator_probe() warn: 'tps->dev->of_node' was not incremented

-- 
With Best Regards,
Andy Shevchenko