On Wed, 20 Dec 2023 16:11:44 -0600
Rob Herring <robh@xxxxxxxxxx> wrote:
> On Sun, Dec 17, 2023 at 06:46:45PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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
