[RFC,2/5] of: Introduce for_each_child_of_node_scoped() to automate of_node_put() handling

  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.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/linux/of.h | 6 ++++++
 1 file changed, 6 insertions(+)
  

On Sun, Jan 28, 2024 at 10:06 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> 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.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  include/linux/of.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 50e882ee91da..f822226eac6d 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1434,6 +1434,12 @@ static inline int of_property_read_s32(const struct device_node *np,
>         for (child = of_get_next_available_child(parent, NULL); child != NULL; \
>              child = of_get_next_available_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_available_child(parent, child))

Doesn't this need to match the initializer (of_get_next_child)?
Otherwise it seems like the first node could be a disabled node but no
other disabled nodes would be included in the iteration.

It seems like we would want two macros, one for each variation,
analogous to for_each_child_of_node() and
for_each_available_child_of_node().


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

On Sun, 28 Jan 2024, David Lechner wrote:

> On Sun, Jan 28, 2024 at 10:06 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > 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.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  include/linux/of.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index 50e882ee91da..f822226eac6d 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -1434,6 +1434,12 @@ static inline int of_property_read_s32(const struct device_node *np,
> >         for (child = of_get_next_available_child(parent, NULL); child != NULL; \
> >              child = of_get_next_available_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_available_child(parent, child))
>
> Doesn't this need to match the initializer (of_get_next_child)?
> Otherwise it seems like the first node could be a disabled node but no
> other disabled nodes would be included in the iteration.
>
> It seems like we would want two macros, one for each variation,
> analogous to for_each_child_of_node() and
> for_each_available_child_of_node().

There are a bunch of iterators, and I guess a scoped version is needed for
each of them?

julia


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

On Sun, Jan 28, 2024 at 03:11:01PM -0600, David Lechner wrote:
> On Sun, Jan 28, 2024 at 10:06 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > 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.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  include/linux/of.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index 50e882ee91da..f822226eac6d 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -1434,6 +1434,12 @@ static inline int of_property_read_s32(const struct device_node *np,
> >         for (child = of_get_next_available_child(parent, NULL); child != NULL; \
> >              child = of_get_next_available_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_available_child(parent, child))
> 
> Doesn't this need to match the initializer (of_get_next_child)?
> Otherwise it seems like the first node could be a disabled node but no
> other disabled nodes would be included in the iteration.
> 
> It seems like we would want two macros, one for each variation,
> analogous to for_each_child_of_node() and
> for_each_available_child_of_node().

Yes, but really I'd like these the other way around. 'available' should 
be the default as disabled should really be the same as a node not 
present except for a few cases where it is not.

I bring it up only because if we're changing things then it is a 
convenient time to change this. That's really a side issue to sorting 
out how this new way should work.

Rob
  

On Sun, 28 Jan 2024 15:11:01 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On Sun, Jan 28, 2024 at 10:06 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > 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.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  include/linux/of.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index 50e882ee91da..f822226eac6d 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -1434,6 +1434,12 @@ static inline int of_property_read_s32(const struct device_node *np,
> >         for (child = of_get_next_available_child(parent, NULL); child != NULL; \
> >              child = of_get_next_available_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_available_child(parent, child))  
> 
> Doesn't this need to match the initializer (of_get_next_child)?
> Otherwise it seems like the first node could be a disabled node but no
> other disabled nodes would be included in the iteration.

FwIW that was was entirely unintentional.  Not sure how it happened :(
Anyhow, now will be for_each_available_child_of_node_scoped() with the
right first call.

> 
> It seems like we would want two macros, one for each variation,
> analogous to for_each_child_of_node() and
> for_each_available_child_of_node().
> 
> 
> > +
> >  #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.43.0
> >
> >