[PATCH 00/18] leds: switch to device_for_each_child_node_scoped()

[PATCH 00/18] leds: switch to device_for_each_child_node_scoped()

[Javier Carrasco]

This series switches from the device_for_each_child_node() macro to its
scoped variant, which in general makes the code more robust if new early
exits are added to the loops, because there is no need for explicit
calls to fwnode_handle_put(). Depending on the complexity of the loop
and its error handling, the code gets simplified and it gets easier to
follow.

The non-scoped variant of the macro is error-prone, and it has been the
source of multiple bugs where the child node refcount was not
decremented accordingly in error paths within the loops. The first patch
of this series is a good example, which fixes that kind of bug that is
regularly found in node iterators.

The uses of device_for_each_child_node() with no early exits have been
left untouched because their simpilicty justifies the non-scoped
variant.

Note that the child node is now declared in the macro, and therefore the
explicit declaration is no longer required.

The general functionality should not be affected by this modification.
If functional changes are found, please report them back as errors.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (18):
      leds: flash: mt6360: fix device_for_each_child_node() refcounting in error paths
      leds: flash: mt6370: switch to device_for_each_child_node_scoped()
      leds: flash: leds-qcom-flash: switch to device_for_each_child_node_scoped()
      leds: aw200xx: switch to device_for_each_child_node_scoped()
      leds: cr0014114: switch to device_for_each_child_node_scoped()
      leds: el15203000: switch to device_for_each_child_node_scoped()
      leds: gpio: switch to device_for_each_child_node_scoped()
      leds: lm3532: switch to device_for_each_child_node_scoped()
      leds: lm3697: switch to device_for_each_child_node_scoped()
      leds: lp50xx: switch to device_for_each_child_node_scoped()
      leds: max77650: switch to device_for_each_child_node_scoped()
      leds: ns2: switch to device_for_each_child_node_scoped()
      leds: pca963x: switch to device_for_each_child_node_scoped()
      leds: pwm: switch to device_for_each_child_node_scoped()
      leds: sun50i-a100: switch to device_for_each_child_node_scoped()
      leds: tca6507: switch to device_for_each_child_node_scoped()
      leds: rgb: ktd202x: switch to device_for_each_child_node_scoped()
      leds: rgb: mt6370: switch to device_for_each_child_node_scoped()

 drivers/leds/flash/leds-mt6360.c       |  3 +--
 drivers/leds/flash/leds-mt6370-flash.c | 11 +++-------
 drivers/leds/flash/leds-qcom-flash.c   |  4 +---
 drivers/leds/leds-aw200xx.c            |  7 ++-----
 drivers/leds/leds-cr0014114.c          |  4 +---
 drivers/leds/leds-el15203000.c         | 14 ++++---------
 drivers/leds/leds-gpio.c               |  9 +++------
 drivers/leds/leds-lm3532.c             | 18 +++++++----------
 drivers/leds/leds-lm3697.c             | 18 ++++++-----------
 drivers/leds/leds-lp50xx.c             | 21 +++++++------------
 drivers/leds/leds-max77650.c           | 18 ++++++-----------
 drivers/leds/leds-ns2.c                |  7 ++-----
 drivers/leds/leds-pca963x.c            | 11 +++-------
 drivers/leds/leds-pwm.c                | 15 ++++----------
 drivers/leds/leds-sun50i-a100.c        | 27 +++++++++----------------
 drivers/leds/leds-tca6507.c            |  7 ++-----
 drivers/leds/rgb/leds-ktd202x.c        |  8 +++-----
 drivers/leds/rgb/leds-mt6370-rgb.c     | 37 ++++++++++------------------------
 18 files changed, 75 insertions(+), 164 deletions(-)
---
base-commit: 92fc9636d1471b7f68bfee70c776f7f77e747b97
change-id: 20240926-leds_device_for_each_child_node_scoped-5a95255413fa

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

[PATCH 01/18] leds: flash: mt6360: fix device_for_each_child_node() refcounting in error paths

[Javier Carrasco]

The device_for_each_child_node() macro requires explicit calls to
fwnode_handle_put() upon early exits to avoid memory leaks, and in
this case the error paths are handled after jumping to
'out_flash_realease', which misses that required call to
to decrement the refcount of the child node.

A more elegant and robust solution is using the scoped variant of the
loop, which automatically handles such early exits.

Fix the child node refcounting in the error paths by using
device_for_each_child_node_scoped().

Cc: stable@vger.kernel.org
Fixes: 679f8652064b ("leds: Add mt6360 driver")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/flash/leds-mt6360.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/leds/flash/leds-mt6360.c b/drivers/leds/flash/leds-mt6360.c
index 4c74f1cf01f0..676236c19ec4 100644
--- a/drivers/leds/flash/leds-mt6360.c
+++ b/drivers/leds/flash/leds-mt6360.c
@@ -784,7 +784,6 @@ static void mt6360_v4l2_flash_release(struct mt6360_priv *priv)
 static int mt6360_led_probe(struct platform_device *pdev)
 {
 	struct mt6360_priv *priv;
-	struct fwnode_handle *child;
 	size_t count;
 	int i = 0, ret;
 
@@ -811,7 +810,7 @@ static int mt6360_led_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	device_for_each_child_node(&pdev->dev, child) {
+	device_for_each_child_node_scoped(&pdev->dev, child) {
 		struct mt6360_led *led = priv->leds + i;
 		struct led_init_data init_data = { .fwnode = child, };
 		u32 reg, led_color;

-- 
2.43.0

[PATCH 02/18] leds: flash: mt6370: switch to device_for_each_child_node_scoped()

[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 paths.

This also 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/leds/flash/leds-mt6370-flash.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/flash/leds-mt6370-flash.c b/drivers/leds/flash/leds-mt6370-flash.c
index 912d9d622320..dbdbe92309db 100644
--- a/drivers/leds/flash/leds-mt6370-flash.c
+++ b/drivers/leds/flash/leds-mt6370-flash.c
@@ -509,7 +509,6 @@ static int mt6370_led_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct mt6370_priv *priv;
-	struct fwnode_handle *child;
 	size_t count;
 	int i = 0, ret;
 
@@ -529,22 +528,18 @@ static int mt6370_led_probe(struct platform_device *pdev)
 	if (!priv->regmap)
 		return dev_err_probe(dev, -ENODEV, "Failed to get parent regmap\n");
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		struct mt6370_led *led = priv->leds + i;
 
 		led->priv = priv;
 
 		ret = mt6370_init_flash_properties(dev, led, child);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
 
 		ret = mt6370_led_register(dev, led, child);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
 
 		i++;
 	}

-- 
2.43.0

[PATCH 03/18] leds: flash: leds-qcom-flash: switch to device_for_each_child_node_scoped()

[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 paths.

This also 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/leds/flash/leds-qcom-flash.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/leds/flash/leds-qcom-flash.c b/drivers/leds/flash/leds-qcom-flash.c
index 41ce034f700e..ab848c5acd2d 100644
--- a/drivers/leds/flash/leds-qcom-flash.c
+++ b/drivers/leds/flash/leds-qcom-flash.c
@@ -812,7 +812,6 @@ static int qcom_flash_led_probe(struct platform_device *pdev)
 {
 	struct qcom_flash_data *flash_data;
 	struct qcom_flash_led *led;
-	struct fwnode_handle *child;
 	struct device *dev = &pdev->dev;
 	struct regmap *regmap;
 	struct reg_field *regs;
@@ -896,7 +895,7 @@ static int qcom_flash_led_probe(struct platform_device *pdev)
 	if (!flash_data->v4l2_flash)
 		return -ENOMEM;
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
 		if (!led) {
 			rc = -ENOMEM;
@@ -914,7 +913,6 @@ static int qcom_flash_led_probe(struct platform_device *pdev)
 	return 0;
 
 release:
-	fwnode_handle_put(child);
 	while (flash_data->v4l2_flash[flash_data->leds_count] && flash_data->leds_count)
 		v4l2_flash_release(flash_data->v4l2_flash[flash_data->leds_count--]);
 	return rc;

-- 
2.43.0

[PATCH 04/18] leds: aw200xx: switch to device_for_each_child_node_scoped()

[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 paths.

This also 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/leds/leds-aw200xx.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index f9d9844e0273..08cca128458c 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -409,7 +409,6 @@ static int aw200xx_probe_get_display_rows(struct device *dev,
 
 static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
 {
-	struct fwnode_handle *child;
 	u32 current_min, current_max, min_uA;
 	int ret;
 	int i;
@@ -424,7 +423,7 @@ static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
 	min_uA = UINT_MAX;
 	i = 0;
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		struct led_init_data init_data = {};
 		struct aw200xx_led *led;
 		u32 source, imax;
@@ -468,10 +467,8 @@ static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
 
 		ret = devm_led_classdev_register_ext(dev, &led->cdev,
 						     &init_data);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			break;
-		}
 
 		i++;
 	}

-- 
2.43.0

[PATCH 05/18] leds: cr0014114: switch to device_for_each_child_node_scoped()

[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 paths.

This also 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/leds/leds-cr0014114.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/leds/leds-cr0014114.c b/drivers/leds/leds-cr0014114.c
index c9914fc51f20..7e51c374edd4 100644
--- a/drivers/leds/leds-cr0014114.c
+++ b/drivers/leds/leds-cr0014114.c
@@ -181,11 +181,10 @@ static int cr0014114_probe_dt(struct cr0014114 *priv)
 {
 	size_t			i = 0;
 	struct cr0014114_led	*led;
-	struct fwnode_handle	*child;
 	struct led_init_data	init_data = {};
 	int			ret;
 
-	device_for_each_child_node(priv->dev, child) {
+	device_for_each_child_node_scoped(priv->dev, child) {
 		led = &priv->leds[i];
 
 		led->priv			  = priv;
@@ -201,7 +200,6 @@ static int cr0014114_probe_dt(struct cr0014114 *priv)
 		if (ret) {
 			dev_err(priv->dev,
 				"failed to register LED device, err %d", ret);
-			fwnode_handle_put(child);
 			return ret;
 		}
 

-- 
2.43.0

[PATCH 06/18] leds: el15203000: switch to device_for_each_child_node_scoped()

[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 paths.

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

After switching to the scoped variant, there is no longer need for a
jump to 'err_child_out', as an immediate return is possible.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/leds-el15203000.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-el15203000.c b/drivers/leds/leds-el15203000.c
index d40194a3029f..e26d1654bd0d 100644
--- a/drivers/leds/leds-el15203000.c
+++ b/drivers/leds/leds-el15203000.c
@@ -237,22 +237,20 @@ static int el15203000_pattern_clear(struct led_classdev *ldev)
 static int el15203000_probe_dt(struct el15203000 *priv)
 {
 	struct el15203000_led	*led = priv->leds;
-	struct fwnode_handle	*child;
 	int			ret;
 
-	device_for_each_child_node(priv->dev, child) {
+	device_for_each_child_node_scoped(priv->dev, child) {
 		struct led_init_data init_data = {};
 
 		ret = fwnode_property_read_u32(child, "reg", &led->reg);
 		if (ret) {
 			dev_err(priv->dev, "LED without ID number");
-			goto err_child_out;
+			return ret;
 		}
 
 		if (led->reg > U8_MAX) {
 			dev_err(priv->dev, "LED value %d is invalid", led->reg);
-			ret = -EINVAL;
-			goto err_child_out;
+			return -EINVAL;
 		}
 
 		led->priv			  = priv;
@@ -274,17 +272,13 @@ static int el15203000_probe_dt(struct el15203000 *priv)
 			dev_err(priv->dev,
 				"failed to register LED device %s, err %d",
 				led->ldev.name, ret);
-			goto err_child_out;
+			return ret;
 		}
 
 		led++;
 	}
 
 	return 0;
-
-err_child_out:
-	fwnode_handle_put(child);
-	return ret;
 }
 
 static int el15203000_probe(struct spi_device *spi)

-- 
2.43.0

[PATCH 07/18] leds: gpio: switch to device_for_each_child_node_scoped()

[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 paths.

This also 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/leds/leds-gpio.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 4d1612d557c8..7d4488191241 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -148,7 +148,6 @@ struct gpio_leds_priv {
 
 static struct gpio_leds_priv *gpio_leds_create(struct device *dev)
 {
-	struct fwnode_handle *child;
 	struct gpio_leds_priv *priv;
 	int count, used, ret;
 
@@ -162,7 +161,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct device *dev)
 	priv->num_leds = count;
 	used = 0;
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		struct gpio_led_data *led_dat = &priv->leds[used];
 		struct gpio_led led = {};
 
@@ -176,7 +175,6 @@ static struct gpio_leds_priv *gpio_leds_create(struct device *dev)
 		if (IS_ERR(led.gpiod)) {
 			dev_err_probe(dev, PTR_ERR(led.gpiod), "Failed to get GPIO '%pfw'\n",
 				      child);
-			fwnode_handle_put(child);
 			return ERR_CAST(led.gpiod);
 		}
 
@@ -192,10 +190,9 @@ static struct gpio_leds_priv *gpio_leds_create(struct device *dev)
 			led.panic_indicator = 1;
 
 		ret = create_gpio_led(&led, led_dat, dev, child, NULL);
-		if (ret < 0) {
-			fwnode_handle_put(child);
+		if (ret < 0)
 			return ERR_PTR(ret);
-		}
+
 		/* Set gpiod label to match the corresponding LED name. */
 		gpiod_set_consumer_name(led_dat->gpiod,
 					led_dat->cdev.dev->kobj.name);

-- 
2.43.0

[PATCH 08/18] leds: lm3532: switch to device_for_each_child_node_scoped()

[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 paths.

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

After switching to the scoped variant, there is no longer need for a
jump to 'child_out', as an immediate return is possible.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/leds-lm3532.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 54b5650877f7..24dc8ad27bb3 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -551,7 +551,6 @@ static void gpio_set_low_action(void *data)
 
 static int lm3532_parse_node(struct lm3532_data *priv)
 {
-	struct fwnode_handle *child = NULL;
 	struct lm3532_led *led;
 	int control_bank;
 	u32 ramp_time;
@@ -587,7 +586,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 	else
 		priv->runtime_ramp_down = lm3532_get_ramp_index(ramp_time);
 
-	device_for_each_child_node(priv->dev, child) {
+	device_for_each_child_node_scoped(priv->dev, child) {
 		struct led_init_data idata = {
 			.fwnode = child,
 			.default_label = ":",
@@ -599,7 +598,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 		ret = fwnode_property_read_u32(child, "reg", &control_bank);
 		if (ret) {
 			dev_err(&priv->client->dev, "reg property missing\n");
-			goto child_out;
+			return ret;
 		}
 
 		if (control_bank > LM3532_CONTROL_C) {
@@ -613,7 +612,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 					       &led->mode);
 		if (ret) {
 			dev_err(&priv->client->dev, "ti,led-mode property missing\n");
-			goto child_out;
+			return ret;
 		}
 
 		if (fwnode_property_present(child, "led-max-microamp") &&
@@ -647,7 +646,7 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 						    led->num_leds);
 		if (ret) {
 			dev_err(&priv->client->dev, "led-sources property missing\n");
-			goto child_out;
+			return ret;
 		}
 
 		led->priv = priv;
@@ -657,23 +656,20 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 		if (ret) {
 			dev_err(&priv->client->dev, "led register err: %d\n",
 				ret);
-			goto child_out;
+			return ret;
 		}
 
 		ret = lm3532_init_registers(led);
 		if (ret) {
 			dev_err(&priv->client->dev, "register init err: %d\n",
 				ret);
-			goto child_out;
+			return ret;
 		}
 
 		i++;
 	}
-	return 0;
 
-child_out:
-	fwnode_handle_put(child);
-	return ret;
+	return 0;
 }
 
 static int lm3532_probe(struct i2c_client *client)

-- 
2.43.0

[PATCH 09/18] leds: lm3697: switch to device_for_each_child_node_scoped()

[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 paths.

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

After switching to the scoped variant, there is no longer need for a
jump to 'child_out', as an immediate return is possible.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/leds-lm3697.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
index 99de2a331727..7ad232780a31 100644
--- a/drivers/leds/leds-lm3697.c
+++ b/drivers/leds/leds-lm3697.c
@@ -202,7 +202,6 @@ static int lm3697_init(struct lm3697 *priv)
 
 static int lm3697_probe_dt(struct lm3697 *priv)
 {
-	struct fwnode_handle *child = NULL;
 	struct device *dev = priv->dev;
 	struct lm3697_led *led;
 	int ret = -EINVAL;
@@ -220,19 +219,18 @@ static int lm3697_probe_dt(struct lm3697 *priv)
 	if (IS_ERR(priv->regulator))
 		priv->regulator = NULL;
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		struct led_init_data init_data = {};
 
 		ret = fwnode_property_read_u32(child, "reg", &control_bank);
 		if (ret) {
 			dev_err(dev, "reg property missing\n");
-			goto child_out;
+			return ret;
 		}
 
 		if (control_bank > LM3697_CONTROL_B) {
 			dev_err(dev, "reg property is invalid\n");
-			ret = -EINVAL;
-			goto child_out;
+			return -EINVAL;
 		}
 
 		led = &priv->leds[i];
@@ -262,7 +260,7 @@ static int lm3697_probe_dt(struct lm3697 *priv)
 						    led->num_leds);
 		if (ret) {
 			dev_err(dev, "led-sources property missing\n");
-			goto child_out;
+			return ret;
 		}
 
 		for (j = 0; j < led->num_leds; j++)
@@ -286,17 +284,13 @@ static int lm3697_probe_dt(struct lm3697 *priv)
 						     &init_data);
 		if (ret) {
 			dev_err(dev, "led register err: %d\n", ret);
-			goto child_out;
+			return ret;
 		}
 
 		i++;
 	}
 
-	return ret;
-
-child_out:
-	fwnode_handle_put(child);
-	return ret;
+	return 0;
 }
 
 static int lm3697_probe(struct i2c_client *client)

-- 
2.43.0

[PATCH 10/18] leds: lp50xx: switch to device_for_each_child_node_scoped()

[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 paths.

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

After switching to the scoped variant, there is no longer need for a
jump to 'child_out', as an immediate return is possible.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/leds-lp50xx.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
index 175d4b06659b..32ca45aec76c 100644
--- a/drivers/leds/leds-lp50xx.c
+++ b/drivers/leds/leds-lp50xx.c
@@ -434,7 +434,6 @@ static int lp50xx_probe_leds(struct fwnode_handle *child, struct lp50xx *priv,
 
 static int lp50xx_probe_dt(struct lp50xx *priv)
 {
-	struct fwnode_handle *child = NULL;
 	struct fwnode_handle *led_node = NULL;
 	struct led_init_data init_data = {};
 	struct led_classdev *led_cdev;
@@ -454,17 +453,17 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
 	if (IS_ERR(priv->regulator))
 		priv->regulator = NULL;
 
-	device_for_each_child_node(priv->dev, child) {
+	device_for_each_child_node_scoped(priv->dev, child) {
 		led = &priv->leds[i];
 		ret = fwnode_property_count_u32(child, "reg");
 		if (ret < 0) {
 			dev_err(priv->dev, "reg property is invalid\n");
-			goto child_out;
+			return ret;
 		}
 
 		ret = lp50xx_probe_leds(child, priv, led, ret);
 		if (ret)
-			goto child_out;
+			return ret;
 
 		init_data.fwnode = child;
 		num_colors = 0;
@@ -475,10 +474,8 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
 		 */
 		mc_led_info = devm_kcalloc(priv->dev, LP50XX_LEDS_PER_MODULE,
 					   sizeof(*mc_led_info), GFP_KERNEL);
-		if (!mc_led_info) {
-			ret = -ENOMEM;
-			goto child_out;
-		}
+		if (!mc_led_info)
+			return -ENOMEM;
 
 		fwnode_for_each_child_node(child, led_node) {
 			ret = fwnode_property_read_u32(led_node, "color",
@@ -486,7 +483,7 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
 			if (ret) {
 				fwnode_handle_put(led_node);
 				dev_err(priv->dev, "Cannot read color\n");
-				goto child_out;
+				return ret;
 			}
 
 			mc_led_info[num_colors].color_index = color_id;
@@ -504,16 +501,12 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
 						       &init_data);
 		if (ret) {
 			dev_err(priv->dev, "led register err: %d\n", ret);
-			goto child_out;
+			return ret;
 		}
 		i++;
 	}
 
 	return 0;
-
-child_out:
-	fwnode_handle_put(child);
-	return ret;
 }
 
 static int lp50xx_probe(struct i2c_client *client)

-- 
2.43.0

[PATCH 11/18] leds: max77650: switch to device_for_each_child_node_scoped()

[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 paths.

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

After switching to the scoped variant, there is no longer need for a
jump to 'err_node_out', as an immediate return is possible.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/leds-max77650.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-max77650.c b/drivers/leds/leds-max77650.c
index 1eeac56b0014..f8c47078a3bb 100644
--- a/drivers/leds/leds-max77650.c
+++ b/drivers/leds/leds-max77650.c
@@ -62,7 +62,6 @@ static int max77650_led_brightness_set(struct led_classdev *cdev,
 
 static int max77650_led_probe(struct platform_device *pdev)
 {
-	struct fwnode_handle *child;
 	struct max77650_led *leds, *led;
 	struct device *dev;
 	struct regmap *map;
@@ -84,14 +83,12 @@ static int max77650_led_probe(struct platform_device *pdev)
 	if (!num_leds || num_leds > MAX77650_LED_NUM_LEDS)
 		return -ENODEV;
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		struct led_init_data init_data = {};
 
 		rv = fwnode_property_read_u32(child, "reg", &reg);
-		if (rv || reg >= MAX77650_LED_NUM_LEDS) {
-			rv = -EINVAL;
-			goto err_node_put;
-		}
+		if (rv || reg >= MAX77650_LED_NUM_LEDS)
+			return -EINVAL;
 
 		led = &leds[reg];
 		led->map = map;
@@ -108,23 +105,20 @@ static int max77650_led_probe(struct platform_device *pdev)
 		rv = devm_led_classdev_register_ext(dev, &led->cdev,
 						    &init_data);
 		if (rv)
-			goto err_node_put;
+			return rv;
 
 		rv = regmap_write(map, led->regA, MAX77650_LED_A_DEFAULT);
 		if (rv)
-			goto err_node_put;
+			return rv;
 
 		rv = regmap_write(map, led->regB, MAX77650_LED_B_DEFAULT);
 		if (rv)
-			goto err_node_put;
+			return rv;
 	}
 
 	return regmap_write(map,
 			    MAX77650_REG_CNFG_LED_TOP,
 			    MAX77650_LED_TOP_DEFAULT);
-err_node_put:
-	fwnode_handle_put(child);
-	return rv;
 }
 
 static const struct of_device_id max77650_led_of_match[] = {

-- 
2.43.0

[PATCH 12/18] leds: ns2: switch to device_for_each_child_node_scoped()

[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 also 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/leds/leds-ns2.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
index f3010c472bbd..4c6f04a5bd87 100644
--- a/drivers/leds/leds-ns2.c
+++ b/drivers/leds/leds-ns2.c
@@ -238,7 +238,6 @@ static int ns2_led_register(struct device *dev, struct fwnode_handle *node,
 static int ns2_led_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct fwnode_handle *child;
 	struct ns2_led *leds;
 	int count;
 	int ret;
@@ -251,12 +250,10 @@ static int ns2_led_probe(struct platform_device *pdev)
 	if (!leds)
 		return -ENOMEM;
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		ret = ns2_led_register(dev, child, leds++);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
 	}
 
 	return 0;

-- 
2.43.0

[PATCH 13/18] leds: pca963x: switch to device_for_each_child_node_scoped()

[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 paths.

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

After switching to the scoped variant, there is no longer need for a
jump to 'err', as an immediate return is possible.

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

diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
index b53905da3592..050e93b04884 100644
--- a/drivers/leds/leds-pca963x.c
+++ b/drivers/leds/leds-pca963x.c
@@ -306,7 +306,6 @@ static int pca963x_register_leds(struct i2c_client *client,
 	struct pca963x_chipdef *chipdef = chip->chipdef;
 	struct pca963x_led *led = chip->leds;
 	struct device *dev = &client->dev;
-	struct fwnode_handle *child;
 	bool hw_blink;
 	s32 mode2;
 	u32 reg;
@@ -338,7 +337,7 @@ static int pca963x_register_leds(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		struct led_init_data init_data = {};
 		char default_label[32];
 
@@ -346,8 +345,7 @@ static int pca963x_register_leds(struct i2c_client *client,
 		if (ret || reg >= chipdef->n_leds) {
 			dev_err(dev, "Invalid 'reg' property for node %pfw\n",
 				child);
-			ret = -EINVAL;
-			goto err;
+			return -EINVAL;
 		}
 
 		led->led_num = reg;
@@ -369,16 +367,13 @@ static int pca963x_register_leds(struct i2c_client *client,
 		if (ret) {
 			dev_err(dev, "Failed to register LED for node %pfw\n",
 				child);
-			goto err;
+			return ret;
 		}
 
 		++led;
 	}
 
 	return 0;
-err:
-	fwnode_handle_put(child);
-	return ret;
 }
 
 static int pca963x_suspend(struct device *dev)

-- 
2.43.0

[PATCH 14/18] leds: pwm: switch to device_for_each_child_node_scoped()

[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 paths.

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

After switching to the scoped variant, there is no longer need for a
jump to 'err_child_out', as an immediate return is possible.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/leds-pwm.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index e1b414b40353..7961dca0db2f 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -140,21 +140,18 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 
 static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
 {
-	struct fwnode_handle *fwnode;
 	struct led_pwm led;
 	int ret;
 
-	device_for_each_child_node(dev, fwnode) {
+	device_for_each_child_node_scoped(dev, fwnode) {
 		memset(&led, 0, sizeof(led));
 
 		ret = fwnode_property_read_string(fwnode, "label", &led.name);
 		if (ret && is_of_node(fwnode))
 			led.name = to_of_node(fwnode)->name;
 
-		if (!led.name) {
-			ret = -EINVAL;
-			goto err_child_out;
-		}
+		if (!led.name)
+			return -EINVAL;
 
 		led.active_low = fwnode_property_read_bool(fwnode,
 							   "active-low");
@@ -165,14 +162,10 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
 
 		ret = led_pwm_add(dev, priv, &led, fwnode);
 		if (ret)
-			goto err_child_out;
+			return ret;
 	}
 
 	return 0;
-
-err_child_out:
-	fwnode_handle_put(fwnode);
-	return ret;
 }
 
 static int led_pwm_probe(struct platform_device *pdev)

-- 
2.43.0

[PATCH 15/18] leds: sun50i-a100: switch to device_for_each_child_node_scoped()

[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 paths.

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

The error handling after 'err_put_child' has been moved to the only goto
that jumps to it (second device_for_each_child_node()), and the call to
fwnode_handle_put() has been removed accordingly.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/leds-sun50i-a100.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/leds/leds-sun50i-a100.c b/drivers/leds/leds-sun50i-a100.c
index 4c468d487486..03f1b6424692 100644
--- a/drivers/leds/leds-sun50i-a100.c
+++ b/drivers/leds/leds-sun50i-a100.c
@@ -392,7 +392,6 @@ static int sun50i_a100_ledc_probe(struct platform_device *pdev)
 	struct sun50i_a100_ledc_led *led;
 	struct device *dev = &pdev->dev;
 	struct sun50i_a100_ledc *priv;
-	struct fwnode_handle *child;
 	struct resource *mem;
 	u32 max_addr = 0;
 	u32 num_leds = 0;
@@ -402,21 +401,17 @@ static int sun50i_a100_ledc_probe(struct platform_device *pdev)
 	 * The maximum LED address must be known in sun50i_a100_ledc_resume() before
 	 * class device registration, so parse and validate the subnodes up front.
 	 */
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		u32 addr, color;
 
 		ret = fwnode_property_read_u32(child, "reg", &addr);
-		if (ret || addr >= LEDC_MAX_LEDS) {
-			fwnode_handle_put(child);
+		if (ret || addr >= LEDC_MAX_LEDS)
 			return dev_err_probe(dev, -EINVAL, "'reg' must be between 0 and %d\n",
 					     LEDC_MAX_LEDS - 1);
-		}
 
 		ret = fwnode_property_read_u32(child, "color", &color);
-		if (ret || color != LED_COLOR_ID_RGB) {
-			fwnode_handle_put(child);
+		if (ret || color != LED_COLOR_ID_RGB)
 			return dev_err_probe(dev, -EINVAL, "'color' must be LED_COLOR_ID_RGB\n");
-		}
 
 		max_addr = max(max_addr, addr);
 		num_leds++;
@@ -502,7 +497,7 @@ static int sun50i_a100_ledc_probe(struct platform_device *pdev)
 		return ret;
 
 	led = priv->leds;
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		struct led_classdev *cdev;
 
 		/* The node was already validated above. */
@@ -527,7 +522,11 @@ static int sun50i_a100_ledc_probe(struct platform_device *pdev)
 		ret = led_classdev_multicolor_register_ext(dev, &led->mc_cdev, &init_data);
 		if (ret) {
 			dev_err_probe(dev, ret, "Failed to register multicolor LED %u", led->addr);
-			goto err_put_child;
+			while (led-- > priv->leds)
+				led_classdev_multicolor_unregister(&led->mc_cdev);
+			sun50i_a100_ledc_suspend(&pdev->dev);
+
+			return ret;
 		}
 
 		led++;
@@ -536,14 +535,6 @@ static int sun50i_a100_ledc_probe(struct platform_device *pdev)
 	dev_info(dev, "Registered %u LEDs\n", num_leds);
 
 	return 0;
-
-err_put_child:
-	fwnode_handle_put(child);
-	while (led-- > priv->leds)
-		led_classdev_multicolor_unregister(&led->mc_cdev);
-	sun50i_a100_ledc_suspend(&pdev->dev);
-
-	return ret;
 }
 
 static void sun50i_a100_ledc_remove(struct platform_device *pdev)

-- 
2.43.0

[PATCH 16/18] leds: tca6507: switch to device_for_each_child_node_scoped()

[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 also 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/leds/leds-tca6507.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
index 4f22f4224946..acbd8169723c 100644
--- a/drivers/leds/leds-tca6507.c
+++ b/drivers/leds/leds-tca6507.c
@@ -658,7 +658,6 @@ static struct tca6507_platform_data *
 tca6507_led_dt_init(struct device *dev)
 {
 	struct tca6507_platform_data *pdata;
-	struct fwnode_handle *child;
 	struct led_info *tca_leds;
 	int count;
 
@@ -671,7 +670,7 @@ tca6507_led_dt_init(struct device *dev)
 	if (!tca_leds)
 		return ERR_PTR(-ENOMEM);
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		struct led_info led;
 		u32 reg;
 		int ret;
@@ -688,10 +687,8 @@ tca6507_led_dt_init(struct device *dev)
 			led.flags |= TCA6507_MAKE_GPIO;
 
 		ret = fwnode_property_read_u32(child, "reg", &reg);
-		if (ret || reg >= NUM_LEDS) {
-			fwnode_handle_put(child);
+		if (ret || reg >= NUM_LEDS)
 			return ERR_PTR(ret ? : -EINVAL);
-		}
 
 		tca_leds[reg] = led;
 	}

-- 
2.43.0

[PATCH 17/18] leds: rgb: ktd202x: switch to device_for_each_child_node_scoped()

[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 also 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/leds/rgb/leds-ktd202x.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
index d5c442163c46..04e62faa3a00 100644
--- a/drivers/leds/rgb/leds-ktd202x.c
+++ b/drivers/leds/rgb/leds-ktd202x.c
@@ -495,7 +495,6 @@ static int ktd202x_add_led(struct ktd202x *chip, struct fwnode_handle *fwnode, u
 
 static int ktd202x_probe_fw(struct ktd202x *chip)
 {
-	struct fwnode_handle *child;
 	struct device *dev = chip->dev;
 	int count;
 	int i = 0;
@@ -509,13 +508,12 @@ static int ktd202x_probe_fw(struct ktd202x *chip)
 	/* Allow the device to execute the complete reset */
 	usleep_range(200, 300);
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		int ret = ktd202x_add_led(chip, child, i);
 
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
+
 		i++;
 	}
 

-- 
2.43.0

[PATCH 18/18] leds: rgb: mt6370: switch to device_for_each_child_node_scoped()

[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 paths.

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

After switching to the scoped variant, there is no longer need for a
jump to 'fwnode_release', as an immediate return is possible. Given that
the loop is called in the probe function, and it already uses
dev_err_probe(), the common "dev_err() + return" has been updated as
well.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/rgb/leds-mt6370-rgb.c | 37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/drivers/leds/rgb/leds-mt6370-rgb.c b/drivers/leds/rgb/leds-mt6370-rgb.c
index 359ef00498b4..fe76e8e27f9c 100644
--- a/drivers/leds/rgb/leds-mt6370-rgb.c
+++ b/drivers/leds/rgb/leds-mt6370-rgb.c
@@ -905,7 +905,6 @@ static int mt6370_leds_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct mt6370_priv *priv;
-	struct fwnode_handle *child;
 	size_t count;
 	unsigned int i = 0;
 	int ret;
@@ -936,37 +935,27 @@ static int mt6370_leds_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to allocate regmap field\n");
 
-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		struct mt6370_led *led = priv->leds + i++;
 		struct led_init_data init_data = { .fwnode = child };
 		u32 reg, color;
 
 		ret = fwnode_property_read_u32(child, "reg", &reg);
-		if (ret) {
-			dev_err(dev, "Failed to parse reg property\n");
-			goto fwnode_release;
-		}
+		if (ret)
+			dev_err_probe(dev, ret, "Failed to parse reg property\n");
 
-		if (reg >= MT6370_MAX_LEDS) {
-			ret = -EINVAL;
-			dev_err(dev, "Error reg property number\n");
-			goto fwnode_release;
-		}
+		if (reg >= MT6370_MAX_LEDS)
+			return dev_err_probe(dev, -EINVAL, "Error reg property number\n");
 
 		ret = fwnode_property_read_u32(child, "color", &color);
-		if (ret) {
-			dev_err(dev, "Failed to parse color property\n");
-			goto fwnode_release;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to parse color property\n");
 
 		if (color == LED_COLOR_ID_RGB || color == LED_COLOR_ID_MULTI)
 			reg = MT6370_VIRTUAL_MULTICOLOR;
 
-		if (priv->leds_active & BIT(reg)) {
-			ret = -EINVAL;
-			dev_err(dev, "Duplicate reg property\n");
-			goto fwnode_release;
-		}
+		if (priv->leds_active & BIT(reg))
+			return dev_err_probe(dev, -EINVAL, "Duplicate reg property\n");
 
 		priv->leds_active |= BIT(reg);
 
@@ -975,18 +964,14 @@ static int mt6370_leds_probe(struct platform_device *pdev)
 
 		ret = mt6370_init_led_properties(dev, led, &init_data);
 		if (ret)
-			goto fwnode_release;
+			return ret;
 
 		ret = mt6370_led_register(dev, led, &init_data);
 		if (ret)
-			goto fwnode_release;
+			return ret;
 	}
 
 	return 0;
-
-fwnode_release:
-	fwnode_handle_put(child);
-	return ret;
 }
 
 static const struct of_device_id mt6370_rgbled_device_table[] = {

-- 
2.43.0

Re: [PATCH 11/18] leds: max77650: switch to device_for_each_child_node_scoped()

[Bartosz Golaszewski]

On Fri, Sep 27, 2024 at 1:21 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 paths.
>
> This also prevents possible memory leaks if new error paths are added
> without the required call to fwnode_handle_put().
>
> After switching to the scoped variant, there is no longer need for a
> jump to 'err_node_out', as an immediate return is possible.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH 00/18] leds: switch to device_for_each_child_node_scoped()

[Lee Jones]

On Fri, 27 Sep 2024 01:20:51 +0200, Javier Carrasco wrote:
> This series switches from the device_for_each_child_node() macro to its
> scoped variant, which in general makes the code more robust if new early
> exits are added to the loops, because there is no need for explicit
> calls to fwnode_handle_put(). Depending on the complexity of the loop
> and its error handling, the code gets simplified and it gets easier to
> follow.
> 
> [...]

Applied, thanks!

[01/18] leds: flash: mt6360: fix device_for_each_child_node() refcounting in error paths
        commit: 73b03b27736e440e3009fe1319cbc82d2cd1290c
[02/18] leds: flash: mt6370: switch to device_for_each_child_node_scoped()
        commit: 19d1cc765e7d477070ddda02c9a07a1ebcdf4b2d
[03/18] leds: flash: leds-qcom-flash: switch to device_for_each_child_node_scoped()
        commit: f64dd42a4f939fb5acc3f3568ef2118487617996
[04/18] leds: aw200xx: switch to device_for_each_child_node_scoped()
        commit: a361af3c1622d4b8ede54493fa88633fb12201d0
[05/18] leds: cr0014114: switch to device_for_each_child_node_scoped()
        commit: 65135e2ccf5ad0853c1df0ffeefc372066a62909
[06/18] leds: el15203000: switch to device_for_each_child_node_scoped()
        commit: 9e445e28ae0c6fe24369127cf2302cd4f3a1b42b
[07/18] leds: gpio: switch to device_for_each_child_node_scoped()
        commit: 42b49671602f9badb14fd2c32e6791a24d8cbf02
[08/18] leds: lm3532: switch to device_for_each_child_node_scoped()
        commit: 7bd4b9277b9831d115f14d26000c0ba32c83d109
[09/18] leds: lm3697: switch to device_for_each_child_node_scoped()
        commit: 6e2d1d83b70bd736228529fd1cb4f98e0ab77eb8
[10/18] leds: lp50xx: switch to device_for_each_child_node_scoped()
        commit: ba35b9a4c1b074218880c47ca09d19a3c69f904d
[11/18] leds: max77650: switch to device_for_each_child_node_scoped()
        commit: 4ab3ae432da1706b5e1624ecea3c670faaec39d7
[12/18] leds: ns2: switch to device_for_each_child_node_scoped()
        commit: 5b5d936db0d2fb9e81d240ed91d062b8c8f0d224
[13/18] leds: pca963x: switch to device_for_each_child_node_scoped()
        commit: dea90acb09324efe640ab69766c12d8d387ee97f
[14/18] leds: pwm: switch to device_for_each_child_node_scoped()
        commit: e3456071853597229012622c97b76109c0fa8754
[15/18] leds: sun50i-a100: switch to device_for_each_child_node_scoped()
        commit: 8cf103de9a002fb02125491c06d9cd60762d70e5
[16/18] leds: tca6507: switch to device_for_each_child_node_scoped()
        commit: 01728d041986a6992d0b2499e88db4569e65a535
[17/18] leds: rgb: ktd202x: switch to device_for_each_child_node_scoped()
        commit: 48259638fe5986afe8ed2a49e35f0641d953c311
[18/18] leds: rgb: mt6370: switch to device_for_each_child_node_scoped()
        commit: bf3fba727695dcd1ac3f9d17d88845223f56c14f

--
Lee Jones [李琼斯]

Re: [PATCH 00/18] leds: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

Fri, Sep 27, 2024 at 01:20:51AM +0200, Javier Carrasco kirjoitti:
> This series switches from the device_for_each_child_node() macro to its
> scoped variant, which in general makes the code more robust if new early
> exits are added to the loops, because there is no need for explicit
> calls to fwnode_handle_put(). Depending on the complexity of the loop
> and its error handling, the code gets simplified and it gets easier to
> follow.
> 
> The non-scoped variant of the macro is error-prone, and it has been the
> source of multiple bugs where the child node refcount was not
> decremented accordingly in error paths within the loops. The first patch
> of this series is a good example, which fixes that kind of bug that is
> regularly found in node iterators.
> 
> The uses of device_for_each_child_node() with no early exits have been
> left untouched because their simpilicty justifies the non-scoped
> variant.
> 
> Note that the child node is now declared in the macro, and therefore the
> explicit declaration is no longer required.
> 
> The general functionality should not be affected by this modification.
> If functional changes are found, please report them back as errors.


Thank you for this series. It may now benefit from the next steps:

1) converting to return dev_err_probe() or dev_warn_probe() directly since
   the goto had been replaced by direct return, hence saving even more LoCs;

2) dropping or refactoring the complex conditions due to your nice series
   being applied.

I'll comment individually on some to show what I meant by the above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 05/18] leds: cr0014114: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

Fri, Sep 27, 2024 at 01:20:56AM +0200, Javier Carrasco kirjoitti:
> 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 paths.
> 
> This also prevents possible memory leaks if new error paths are added
> without the required call to fwnode_handle_put().

...

>  		if (ret) {
>  			dev_err(priv->dev,
>  				"failed to register LED device, err %d", ret);
> -			fwnode_handle_put(child);
>  			return ret;

Now it can be
			return dev_err_probe(..., ret, "failed to register LED device");

>  		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/18] leds: el15203000: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

Fri, Sep 27, 2024 at 01:20:57AM +0200, Javier Carrasco kirjoitti:
> 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 paths.
> 
> This also prevents possible memory leaks if new error paths are added
> without the required call to fwnode_handle_put().
> 
> After switching to the scoped variant, there is no longer need for a
> jump to 'err_child_out', as an immediate return is possible.

...

>  		if (ret) {
>  			dev_err(priv->dev, "LED without ID number");
> -			goto err_child_out;
> +			return ret;

Now

			return dev_err_probe(...);

>  		}
>  
>  		if (led->reg > U8_MAX) {
>  			dev_err(priv->dev, "LED value %d is invalid", led->reg);
> -			ret = -EINVAL;
> -			goto err_child_out;
> +			return -EINVAL;

Ditto.

>  		}

>  			dev_err(priv->dev,
>  				"failed to register LED device %s, err %d",
>  				led->ldev.name, ret);
> -			goto err_child_out;
> +			return ret;

Ditto.

			return dev_err_probe(priv->dev, ret, "failed to register LED device %s\n",
				led->ldev.name);

...

Also notice missed '\n' at the end of the strings (and yes, I know
that it's not a problem for dev_*() macros, but still...).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 07/18] leds: gpio: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

Fri, Sep 27, 2024 at 01:20:58AM +0200, Javier Carrasco kirjoitti:
> 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 paths.
> 
> This also prevents possible memory leaks if new error paths are added
> without the required call to fwnode_handle_put().

...

>  		if (IS_ERR(led.gpiod)) {
>  			dev_err_probe(dev, PTR_ERR(led.gpiod), "Failed to get GPIO '%pfw'\n",
>  				      child);
> -			fwnode_handle_put(child);
>  			return ERR_CAST(led.gpiod);

Here you may use dev_err_ptr_probe()

>  		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 08/18] leds: lm3532: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

Fri, Sep 27, 2024 at 01:20:59AM +0200, Javier Carrasco kirjoitti:
> 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 paths.
> 
> This also prevents possible memory leaks if new error paths are added
> without the required call to fwnode_handle_put().
> 
> After switching to the scoped variant, there is no longer need for a
> jump to 'child_out', as an immediate return is possible.

...

>  		if (ret) {
>  			dev_err(&priv->client->dev, "reg property missing\n");
> -			goto child_out;
> +			return ret;

return dev_err_probe(...);

>  		}

...

>  		if (ret) {
>  			dev_err(&priv->client->dev, "ti,led-mode property missing\n");
> -			goto child_out;
> +			return ret;
>  		}

...

>  		if (ret) {
>  			dev_err(&priv->client->dev, "led-sources property missing\n");
> -			goto child_out;
> +			return ret;
>  		}

...

>  		if (ret) {
>  			dev_err(&priv->client->dev, "led register err: %d\n",
>  				ret);
> -			goto child_out;
> +			return ret;
>  		}
>  
>  		ret = lm3532_init_registers(led);
>  		if (ret) {
>  			dev_err(&priv->client->dev, "register init err: %d\n",
>  				ret);
> -			goto child_out;
> +			return ret;
>  		}

Ditto for all above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 09/18] leds: lm3697: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

Fri, Sep 27, 2024 at 01:21:00AM +0200, Javier Carrasco kirjoitti:
> 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 paths.
> 
> This also prevents possible memory leaks if new error paths are added
> without the required call to fwnode_handle_put().
> 
> After switching to the scoped variant, there is no longer need for a
> jump to 'child_out', as an immediate return is possible.

Same here and so on...
So, please revisit these drivers for the possible improvements.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 16/18] leds: tca6507: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

Fri, Sep 27, 2024 at 01:21:07AM +0200, Javier Carrasco kirjoitti:
> 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 also prevents possible memory leaks if new error paths are added
> without the required call to fwnode_handle_put().

...

>  		ret = fwnode_property_read_u32(child, "reg", &reg);
> -		if (ret || reg >= NUM_LEDS) {
> -			fwnode_handle_put(child);
> +		if (ret || reg >= NUM_LEDS)
>  			return ERR_PTR(ret ? : -EINVAL);
> -		}

It's now two nested conditionals instead of two independent ones:

		if (ret)
			return ERR_PTR(ret);
		if (reg >= NUM_LEDS)
			return ERR_PTR(-EINVAL);

I believe my wariant is much better to read, understand, and maintain.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 15/18] leds: sun50i-a100: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

Fri, Sep 27, 2024 at 01:21:06AM +0200, Javier Carrasco kirjoitti:
> 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 paths.
> 
> This also prevents possible memory leaks if new error paths are added
> without the required call to fwnode_handle_put().
> 
> The error handling after 'err_put_child' has been moved to the only goto
> that jumps to it (second device_for_each_child_node()), and the call to
> fwnode_handle_put() has been removed accordingly.

...

>  		ret = fwnode_property_read_u32(child, "reg", &addr);
> -		if (ret || addr >= LEDC_MAX_LEDS) {
> -			fwnode_handle_put(child);
> +		if (ret || addr >= LEDC_MAX_LEDS)
>  			return dev_err_probe(dev, -EINVAL, "'reg' must be between 0 and %d\n",
>  					     LEDC_MAX_LEDS - 1);
> -		}

This is a misleading message, what should be done is to split them:

		if (ret)
			reutrn ret;
		if (addr >= LEDC_MAX_LEDS)
			return dev_err_probe(dev, -EINVAL, "'reg' must be between 0 and %d\n",
					     LEDC_MAX_LEDS - 1);


>  		ret = fwnode_property_read_u32(child, "color", &color);
> -		if (ret || color != LED_COLOR_ID_RGB) {
> -			fwnode_handle_put(child);
> +		if (ret || color != LED_COLOR_ID_RGB)
>  			return dev_err_probe(dev, -EINVAL, "'color' must be LED_COLOR_ID_RGB\n");
> -		}

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 13/18] leds: pca963x: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

Fri, Sep 27, 2024 at 01:21:04AM +0200, Javier Carrasco kirjoitti:
> 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 paths.
> 
> This also prevents possible memory leaks if new error paths are added
> without the required call to fwnode_handle_put().
> 
> After switching to the scoped variant, there is no longer need for a
> jump to 'err', as an immediate return is possible.

...

>  		if (ret || reg >= chipdef->n_leds) {
>  			dev_err(dev, "Invalid 'reg' property for node %pfw\n",
>  				child);
> -			ret = -EINVAL;
> -			goto err;
> +			return -EINVAL;
>  		}

I'm not sure how interpret this message, but the problem here is the shadowing
of the original error code. Hence I would split this:

		if (ret)
			return ret; // possibly return dev_err_probe() with another message
		if (reg >= chipdef->n_leds)
			return dev_err_probe(dev, -EINVAL, "Invalid 'reg' property for node %pfw\n",
					     child);

...

>  		if (ret) {
>  			dev_err(dev, "Failed to register LED for node %pfw\n",
>  				child);
> -			goto err;
> +			return ret;

			return dev_err_probe(...);

>  		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 11/18] leds: max77650: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

Fri, Sep 27, 2024 at 01:21:02AM +0200, Javier Carrasco kirjoitti:
> 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 paths.
> 
> This also prevents possible memory leaks if new error paths are added
> without the required call to fwnode_handle_put().
> 
> After switching to the scoped variant, there is no longer need for a
> jump to 'err_node_out', as an immediate return is possible.

...

>  		rv = fwnode_property_read_u32(child, "reg", &reg);
> -		if (rv || reg >= MAX77650_LED_NUM_LEDS) {
> -			rv = -EINVAL;
> -			goto err_node_put;
> -		}
> +		if (rv || reg >= MAX77650_LED_NUM_LEDS)
> +			return -EINVAL;

Again (yes, I know that is original issue, but with your series applied
it may be nicely resolved), shadowing error code. Should be

		if (rv)
			return rv;
		if (reg >= MAX77650_LED_NUM_LEDS)
			return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko