[PATCH 0/3] gpio: mpc8xxx: Fix 3 errors related to the error handling path of the 'mpc8xxx_probe()'

[PATCH 0/3] gpio: mpc8xxx: Fix 3 errors related to the error handling path of the 'mpc8xxx_probe()'

[Christophe JAILLET]

This has been split in 3 patches because:
  - the root issue of patch 1 and 3 is not the same commit as the one for patch 2.
  - the strategy to fix the issues is not the same
     - patch 1: add a new call in the rror handling path
     - patch 3: switch to a resource managed function

They could be merged together if easier to review. The subject would then be
something like "gpio: mpc8xxx: Fix the error handling path of 'mpc8xxx_probe()'"

If prefered, 'devm_add_action_or_reset()' could be used to switch the probe to
a fully managed resource function and remove the 'remove' function.
That's mostly a matter of taste.

If such an option is preferred, I'm a bit puzzled by the
'irq_set_chained_handler_and_data()' call in the remove function because I
don't see why it is there.
Also see the comment at the end of patch 1 also related to this function call.


Christophe JAILLET (3):
  gpio: mpc8xxx: Fix a resources leak in the error handling path of
    'mpc8xxx_probe()'
  gpio: mpc8xxx: Fix a potential double iounmap call in
    'mpc8xxx_probe()'
  gpio: mpc8xxx: Use 'devm_gpiochip_add_data()' to simplify the code and
    avoid a leak

 drivers/gpio/gpio-mpc8xxx.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

-- 
2.30.2

[PATCH 1/3] gpio: mpc8xxx: Fix a resources leak in the error handling path of 'mpc8xxx_probe()'

[Christophe JAILLET]

Commit 698b8eeaed72 ("gpio/mpc8xxx: change irq handler from chained to normal")
has introduced a new 'goto err;' at the very end of the function, but has
not updated the error handling path accordingly.

Add the now missing 'irq_domain_remove()' call which balances a previous
'irq_domain_create_linear() call.

Fixes: 698b8eeaed72 ("gpio/mpc8xxx: change irq handler from chained to normal")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Is the 'irq_set_chained_handler_and_data()' of the remove function also
needed here?
---
 drivers/gpio/gpio-mpc8xxx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
index 67dc38976ab6..241bcc80612e 100644
--- a/drivers/gpio/gpio-mpc8xxx.c
+++ b/drivers/gpio/gpio-mpc8xxx.c
@@ -416,6 +416,8 @@ static int mpc8xxx_probe(struct platform_device *pdev)
 
 	return 0;
 err:
+	if (mpc8xxx_gc->irq)
+		irq_domain_remove(mpc8xxx_gc->irq);
 	iounmap(mpc8xxx_gc->regs);
 	return ret;
 }
-- 
2.30.2

Re: [PATCH 1/3] gpio: mpc8xxx: Fix a resources leak in the error handling path of 'mpc8xxx_probe()'

[Bartosz Golaszewski]

On Fri, Aug 20, 2021 at 5:37 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Commit 698b8eeaed72 ("gpio/mpc8xxx: change irq handler from chained to normal")
> has introduced a new 'goto err;' at the very end of the function, but has
> not updated the error handling path accordingly.
>
> Add the now missing 'irq_domain_remove()' call which balances a previous
> 'irq_domain_create_linear() call.
>
> Fixes: 698b8eeaed72 ("gpio/mpc8xxx: change irq handler from chained to normal")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Is the 'irq_set_chained_handler_and_data()' of the remove function also
> needed here?
> ---
>  drivers/gpio/gpio-mpc8xxx.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
> index 67dc38976ab6..241bcc80612e 100644
> --- a/drivers/gpio/gpio-mpc8xxx.c
> +++ b/drivers/gpio/gpio-mpc8xxx.c
> @@ -416,6 +416,8 @@ static int mpc8xxx_probe(struct platform_device *pdev)
>
>         return 0;
>  err:
> +       if (mpc8xxx_gc->irq)
> +               irq_domain_remove(mpc8xxx_gc->irq);
>         iounmap(mpc8xxx_gc->regs);
>         return ret;
>  }
> --
> 2.30.2
>

Applied, thanks!

Bart

[PATCH 2/3] gpio: mpc8xxx: Fix a potential double iounmap call in 'mpc8xxx_probe()'

[Christophe JAILLET]

Commit 76c47d1449fc ("gpio: mpc8xxx: Add ACPI support") has switched to a
managed version when dealing with 'mpc8xxx_gc->regs'. So the corresponding
'iounmap()' call in the error handling path and in the remove should be
removed to avoid a double unmap.

This also allows some simplification in the probe. All the error handling
paths related to managed resources can be direct returns and a NULL check
in what remains in the error handling path can be removed.

Fixes: 76c47d1449fc ("gpio: mpc8xxx: Add ACPI support")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/gpio/gpio-mpc8xxx.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
index 241bcc80612e..fa4aaeced3f1 100644
--- a/drivers/gpio/gpio-mpc8xxx.c
+++ b/drivers/gpio/gpio-mpc8xxx.c
@@ -332,7 +332,7 @@ static int mpc8xxx_probe(struct platform_device *pdev)
 				 mpc8xxx_gc->regs + GPIO_DIR, NULL,
 				 BGPIOF_BIG_ENDIAN);
 		if (ret)
-			goto err;
+			return ret;
 		dev_dbg(&pdev->dev, "GPIO registers are LITTLE endian\n");
 	} else {
 		ret = bgpio_init(gc, &pdev->dev, 4,
@@ -342,7 +342,7 @@ static int mpc8xxx_probe(struct platform_device *pdev)
 				 BGPIOF_BIG_ENDIAN
 				 | BGPIOF_BIG_ENDIAN_BYTE_ORDER);
 		if (ret)
-			goto err;
+			return ret;
 		dev_dbg(&pdev->dev, "GPIO registers are BIG endian\n");
 	}
 
@@ -384,7 +384,7 @@ static int mpc8xxx_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(&pdev->dev,
 			"GPIO chip registration failed with status %d\n", ret);
-		goto err;
+		return ret;
 	}
 
 	mpc8xxx_gc->irqn = platform_get_irq(pdev, 0);
@@ -416,9 +416,7 @@ static int mpc8xxx_probe(struct platform_device *pdev)
 
 	return 0;
 err:
-	if (mpc8xxx_gc->irq)
-		irq_domain_remove(mpc8xxx_gc->irq);
-	iounmap(mpc8xxx_gc->regs);
+	irq_domain_remove(mpc8xxx_gc->irq);
 	return ret;
 }
 
@@ -432,7 +430,6 @@ static int mpc8xxx_remove(struct platform_device *pdev)
 	}
 
 	gpiochip_remove(&mpc8xxx_gc->gc);
-	iounmap(mpc8xxx_gc->regs);
 
 	return 0;
 }
-- 
2.30.2

Re: [PATCH 2/3] gpio: mpc8xxx: Fix a potential double iounmap call in 'mpc8xxx_probe()'

[Bartosz Golaszewski]

On Fri, Aug 20, 2021 at 5:38 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Commit 76c47d1449fc ("gpio: mpc8xxx: Add ACPI support") has switched to a
> managed version when dealing with 'mpc8xxx_gc->regs'. So the corresponding
> 'iounmap()' call in the error handling path and in the remove should be
> removed to avoid a double unmap.
>
> This also allows some simplification in the probe. All the error handling
> paths related to managed resources can be direct returns and a NULL check
> in what remains in the error handling path can be removed.
>
> Fixes: 76c47d1449fc ("gpio: mpc8xxx: Add ACPI support")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/gpio/gpio-mpc8xxx.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
> index 241bcc80612e..fa4aaeced3f1 100644
> --- a/drivers/gpio/gpio-mpc8xxx.c
> +++ b/drivers/gpio/gpio-mpc8xxx.c
> @@ -332,7 +332,7 @@ static int mpc8xxx_probe(struct platform_device *pdev)
>                                  mpc8xxx_gc->regs + GPIO_DIR, NULL,
>                                  BGPIOF_BIG_ENDIAN);
>                 if (ret)
> -                       goto err;
> +                       return ret;
>                 dev_dbg(&pdev->dev, "GPIO registers are LITTLE endian\n");
>         } else {
>                 ret = bgpio_init(gc, &pdev->dev, 4,
> @@ -342,7 +342,7 @@ static int mpc8xxx_probe(struct platform_device *pdev)
>                                  BGPIOF_BIG_ENDIAN
>                                  | BGPIOF_BIG_ENDIAN_BYTE_ORDER);
>                 if (ret)
> -                       goto err;
> +                       return ret;
>                 dev_dbg(&pdev->dev, "GPIO registers are BIG endian\n");
>         }
>
> @@ -384,7 +384,7 @@ static int mpc8xxx_probe(struct platform_device *pdev)
>         if (ret) {
>                 dev_err(&pdev->dev,
>                         "GPIO chip registration failed with status %d\n", ret);
> -               goto err;
> +               return ret;
>         }
>
>         mpc8xxx_gc->irqn = platform_get_irq(pdev, 0);
> @@ -416,9 +416,7 @@ static int mpc8xxx_probe(struct platform_device *pdev)
>
>         return 0;
>  err:
> -       if (mpc8xxx_gc->irq)
> -               irq_domain_remove(mpc8xxx_gc->irq);
> -       iounmap(mpc8xxx_gc->regs);
> +       irq_domain_remove(mpc8xxx_gc->irq);
>         return ret;
>  }
>
> @@ -432,7 +430,6 @@ static int mpc8xxx_remove(struct platform_device *pdev)
>         }
>
>         gpiochip_remove(&mpc8xxx_gc->gc);
> -       iounmap(mpc8xxx_gc->regs);
>
>         return 0;
>  }
> --
> 2.30.2
>

Applied, thanks!

Bart

[PATCH 3/3] gpio: mpc8xxx: Use 'devm_gpiochip_add_data()' to simplify the code and avoid a leak

[Christophe JAILLET]

If an error occurs after a 'gpiochip_add_data()' call it must be undone by
a corresponding 'gpiochip_remove()' as already done in the remove function.

To simplify the code a fix a leak in the error handling path of the probe,
use the managed version instead (i.e. 'devm_gpiochip_add_data()')

Fixes: 698b8eeaed72 ("gpio/mpc8xxx: change irq handler from chained to normal")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/gpio/gpio-mpc8xxx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
index fa4aaeced3f1..70d6ae20b1da 100644
--- a/drivers/gpio/gpio-mpc8xxx.c
+++ b/drivers/gpio/gpio-mpc8xxx.c
@@ -380,7 +380,7 @@ static int mpc8xxx_probe(struct platform_device *pdev)
 	    is_acpi_node(fwnode))
 		gc->write_reg(mpc8xxx_gc->regs + GPIO_IBE, 0xffffffff);
 
-	ret = gpiochip_add_data(gc, mpc8xxx_gc);
+	ret = devm_gpiochip_add_data(&pdev->dev, gc, mpc8xxx_gc);
 	if (ret) {
 		dev_err(&pdev->dev,
 			"GPIO chip registration failed with status %d\n", ret);
@@ -429,8 +429,6 @@ static int mpc8xxx_remove(struct platform_device *pdev)
 		irq_domain_remove(mpc8xxx_gc->irq);
 	}
 
-	gpiochip_remove(&mpc8xxx_gc->gc);
-
 	return 0;
 }
 
-- 
2.30.2

Re: [PATCH 3/3] gpio: mpc8xxx: Use 'devm_gpiochip_add_data()' to simplify the code and avoid a leak

[Bartosz Golaszewski]

On Fri, Aug 20, 2021 at 5:38 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> If an error occurs after a 'gpiochip_add_data()' call it must be undone by
> a corresponding 'gpiochip_remove()' as already done in the remove function.
>
> To simplify the code a fix a leak in the error handling path of the probe,
> use the managed version instead (i.e. 'devm_gpiochip_add_data()')
>
> Fixes: 698b8eeaed72 ("gpio/mpc8xxx: change irq handler from chained to normal")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/gpio/gpio-mpc8xxx.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
> index fa4aaeced3f1..70d6ae20b1da 100644
> --- a/drivers/gpio/gpio-mpc8xxx.c
> +++ b/drivers/gpio/gpio-mpc8xxx.c
> @@ -380,7 +380,7 @@ static int mpc8xxx_probe(struct platform_device *pdev)
>             is_acpi_node(fwnode))
>                 gc->write_reg(mpc8xxx_gc->regs + GPIO_IBE, 0xffffffff);
>
> -       ret = gpiochip_add_data(gc, mpc8xxx_gc);
> +       ret = devm_gpiochip_add_data(&pdev->dev, gc, mpc8xxx_gc);
>         if (ret) {
>                 dev_err(&pdev->dev,
>                         "GPIO chip registration failed with status %d\n", ret);
> @@ -429,8 +429,6 @@ static int mpc8xxx_remove(struct platform_device *pdev)
>                 irq_domain_remove(mpc8xxx_gc->irq);
>         }
>
> -       gpiochip_remove(&mpc8xxx_gc->gc);
> -
>         return 0;
>  }
>
> --
> 2.30.2
>

Applied, thanks!

Bart

Re: [PATCH 0/3] gpio: mpc8xxx: Fix 3 errors related to the error handling path of the 'mpc8xxx_probe()'

[Bartosz Golaszewski]

On Fri, Aug 20, 2021 at 5:37 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> This has been split in 3 patches because:
>   - the root issue of patch 1 and 3 is not the same commit as the one for patch 2.
>   - the strategy to fix the issues is not the same
>      - patch 1: add a new call in the rror handling path
>      - patch 3: switch to a resource managed function
>
> They could be merged together if easier to review. The subject would then be
> something like "gpio: mpc8xxx: Fix the error handling path of 'mpc8xxx_probe()'"
>
> If prefered, 'devm_add_action_or_reset()' could be used to switch the probe to
> a fully managed resource function and remove the 'remove' function.
> That's mostly a matter of taste.
>
> If such an option is preferred, I'm a bit puzzled by the
> 'irq_set_chained_handler_and_data()' call in the remove function because I
> don't see why it is there.
> Also see the comment at the end of patch 1 also related to this function call.
>
>
> Christophe JAILLET (3):
>   gpio: mpc8xxx: Fix a resources leak in the error handling path of
>     'mpc8xxx_probe()'
>   gpio: mpc8xxx: Fix a potential double iounmap call in
>     'mpc8xxx_probe()'
>   gpio: mpc8xxx: Use 'devm_gpiochip_add_data()' to simplify the code and
>     avoid a leak
>
>  drivers/gpio/gpio-mpc8xxx.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>
> --
> 2.30.2
>

Hi Christophe,

These look good but I'm leaving for a couple days in an hour so I
won't be able to send a PR to Linus before the next release. I'll make
these part of my regular PR for the upcoming merge window.

Bart