| Message ID | 20240211192540.340682-2-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling. | expand |
Hi Jonathan, On Sun, Feb 11, 2024 at 07:25:27PM +0000, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Useful 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 > and will automatically release the reference on the variable leaving > scope. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > include/linux/property.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/linux/property.h b/include/linux/property.h > index e6516d0b7d52..bcda028f1a33 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> > @@ -188,6 +189,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)) fwnode_handle_put() can be safely called on NULL or error pointer fwnode so you can remove the check. > > 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);
On Mon, 12 Feb 2024 08:49:23 +0000 Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > Hi Jonathan, > > On Sun, Feb 11, 2024 at 07:25:27PM +0000, Jonathan Cameron wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Useful 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 > > and will automatically release the reference on the variable leaving > > scope. > > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > include/linux/property.h | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/include/linux/property.h b/include/linux/property.h > > index e6516d0b7d52..bcda028f1a33 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> > > @@ -188,6 +189,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)) > > fwnode_handle_put() can be safely called on NULL or error pointer fwnode so > you can remove the check. Was discussed in the RFC thread (where i didn't have this protection) https://lore.kernel.org/linux-iio/20240108125117.000010fb@Huawei.com/ includes a reference to Linus Torvald's view on this. All comes down to compiler visibility and optimization opportunities, which are improved if the check is in the macro definition. Jonathan > > > > > 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); >
On Mon, Feb 12, 2024 at 08:49:23AM +0000, Sakari Ailus wrote: > On Sun, Feb 11, 2024 at 07:25:27PM +0000, Jonathan Cameron wrote: ... > > +DEFINE_FREE(fwnode_handle, struct fwnode_handle *, > > + if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T)) > > fwnode_handle_put() can be safely called on NULL or error pointer fwnode Yes. > so you can remove the check. No. (Technically "yes", but better "no".) This has been discussed a lot, including the LWN wrap-up about the cleanup.h.
On Sun, Feb 11, 2024 at 07:25:27PM +0000, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Useful 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 > and will automatically release the reference on the variable leaving > scope. ... > 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, but it's a minor comment anyway. > +DEFINE_FREE(fwnode_handle, struct fwnode_handle *, > + if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T))
Hi Jonathan, On Mon, Feb 12, 2024 at 11:42:06AM +0000, Jonathan Cameron wrote: > On Mon, 12 Feb 2024 08:49:23 +0000 > Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > > Hi Jonathan, > > > > On Sun, Feb 11, 2024 at 07:25:27PM +0000, Jonathan Cameron wrote: > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > Useful 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 > > > and will automatically release the reference on the variable leaving > > > scope. > > > > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > --- > > > include/linux/property.h | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/include/linux/property.h b/include/linux/property.h > > > index e6516d0b7d52..bcda028f1a33 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> > > > @@ -188,6 +189,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)) > > > > fwnode_handle_put() can be safely called on NULL or error pointer fwnode so > > you can remove the check. > > Was discussed in the RFC thread (where i didn't have this protection) > > https://lore.kernel.org/linux-iio/20240108125117.000010fb@Huawei.com/ > includes a reference to Linus Torvald's view on this. > > All comes down to compiler visibility and optimization opportunities, which are improved > if the check is in the macro definition. Hmm. In that case I'd rather make fwnode_handle_put() and similar trivial functions macros. There's no need to add cruft here and about a 100-fold number of callers will get the same benefit.
On Mon, Feb 12, 2024 at 12:36:46PM +0000, Sakari Ailus wrote: > On Mon, Feb 12, 2024 at 11:42:06AM +0000, Jonathan Cameron wrote: ... > Hmm. In that case I'd rather make fwnode_handle_put() and similar trivial > functions macros. This will kill the type-checking opportunity, so I'm against this move.
On Mon, Feb 12, 2024 at 02:46:49PM +0200, Andy Shevchenko wrote: > On Mon, Feb 12, 2024 at 12:36:46PM +0000, Sakari Ailus wrote: > > On Mon, Feb 12, 2024 at 11:42:06AM +0000, Jonathan Cameron wrote: > > ... > > > Hmm. In that case I'd rather make fwnode_handle_put() and similar trivial > > functions macros. > > This will kill the type-checking opportunity, so I'm against this move. Then it could be made static inline and moved to the header. I suppose for modern compilers there should be no difference in between the two optimisation-wise.
On Mon, 12 Feb 2024 12:58:03 +0000 Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > On Mon, Feb 12, 2024 at 02:46:49PM +0200, Andy Shevchenko wrote: > > On Mon, Feb 12, 2024 at 12:36:46PM +0000, Sakari Ailus wrote: > > > On Mon, Feb 12, 2024 at 11:42:06AM +0000, Jonathan Cameron wrote: > > > > ... > > > > > Hmm. In that case I'd rather make fwnode_handle_put() and similar trivial > > > functions macros. > > > > This will kill the type-checking opportunity, so I'm against this move. > > Then it could be made static inline and moved to the header. I suppose for > modern compilers there should be no difference in between the two > optimisation-wise. > Sure - will be a bit fiddly as this is only worth doing if we drop the internal check that buried several macros deep. 1. rename existing fwnode_handle_put() to __fwnode_handle_put() 2. Make __fwnode_handle_put() call a new set of macros #define fwnode_has_op_nocheck(fwnode, op) \ (fwnode)->ops && (fwnode)->ops->op #define fwnode_call_void_op_nocheck(fwnode, op, .... \ do { if (fwnode_had_op_nocheck(fwnode, op)) \ (fwnode)->ops->op(fwnode, ## __VA_ARGS__); } while (false); 3. Add new static inline fwnode_handle_put(struct fwnode_handle *fwnode) { if (!IS_ERR_OR_NULL(fwnode)) __fwnode_handle_put(fwnode); } Or something like that. I'm fine with doing that if conclusion is the complexity of the change is worth it. Jonathan
On Tue, 13 Feb 2024 10:22:45 +0000 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Mon, 12 Feb 2024 12:58:03 +0000 > Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > > On Mon, Feb 12, 2024 at 02:46:49PM +0200, Andy Shevchenko wrote: > > > On Mon, Feb 12, 2024 at 12:36:46PM +0000, Sakari Ailus wrote: > > > > On Mon, Feb 12, 2024 at 11:42:06AM +0000, Jonathan Cameron wrote: > > > > > > ... > > > > > > > Hmm. In that case I'd rather make fwnode_handle_put() and similar trivial > > > > functions macros. > > > > > > This will kill the type-checking opportunity, so I'm against this move. > > > > Then it could be made static inline and moved to the header. I suppose for > > modern compilers there should be no difference in between the two > > optimisation-wise. > > > > Sure - will be a bit fiddly as this is only worth doing if we drop > the internal check that buried several macros deep. Not enough coffee yesterday. We can just move the the existing fwnode_handle_put() to property.h as that includes fwnode.h has all the definitions in it which we need to be able to see. I think that should be uncontroversial? Jonathan > > 1. rename existing fwnode_handle_put() to __fwnode_handle_put() > 2. Make __fwnode_handle_put() call a new set of macros > #define fwnode_has_op_nocheck(fwnode, op) \ > (fwnode)->ops && (fwnode)->ops->op > > #define fwnode_call_void_op_nocheck(fwnode, op, .... \ > do { > if (fwnode_had_op_nocheck(fwnode, op)) \ > (fwnode)->ops->op(fwnode, ## __VA_ARGS__); > } while (false); > > 3. Add new > static inline fwnode_handle_put(struct fwnode_handle *fwnode) > { > if (!IS_ERR_OR_NULL(fwnode)) > __fwnode_handle_put(fwnode); > } > > Or something like that. > > I'm fine with doing that if conclusion is the complexity of the change > is worth it. > > Jonathan >
On Wed, Feb 14, 2024 at 02:09:38PM +0000, Jonathan Cameron wrote: > On Tue, 13 Feb 2024 10:22:45 +0000 > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > > On Mon, 12 Feb 2024 12:58:03 +0000 > > Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > > > > On Mon, Feb 12, 2024 at 02:46:49PM +0200, Andy Shevchenko wrote: > > > > On Mon, Feb 12, 2024 at 12:36:46PM +0000, Sakari Ailus wrote: > > > > > On Mon, Feb 12, 2024 at 11:42:06AM +0000, Jonathan Cameron wrote: > > > > > > > > ... > > > > > > > > > Hmm. In that case I'd rather make fwnode_handle_put() and similar trivial > > > > > functions macros. > > > > > > > > This will kill the type-checking opportunity, so I'm against this move. > > > > > > Then it could be made static inline and moved to the header. I suppose for > > > modern compilers there should be no difference in between the two > > > optimisation-wise. > > > > > > > Sure - will be a bit fiddly as this is only worth doing if we drop > > the internal check that buried several macros deep. > > Not enough coffee yesterday. We can just move the the existing > fwnode_handle_put() to property.h as that includes fwnode.h has > all the definitions in it which we need to be able to see. > > I think that should be uncontroversial? I agree.
diff --git a/include/linux/property.h b/include/linux/property.h index e6516d0b7d52..bcda028f1a33 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> @@ -188,6 +189,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);