Linux-IIO Archive mirror
 help / color / mirror / Atom feed
* [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops.
@ 2024-02-25 14:27 Jonathan Cameron
  2024-02-25 14:27 ` [RESEND PATCH v2 1/4] of: Add cleanup.h based auto release via __free(device_node) markings Jonathan Cameron
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-25 14:27 UTC (permalink / raw
  To: devicetree, linux-iio, Rob Herring, Frank Rowand, linux-kernel,
	Julia Lawall
  Cc: Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut,
	Jonathan Cameron

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Some discussion occured on previous posting.
https://lore.kernel.org/linux-iio/20240223124432.26443-1-Jonathan.Cameron@huawei.com/

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

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

-- 
2.44.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RESEND PATCH v2 1/4] of: Add cleanup.h based auto release via __free(device_node) markings.
  2024-02-25 14:27 [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops Jonathan Cameron
@ 2024-02-25 14:27 ` Jonathan Cameron
  2024-02-25 14:27 ` [RESEND PATCH v2 2/4] of: Introduce for_each_*_child_of_node_scoped() to automate of_node_put() handling Jonathan Cameron
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-25 14:27 UTC (permalink / raw
  To: devicetree, linux-iio, Rob Herring, Frank Rowand, linux-kernel,
	Julia Lawall
  Cc: Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut,
	Jonathan Cameron

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RESEND PATCH v2 2/4] of: Introduce for_each_*_child_of_node_scoped() to automate of_node_put() handling
  2024-02-25 14:27 [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops Jonathan Cameron
  2024-02-25 14:27 ` [RESEND PATCH v2 1/4] of: Add cleanup.h based auto release via __free(device_node) markings Jonathan Cameron
@ 2024-02-25 14:27 ` Jonathan Cameron
  2024-02-25 14:27 ` [RESEND PATCH v2 3/4] of: unittest: Use for_each_child_of_node_scoped() Jonathan Cameron
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-25 14:27 UTC (permalink / raw
  To: devicetree, linux-iio, Rob Herring, Frank Rowand, linux-kernel,
	Julia Lawall
  Cc: Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut,
	Jonathan Cameron

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RESEND PATCH v2 3/4] of: unittest: Use for_each_child_of_node_scoped()
  2024-02-25 14:27 [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops Jonathan Cameron
  2024-02-25 14:27 ` [RESEND PATCH v2 1/4] of: Add cleanup.h based auto release via __free(device_node) markings Jonathan Cameron
  2024-02-25 14:27 ` [RESEND PATCH v2 2/4] of: Introduce for_each_*_child_of_node_scoped() to automate of_node_put() handling Jonathan Cameron
@ 2024-02-25 14:27 ` Jonathan Cameron
  2024-02-25 14:27 ` [RESEND PATCH v2 4/4] iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped() Jonathan Cameron
  2024-03-01 22:39 ` [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops Rob Herring
  4 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-25 14:27 UTC (permalink / raw
  To: devicetree, linux-iio, Rob Herring, Frank Rowand, linux-kernel,
	Julia Lawall
  Cc: Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut,
	Jonathan Cameron

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RESEND PATCH v2 4/4] iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped()
  2024-02-25 14:27 [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops Jonathan Cameron
                   ` (2 preceding siblings ...)
  2024-02-25 14:27 ` [RESEND PATCH v2 3/4] of: unittest: Use for_each_child_of_node_scoped() Jonathan Cameron
@ 2024-02-25 14:27 ` Jonathan Cameron
  2024-03-25 19:53   ` Jonathan Cameron
  2024-03-01 22:39 ` [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops Rob Herring
  4 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2024-02-25 14:27 UTC (permalink / raw
  To: devicetree, linux-iio, Rob Herring, Frank Rowand, linux-kernel,
	Julia Lawall
  Cc: Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut,
	Jonathan Cameron

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops.
  2024-02-25 14:27 [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops Jonathan Cameron
                   ` (3 preceding siblings ...)
  2024-02-25 14:27 ` [RESEND PATCH v2 4/4] iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped() Jonathan Cameron
@ 2024-03-01 22:39 ` Rob Herring
  2024-03-03 11:56   ` Jonathan Cameron
  4 siblings, 1 reply; 11+ messages in thread
From: Rob Herring @ 2024-03-01 22:39 UTC (permalink / raw
  To: Jonathan Cameron
  Cc: devicetree, linux-iio, Frank Rowand, linux-kernel, Julia Lawall,
	Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut,
	Jonathan Cameron

On Sun, Feb 25, 2024 at 02:27:10PM +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Some discussion occured on previous posting.
> https://lore.kernel.org/linux-iio/20240223124432.26443-1-Jonathan.Cameron@huawei.com/
> 
> 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/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.
> 
> 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 <robh@kernel.org>

I've got some drivers/of/ conversions too, but they are probably next 
cycle at this point.

Rob

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops.
  2024-03-01 22:39 ` [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops Rob Herring
@ 2024-03-03 11:56   ` Jonathan Cameron
  2024-03-09 17:33     ` Jonathan Cameron
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2024-03-03 11:56 UTC (permalink / raw
  To: Rob Herring
  Cc: devicetree, linux-iio, Frank Rowand, linux-kernel, Julia Lawall,
	Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut,
	Jonathan Cameron

On Fri, 1 Mar 2024 16:39:42 -0600
Rob Herring <robh@kernel.org> wrote:

> On Sun, Feb 25, 2024 at 02:27:10PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Some discussion occured on previous posting.
> > https://lore.kernel.org/linux-iio/20240223124432.26443-1-Jonathan.Cameron@huawei.com/
> > 
> > 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/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.
> > 
> > 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 <robh@kernel.org>
> 
> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops.
  2024-03-03 11:56   ` Jonathan Cameron
@ 2024-03-09 17:33     ` Jonathan Cameron
  2024-03-12 18:10       ` Rob Herring
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2024-03-09 17:33 UTC (permalink / raw
  To: Rob Herring, Greg Kroah-Hartman
  Cc: devicetree, linux-iio, Frank Rowand, Julia Lawall, Peter Zijlstra,
	Andy Shevchenko, marek.vasut, Jonathan Cameron

On Sun, 3 Mar 2024 11:56:33 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 1 Mar 2024 16:39:42 -0600
> Rob Herring <robh@kernel.org> wrote:
> 
> > On Sun, Feb 25, 2024 at 02:27:10PM +0000, Jonathan Cameron wrote:  
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > Some discussion occured on previous posting.
> > > https://lore.kernel.org/linux-iio/20240223124432.26443-1-Jonathan.Cameron@huawei.com/
> > > 
> > > 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/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.
> > > 
> > > 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 <robh@kernel.org>
> > 
> > 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.
> 

Greg, would you consider a last minute pull request for these, or picking them up
directly?  It would be helpful for Rob's follow ups and the work Julia is doing
with coccinelle and automating of locating cases to apply this approach.

If the device_for_each_child_node_scoped() series is fine this is almostly
exactly the same thing for the device tree specific case. Not sure what your
plans are for that pull request so I might be jumping the gun.

If not (and assuming the generic property version does make it in) I'll do
an immutable branch based on rc1 so that others can build on this via that.
Fiddlier solution for everyone but given how late we are, perhaps the wiser
one.

FWIW they've been in linux-next a week without issue.

Jonathan


> Thanks,
> 
> Jonathan
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops.
  2024-03-09 17:33     ` Jonathan Cameron
@ 2024-03-12 18:10       ` Rob Herring
  2024-03-13  9:18         ` Jonathan Cameron
  0 siblings, 1 reply; 11+ messages in thread
From: Rob Herring @ 2024-03-12 18:10 UTC (permalink / raw
  To: Jonathan Cameron, Greg Kroah-Hartman
  Cc: devicetree, linux-iio, Frank Rowand, Julia Lawall, Peter Zijlstra,
	Andy Shevchenko, marek.vasut, Jonathan Cameron

On Sat, Mar 9, 2024 at 10:33 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 3 Mar 2024 11:56:33 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
>
> > On Fri, 1 Mar 2024 16:39:42 -0600
> > Rob Herring <robh@kernel.org> wrote:
> >
> > > On Sun, Feb 25, 2024 at 02:27:10PM +0000, Jonathan Cameron wrote:
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >
> > > > Some discussion occured on previous posting.
> > > > https://lore.kernel.org/linux-iio/20240223124432.26443-1-Jonathan.Cameron@huawei.com/
> > > >
> > > > 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/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.
> > > >
> > > > 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 <robh@kernel.org>
> > >
> > > 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.
> >
>
> Greg, would you consider a last minute pull request for these, or picking them up
> directly?  It would be helpful for Rob's follow ups and the work Julia is doing
> with coccinelle and automating of locating cases to apply this approach.
>
> If the device_for_each_child_node_scoped() series is fine this is almostly
> exactly the same thing for the device tree specific case. Not sure what your
> plans are for that pull request so I might be jumping the gun.
>
> If not (and assuming the generic property version does make it in) I'll do
> an immutable branch based on rc1 so that others can build on this via that.
> Fiddlier solution for everyone but given how late we are, perhaps the wiser
> one.

I'm happy to pick up the first 3 patches for 6.9 if you want.

Rob

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops.
  2024-03-12 18:10       ` Rob Herring
@ 2024-03-13  9:18         ` Jonathan Cameron
  0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-03-13  9:18 UTC (permalink / raw
  To: Rob Herring
  Cc: Jonathan Cameron, Greg Kroah-Hartman, devicetree, linux-iio,
	Frank Rowand, Julia Lawall, Peter Zijlstra, Andy Shevchenko,
	marek.vasut

On Tue, 12 Mar 2024 12:10:46 -0600
Rob Herring <robh@kernel.org> wrote:

> On Sat, Mar 9, 2024 at 10:33 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 3 Mar 2024 11:56:33 +0000
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >  
> > > On Fri, 1 Mar 2024 16:39:42 -0600
> > > Rob Herring <robh@kernel.org> wrote:
> > >  
> > > > On Sun, Feb 25, 2024 at 02:27:10PM +0000, Jonathan Cameron wrote:  
> > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > >
> > > > > Some discussion occured on previous posting.
> > > > > https://lore.kernel.org/linux-iio/20240223124432.26443-1-Jonathan.Cameron@huawei.com/
> > > > >
> > > > > 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/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.
> > > > >
> > > > > 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 <robh@kernel.org>
> > > >
> > > > 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.
> > >  
> >
> > Greg, would you consider a last minute pull request for these, or picking them up
> > directly?  It would be helpful for Rob's follow ups and the work Julia is doing
> > with coccinelle and automating of locating cases to apply this approach.
> >
> > If the device_for_each_child_node_scoped() series is fine this is almostly
> > exactly the same thing for the device tree specific case. Not sure what your
> > plans are for that pull request so I might be jumping the gun.
> >
> > If not (and assuming the generic property version does make it in) I'll do
> > an immutable branch based on rc1 so that others can build on this via that.
> > Fiddlier solution for everyone but given how late we are, perhaps the wiser
> > one.  
> 
> I'm happy to pick up the first 3 patches for 6.9 if you want.

Thanks Rob, that would be great.

Jonathan

> 
> Rob
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RESEND PATCH v2 4/4] iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped()
  2024-02-25 14:27 ` [RESEND PATCH v2 4/4] iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped() Jonathan Cameron
@ 2024-03-25 19:53   ` Jonathan Cameron
  0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-03-25 19:53 UTC (permalink / raw
  To: devicetree, linux-iio, Rob Herring, Frank Rowand, linux-kernel,
	Julia Lawall
  Cc: Peter Zijlstra, Andy Shevchenko, Greg Kroah-Hartman, marek.vasut,
	Jonathan Cameron

On Sun, 25 Feb 2024 14:27:14 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 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>
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)


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-03-25 19:53 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-25 14:27 [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops Jonathan Cameron
2024-02-25 14:27 ` [RESEND PATCH v2 1/4] of: Add cleanup.h based auto release via __free(device_node) markings Jonathan Cameron
2024-02-25 14:27 ` [RESEND PATCH v2 2/4] of: Introduce for_each_*_child_of_node_scoped() to automate of_node_put() handling Jonathan Cameron
2024-02-25 14:27 ` [RESEND PATCH v2 3/4] of: unittest: Use for_each_child_of_node_scoped() Jonathan Cameron
2024-02-25 14:27 ` [RESEND PATCH v2 4/4] iio: adc: rcar-gyroadc: use for_each_available_child_node_scoped() Jonathan Cameron
2024-03-25 19:53   ` Jonathan Cameron
2024-03-01 22:39 ` [RESEND PATCH v2 0/4] of: automate of_node_put() - new approach to loops Rob Herring
2024-03-03 11:56   ` Jonathan Cameron
2024-03-09 17:33     ` Jonathan Cameron
2024-03-12 18:10       ` Rob Herring
2024-03-13  9:18         ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).