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

[Jonathan Cameron <jic23@kernel.org>]

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