From: Julia Lawall <julia.lawall@inria.fr>
To: Jonathan Cameron <jic23@kernel.org>
Cc: linux-iio@vger.kernel.org, "Rob Herring" <robh@kernel.org>,
"Frank Rowand" <frowand.list@gmail.com>,
linux-kernel@vger.kernel.org,
"Julia Lawall" <Julia.Lawall@inria.fr>,
"Nicolas Palix" <nicolas.palix@imag.fr>,
"Sumera Priyadarsini" <sylphrenadin@gmail.com>,
"Rafael J . Wysocki" <rafael@kernel.org>,
"Len Brown" <lenb@kernel.org>,
linux-acpi@vger.kernel.org,
"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Nuno Sá" <nuno.sa@analog.com>,
"Jonathan Cameron" <Jonathan.Cameron@huawei.com>
Subject: Re: [RFC PATCH 0/5] of: automate of_node_put() - new approach to loops.
Date: Sun, 28 Jan 2024 19:06:53 +0100 (CET) [thread overview]
Message-ID: <alpine.DEB.2.22.394.2401281903550.3119@hadrien> (raw)
In-Reply-To: <20240128160542.178315-1-jic23@kernel.org>
On Sun, 28 Jan 2024, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> +CC includes peopleinterested in property.h equivalents to minimize
> duplication of discussion. Outcome of this discussion will affect:
> https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/
> [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.
>
> In discussion of previous approach with Rob Herring we talked about various
> ways to avoid a disconnect between the declaration of the __free(device_node)
> and the first non NULL assignment. Making this connection clear is useful for 2
> reasons:
> 1) Avoids out of order cleanup with respect to other cleanup.h usage.
> 2) Avoids disconnect between how cleanup is to be done and how the reference
> was acquired in the first place.
>
> https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/
>
> The options we discussed are:
>
> 1) Ignore this issue and merge original set.
>
> 2) Always put the declaration just before the for loop and don't set it NULL.
>
> {
> int ret;
>
> ret = ... and other fun code.
>
> struct device_node *child __free(device_node);
> for_each_child_of_node(np, child) {
> }
> }
>
> This works but careful review is needed to ensure that this unusual pattern is
> followed. We don't set it to NULL as the loop will do that anyway if there are
> no child nodes, or the loop finishes without an early break or return.
>
> 3) Introduced the pointer to auto put device_node only within the
> for loop scope.
>
> +#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))
> +
>
> This series is presenting option 3. I only implemented this loop out of
> all the similar ones and it is only compile tested.
>
> Disadvantage Rob raised is that it isn't obvious this macro will instantiate
> a struct device_node *child. I can't see a way around that other than option 2
> above, but all suggestions welcome. Note that if a conversion leaves an
> 'external' struct device_node *child variable, in many cases the compiler
> will catch that as an unused variable. We don't currently run shaddow
> variable detection in normal kernel builds, but that could also be used
> to catch such bugs.
>
> All comments welcome.
It looks promising to get rid of a lot of clunky and error-prone
error-handling code.
I guess that
for_each_child_of_node_scoped(parent, struct device_node *, child)
would seem too verbose?
There are a lot of opportunities for device_node loops, but also for some
more obscure loops over other types. And there are a lot of of_node_puts
that could be eliminated independent of loops.
julia
>
> Jonathan Cameron (5):
> of: Add cleanup.h based auto release via __free(device_node) markings.
> of: Introduce for_each_child_of_node_scoped() to automate
> of_node_put() handling
> of: unittest: Use __free(device_node)
> iio: adc: fsl-imx25-gcq: Use for_each_child_node_scoped()
> iio: adc: rcar-gyroadc: use for_each_child_node_scoped()
>
> drivers/iio/adc/fsl-imx25-gcq.c | 13 +++----------
> drivers/iio/adc/rcar-gyroadc.c | 21 ++++++---------------
> drivers/of/unittest.c | 11 +++--------
> include/linux/of.h | 8 ++++++++
> 4 files changed, 20 insertions(+), 33 deletions(-)
>
> --
> 2.43.0
>
>
next prev parent reply other threads:[~2024-01-28 18:07 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-28 16:05 [RFC PATCH 0/5] of: automate of_node_put() - new approach to loops Jonathan Cameron
2024-01-28 16:05 ` [RFC PATCH 1/5] of: Add cleanup.h based auto release via __free(device_node) markings Jonathan Cameron
2024-01-28 16:05 ` [RFC PATCH 2/5] of: Introduce for_each_child_of_node_scoped() to automate of_node_put() handling Jonathan Cameron
2024-01-28 21:11 ` David Lechner
2024-01-29 6:54 ` Julia Lawall
2024-01-29 11:44 ` Jonathan Cameron
2024-01-31 23:51 ` Rob Herring
2024-02-01 15:17 ` Jonathan Cameron
2024-02-04 19:56 ` Jonathan Cameron
2024-02-04 20:52 ` Jonathan Cameron
2024-01-28 16:05 ` [RFC PATCH 3/5] of: unittest: Use for_each_child_of_node_scoped() Jonathan Cameron
2024-01-28 16:05 ` [RFC PATCH 4/5] iio: adc: fsl-imx25-gcq: Use for_each_child_node_scoped() Jonathan Cameron
2024-01-28 16:05 ` [RFC PATCH 5/5] iio: adc: rcar-gyroadc: use for_each_child_node_scoped() Jonathan Cameron
2024-01-28 18:06 ` Julia Lawall [this message]
2024-01-29 11:42 ` [RFC PATCH 0/5] of: automate of_node_put() - new approach to loops Jonathan Cameron
2024-01-29 14:02 ` Julia Lawall
2024-01-29 19:52 ` Jonathan Cameron
2024-01-29 20:29 ` Julia Lawall
2024-01-30 9:38 ` Jonathan Cameron
2024-01-30 10:26 ` Julia Lawall
2024-01-31 21:38 ` Julia Lawall
2024-02-04 21:08 ` Jonathan Cameron
2024-02-04 21:34 ` Julia Lawall
2024-02-05 9:27 ` Jonathan Cameron
2024-02-01 11:20 ` Andy Shevchenko
2024-02-01 15:21 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.22.394.2401281903550.3119@hadrien \
--to=julia.lawall@inria.fr \
--cc=Jonathan.Cameron@huawei.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=frowand.list@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=jic23@kernel.org \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nicolas.palix@imag.fr \
--cc=nuno.sa@analog.com \
--cc=rafael@kernel.org \
--cc=robh@kernel.org \
--cc=sylphrenadin@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).