| Message ID | 20240101172611.694830-2-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling. | expand |
On Mon, Jan 01, 2024 at 05:25:59PM +0000, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > This allows the following > > struct fwnode_handle *child __free(kfree) = NULL; > > 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); > +DEFINE_FREE(fwnode_handle, struct fwnode_handle *, fwnode_handle_put(_T)) In GPIO we have something similar and PeterZ explained there why if (_T) is important, hence this should be DEFINE_FREE(fwnode_handle, struct fwnode_handle *, if (_T) fwnode_handle_put(_T)) or even DEFINE_FREE(fwnode_handle, struct fwnode_handle *, if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T)) as we accept in many calls an error pointer as unset / undefined firmware node handle.
On 6 January 2024 15:16:53 GMT, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >On Mon, Jan 01, 2024 at 05:25:59PM +0000, Jonathan Cameron wrote: >> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> >> This allows the following >> >> struct fwnode_handle *child __free(kfree) = NULL; >> >> 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); >> +DEFINE_FREE(fwnode_handle, struct fwnode_handle *, fwnode_handle_put(_T)) > >In GPIO we have something similar and PeterZ explained there why if (_T) is >important, hence this should be I can't find the reference unfortunately. > >DEFINE_FREE(fwnode_handle, struct fwnode_handle *, if (_T) fwnode_handle_put(_T)) > >or even > >DEFINE_FREE(fwnode_handle, struct fwnode_handle *, if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T)) > >as we accept in many calls an error pointer as unset / undefined firmware node >handle. The function called has a protection for null and error inputs so I'm not sure why extra protection is needed? J > >
On 06/01/2024 18.53, Jonathan Cameron wrote: > > ething similar and PeterZ explained there why if (_T) is >> important, hence this should be > > I can't find the reference unfortunately. > >> >> DEFINE_FREE(fwnode_handle, struct fwnode_handle *, if (_T) fwnode_handle_put(_T)) >> >> or even >> >> DEFINE_FREE(fwnode_handle, struct fwnode_handle *, if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T)) >> >> as we accept in many calls an error pointer as unset / undefined firmware node >> handle. > > The function called has a protection for null > and error inputs so I'm not sure why extra protection > is needed? IIRC, it's for code generation, avoiding emitting the call to the cleanup function on the code paths where the compiler knows the argument is NULL. And on the other return paths, the compiler most likely knows the value is non-NULL, so the conditional is elided (but of course not the put call). Read ERR_OR_NULL as appropriate. Rasmus
On Sun, 7 Jan 2024 22:05:35 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On 06/01/2024 18.53, Jonathan Cameron wrote: > > > > ething similar and PeterZ explained there why if (_T) is > >> important, hence this should be > > > > I can't find the reference unfortunately. > > > >> > >> DEFINE_FREE(fwnode_handle, struct fwnode_handle *, if (_T) fwnode_handle_put(_T)) > >> > >> or even > >> > >> DEFINE_FREE(fwnode_handle, struct fwnode_handle *, if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T)) > >> > >> as we accept in many calls an error pointer as unset / undefined firmware node > >> handle. > > > > The function called has a protection for null > > and error inputs so I'm not sure why extra protection > > is needed? > > IIRC, it's for code generation, avoiding emitting the call to the > cleanup function on the code paths where the compiler knows the argument > is NULL. And on the other return paths, the compiler most likely knows > the value is non-NULL, so the conditional is elided (but of course not > the put call). Read ERR_OR_NULL as appropriate. > > Rasmus > > Thanks. Makes sense. A reference in another thread where this was being discussed lead me to a reference to Linus arguing for just this: https://lore.kernel.org/all/20230814161731.GN776869@hirez.programming.kicks-ass.net/
diff --git a/include/linux/property.h b/include/linux/property.h index 2b8f07fc68a9..135ac540f8fe 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -12,6 +12,7 @@ #include <linux/args.h> #include <linux/bits.h> +#include <linux/cleanup.h> #include <linux/fwnode.h> #include <linux/stddef.h> #include <linux/types.h> @@ -161,6 +162,7 @@ struct fwnode_handle *device_get_named_child_node(const struct device *dev, struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode); void fwnode_handle_put(struct fwnode_handle *fwnode); +DEFINE_FREE(fwnode_handle, struct fwnode_handle *, 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);