| Message ID | 20241024015909.58654-2-zhangzekun11@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Get the device_node before calling of_find_node_by_name() | expand |
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
在 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
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
在 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
> > 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
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");
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(+)