Re: [PATCH 01/13] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.

From: Jonathan Cameron <jic23@kernel.org>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-iio@vger.kernel.org,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	"Len Brown" <lenb@kernel.org>,
	linux-acpi@vger.kernel.org,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Daniel Scally" <djrscally@gmail.com>,
	"Heikki Krogerus" <heikki.krogerus@linux.intel.com>,
	"Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Nuno Sá" <nuno.sa@analog.com>,
	"Cosmin Tanislav" <cosmin.tanislav@analog.com>,
	"Mihail Chindris" <mihail.chindris@analog.com>,
	"Rasmus Villemoes" <linux@rasmusvillemoes.dk>,
	"Tomislav Denis" <tomislav.denis@avl.com>,
	"Marek Vasut" <marex@denx.de>,
	"Olivier Moysan" <olivier.moysan@foss.st.com>,
	"Fabrice Gasnier" <fabrice.gasnier@foss.st.com>,
	"Lad Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"Dmitry Baryshkov" <dmitry.baryshkov@linaro.org>,
	"Marijn Suijten" <marijn.suijten@somainline.org>,
	"Marius Cristea" <marius.cristea@microchip.com>,
	"Ibrahim Tilki" <Ibrahim.Tilki@analog.com>,
	"Jonathan Cameron" <Jonathan.Cameron@huawei.com>,
	"Rob Herring" <robh@kernel.org>
Subject: Re: [PATCH 01/13] device property: Add cleanup.h based fwnode_handle_put() scope based cleanup.
Date: Sun, 21 Jan 2024 16:38:29 +0000	[thread overview]
Message-ID: <20240121163714.3670498f@jic23-huawei> (raw)
In-Reply-To: <Za0N_5Hp2s-uwOoM@smile.fi.intel.com>

On Sun, 21 Jan 2024 14:28:47 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sun, Jan 14, 2024 at 05:19:57PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > This allows the following
> > 
> > struct fwnode_handle *child __free(kfree) = NULL;

That's garbage.  Should be __free(fwnode_handle)!

> > 
> > device_for_each_child_node(dev, child) {
> > 	if (false)
> > 		return -EINVAL;
> > }
> > 
> > without the fwnode_handle_put() call which tends to complicate early
> > exits from such loops and lead to resource leak bugs.
> > 
> > Can also be used where the fwnode_handle was obtained from a call
> > such as fwnode_find_reference() as it will safely do nothing if
> > IS_ERR() is true.  
> 
> ...
> 
> >  struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
> >  void fwnode_handle_put(struct fwnode_handle *fwnode);  
> 
> I would add a blank line here
> 
> > +DEFINE_FREE(fwnode_handle, struct fwnode_handle *,
> > +	    if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T))
> >  
> >  int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
> >  int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);  
> 
> With the above,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
Thanks Andy - however..

The discussion with Rob about the DT equivalent took an interesting turn.

He raised the concern that the __free was not always tightly coupled with the
equivalent of  device_for_each_child_node() which as per similar
discussions elsewhere results in:
a) Potentially wrong ordering if there is other cleanup.h based stuff going on
   in the same function.
b) A lack of association between the setup of the free and what it is undoing.
  (this was the one Rob pointed at).

I proposed two options that here map to
1) Always drag the declaration next to the device_for_each_child_node()
   and intentionally don't set it to NULL.

{
	.... stuff....

	struct fwnode_handle *child __free(fwnode);
	device_for_each_child_node(dev, child) {
	}

2) Scoped version of the loops themselves.

#define device_for_each_child_node_scoped(dev, child)				\
	for (struct fw_node_handle *child __free(fwnode_handle)                 \
		 = device_get_next_child_node(dev, NULL);                       \
	    child; child = device_get_next_child_node(dev, child))

So that the child only exists at all in the scope of the loop.

What do you think of the options?

DT thread is here:
https://lore.kernel.org/linux-iio/20240114165358.119916-1-jic23@kernel.org/T/#t

Jonathan