| Message ID | 20240101172611.694830-2-jic23@kernel.org |
|---|---|
| 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 > >
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);