| Message ID | 20240114172009.179893-1-jic23@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling. | expand |
On Sun, Jan 14, 2024 at 05:19:56PM +0000, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Chances since RFC. > - Add if (!IS_ERR_OR_NULL(_T)) check as suggested by Andy Shevchenko. > This may allow the compiler to optimize cases where it can tell that > this check will fail rather than calling into fwnode_handle_put(). FWIW, this nuance is mentioned in [1] as: "Within the macro, this declaration is creating a new function called __free_kfree() that makes a call to kfree() if the passed-in pointer is not NULL. Nobody will ever call that function directly, but the declaration makes it possible to write code like: ..." Not so explicit, but still... [1]: https://lwn.net/Articles/934679/
From: Jonathan Cameron <Jonathan.Cameron@huawei.com> Chances since RFC. - Add if (!IS_ERR_OR_NULL(_T)) check as suggested by Andy Shevchenko. This may allow the compiler to optimize cases where it can tell that this check will fail rather than calling into fwnode_handle_put(). 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. I have tested the device tree only version: https://lore.kernel.org/linux-iio/20231217184648.185236-1-jic23@kernel.org/ which is very similar. Failing to release the references on early exit from loops over child nodes and similar are a fairly common source of bugs. The need to explicitly release the references via fwnode_handle_put() also complicate the code. The first patch enables struct fwnode_handle *child __free(fwnode_handle) = NULL; device_for_each_child_node(dev, child) { if (err) /* * Previously needed a fwnode_handle_put() here, * will now be called automatically as well leave * the scope within which the cleanup is registered */ return err; } /* * The if (!IS_ERR_OR_NULL(_T)) check will fail here as the pointer will be * NULL and thus fwnode_handle_put() will not be called. It would be * functionally correct to call it with a NULL pointer but that reduces * the opportunities for the compiler to optimize out the call. */ } 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 first patch on it, so that we can start making use of this in other areas of the kernel without having to wait too long. Jonathan Cameron (13): device property: Add cleanup.h based fwnode_handle_put() scope based cleanup. iio: adc: max11410: Use __free(fwnode_handle) to replace fwnode_handle_put() calls iio: adc: mcp3564: Use __free(fwnode_handle) to replace fwnode_handle_put() calls iio: adc: qcom-spmi-adc5: Use __free(fwnode_handle) to replace fwnode_handle_put() calls iio: adc: rzg2l_adc: Use __free(fwnode_handle) to replace fwnode_handle_put() calls iio: adc: stm32: Use __free(fwnode_handle) to replace fwnode_handle_put() calls iio: adc: ti-ads1015: Use __free(fwnode_handle) to replace fwnode_handle_put() calls iio: adc: ti-ads131e08: Use __free(fwnode_handle) to replace fwnode_handle_put() calls iio: addac: ad74413r: Use __free(fwnode_handle) to replace fwnode_handle_put() calls iio: dac: ad3552: Use __free(fwnode_handle) to replace fwnode_handle_put() calls iio: dac: ad5770r: Use __free(fwnode_handle) to replace fwnode_handle_put() calls iio: dac: ltc2688: Use __free(fwnode_handle) to replace fwnode_handle_put() calls iio: temp: ltc2983: Use __free(fwnode_handle) to replace fwnode_handle_put() calls drivers/iio/adc/max11410.c | 26 ++++-------- drivers/iio/adc/mcp3564.c | 15 ++++--- drivers/iio/adc/qcom-spmi-adc5.c | 6 +-- drivers/iio/adc/rzg2l_adc.c | 10 ++--- drivers/iio/adc/stm32-adc.c | 62 +++++++++++---------------- drivers/iio/adc/ti-ads1015.c | 4 +- drivers/iio/adc/ti-ads131e08.c | 12 ++---- drivers/iio/addac/ad74413r.c | 9 +--- drivers/iio/dac/ad3552r.c | 50 +++++++++------------- drivers/iio/dac/ad5770r.c | 18 +++----- drivers/iio/dac/ltc2688.c | 23 +++------- drivers/iio/temperature/ltc2983.c | 70 ++++++++++--------------------- include/linux/property.h | 3 ++ 13 files changed, 105 insertions(+), 203 deletions(-)