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

[Jonathan Cameron <jic23@kernel.org>]

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