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