[PATCH 0/3] gpio: switch to device_for_each_chilld_node_scoped()

[PATCH 0/3] gpio: switch to device_for_each_chilld_node_scoped()

[Javier Carrasco]

This series switches from the device_for_each_child_node() macro to its
scoped variant. This 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(), which also simplifies existing code.

The non-scoped macros to walk over nodes turn error-prone as soon as
the loop contains early exits (break, goto, return), and patches to
fix them show up regularly, sometimes due to new error paths in an
existing loop [1].

The uses of device_for_each_child_node() with no early exits have been
left untouched because their simpilicty justifies the non-scoped
variant. In particular for gpio, there is a single case in gpio-hisi.c
where the loop does not have early exists. But if desired, it can be
easily converted as well to enforce the usage of the scoped variant when
"borrowing" existing code.

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.
Apart from automatically decrementing the node's refcount when it goes
out of scope, the scoped variant works exactly the same as the
non-scoped.

If functional changes are found, please report them back as errors.

Just in case, and even though the scoped macro already has multiple
users, I carried out a quick test with gpio-sim and a few nodes defined
in a devicetree, and everything works as expected.

Link:
https://lore.kernel.org/all/20240901160829.709296395@linuxfoundation.org/
[1]

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (3):
      gpio: dwapb: switch to device_for_each_child_node_scoped()
      gpio: sim: switch to device_for_each_child_node_scoped()
      gpio: acpi: switch to device_for_each_child_node_scoped()

 drivers/gpio/gpio-dwapb.c   | 4 +---
 drivers/gpio/gpio-sim.c     | 7 ++-----
 drivers/gpio/gpiolib-acpi.c | 4 +---
 3 files changed, 4 insertions(+), 11 deletions(-)
---
base-commit: 40e0c9d414f57d450e3ad03c12765e797fc3fede
change-id: 20240927-gpio_device_for_each_child_node_scoped-a2071fe5c8c6

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

[PATCH 1/3] gpio: dwapb: 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 a  call 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/gpio/gpio-dwapb.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 798235791f70..bd374fc27174 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -571,7 +571,6 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
 
 static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 {
-	struct fwnode_handle *fwnode;
 	struct dwapb_platform_data *pdata;
 	struct dwapb_port_property *pp;
 	int nports;
@@ -592,7 +591,7 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 	pdata->nports = nports;
 
 	i = 0;
-	device_for_each_child_node(dev, fwnode)  {
+	device_for_each_child_node_scoped(dev, fwnode)  {
 		pp = &pdata->properties[i++];
 		pp->fwnode = fwnode;
 
@@ -600,7 +599,6 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 		    pp->idx >= DWAPB_MAX_PORTS) {
 			dev_err(dev,
 				"missing/invalid port index for port%d\n", i);
-			fwnode_handle_put(fwnode);
 			return ERR_PTR(-EINVAL);
 		}
 

-- 
2.43.0

Re: [PATCH 1/3] gpio: dwapb: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

On Sat, Sep 28, 2024 at 09:47:35PM +0200, Javier Carrasco wrote:
> Switch to device_for_each_child_node_scoped() to simplify the code by
> removing the need for a  call 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().

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] gpio: dwapb: switch to device_for_each_child_node_scoped()

[Serge Semin]

On Sat, Sep 28, 2024 at 09:47:35PM GMT, Javier Carrasco wrote:
> Switch to device_for_each_child_node_scoped() to simplify the code by
> removing the need for a  call 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>

Acked-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> ---
>  drivers/gpio/gpio-dwapb.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 798235791f70..bd374fc27174 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -571,7 +571,6 @@ static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
>  
>  static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
>  {
> -	struct fwnode_handle *fwnode;
>  	struct dwapb_platform_data *pdata;
>  	struct dwapb_port_property *pp;
>  	int nports;
> @@ -592,7 +591,7 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
>  	pdata->nports = nports;
>  
>  	i = 0;
> -	device_for_each_child_node(dev, fwnode)  {
> +	device_for_each_child_node_scoped(dev, fwnode)  {
>  		pp = &pdata->properties[i++];
>  		pp->fwnode = fwnode;
>  
> @@ -600,7 +599,6 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
>  		    pp->idx >= DWAPB_MAX_PORTS) {
>  			dev_err(dev,
>  				"missing/invalid port index for port%d\n", i);
> -			fwnode_handle_put(fwnode);
>  			return ERR_PTR(-EINVAL);
>  		}
>  
> 
> -- 
> 2.43.0
> 

Re: [PATCH 1/3] gpio: dwapb: switch to device_for_each_child_node_scoped()

[Linus Walleij]

On Sat, Sep 28, 2024 at 9:47 PM 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 a  call 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>

Neat.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 2/3] gpio: sim: 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 a call 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/gpio/gpio-sim.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index dcca1d7f173e..f387dad81f29 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -520,15 +520,12 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
 static int gpio_sim_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct fwnode_handle *swnode;
 	int ret;
 
-	device_for_each_child_node(dev, swnode) {
+	device_for_each_child_node_scoped(dev, swnode) {
 		ret = gpio_sim_add_bank(swnode, dev);
-		if (ret) {
-			fwnode_handle_put(swnode);
+		if (ret)
 			return ret;
-		}
 	}
 
 	return 0;

-- 
2.43.0

Re: [PATCH 2/3] gpio: sim: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

On Sat, Sep 28, 2024 at 09:47:36PM +0200, Javier Carrasco wrote:
> Switch to device_for_each_child_node_scoped() to simplify the code by
> removing the need for a call 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().

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/3] gpio: acpi: 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 a call 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/gpio/gpiolib-acpi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 78ecd56123a3..1f9fe50bba00 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1315,9 +1315,8 @@ acpi_gpiochip_parse_own_gpio(struct acpi_gpio_chip *achip,
 static void acpi_gpiochip_scan_gpios(struct acpi_gpio_chip *achip)
 {
 	struct gpio_chip *chip = achip->chip;
-	struct fwnode_handle *fwnode;
 
-	device_for_each_child_node(chip->parent, fwnode) {
+	device_for_each_child_node_scoped(chip->parent, fwnode) {
 		unsigned long lflags;
 		enum gpiod_flags dflags;
 		struct gpio_desc *desc;
@@ -1335,7 +1334,6 @@ static void acpi_gpiochip_scan_gpios(struct acpi_gpio_chip *achip)
 		ret = gpiod_hog(desc, name, lflags, dflags);
 		if (ret) {
 			dev_err(chip->parent, "Failed to hog GPIO\n");
-			fwnode_handle_put(fwnode);
 			return;
 		}
 	}

-- 
2.43.0

Re: [PATCH 3/3] gpio: acpi: switch to device_for_each_child_node_scoped()

[Andy Shevchenko]

On Sat, Sep 28, 2024 at 09:47:37PM +0200, Javier Carrasco wrote:
> Switch to device_for_each_child_node_scoped() to simplify the code by
> removing the need for a call 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().

Pushed to my review and testing queue, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: (subset) [PATCH 0/3] gpio: switch to device_for_each_chilld_node_scoped()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Sat, 28 Sep 2024 21:47:34 +0200, Javier Carrasco wrote:
> This series switches from the device_for_each_child_node() macro to its
> scoped variant. This 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(), which also simplifies existing code.
> 
> The non-scoped macros to walk over nodes turn error-prone as soon as
> the loop contains early exits (break, goto, return), and patches to
> fix them show up regularly, sometimes due to new error paths in an
> existing loop [1].
> 
> [...]

Applied, thanks!

[1/3] gpio: dwapb: switch to device_for_each_child_node_scoped()
      commit: 0a53be8e080b53ef922e90204999f4ccef29cd57
[2/3] gpio: sim: switch to device_for_each_child_node_scoped()
      commit: d64d0287f4bc7013c60b07e34e43c3fc558e3808

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>