diff mbox series

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

Message ID 20240114172009.179893-2-jic23@kernel.org (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling. | expand

Commit Message

Jonathan Cameron Jan. 14, 2024, 5:19 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>
---
v1: Thanks to Andy for reviewing the RFC.
    Add check for if (!IS_ERR_OR_NULL(_T)) to allow the compiler to optimize
    cases where it knows the passed in parameter is NULL or an error pointer.
---
 include/linux/property.h | 3 +++
 1 file changed, 3 insertions(+)

Comments

Andy Shevchenko Jan. 21, 2024, 12:28 p.m. UTC | #1
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;
> 
> 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>
Jonathan Cameron Jan. 21, 2024, 4:38 p.m. UTC | #2
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
Lukas Wunner Jan. 21, 2024, 6:06 p.m. UTC | #3
On Sun, Jan 14, 2024 at 05:19:57PM +0000, Jonathan Cameron wrote:
> This allows the following
> 
> struct fwnode_handle *child __free(fwnode_handle) = 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>
> ---
> v1: Thanks to Andy for reviewing the RFC.
>     Add check for if (!IS_ERR_OR_NULL(_T)) to allow the compiler to optimize
>     cases where it knows the passed in parameter is NULL or an error pointer.

Heads-up:  Using IS_ERR_OR_NULL() in DEFINE_FREE() macros bloats
the code with additional IS_ERR() checks and NULL pointer checks.

See the detailed explanation in this patch which adds a DEFINE_FREE()
macro for x509_free_certificate():

https://lore.kernel.org/all/70ecd3904a70d2b92f8f1e04365a2b9ce66fac25.1705857475.git.lukas@wunner.de/

I'm wondering if a solution might be to stop returning IS_ERR()
from "constructors" such as x509_cert_parse() and instead assign
the created "object" (x509_certificate) to a call-by-reference
pointer and return an integer.  If the returned integer is not 0,
inhibit "destruction" of the "object" with no_free_ptr().

Thoughts?


> +DEFINE_FREE(fwnode_handle, struct fwnode_handle *,
> +	    if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T))

If you do not align the "if" to the opening parenthesis,
checkpatch --strict complains:
"CHECK: Alignment should match open parenthesis"

If you do align to the opening parenthesis, it complains:
"WARNING: Statements should start on a tabstop"

I chose the latter for x509_free_certificate() for aesthetic reasons.

Either way, checkpatch still emits:

ERROR: trailing statements should be on next line"
#183: FILE: crypto/asymmetric_keys/x509_parser.h:49:
+	    if (!IS_ERR_OR_NULL(_T)) x509_free_certificate(_T))

Can't make it happy with these new-fangled DEFINE_FREE macros it seems. :(

Thanks,

Lukas
Lukas Wunner Jan. 21, 2024, 6:20 p.m. UTC | #4
On Sun, Jan 21, 2024 at 07:06:03PM +0100, Lukas Wunner wrote:
> On Sun, Jan 14, 2024 at 05:19:57PM +0000, Jonathan Cameron wrote:
> > v1: Thanks to Andy for reviewing the RFC.
> >     Add check for if (!IS_ERR_OR_NULL(_T)) to allow the compiler to optimize
> >     cases where it knows the passed in parameter is NULL or an error pointer.
> 
> Heads-up:  Using IS_ERR_OR_NULL() in DEFINE_FREE() macros bloats
> the code with additional IS_ERR() checks and NULL pointer checks.
> 
> See the detailed explanation in this patch which adds a DEFINE_FREE()
> macro for x509_free_certificate():
> 
> https://lore.kernel.org/all/70ecd3904a70d2b92f8f1e04365a2b9ce66fac25.1705857475.git.lukas@wunner.de/
> 
> I'm wondering if a solution might be to stop returning IS_ERR()
> from "constructors" such as x509_cert_parse() and instead assign
> the created "object" (x509_certificate) to a call-by-reference
> pointer and return an integer.  If the returned integer is not 0,
> inhibit "destruction" of the "object" with no_free_ptr().

Another idea would be to use a call-by-reference pointer and check
the pointer instead of the return code.

E.g.:

DEFINE_FREE(x509_free_certificate, struct x509_certificate *,
	    if (_T) x509_free_certificate(_T))

...

	struct x509_certificate __free(x509_free_certificate) = NULL;
	int ret;

	ret = x509_cert_parse(&cert, buf, len);
	if (!cert)
		return ret;
diff mbox series

Patch

diff --git a/include/linux/property.h b/include/linux/property.h
index 2b8f07fc68a9..9f3190d902ab 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,8 @@  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 *,
+	    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);