[PATCH net 0/2] Get the device_node before calling of_find_node_by_name()

[PATCH net 0/2] Get the device_node before calling of_find_node_by_name()

[Zhang Zekun]

of_find_node_by_name() will decrease the refount of the device node.
Get the device_node before call to it.

Zhang Zekun (2):
  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()

 drivers/net/ethernet/broadcom/asp2/bcmasp.c | 1 +
 drivers/net/pse-pd/tps23881.c               | 1 +
 2 files changed, 2 insertions(+)

-- 
2.17.1

[PATCH net 1/2] 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>
Reviewed-by: Justin Chen <justin.chen@broadcom.com>
---
 drivers/net/ethernet/broadcom/asp2/bcmasp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
index 297c2682a9cf..517593c58945 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
@@ -1367,6 +1367,7 @@ static int bcmasp_probe(struct platform_device *pdev)
 	bcmasp_core_init(priv);
 	bcmasp_core_init_filters(priv);
 
+	of_node_get(dev->of_node);
 	ports_node = of_find_node_by_name(dev->of_node, "ethernet-ports");
 	if (!ports_node) {
 		dev_warn(dev, "No ports found\n");
-- 
2.17.1

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

[Andrew Lunn]

On Thu, Oct 24, 2024 at 09:59:08AM +0800, 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.

This seems like an odd design. Why does it decrement the reference
count?

Rather than adding missing of_node_get() everywhere, should we be
thinking about the design and fixing it to be more logical?

	Andrew

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

[zhangzekun (A)]

在 2024/10/24 19:56, Andrew Lunn 写道:
> On Thu, Oct 24, 2024 at 09:59:08AM +0800, 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.
> 
> This seems like an odd design. Why does it decrement the reference
> count?
> 
> Rather than adding missing of_node_get() everywhere, should we be
> thinking about the design and fixing it to be more logical?
> 
> 	Andrew

Hi, Andrew,

of_find* API is used as a iterater of the for loop defined in 
"include/linux/of.h", which is already wildly used. I think is 
reasonable to put the previous node when implement a loop, besides, the 
functionality has been documented before the definiton of of_find* APIs.
The logical change of these series of APIs would require a large number 
of conversions in the driver subsys, and I don't think it it necessary.

Thanks,
Zekun

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

[Andrew Lunn]

On Fri, Oct 25, 2024 at 10:41:22AM +0800, zhangzekun (A) wrote:
> 
> 
> 在 2024/10/24 19:56, Andrew Lunn 写道:
> > On Thu, Oct 24, 2024 at 09:59:08AM +0800, 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.
> > 
> > This seems like an odd design. Why does it decrement the reference
> > count?
> > 
> > Rather than adding missing of_node_get() everywhere, should we be
> > thinking about the design and fixing it to be more logical?
> > 
> > 	Andrew
> 
> Hi, Andrew,
> 
> of_find* API is used as a iterater of the for loop defined in
> "include/linux/of.h", which is already wildly used. I think is reasonable to
> put the previous node when implement a loop, besides, the functionality has
> been documented before the definiton of of_find* APIs.
> The logical change of these series of APIs would require a large number of
> conversions in the driver subsys, and I don't think it it necessary.

Do you have a rough idea how many missing gets there are because of
this poor design?

A quick back of the envelope analysis, which will be inaccurate...

$ grep -r of_find_node_by_name | wc
     68     348    5154

Now, a lot of these pass NULL as the node pointer:

$ grep -r of_find_node_by_name | grep NULL | wc
     47     232    3456

so there are only about 20 call sites which are interesting. Have you
looked at them all?

What percentage of these are not in a loop and hence don't want the
decrement?

What percentage are broken?

We have to assume a similar number of new instances will also be
broken, so you have an endless job of fixing these new broken cases as
they are added.

If you found that 15 of the 20 are broken, it makes little difference
changing the call to one which is not broken by design. If it is only
5 from 20 which are broken, then yes, it might be considered pointless
churn changing them all, and you have a little job for life...

	Andrew

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

[zhangzekun (A)]

在 2024/10/25 21:14, Andrew Lunn 写道:
> On Fri, Oct 25, 2024 at 10:41:22AM +0800, zhangzekun (A) wrote:
>>
>>
>> 在 2024/10/24 19:56, Andrew Lunn 写道:
>>> On Thu, Oct 24, 2024 at 09:59:08AM +0800, 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.
>>>
>>> This seems like an odd design. Why does it decrement the reference
>>> count?
>>>
>>> Rather than adding missing of_node_get() everywhere, should we be
>>> thinking about the design and fixing it to be more logical?
>>>
>>> 	Andrew
>>
>> Hi, Andrew,
>>
>> of_find* API is used as a iterater of the for loop defined in
>> "include/linux/of.h", which is already wildly used. I think is reasonable to
>> put the previous node when implement a loop, besides, the functionality has
>> been documented before the definiton of of_find* APIs.
>> The logical change of these series of APIs would require a large number of
>> conversions in the driver subsys, and I don't think it it necessary.
> 
> Do you have a rough idea how many missing gets there are because of
> this poor design?
> 
> A quick back of the envelope analysis, which will be inaccurate...
> 
> $ grep -r of_find_node_by_name | wc
>       68     348    5154
> 
> Now, a lot of these pass NULL as the node pointer:
> 
> $ grep -r of_find_node_by_name | grep NULL | wc
>       47     232    3456
> 
> so there are only about 20 call sites which are interesting. Have you
> looked at them all?
> 
> What percentage of these are not in a loop and hence don't want the
> decrement?
> 
> What percentage are broken?
> 
> We have to assume a similar number of new instances will also be
> broken, so you have an endless job of fixing these new broken cases as
> they are added.
> 
> If you found that 15 of the 20 are broken, it makes little difference
> changing the call to one which is not broken by design. If it is only
> 5 from 20 which are broken, then yes, it might be considered pointless
> churn changing them all, and you have a little job for life...
> 
> 	Andrew

Hi, Andrew,

There are about 10/20 call sites might have this problem, spreading in 
six files. May be we can fix these problems instead of adding a new API?

Thanks,
Zekun

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

[Andrew Lunn]

> > Do you have a rough idea how many missing gets there are because of
> > this poor design?
> > 
> > A quick back of the envelope analysis, which will be inaccurate...
> > 
> > $ grep -r of_find_node_by_name | wc
> >       68     348    5154
> > 
> > Now, a lot of these pass NULL as the node pointer:
> > 
> > $ grep -r of_find_node_by_name | grep NULL | wc
> >       47     232    3456
> > 
> > so there are only about 20 call sites which are interesting. Have you
> > looked at them all?
> > 
> > What percentage of these are not in a loop and hence don't want the
> > decrement?
> > 
> > What percentage are broken?
> > 
> > We have to assume a similar number of new instances will also be
> > broken, so you have an endless job of fixing these new broken cases as
> > they are added.
> > 
> > If you found that 15 of the 20 are broken, it makes little difference
> > changing the call to one which is not broken by design. If it is only
> > 5 from 20 which are broken, then yes, it might be considered pointless
> > churn changing them all, and you have a little job for life...
> > 
> > 	Andrew
> 
> Hi, Andrew,
> 
> There are about 10/20 call sites might have this problem, spreading in six
> files. May be we can fix these problems instead of adding a new API?

So you are saying 50% of the call sites are wrong! We should fix the
API if so many developers are getting it wrong.

	Andrew

[PATCH net 2/2] 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
index 5c4e88be46ee..f5c04dd5be37 100644
--- a/drivers/net/pse-pd/tps23881.c
+++ b/drivers/net/pse-pd/tps23881.c
@@ -216,6 +216,7 @@ tps23881_get_of_channels(struct tps23881_priv *priv,
 	if (!priv->np)
 		return -EINVAL;
 
+	of_node_get(priv->np);
 	channels_node = of_find_node_by_name(priv->np, "channels");
 	if (!channels_node)
 		return -EINVAL;
-- 
2.17.1

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

[Oleksij Rempel]

On Thu, Oct 24, 2024 at 09:59:09AM +0800, Zhang Zekun wrote:
> 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>

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!
-- 
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 net 2/2] net: pse-pd: Add missing of_node_get() before of_find_node_by_name()

[Kory Maincent]

On Thu, 24 Oct 2024 09:59:09 +0800
Zhang Zekun <zhangzekun11@huawei.com> wrote:

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

Acked-by: Kory Maincent <kory.maincent@bootlin.com>

Thank you!

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net 0/2] Get the device_node before calling of_find_node_by_name()

[Jakub Kicinski]

On Thu, 24 Oct 2024 09:59:07 +0800 Zhang Zekun wrote:
> of_find_node_by_name() will decrease the refount of the device node.
> Get the device_node before call to it.

Doing some quick grepping I think Andrew is completely right.
Most callers either get this wrong or call get() immediately prior.
Maybe add a new helper with more suitable semantics?

The goal is not to fix the bugs but to prevent them in the first place.
-- 
pw-bot: cr