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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Fri, Aug 20, 2021 at 5:37 PM Christophe JAILLET
<[email protected]> 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
On Fri, Aug 20, 2021 at 5:38 PM Christophe JAILLET
<[email protected]> 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 <[email protected]>
> ---
> 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
On Fri, Aug 20, 2021 at 5:37 PM Christophe JAILLET
<[email protected]> 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 <[email protected]>
> ---
> 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
On Fri, Aug 20, 2021 at 5:38 PM Christophe JAILLET
<[email protected]> 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 <[email protected]>
> ---
> 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