Received-SPF: pass (google.com: domain of linux-kernel+bounces-41872-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sun, 28 Jan 2024 19:06:53 +0100 (CET)
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>, 
    =?ISO-8859-15?Q?Nuno_S=E1?= <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.
In-Reply-To: <20240128160542.178315-1-jic23@kernel.org>
Message-ID: <alpine.DEB.2.22.394.2401281903550.3119@hadrien>
References: <20240128160542.178315-1-jic23@kernel.org>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII


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