[RFC PATCH 0/4] of: Automate handling of of_node_put()

[RFC PATCH 0/4] of: Automate handling of of_node_put()

[Jonathan Cameron]

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

Recent addition of scope based cleanup (linux/cleanup.h) allows us
to avoid a large number of places where error handlers and early
returns have to carefully deal with left over resources.
The need to call of_node_put() on breaking out of loops over child
nodes is one of these cases and this series is to address that.

I have a similar series for property.h which I'll send out shortly
if this one sees no show stoppers.

Jonathan

Jonathan Cameron (4):
  of: Add cleanup.h based autorelease via __free(device_node) markings.
  of: unittest: Use __free(device_node)
  iio: adc: fsl-imx25-gcq: Use __free(device_node)
  iio: adc: rcar-gyroadc: use __free(device_node)

 drivers/iio/adc/fsl-imx25-gcq.c | 12 +++---------
 drivers/iio/adc/rcar-gyroadc.c  | 20 ++++++--------------
 drivers/of/unittest.c           | 10 +++-------
 include/linux/of.h              |  2 ++
 4 files changed, 14 insertions(+), 30 deletions(-)

-- 
2.43.0

[RFC PATCH 1/4] of: Add cleanup.h based autorelease via __free(device_node) markings.

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

In this simile example the gains are small but there are some very
complex error handling cases burried in these loops that wil be
greatly simplified by enabling early returns with out the need
for this manual of_node_put() call.

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

[RFC PATCH 2/4] of: unittest: Use __free(device_node)

[Jonathan Cameron]

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

A simple example of the utility of this autocleanup approach to
handling of_node_put()

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/of/unittest.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e9e90e96600e..b6d9edb831f0 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -233,27 +233,23 @@ static void __init of_unittest_dynamic(void)
 
 static int __init of_unittest_check_node_linkage(struct device_node *np)
 {
-	struct device_node *child;
+	struct device_node *child __free(device_node) = NULL;
 	int count = 0, rc;
 
 	for_each_child_of_node(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.43.0

[RFC PATCH 3/4] iio: adc: fsl-imx25-gcq: Use __free(device_node)

[Jonathan Cameron]

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

Using automated cleanup reduces chance of an reference count leak
and simplfies the code.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/fsl-imx25-gcq.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
index 68c813de0605..e04f92d7a953 100644
--- a/drivers/iio/adc/fsl-imx25-gcq.c
+++ b/drivers/iio/adc/fsl-imx25-gcq.c
@@ -199,7 +199,7 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
 			       struct mx25_gcq_priv *priv)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct device_node *child;
+	struct device_node *child __free(device_node) = NULL;
 	struct device *dev = &pdev->dev;
 	int ret, i;
 
@@ -224,14 +224,12 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
 		ret = of_property_read_u32(child, "reg", &reg);
 		if (ret) {
 			dev_err(dev, "Failed to get reg property\n");
-			of_node_put(child);
 			return ret;
 		}
 
 		if (reg >= MX25_NUM_CFGS) {
 			dev_err(dev,
 				"reg value is greater than the number of available configuration registers\n");
-			of_node_put(child);
 			return -EINVAL;
 		}
 
@@ -243,10 +241,9 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
 		case MX25_ADC_REFP_XP:
 		case MX25_ADC_REFP_YP:
 			ret = mx25_gcq_ext_regulator_setup(&pdev->dev, priv, refp);
-			if (ret) {
-				of_node_put(child);
+			if (ret)
 				return ret;
-			}
+
 			priv->channel_vref_mv[reg] =
 				regulator_get_voltage(priv->vref[refp]);
 			/* Conversion from uV to mV */
@@ -257,7 +254,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
 			break;
 		default:
 			dev_err(dev, "Invalid positive reference %d\n", refp);
-			of_node_put(child);
 			return -EINVAL;
 		}
 
@@ -270,12 +266,10 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
 
 		if ((refp & MX25_ADCQ_CFG_REFP_MASK) != refp) {
 			dev_err(dev, "Invalid fsl,adc-refp property value\n");
-			of_node_put(child);
 			return -EINVAL;
 		}
 		if ((refn & MX25_ADCQ_CFG_REFN_MASK) != refn) {
 			dev_err(dev, "Invalid fsl,adc-refn property value\n");
-			of_node_put(child);
 			return -EINVAL;
 		}
 
-- 
2.43.0

[RFC PATCH 4/4] iio: adc: rcar-gyroadc: use __free(device_node)

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

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/rcar-gyroadc.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
index d524f2e8e927..9d6227b31724 100644
--- a/drivers/iio/adc/rcar-gyroadc.c
+++ b/drivers/iio/adc/rcar-gyroadc.c
@@ -318,7 +318,7 @@ 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 device_node *child __free(device_node) = NULL;
 	struct regulator *vref;
 	unsigned int reg;
 	unsigned int adcmode = -1, childmode;
@@ -352,7 +352,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 +369,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 +377,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 +386,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 +396,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 +421,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 +431,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.43.0

Re: [RFC PATCH 1/4] of: Add cleanup.h based autorelease via __free(device_node) markings.

[Rob Herring]

On Sun, Dec 17, 2023 at 06:46:45PM +0000, Jonathan Cameron wrote:
> 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()
> 
> In this simile example the gains are small but there are some very

typo

> complex error handling cases burried in these loops that wil be
> greatly simplified by enabling early returns with out the need
> for this manual of_node_put() call.

Neat!

I guess that now that the coccinelle check has fixed many, we can update 
it to the new way and start fixing them all again. We should update the 
coccinelle script with the new way. See 
scripts/coccinelle/iterators/for_each_child.cocci.

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

of_node_put() can be called with NULL, so do we need the "if (_T)"?

Rob

Re: [RFC PATCH 1/4] of: Add cleanup.h based autorelease via __free(device_node) markings.

[Jonathan Cameron]

On Wed, 20 Dec 2023 16:11:44 -0600
Rob Herring <robh@kernel.org> wrote:

> On Sun, Dec 17, 2023 at 06:46:45PM +0000, Jonathan Cameron wrote:
> > 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()
> > 
> > In this simile example the gains are small but there are some very  
> 
> typo
> 
> > complex error handling cases burried in these loops that wil be
> > greatly simplified by enabling early returns with out the need
> > for this manual of_node_put() call.  
> 
> Neat!
> 
> I guess that now that the coccinelle check has fixed many, we can update 
> it to the new way and start fixing them all again. We should update the 
> coccinelle script with the new way. See 
> scripts/coccinelle/iterators/for_each_child.cocci.

If the holiday season gets dull enough I'll take a look at updating that
as well. Been a long time since I last messed with coccinelle.

Given this is just a simplification rather than a fix, there would be no rush
to convert things over but we definitely don't want the coccinelle script
to generate lots of false positives.  + we should perhaps add a
script to try and catch the opposite (double free) as a result of
using this automated cleanup.

> 
> > 
> > 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))  
> 
> of_node_put() can be called with NULL, so do we need the "if (_T)"?

Nope - should be fine to call it without. I was being lazy and didn't check :)

> 
> Rob

Re: [RFC PATCH 1/4] of: Add cleanup.h based autorelease via __free(device_node) markings.

[Jonathan Cameron]

> >   
> > > 
> > > 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))    
> > 
> > of_node_put() can be called with NULL, so do we need the "if (_T)"?  
> 
> Nope - should be fine to call it without. I was being lazy and didn't check :)

Seems there has been a lot of discussion of this for similar cases and
consensus is to keep the if (_T)
e.g. 

https://lore.kernel.org/all/6596edda327c8_8dc68294b2@dwillia2-xfh.jf.intel.com.notmuch/

> 
> > 
> > Rob  
> 
> 

Re: [RFC PATCH 1/4] of: Add cleanup.h based autorelease via __free(device_node) markings.

[Jonathan Cameron]

On Thu, 21 Dec 2023 10:54:34 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 20 Dec 2023 16:11:44 -0600
> Rob Herring <robh@kernel.org> wrote:
> 
> > On Sun, Dec 17, 2023 at 06:46:45PM +0000, Jonathan Cameron wrote:  
> > > 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()
> > > 
> > > In this simile example the gains are small but there are some very    
> > 
> > typo
> >   
> > > complex error handling cases burried in these loops that wil be
> > > greatly simplified by enabling early returns with out the need
> > > for this manual of_node_put() call.    
> > 
> > Neat!
> > 
> > I guess that now that the coccinelle check has fixed many, we can update 
> > it to the new way and start fixing them all again. We should update the 
> > coccinelle script with the new way. See 
> > scripts/coccinelle/iterators/for_each_child.cocci.  
> 
> If the holiday season gets dull enough I'll take a look at updating that
> as well. Been a long time since I last messed with coccinelle.
> 
> Given this is just a simplification rather than a fix, there would be no rush
> to convert things over but we definitely don't want the coccinelle script
> to generate lots of false positives.  + we should perhaps add a
> script to try and catch the opposite (double free) as a result of
> using this automated cleanup.
Hi Rob,

As things currently stand the script doesn't trigger on a
struct device_node __free(device_node); (which is wrong anyway)
or
struct device_node __free(device_node) = NULL;

So we at least don't cause a flurry of false positives via these
changes.

I'm not keen to add an upstream check to encourage conversion over
to this new approach simply because there is no great rush to do it
and it's easy enough to use grep to find potential targets today.

Also strongly motivated by the fact I don't really have time to
learn coccinelle (however useful that would be in the long run!)

As such I'll tidy these up a bit and send out a non RFC version with
cover letter additions to mention we don't cause false positives and
that a coccinelle script to find candidates might make sense in the
longer term.  It may also make sense to add checks that we don't manually
release the node on error paths without making sure to steal the pointer
(which sets it to NULL to avoid problems).

+CC various Coccinelle folk even though I'm proposing to not do any
coccinelle scripting for now.

Jonathan