mbox series

[00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling.

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

Message

Jonathan Cameron Jan. 14, 2024, 5:19 p.m. UTC
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(-)

Comments

Andy Shevchenko Jan. 21, 2024, 12:27 p.m. UTC | #1
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/