On Sun, 21 Jan 2024 14:28:47 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> On Sun, Jan 14, 2024 at 05:19:57PM +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
>
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@xxxxxxxxxx/T/#t
Jonathan
