| Message ID | 20240114172009.179893-2-jic23@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling. | expand |
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>
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
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
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 --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);