diff mbox series

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

Message ID 20240128160542.178315-3-jic23@kernel.org (mailing list archive)
State RFC, archived
Headers show
Series of: automate of_node_put() - new approach to loops. | expand

Commit Message

Jonathan Cameron Jan. 28, 2024, 4:05 p.m. UTC
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(+)

Comments

David Lechner Jan. 28, 2024, 9:11 p.m. UTC | #1
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
>
>
Julia Lawall Jan. 29, 2024, 6:54 a.m. UTC | #2
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
> >
> >
>
Jonathan Cameron Jan. 29, 2024, 11:44 a.m. UTC | #3
On Mon, 29 Jan 2024 07:54:57 +0100 (CET)
Julia Lawall <julia.lawall@inria.fr> wrote:

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

Yes. I just didn't want to add too much to the RFC. I'd want to
convert a user of each as part of the patch set introducing the new
loop definitions.

Jonathan

> 
> 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
> > >
> > >  
> >
Rob Herring (Arm) Jan. 31, 2024, 11:51 p.m. UTC | #4
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
Jonathan Cameron Feb. 1, 2024, 3:17 p.m. UTC | #5
On Wed, 31 Jan 2024 17:51:48 -0600
Rob Herring <robh@kernel.org> wrote:

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

Happy to push that forwards by not initially defining the non available version
of this scoped form. So we will just have

for_each_avaiable_child_of_node_scoped()

Short and snappy it isn't but such is life.

Jonathan

> 
> Rob
>
Jonathan Cameron Feb. 4, 2024, 7:56 p.m. UTC | #6
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
> >
> >
Jonathan Cameron Feb. 4, 2024, 8:52 p.m. UTC | #7
On Sun, 4 Feb 2024 19:56:11 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

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

*sigh* Which I can't use for the unittest case (tests fail as a result
as some nodes that need to be covered are not available) so both will need to exist
though we can hopefully review each change for whether the appropriate one
is used afterwards (ignoring whether it was with the non scoped versions)

Jonathan

> 
> > 
> > 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
> > >
> > >    
> 
>
diff mbox series

Patch

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))
+
 #define for_each_of_cpu_node(cpu) \
 	for (cpu = of_get_next_cpu_node(NULL); cpu != NULL; \
 	     cpu = of_get_next_cpu_node(cpu))