diff mbox series

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

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

Commit Message

Jonathan Cameron Jan. 1, 2024, 5:25 p.m. UTC
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.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/linux/property.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

Andy Shevchenko Jan. 6, 2024, 3:16 p.m. UTC | #1
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.
Jonathan Cameron Jan. 6, 2024, 5:53 p.m. UTC | #2
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 mbox series

Patch

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);