[PATCH 0/4] iio: adc: use scoped device_for_each_childe_node()

[PATCH 0/4] iio: adc: use scoped device_for_each_childe_node()

[Javier Carrasco]

The device_for_each_child_node() macro requires calls to
fwnode_handle_put() upon early exits (break/return), and that has been a
constant source of bugs in the kernel.

This series switches to the more secure, scoped version of the macro
in the IIO subsystem, wherever the loop contains error paths. This
change simplifies the code and removes the explicit calls to
fwnode_handle_put(). In all cases the child node is only used for
parsing, and not assigned to be used later.

The straightforward uses of the loop with no error paths have been left
untouched, as their simplicity justifies the non-scoped variant.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (4):
      iio: adc: qcom-pm8xxx-xoadc: use scoped device_for_each_child_node()
      iio: adc: qcom-spmi-vadc: use scoped device_for_each_child_node()
      iio: adc: sun20i-gpadc: use scoped device_for_each_child_node()
      iio: adc: ad5755: use scoped device_for_each_child_node()

 drivers/iio/adc/qcom-pm8xxx-xoadc.c |  8 +++-----
 drivers/iio/adc/qcom-spmi-vadc.c    |  7 ++-----
 drivers/iio/adc/sun20i-gpadc-iio.c  |  7 ++-----
 drivers/iio/dac/ad5755.c            | 11 +++--------
 4 files changed, 10 insertions(+), 23 deletions(-)
---
base-commit: 92fc9636d1471b7f68bfee70c776f7f77e747b97
change-id: 20240926-iio_device_for_each_child_node_scoped-cb534e6f5d9b

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 1/4] iio: adc: qcom-pm8xxx-xoadc: use scoped device_for_each_child_node()

[Javier Carrasco]

Switch to device_for_each_child_node_scoped() to simplify the code by
removing the need for calls to fwnode_handle_put() in the error path.

This prevents possible memory leaks if new error paths are added without
the required call to fwnode_handle_put().

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/adc/qcom-pm8xxx-xoadc.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/qcom-pm8xxx-xoadc.c b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
index 9e1112f5acc6..311e9a804ded 100644
--- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c
+++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
@@ -821,7 +821,6 @@ static int pm8xxx_xoadc_parse_channel(struct device *dev,
 
 static int pm8xxx_xoadc_parse_channels(struct pm8xxx_xoadc *adc)
 {
-	struct fwnode_handle *child;
 	struct pm8xxx_chan_info *ch;
 	int ret;
 	int i;
@@ -844,16 +843,15 @@ static int pm8xxx_xoadc_parse_channels(struct pm8xxx_xoadc *adc)
 		return -ENOMEM;
 
 	i = 0;
-	device_for_each_child_node(adc->dev, child) {
+	device_for_each_child_node_scoped(adc->dev, child) {
 		ch = &adc->chans[i];
 		ret = pm8xxx_xoadc_parse_channel(adc->dev, child,
 						 adc->variant->channels,
 						 &adc->iio_chans[i],
 						 ch);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
+
 		i++;
 	}
 

-- 
2.43.0

[PATCH 2/4] iio: adc: qcom-spmi-vadc: use scoped device_for_each_child_node()

[Javier Carrasco]

Switch to device_for_each_child_node_scoped() to simplify the code by
removing the need for calls to fwnode_handle_put() in the error path.

This prevents possible memory leaks if new error paths are added without
the required call to fwnode_handle_put().

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/adc/qcom-spmi-vadc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c
index f5c6f1f27b2c..00a7f0982025 100644
--- a/drivers/iio/adc/qcom-spmi-vadc.c
+++ b/drivers/iio/adc/qcom-spmi-vadc.c
@@ -754,7 +754,6 @@ static int vadc_get_fw_data(struct vadc_priv *vadc)
 	const struct vadc_channels *vadc_chan;
 	struct iio_chan_spec *iio_chan;
 	struct vadc_channel_prop prop;
-	struct fwnode_handle *child;
 	unsigned int index = 0;
 	int ret;
 
@@ -774,12 +773,10 @@ static int vadc_get_fw_data(struct vadc_priv *vadc)
 
 	iio_chan = vadc->iio_chans;
 
-	device_for_each_child_node(vadc->dev, child) {
+	device_for_each_child_node_scoped(vadc->dev, child) {
 		ret = vadc_get_fw_channel_data(vadc->dev, &prop, child);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
 
 		prop.scale_fn_type = vadc_chans[prop.channel].scale_fn_type;
 		vadc->chan_props[index] = prop;

-- 
2.43.0

[PATCH 3/4] iio: adc: sun20i-gpadc: use scoped device_for_each_child_node()

[Javier Carrasco]

Switch to device_for_each_child_node_scoped() to simplify the code by
removing the need for calls to fwnode_handle_put() in the error path.

This prevents possible memory leaks if new error paths are added without
the required call to fwnode_handle_put().

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/adc/sun20i-gpadc-iio.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c
index 6a893d484cf7..136b8d9c294f 100644
--- a/drivers/iio/adc/sun20i-gpadc-iio.c
+++ b/drivers/iio/adc/sun20i-gpadc-iio.c
@@ -155,7 +155,6 @@ static int sun20i_gpadc_alloc_channels(struct iio_dev *indio_dev,
 	unsigned int channel;
 	int num_channels, i, ret;
 	struct iio_chan_spec *channels;
-	struct fwnode_handle *node;
 
 	num_channels = device_get_child_node_count(dev);
 	if (num_channels == 0)
@@ -167,12 +166,10 @@ static int sun20i_gpadc_alloc_channels(struct iio_dev *indio_dev,
 		return -ENOMEM;
 
 	i = 0;
-	device_for_each_child_node(dev, node) {
+	device_for_each_child_node_scoped(dev, node) {
 		ret = fwnode_property_read_u32(node, "reg", &channel);
-		if (ret) {
-			fwnode_handle_put(node);
+		if (ret)
 			return dev_err_probe(dev, ret, "invalid channel number\n");
-		}
 
 		channels[i].type = IIO_VOLTAGE;
 		channels[i].indexed = 1;

-- 
2.43.0

Re: [PATCH 3/4] iio: adc: sun20i-gpadc: use scoped device_for_each_child_node()

[Chen-Yu Tsai]

On Fri, Sep 27, 2024 at 12:08 AM Javier Carrasco
<javier.carrasco.cruz@gmail.com> wrote:
>
> Switch to device_for_each_child_node_scoped() to simplify the code by
> removing the need for calls to fwnode_handle_put() in the error path.
>
> This prevents possible memory leaks if new error paths are added without
> the required call to fwnode_handle_put().
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

> ---
>  drivers/iio/adc/sun20i-gpadc-iio.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c
> index 6a893d484cf7..136b8d9c294f 100644
> --- a/drivers/iio/adc/sun20i-gpadc-iio.c
> +++ b/drivers/iio/adc/sun20i-gpadc-iio.c
> @@ -155,7 +155,6 @@ static int sun20i_gpadc_alloc_channels(struct iio_dev *indio_dev,
>         unsigned int channel;
>         int num_channels, i, ret;
>         struct iio_chan_spec *channels;
> -       struct fwnode_handle *node;
>
>         num_channels = device_get_child_node_count(dev);
>         if (num_channels == 0)
> @@ -167,12 +166,10 @@ static int sun20i_gpadc_alloc_channels(struct iio_dev *indio_dev,
>                 return -ENOMEM;
>
>         i = 0;
> -       device_for_each_child_node(dev, node) {
> +       device_for_each_child_node_scoped(dev, node) {
>                 ret = fwnode_property_read_u32(node, "reg", &channel);
> -               if (ret) {
> -                       fwnode_handle_put(node);
> +               if (ret)
>                         return dev_err_probe(dev, ret, "invalid channel number\n");
> -               }
>
>                 channels[i].type = IIO_VOLTAGE;
>                 channels[i].indexed = 1;
>
> --
> 2.43.0
>

[PATCH 4/4] iio: adc: ad5755: use scoped device_for_each_child_node()

[Javier Carrasco]

Switch to device_for_each_child_node_scoped() to simplify the code by
removing the need for calls to fwnode_handle_put() in the error path, in
this particular case dropping the jump to error_out as well.

This prevents possible memory leaks if new error paths are added without
the required call to fwnode_handle_put().

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/dac/ad5755.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/dac/ad5755.c b/drivers/iio/dac/ad5755.c
index 0b24cb19ac9d..05e80b6ae2cc 100644
--- a/drivers/iio/dac/ad5755.c
+++ b/drivers/iio/dac/ad5755.c
@@ -699,7 +699,6 @@ static const struct ad5755_platform_data ad5755_default_pdata = {
 
 static struct ad5755_platform_data *ad5755_parse_fw(struct device *dev)
 {
-	struct fwnode_handle *pp;
 	struct ad5755_platform_data *pdata;
 	unsigned int tmp;
 	unsigned int tmparray[3];
@@ -746,11 +745,12 @@ static struct ad5755_platform_data *ad5755_parse_fw(struct device *dev)
 	}
 
 	devnr = 0;
-	device_for_each_child_node(dev, pp) {
+	device_for_each_child_node_scoped(dev, pp) {
 		if (devnr >= AD5755_NUM_CHANNELS) {
 			dev_err(dev,
 				"There are too many channels defined in DT\n");
-			goto error_out;
+			devm_kfree(dev, pdata);
+			return NULL;
 		}
 
 		pdata->dac[devnr].mode = AD5755_MODE_CURRENT_4mA_20mA;
@@ -800,11 +800,6 @@ static struct ad5755_platform_data *ad5755_parse_fw(struct device *dev)
 	}
 
 	return pdata;
-
- error_out:
-	fwnode_handle_put(pp);
-	devm_kfree(dev, pdata);
-	return NULL;
 }
 
 static int ad5755_probe(struct spi_device *spi)

-- 
2.43.0

Re: [PATCH 0/4] iio: adc: use scoped device_for_each_childe_node()

[Jonathan Cameron]

On Thu, 26 Sep 2024 18:08:36 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The device_for_each_child_node() macro requires calls to
> fwnode_handle_put() upon early exits (break/return), and that has been a
> constant source of bugs in the kernel.
> 
> This series switches to the more secure, scoped version of the macro
> in the IIO subsystem, wherever the loop contains error paths. This
> change simplifies the code and removes the explicit calls to
> fwnode_handle_put(). In all cases the child node is only used for
> parsing, and not assigned to be used later.
> 
> The straightforward uses of the loop with no error paths have been left
> untouched, as their simplicity justifies the non-scoped variant.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
All look good to me.

I'll be rebasing on rc1 anyway, so there is plenty of time for any
other reviews to come in.  In meantime I've queued these up on the testing
branch of iio.git.

Thanks,

Jonathan
> ---
> Javier Carrasco (4):
>       iio: adc: qcom-pm8xxx-xoadc: use scoped device_for_each_child_node()
>       iio: adc: qcom-spmi-vadc: use scoped device_for_each_child_node()
>       iio: adc: sun20i-gpadc: use scoped device_for_each_child_node()
>       iio: adc: ad5755: use scoped device_for_each_child_node()
> 
>  drivers/iio/adc/qcom-pm8xxx-xoadc.c |  8 +++-----
>  drivers/iio/adc/qcom-spmi-vadc.c    |  7 ++-----
>  drivers/iio/adc/sun20i-gpadc-iio.c  |  7 ++-----
>  drivers/iio/dac/ad5755.c            | 11 +++--------
>  4 files changed, 10 insertions(+), 23 deletions(-)
> ---
> base-commit: 92fc9636d1471b7f68bfee70c776f7f77e747b97
> change-id: 20240926-iio_device_for_each_child_node_scoped-cb534e6f5d9b
> 
> Best regards,