From: Jonathan Cameron <[email protected]>
Some discussion occured on previous posting.
https://lore.kernel.org/linux-iio/[email protected]/
Summary:
* fwnode conversions should be considered when applying this
infrastructure to a driver. Perhaps better to move directly to
the generic FW property handling rather than improve existing
of specific code.
* There are lots of potential places to use this based on detections
from Julia's coccinelle scripts linked below.
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/[email protected]/
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/[email protected]/
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(-)
--
2.44.0
From: Jonathan Cameron <[email protected]>
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 <[email protected]>
---
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.44.0
From: Jonathan Cameron <[email protected]>
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 <[email protected]>
---
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.44.0
From: Jonathan Cameron <[email protected]>
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 <[email protected]>
---
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 cfd60e35a899..d353327767b3 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -233,27 +233,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.44.0
From: Jonathan Cameron <[email protected]>
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 <[email protected]>
---
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.44.0
On Sun, Feb 25, 2024 at 02:27:10PM +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <[email protected]>
>
> Some discussion occured on previous posting.
> https://lore.kernel.org/linux-iio/[email protected]/
>
> Summary:
> * fwnode conversions should be considered when applying this
> infrastructure to a driver. Perhaps better to move directly to
> the generic FW property handling rather than improve existing
> of specific code.
> * There are lots of potential places to use this based on detections
> from Julia's coccinelle scripts linked below.
>
> 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/[email protected]/
>
> 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).
I'll let you take it. For the series:
Reviewed-by: Rob Herring <[email protected]>
I've got some drivers/of/ conversions too, but they are probably next
cycle at this point.
Rob
On Fri, 1 Mar 2024 16:39:42 -0600
Rob Herring <[email protected]> wrote:
> On Sun, Feb 25, 2024 at 02:27:10PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <[email protected]>
> >
> > Some discussion occured on previous posting.
> > https://lore.kernel.org/linux-iio/[email protected]/
> >
> > Summary:
> > * fwnode conversions should be considered when applying this
> > infrastructure to a driver. Perhaps better to move directly to
> > the generic FW property handling rather than improve existing
> > of specific code.
> > * There are lots of potential places to use this based on detections
> > from Julia's coccinelle scripts linked below.
> >
> > 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/[email protected]/
> >
> > 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).
>
> I'll let you take it. For the series:
>
> Reviewed-by: Rob Herring <[email protected]>
>
> I've got some drivers/of/ conversions too, but they are probably next
> cycle at this point.
>
> Rob
Thanks Rob,
Whether this makes it for this cycle is probably dependent on whether
Linus does decide to do got to rc8 as hinted at as a possibility
+ whether Greg feels comfortable taking these through his tree
(char-misc is the normal path for IIO). I know various people
are hoping this series makes it, but if doesn't we can do an immutable
tree early next cycle (though obviously that may reduce speed of adoption).
We are discussing the equivalent pull request for the fwnode version here:
https://lore.kernel.org/linux-iio/2024030239-gift-cabdriver-266b@gregkh/T/#m87e7208820ebf6416a77a2973773b65a087b4796
I've optimistically applied this series to my togreg-cleanup branch
and merged that into the togreg branch of iio.git for linux-next to pick up.
Thanks,
Jonathan
On Sun, 25 Feb 2024 14:27:14 +0000
Jonathan Cameron <[email protected]> wrote:
> From: Jonathan Cameron <[email protected]>
>
> 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 <[email protected]>
Applied to the togreg branch of iio.git and pushed out as testing
for all the normal reasons.
Thanks,
Jonathan
> ---
> 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)