diff mbox series

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

Message ID 20240927-leds_device_for_each_child_node_scoped-v1-15-95c0614b38c8@gmail.com
State New
Headers show
Series leds: switch to device_for_each_child_node_scoped() | expand

Commit Message

Javier Carrasco Sept. 26, 2024, 11:21 p.m. UTC 
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(-)

Comments

Andy Shevchenko Nov. 23, 2024, 7:41 p.m. UTC | #1 
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.
diff mbox series

Patch

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)