| Message ID | 20240211192540.340682-1-jic23@kernel.org |
|---|---|
| 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.
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 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.
From: Jonathan Cameron <Jonathan.Cameron@huawei.com> Changes since v1: - Introduced device_for_each_child_node_scoped() We may need equivalents for fwnode_for_each_child_node_scoped() etc, but this is the only one I needed so far. This followed from a discussion of the equivalent patch set for device_for_each_of_node() which lead to bringing the declaration of the handle we are applying the __free() to into the for_* loop initialization. The avoided issues with the declaration (which also effects cleanup order) being nowhere near where it was first set to something non NULL. The disadvantage is that the declaration of that local variable is not obvious from the macro parameters. Bugs due to variable shadowing might occur, though in many cases those are apparent as compiler warnings about use of uninitialized variables. - Reordered patches to drag the ltc2983 which is teh one case that wasn't a loop next to the patch that enables that simpler handling. Also move the struct fwnode_handle *ref declarations to where they are intialized. This may look odd, but Linus and others have stated this is how they prefer this to be done. - Converted the one instance of fwnode_for_each_available_child_node() over to device_for_each_child_node_scoped() as it never needed to be the fwnode version in the first place - that was probably a misunderstanding of _available_ or not. - Dropped tags other than Andy's on the first patch (as that was unchanged other than simplifying the patch description). The code changed too much for me to carry them forwards. As can be seen by the examples from IIO that follow this can save a reasonable amount of complexity and boiler plate code, often enabling additional cleanups in related code such as use of return dev_err_probe(). Merge wise (assuming everyone is happy), I'd propose an immutable branch (in IIO or elsewhere) with the 1st and 3rd patches on it, so that we can start making use of this in other areas of the kernel without having to wait too long. Note I don't have the hardware so this is compile tested only. Hence I'd appreciate some Tested-by tags if anyone can poke one of the effected drivers. Julia Lawal has posted some nice coccinelle magic for the DT equivalents. Referenced from that cover letter. Similar may help us convert more drivers to use this new approach, but often hand tweaking can take additional advantage of other cleanup.h based magic, or things like return dev_err_probe(). https://lore.kernel.org/all/20240211174237.182947-1-jic23@kernel.org/ Jonathan Cameron (14): device property: Add cleanup.h based fwnode_handle_put() scope based cleanup. iio: temp: ltc2983: Use __free(fwnode_handle) to replace fwnode_handle_put() calls device property: Introduce device_for_each_child_node_scoped() iio: adc: max11410: Use device_for_each_child_node_scoped() iio: adc: mcp3564: Use device_for_each_child_node_scopd() iio: adc: qcom-spmi-adc5: Use device_for_each_child_node_scopd() iio: adc: rzg2l_adc: Use device_for_each_child_node_scopd() iio: adc: stm32: Use device_for_each_child_node_scoped() iio: adc: ti-ads1015: Use device_for_each_child_node_scoped() iio: adc: ti-ads131e08: Use device_for_each_child_node_scoped() iio: addac: ad74413r: Use device_for_each_child_node_scoped() iio: dac: ad3552r: Use device_for_each_child_node_scoped() iio: dac: ad5770r: Use device_for_each_child_node_scoped() iio: dac: ltc2688: Use device_for_each_child_node_scoped() drivers/iio/adc/max11410.c | 27 +++-------- drivers/iio/adc/mcp3564.c | 16 +++---- drivers/iio/adc/qcom-spmi-adc5.c | 7 +-- drivers/iio/adc/rzg2l_adc.c | 11 ++--- drivers/iio/adc/stm32-adc.c | 63 ++++++++++--------------- drivers/iio/adc/ti-ads1015.c | 5 +- drivers/iio/adc/ti-ads131e08.c | 13 ++---- drivers/iio/addac/ad74413r.c | 10 +--- drivers/iio/dac/ad3552r.c | 51 ++++++++------------- drivers/iio/dac/ad5770r.c | 19 +++----- drivers/iio/dac/ltc2688.c | 24 +++------- drivers/iio/temperature/ltc2983.c | 76 ++++++++++--------------------- include/linux/property.h | 8 ++++ 13 files changed, 113 insertions(+), 217 deletions(-)