* [PATCH v2 1/4] of: Add cleanup.h based auto release via __free(device_node) markings.
2024-02-23 12:44 [PATCH v2 0/4] of: automate of_node_put() - new approach to loops Jonathan Cameron
@ 2024-02-23 12:44 ` Jonathan Cameron
2024-02-23 12:44 ` [PATCH v2 2/4] of: Introduce for_each_*_child_of_node_scoped() to automate of_node_put() handling Jonathan Cameron
` (4 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-23 12:44 UTC (permalink / raw)
To: linux-iio, Rob Herring, Frank Rowand, linux-kernel, Julia Lawall
Cc: Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut
The recent addition of scope based cleanup support to the kernel
provides a convenient tool to reduce the chances of leaking reference
counts where of_node_put() should have been called in an error path.
This enables
struct device_node *child __free(device_node) = NULL;
for_each_child_of_node(np, child) {
if (test)
return test;
}
with no need for a manual call of of_node_put().
A following patch will reduce the scope of the child variable to the
for loop, to avoid an issues with ordering of autocleanup, and make it
obvious when this assigned a non NULL value.
In this simple example the gains are small but there are some very
complex error handling cases buried in these loops that will be
greatly simplified by enabling early returns with out the need
for this manual of_node_put() call.
Note that there are coccinelle checks in
scripts/coccinelle/iterators/for_each_child.cocci to detect a failure
to call of_node_put(). This new approach does not cause false positives.
Longer term we may want to add scripting to check this new approach is
done correctly with no double of_node_put() calls being introduced due
to the auto cleanup. It may also be useful to script finding places
this new approach is useful.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
include/linux/of.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/of.h b/include/linux/of.h
index 6a9ddf20e79a..50e882ee91da 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -13,6 +13,7 @@
*/
#include <linux/types.h>
#include <linux/bitops.h>
+#include <linux/cleanup.h>
#include <linux/errno.h>
#include <linux/kobject.h>
#include <linux/mod_devicetable.h>
@@ -134,6 +135,7 @@ static inline struct device_node *of_node_get(struct device_node *node)
}
static inline void of_node_put(struct device_node *node) { }
#endif /* !CONFIG_OF_DYNAMIC */
+DEFINE_FREE(device_node, struct device_node *, if (_T) of_node_put(_T))
/* Pointer for first entry in chain of all nodes. */
extern struct device_node *of_root;
--
2.39.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 2/4] of: Introduce for_each_*_child_of_node_scoped() to automate of_node_put() handling
2024-02-23 12:44 [PATCH v2 0/4] of: automate of_node_put() - new approach to loops Jonathan Cameron
2024-02-23 12:44 ` [PATCH v2 1/4] of: Add cleanup.h based auto release via __free(device_node) markings Jonathan Cameron
@ 2024-02-23 12:44 ` Jonathan Cameron
2024-02-23 12:44 ` [PATCH v2 3/4] of: unittest: Use for_each_child_of_node_scoped() Jonathan Cameron
` (3 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-23 12:44 UTC (permalink / raw)
To: linux-iio, Rob Herring, Frank Rowand, linux-kernel, Julia Lawall
Cc: Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut
To avoid issues with out of order cleanup, or ambiguity about when the
auto freed data is first instantiated, do it within the for loop definition.
The disadvantage is that the struct device_node *child variable creation
is not immediately obvious where this is used.
However, in many cases, if there is another definition of
struct device_node *child; the compiler / static analysers will notify us
that it is unused, or uninitialized.
Note that, in the vast majority of cases, the _available_ form should be
used and as code is converted to these scoped handers, we should confirm
that any cases that do not check for available have a good reason not
to.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
include/linux/of.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/include/linux/of.h b/include/linux/of.h
index 50e882ee91da..024dda54b9c7 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1430,10 +1430,23 @@ static inline int of_property_read_s32(const struct device_node *np,
#define for_each_child_of_node(parent, child) \
for (child = of_get_next_child(parent, NULL); child != NULL; \
child = of_get_next_child(parent, child))
+
+#define for_each_child_of_node_scoped(parent, child) \
+ for (struct device_node *child __free(device_node) = \
+ of_get_next_child(parent, NULL); \
+ child != NULL; \
+ child = of_get_next_child(parent, child))
+
#define for_each_available_child_of_node(parent, child) \
for (child = of_get_next_available_child(parent, NULL); child != NULL; \
child = of_get_next_available_child(parent, child))
+#define for_each_available_child_of_node_scoped(parent, child) \
+ for (struct device_node *child __free(device_node) = \
+ of_get_next_available_child(parent, NULL); \
+ child != NULL; \
+ child = of_get_next_available_child(parent, child))
+
#define for_each_of_cpu_node(cpu) \
for (cpu = of_get_next_cpu_node(NULL); cpu != NULL; \
cpu = of_get_next_cpu_node(cpu))
--
2.39.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 3/4] of: unittest: Use for_each_child_of_node_scoped()
2024-02-23 12:44 [PATCH v2 0/4] of: automate of_node_put() - new approach to loops Jonathan Cameron
2024-02-23 12:44 ` [PATCH v2 1/4] of: Add cleanup.h based auto release via __free(device_node) markings Jonathan Cameron
2024-02-23 12:44 ` [PATCH v2 2/4] of: Introduce for_each_*_child_of_node_scoped() to automate of_node_put() handling Jonathan Cameron
@ 2024-02-23 12:44 ` Jonathan Cameron
2024-02-23 12:44 ` [PATCH v2 4/4] iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped() Jonathan Cameron
` (2 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-23 12:44 UTC (permalink / raw)
To: linux-iio, Rob Herring, Frank Rowand, linux-kernel, Julia Lawall
Cc: Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut
A simple example of the utility of this autocleanup approach to
handling of_node_put().
In this particular case some of the nodes needed for the test are
not available and the _available_ version would cause them to be
skipped resulting in a test failure.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/of/unittest.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index d7593bde2d02..567904464a21 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -239,27 +239,22 @@ static void __init of_unittest_dynamic(void)
static int __init of_unittest_check_node_linkage(struct device_node *np)
{
- struct device_node *child;
int count = 0, rc;
- for_each_child_of_node(np, child) {
+ for_each_child_of_node_scoped(np, child) {
if (child->parent != np) {
pr_err("Child node %pOFn links to wrong parent %pOFn\n",
child, np);
- rc = -EINVAL;
- goto put_child;
+ return -EINVAL;
}
rc = of_unittest_check_node_linkage(child);
if (rc < 0)
- goto put_child;
+ return rc;
count += rc;
}
return count + 1;
-put_child:
- of_node_put(child);
- return rc;
}
static void __init of_unittest_check_tree_linkage(void)
--
2.39.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 4/4] iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped()
2024-02-23 12:44 [PATCH v2 0/4] of: automate of_node_put() - new approach to loops Jonathan Cameron
` (2 preceding siblings ...)
2024-02-23 12:44 ` [PATCH v2 3/4] of: unittest: Use for_each_child_of_node_scoped() Jonathan Cameron
@ 2024-02-23 12:44 ` Jonathan Cameron
2024-02-23 15:52 ` [PATCH v2 0/4] of: automate of_node_put() - new approach to loops Andy Shevchenko
2024-02-25 14:25 ` Jonathan Cameron
5 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-23 12:44 UTC (permalink / raw)
To: linux-iio, Rob Herring, Frank Rowand, linux-kernel, Julia Lawall
Cc: Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut
Using automated cleanup to replace of_node_put() handling allows for
a simplfied flow by enabling direct returns on errors.
Non available child nodes should never have been considered; that
is ones where status != okay and was defined.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/rcar-gyroadc.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
index d524f2e8e927..15a21d2860e7 100644
--- a/drivers/iio/adc/rcar-gyroadc.c
+++ b/drivers/iio/adc/rcar-gyroadc.c
@@ -318,7 +318,6 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
struct rcar_gyroadc *priv = iio_priv(indio_dev);
struct device *dev = priv->dev;
struct device_node *np = dev->of_node;
- struct device_node *child;
struct regulator *vref;
unsigned int reg;
unsigned int adcmode = -1, childmode;
@@ -326,7 +325,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
unsigned int num_channels;
int ret, first = 1;
- for_each_child_of_node(np, child) {
+ for_each_available_child_of_node_scoped(np, child) {
of_id = of_match_node(rcar_gyroadc_child_match, child);
if (!of_id) {
dev_err(dev, "Ignoring unsupported ADC \"%pOFn\".",
@@ -352,7 +351,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
break;
default:
- goto err_e_inval;
+ return -EINVAL;
}
/*
@@ -369,7 +368,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
dev_err(dev,
"Failed to get child reg property of ADC \"%pOFn\".\n",
child);
- goto err_of_node_put;
+ return ret;
}
/* Channel number is too high. */
@@ -377,7 +376,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
dev_err(dev,
"Only %i channels supported with %pOFn, but reg = <%i>.\n",
num_channels, child, reg);
- goto err_e_inval;
+ return -EINVAL;
}
}
@@ -386,7 +385,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
dev_err(dev,
"Channel %i uses different ADC mode than the rest.\n",
reg);
- goto err_e_inval;
+ return -EINVAL;
}
/* Channel is valid, grab the regulator. */
@@ -396,8 +395,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
if (IS_ERR(vref)) {
dev_dbg(dev, "Channel %i 'vref' supply not connected.\n",
reg);
- ret = PTR_ERR(vref);
- goto err_of_node_put;
+ return PTR_ERR(vref);
}
priv->vref[reg] = vref;
@@ -422,7 +420,6 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
* we can stop parsing here.
*/
if (childmode == RCAR_GYROADC_MODE_SELECT_1_MB88101A) {
- of_node_put(child);
break;
}
}
@@ -433,12 +430,6 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
}
return 0;
-
-err_e_inval:
- ret = -EINVAL;
-err_of_node_put:
- of_node_put(child);
- return ret;
}
static void rcar_gyroadc_deinit_supplies(struct iio_dev *indio_dev)
--
2.39.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 0/4] of: automate of_node_put() - new approach to loops.
2024-02-23 12:44 [PATCH v2 0/4] of: automate of_node_put() - new approach to loops Jonathan Cameron
` (3 preceding siblings ...)
2024-02-23 12:44 ` [PATCH v2 4/4] iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped() Jonathan Cameron
@ 2024-02-23 15:52 ` Andy Shevchenko
2024-02-23 16:36 ` Jonathan Cameron
2024-02-25 14:25 ` Jonathan Cameron
5 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2024-02-23 15:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, Rob Herring, Frank Rowand, linux-kernel, Julia Lawall,
Peter Zijlstra, Greg Kroah-Hartman, marek.vasut
On Fri, Feb 23, 2024 at 12:44:28PM +0000, Jonathan Cameron wrote:
> The equivalent device_for_each_child_node_scoped() series for
> fwnode will be queued up in IIO for the merge window shortly as
> it has gathered sufficient tags. Hopefully the precdent set there
> for the approach will reassure people that instantiating the
> child variable inside the macro definition is the best approach.
> https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/
>
> v2: Andy suggested most of the original converted set should move to
> generic fwnode / property.h handling. Within IIO that was
> a reasonable observation given we've been trying to move away from
> firmware specific handling for some time. Patches making that change
> to appropriate drivers posted.
> As we discussed there are cases which are not suitable for such
> conversion and this infrastructure still provides clear benefits
> for them.
> iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped()
Is this the only one so far? Or do we have more outside of IIO?
I'm fine with the code if OF maintainers think it's useful.
My concern is to make as many as possible drivers to be converted to
use fwnode instead of OF one.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 0/4] of: automate of_node_put() - new approach to loops.
2024-02-23 15:52 ` [PATCH v2 0/4] of: automate of_node_put() - new approach to loops Andy Shevchenko
@ 2024-02-23 16:36 ` Jonathan Cameron
2024-02-23 16:38 ` Julia Lawall
2024-02-23 16:42 ` Jonathan Cameron
0 siblings, 2 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-23 16:36 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-iio, Rob Herring, Frank Rowand, linux-kernel, Julia Lawall,
Peter Zijlstra, Greg Kroah-Hartman, marek.vasut
On Fri, 23 Feb 2024 17:52:46 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Feb 23, 2024 at 12:44:28PM +0000, Jonathan Cameron wrote:
> > The equivalent device_for_each_child_node_scoped() series for
> > fwnode will be queued up in IIO for the merge window shortly as
> > it has gathered sufficient tags. Hopefully the precdent set there
> > for the approach will reassure people that instantiating the
> > child variable inside the macro definition is the best approach.
> > https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/
> >
> > v2: Andy suggested most of the original converted set should move to
> > generic fwnode / property.h handling. Within IIO that was
> > a reasonable observation given we've been trying to move away from
> > firmware specific handling for some time. Patches making that change
> > to appropriate drivers posted.
> > As we discussed there are cases which are not suitable for such
> > conversion and this infrastructure still provides clear benefits
> > for them.
>
> > iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped()
>
> Is this the only one so far? Or do we have more outside of IIO?
>
> I'm fine with the code if OF maintainers think it's useful.
> My concern is to make as many as possible drivers to be converted to
> use fwnode instead of OF one.
>
Julia wrote a coccinelle script
__free() cases
https://lore.kernel.org/all/alpine.DEB.2.22.394.2401291455430.8649@hadrien/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/4] of: automate of_node_put() - new approach to loops.
2024-02-23 16:36 ` Jonathan Cameron
@ 2024-02-23 16:38 ` Julia Lawall
2024-02-23 17:12 ` Jonathan Cameron
2024-02-23 16:42 ` Jonathan Cameron
1 sibling, 1 reply; 11+ messages in thread
From: Julia Lawall @ 2024-02-23 16:38 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Andy Shevchenko, linux-iio, Rob Herring, Frank Rowand,
linux-kernel, Peter Zijlstra, Greg Kroah-Hartman, marek.vasut
On Fri, 23 Feb 2024, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 17:52:46 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > On Fri, Feb 23, 2024 at 12:44:28PM +0000, Jonathan Cameron wrote:
> > > The equivalent device_for_each_child_node_scoped() series for
> > > fwnode will be queued up in IIO for the merge window shortly as
> > > it has gathered sufficient tags. Hopefully the precdent set there
> > > for the approach will reassure people that instantiating the
> > > child variable inside the macro definition is the best approach.
> > > https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/
> > >
> > > v2: Andy suggested most of the original converted set should move to
> > > generic fwnode / property.h handling. Within IIO that was
> > > a reasonable observation given we've been trying to move away from
> > > firmware specific handling for some time. Patches making that change
> > > to appropriate drivers posted.
> > > As we discussed there are cases which are not suitable for such
> > > conversion and this infrastructure still provides clear benefits
> > > for them.
> >
> > > iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped()
> >
> > Is this the only one so far? Or do we have more outside of IIO?
> >
> > I'm fine with the code if OF maintainers think it's useful.
> > My concern is to make as many as possible drivers to be converted to
> > use fwnode instead of OF one.
> >
> Julia wrote a coccinelle script
> __free() cases
> https://lore.kernel.org/all/alpine.DEB.2.22.394.2401291455430.8649@hadrien/
The script doesn't use fwnode. It gets rid of of_node_put, asssuming that
someone has already set that up for __free.
julia
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/4] of: automate of_node_put() - new approach to loops.
2024-02-23 16:38 ` Julia Lawall
@ 2024-02-23 17:12 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-23 17:12 UTC (permalink / raw)
To: Julia Lawall
Cc: Andy Shevchenko, linux-iio, Rob Herring, Frank Rowand,
linux-kernel, Peter Zijlstra, Greg Kroah-Hartman, marek.vasut
On Fri, 23 Feb 2024 17:38:31 +0100 (CET)
Julia Lawall <julia.lawall@inria.fr> wrote:
> On Fri, 23 Feb 2024, Jonathan Cameron wrote:
>
> > On Fri, 23 Feb 2024 17:52:46 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >
> > > On Fri, Feb 23, 2024 at 12:44:28PM +0000, Jonathan Cameron wrote:
> > > > The equivalent device_for_each_child_node_scoped() series for
> > > > fwnode will be queued up in IIO for the merge window shortly as
> > > > it has gathered sufficient tags. Hopefully the precdent set there
> > > > for the approach will reassure people that instantiating the
> > > > child variable inside the macro definition is the best approach.
> > > > https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/
> > > >
> > > > v2: Andy suggested most of the original converted set should move to
> > > > generic fwnode / property.h handling. Within IIO that was
> > > > a reasonable observation given we've been trying to move away from
> > > > firmware specific handling for some time. Patches making that change
> > > > to appropriate drivers posted.
> > > > As we discussed there are cases which are not suitable for such
> > > > conversion and this infrastructure still provides clear benefits
> > > > for them.
> > >
> > > > iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped()
> > >
> > > Is this the only one so far? Or do we have more outside of IIO?
> > >
> > > I'm fine with the code if OF maintainers think it's useful.
> > > My concern is to make as many as possible drivers to be converted to
> > > use fwnode instead of OF one.
> > >
> > Julia wrote a coccinelle script
> > __free() cases
> > https://lore.kernel.org/all/alpine.DEB.2.22.394.2401291455430.8649@hadrien/
>
> The script doesn't use fwnode. It gets rid of of_node_put, asssuming that
> someone has already set that up for __free.
Question I was addressing was a few lines up.
"Or do we have more outside of IIO?"
I should have addressed it immediately after the question
+ not sent half an answer :(
>
> julia
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/4] of: automate of_node_put() - new approach to loops.
2024-02-23 16:36 ` Jonathan Cameron
2024-02-23 16:38 ` Julia Lawall
@ 2024-02-23 16:42 ` Jonathan Cameron
1 sibling, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-23 16:42 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-iio, Rob Herring, Frank Rowand, linux-kernel, Julia Lawall,
Peter Zijlstra, Greg Kroah-Hartman, marek.vasut
On Fri, 23 Feb 2024 16:36:02 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> On Fri, 23 Feb 2024 17:52:46 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > On Fri, Feb 23, 2024 at 12:44:28PM +0000, Jonathan Cameron wrote:
> > > The equivalent device_for_each_child_node_scoped() series for
> > > fwnode will be queued up in IIO for the merge window shortly as
> > > it has gathered sufficient tags. Hopefully the precdent set there
> > > for the approach will reassure people that instantiating the
> > > child variable inside the macro definition is the best approach.
> > > https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/
> > >
> > > v2: Andy suggested most of the original converted set should move to
> > > generic fwnode / property.h handling. Within IIO that was
> > > a reasonable observation given we've been trying to move away from
> > > firmware specific handling for some time. Patches making that change
> > > to appropriate drivers posted.
> > > As we discussed there are cases which are not suitable for such
> > > conversion and this infrastructure still provides clear benefits
> > > for them.
> >
> > > iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped()
> >
> > Is this the only one so far? Or do we have more outside of IIO?
> >
> > I'm fine with the code if OF maintainers think it's useful.
> > My concern is to make as many as possible drivers to be converted to
> > use fwnode instead of OF one.
> >
> Julia wrote a coccinelle script
> __free() cases (53)
> https://lore.kernel.org/all/alpine.DEB.2.22.394.2401291455430.8649@hadrien/
Gah. Second time today I've hit the wrong key whilst pasting.
loop cases. (73)
https://lore.kernel.org/all/alpine.DEB.2.22.394.2401312234250.3245@hadrien/
Scattered right across the kernel.
No others submitted yet and there is just the one left in IIO after fwnode
conversions. There are other drivers I haven't converted to fwnode for
various reasons, but this isn't useful for them as not all call of_node_put().
Some of the other cases will be suitable for fwnode conversion and that
is definitely the right first choice in many cases.
Agreed, it's down to the OF maintainers to take a call on whether they want
this infrastructure or not. No longer matters much to me for IIO
as you can see.
Jonathan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/4] of: automate of_node_put() - new approach to loops.
2024-02-23 12:44 [PATCH v2 0/4] of: automate of_node_put() - new approach to loops Jonathan Cameron
` (4 preceding siblings ...)
2024-02-23 15:52 ` [PATCH v2 0/4] of: automate of_node_put() - new approach to loops Andy Shevchenko
@ 2024-02-25 14:25 ` Jonathan Cameron
5 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-25 14:25 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, Rob Herring, Frank Rowand, linux-kernel, Julia Lawall,
Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut
On Fri, 23 Feb 2024 12:44:28 +0000
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> The equivalent device_for_each_child_node_scoped() series for
> fwnode will be queued up in IIO for the merge window shortly as
> it has gathered sufficient tags. Hopefully the precdent set there
> for the approach will reassure people that instantiating the
> child variable inside the macro definition is the best approach.
> https://lore.kernel.org/linux-iio/20240217164249.921878-1-jic23@kernel.org/
I missed the devicetree list. Will resend with a brief summary of the
discussion on v2 so far. Sorry for the noise!
>
> v2: Andy suggested most of the original converted set should move to
> generic fwnode / property.h handling. Within IIO that was
> a reasonable observation given we've been trying to move away from
> firmware specific handling for some time. Patches making that change
> to appropriate drivers posted.
> As we discussed there are cases which are not suitable for such
> conversion and this infrastructure still provides clear benefits
> for them.
>
> Ideally it would be good if this introductory series adding the
> infrastructure makes the 6.9 merge window. There are no dependencies
> on work queued in the IIO tree, so this can go via devicetree
> if the maintainers would prefer. I've had some off list messages
> asking when this would be merged, as there is interest in building
> on it next cycle for other parts of the kernel (where conversion to
> fwnode handling may be less appropriate).
>
> The outputs of Julia's scripts linked below show how widely this can be
> easily applied and give a conservative estimate of the complexity reduction
> and code savings. In some cases those drivers should move to fwnode
> and use the equivalent infrastructure there, but many will be unsuitable
> for conversion so this is still good win.
>
> Edited cover letter from v1:
>
> Thanks to Julia Lawal who also posted coccinelle for both types (loop and
> non loop cases)
>
> https://lore.kernel.org/all/alpine.DEB.2.22.394.2401312234250.3245@hadrien/
> https://lore.kernel.org/all/alpine.DEB.2.22.394.2401291455430.8649@hadrien/
>
> The cover letter of the RFC includes information on the various approaches
> considered.
> https://lore.kernel.org/all/20240128160542.178315-1-jic23@kernel.org/
>
> Whilst these macros produce nice reductions in complexity the loops
> still have the unfortunate side effect of hiding the local declaration
> of a struct device_node * which is then used inside the loop.
>
> Julia suggested making that a little more visible via
> #define for_each_child_of_node_scoped(parent, struct device_node *, child)
> but in discussion we both expressed that this doesn't really make things
> all that clear either so I haven't adopted this suggestion.
>
>
>
> Jonathan Cameron (4):
> of: Add cleanup.h based auto release via __free(device_node) markings.
> of: Introduce for_each_*_child_of_node_scoped() to automate
> of_node_put() handling
> of: unittest: Use for_each_child_of_node_scoped()
> iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped()
>
> drivers/iio/adc/rcar-gyroadc.c | 21 ++++++---------------
> drivers/of/unittest.c | 11 +++--------
> include/linux/of.h | 15 +++++++++++++++
> 3 files changed, 24 insertions(+), 23 deletions(-)
>
^ permalink raw reply [flat|nested] 11+ messages in thread