Currently we leak a lot of things when tearing down the IRQs this patch
fixes this cleaning up both the IRQ mappings and the IRQ domain itself.
Signed-off-by: Charles Keepax <[email protected]>
---
Changes since v1:
- Correct handling of ret and error messages from irq_create_mapping
Thanks,
Charles
drivers/mfd/arizona-irq.c | 54 +++++++++++++++++++++++++++++++++++------------
1 file changed, 40 insertions(+), 14 deletions(-)
diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
index 2e01975..bc3b342 100644
--- a/drivers/mfd/arizona-irq.c
+++ b/drivers/mfd/arizona-irq.c
@@ -207,6 +207,7 @@ int arizona_irq_init(struct arizona *arizona)
int ret, i;
const struct regmap_irq_chip *aod, *irq;
struct irq_data *irq_data;
+ unsigned int virq;
arizona->ctrlif_error = true;
@@ -318,24 +319,34 @@ int arizona_irq_init(struct arizona *arizona)
}
if (aod) {
- ret = regmap_add_irq_chip(arizona->regmap,
- irq_create_mapping(arizona->virq, 0),
- IRQF_ONESHOT, 0, aod,
- &arizona->aod_irq_chip);
+ virq = irq_create_mapping(arizona->virq, 0);
+ if (!virq) {
+ dev_err(arizona->dev, "Failed to map AOD IRQs\n");
+ ret = -EINVAL;
+ goto err_domain;
+ }
+
+ ret = regmap_add_irq_chip(arizona->regmap, virq, IRQF_ONESHOT,
+ 0, aod, &arizona->aod_irq_chip);
if (ret != 0) {
dev_err(arizona->dev,
"Failed to add AOD IRQs: %d\n", ret);
- goto err;
+ goto err_map_aod;
}
}
- ret = regmap_add_irq_chip(arizona->regmap,
- irq_create_mapping(arizona->virq, 1),
- IRQF_ONESHOT, 0, irq,
- &arizona->irq_chip);
+ virq = irq_create_mapping(arizona->virq, 1);
+ if (!virq) {
+ dev_err(arizona->dev, "Failed to map main IRQs\n");
+ ret = -EINVAL;
+ goto err_aod;
+ }
+
+ ret = regmap_add_irq_chip(arizona->regmap, virq, IRQF_ONESHOT,
+ 0, irq, &arizona->irq_chip);
if (ret != 0) {
dev_err(arizona->dev, "Failed to add main IRQs: %d\n", ret);
- goto err_aod;
+ goto err_map_main_irq;
}
/* Used to emulate edge trigger and to work around broken pinmux */
@@ -400,23 +411,38 @@ int arizona_irq_init(struct arizona *arizona)
err_main_irq:
regmap_del_irq_chip(irq_find_mapping(arizona->virq, 1),
arizona->irq_chip);
+err_map_main_irq:
+ irq_dispose_mapping(irq_find_mapping(arizona->virq, 1));
err_aod:
regmap_del_irq_chip(irq_find_mapping(arizona->virq, 0),
arizona->aod_irq_chip);
+err_map_aod:
+ irq_dispose_mapping(irq_find_mapping(arizona->virq, 0));
+err_domain:
+ irq_domain_remove(arizona->virq);
err:
return ret;
}
int arizona_irq_exit(struct arizona *arizona)
{
+ unsigned int virq;
+
if (arizona->ctrlif_error)
free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR),
arizona);
free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_BOOT_DONE), arizona);
- regmap_del_irq_chip(irq_find_mapping(arizona->virq, 1),
- arizona->irq_chip);
- regmap_del_irq_chip(irq_find_mapping(arizona->virq, 0),
- arizona->aod_irq_chip);
+
+ virq = irq_find_mapping(arizona->virq, 1);
+ regmap_del_irq_chip(virq, arizona->irq_chip);
+ irq_dispose_mapping(virq);
+
+ virq = irq_find_mapping(arizona->virq, 0);
+ regmap_del_irq_chip(virq, arizona->aod_irq_chip);
+ irq_dispose_mapping(virq);
+
+ irq_domain_remove(arizona->virq);
+
free_irq(arizona->irq, arizona);
return 0;
--
2.1.4
We have arizona_map_irq we might as well use it rather than hard coding
it in several places.
Signed-off-by: Charles Keepax <[email protected]>
---
drivers/mfd/arizona-irq.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
index bc3b342..22f96c9 100644
--- a/drivers/mfd/arizona-irq.c
+++ b/drivers/mfd/arizona-irq.c
@@ -204,7 +204,7 @@ static const struct irq_domain_ops arizona_domain_ops = {
int arizona_irq_init(struct arizona *arizona)
{
int flags = IRQF_ONESHOT;
- int ret, i;
+ int ret;
const struct regmap_irq_chip *aod, *irq;
struct irq_data *irq_data;
unsigned int virq;
@@ -379,9 +379,8 @@ int arizona_irq_init(struct arizona *arizona)
}
/* Make sure the boot done IRQ is unmasked for resumes */
- i = arizona_map_irq(arizona, ARIZONA_IRQ_BOOT_DONE);
- ret = request_threaded_irq(i, NULL, arizona_boot_done, IRQF_ONESHOT,
- "Boot done", arizona);
+ ret = arizona_request_irq(arizona, ARIZONA_IRQ_BOOT_DONE, "Boot done",
+ arizona_boot_done, arizona);
if (ret != 0) {
dev_err(arizona->dev, "Failed to request boot done %d: %d\n",
arizona->irq, ret);
@@ -390,10 +389,9 @@ int arizona_irq_init(struct arizona *arizona)
/* Handle control interface errors in the core */
if (arizona->ctrlif_error) {
- i = arizona_map_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR);
- ret = request_threaded_irq(i, NULL, arizona_ctrlif_err,
- IRQF_ONESHOT,
- "Control interface error", arizona);
+ ret = arizona_request_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR,
+ "Control interface error",
+ arizona_ctrlif_err, arizona);
if (ret != 0) {
dev_err(arizona->dev,
"Failed to request CTRLIF_ERR %d: %d\n",
@@ -405,7 +403,7 @@ int arizona_irq_init(struct arizona *arizona)
return 0;
err_ctrlif:
- free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_BOOT_DONE), arizona);
+ arizona_free_irq(arizona, ARIZONA_IRQ_BOOT_DONE, arizona);
err_boot_done:
free_irq(arizona->irq, arizona);
err_main_irq:
@@ -429,9 +427,8 @@ int arizona_irq_exit(struct arizona *arizona)
unsigned int virq;
if (arizona->ctrlif_error)
- free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR),
- arizona);
- free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_BOOT_DONE), arizona);
+ arizona_free_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR, arizona);
+ arizona_free_irq(arizona, ARIZONA_IRQ_BOOT_DONE, arizona);
virq = irq_find_mapping(arizona->virq, 1);
regmap_del_irq_chip(virq, arizona->irq_chip);
--
2.1.4
As DCVDD will often be supplied by a child node of the MFD, we
can't call mfd_remove_devices as the first step in arizona_dev_exit
as might be expected (tidy up the children before we tidy up the
MFD). We need to disable and put the DCVDD regulator before we call
mfd_remove_devices, to prevent PM runtime from turning this back on we
also need to disable the PM runtime before we do this. Finally we can
not clean up the IRQs until all the MFD children have been removed, as
they may have registered IRQs themselves.
This creates a window of time where the interrupts are enabled but
the PM runtime, on which the IRQ handler depends, is not available,
any interrupts in this window will go unhandled and fill the log with
failed to resume device messages. To avoid this we simply disable the
main IRQ at the start of arizona_dev_exit, we don't need to actually
handle any IRQs in this window as we are removing the driver.
Signed-off-by: Charles Keepax <[email protected]>
---
drivers/mfd/arizona-core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 41767f7..b6d4bc6 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -1553,6 +1553,7 @@ EXPORT_SYMBOL_GPL(arizona_dev_init);
int arizona_dev_exit(struct arizona *arizona)
{
+ disable_irq(arizona->irq);
pm_runtime_disable(arizona->dev);
regulator_disable(arizona->dcvdd);
--
2.1.4
On Mon, 14 Nov 2016, Charles Keepax wrote:
> Currently we leak a lot of things when tearing down the IRQs this patch
> fixes this cleaning up both the IRQ mappings and the IRQ domain itself.
>
> Signed-off-by: Charles Keepax <[email protected]>
> ---
>
> Changes since v1:
> - Correct handling of ret and error messages from irq_create_mapping
>
> Thanks,
> Charles
>
> drivers/mfd/arizona-irq.c | 54 +++++++++++++++++++++++++++++++++++------------
> 1 file changed, 40 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
> index 2e01975..bc3b342 100644
> --- a/drivers/mfd/arizona-irq.c
> +++ b/drivers/mfd/arizona-irq.c
> @@ -207,6 +207,7 @@ int arizona_irq_init(struct arizona *arizona)
> int ret, i;
> const struct regmap_irq_chip *aod, *irq;
> struct irq_data *irq_data;
> + unsigned int virq;
>
> arizona->ctrlif_error = true;
>
> @@ -318,24 +319,34 @@ int arizona_irq_init(struct arizona *arizona)
> }
>
> if (aod) {
> - ret = regmap_add_irq_chip(arizona->regmap,
> - irq_create_mapping(arizona->virq, 0),
> - IRQF_ONESHOT, 0, aod,
> - &arizona->aod_irq_chip);
> + virq = irq_create_mapping(arizona->virq, 0);
I would like to see all of the '0's and '1's defined please.
> + if (!virq) {
> + dev_err(arizona->dev, "Failed to map AOD IRQs\n");
> + ret = -EINVAL;
> + goto err_domain;
> + }
> +
> + ret = regmap_add_irq_chip(arizona->regmap, virq, IRQF_ONESHOT,
> + 0, aod, &arizona->aod_irq_chip);
> if (ret != 0) {
> dev_err(arizona->dev,
> "Failed to add AOD IRQs: %d\n", ret);
> - goto err;
> + goto err_map_aod;
> }
> }
>
> - ret = regmap_add_irq_chip(arizona->regmap,
> - irq_create_mapping(arizona->virq, 1),
> - IRQF_ONESHOT, 0, irq,
> - &arizona->irq_chip);
> + virq = irq_create_mapping(arizona->virq, 1);
> + if (!virq) {
> + dev_err(arizona->dev, "Failed to map main IRQs\n");
> + ret = -EINVAL;
> + goto err_aod;
> + }
> +
> + ret = regmap_add_irq_chip(arizona->regmap, virq, IRQF_ONESHOT,
> + 0, irq, &arizona->irq_chip);
> if (ret != 0) {
> dev_err(arizona->dev, "Failed to add main IRQs: %d\n", ret);
> - goto err_aod;
> + goto err_map_main_irq;
> }
>
> /* Used to emulate edge trigger and to work around broken pinmux */
> @@ -400,23 +411,38 @@ int arizona_irq_init(struct arizona *arizona)
> err_main_irq:
> regmap_del_irq_chip(irq_find_mapping(arizona->virq, 1),
> arizona->irq_chip);
> +err_map_main_irq:
> + irq_dispose_mapping(irq_find_mapping(arizona->virq, 1));
> err_aod:
> regmap_del_irq_chip(irq_find_mapping(arizona->virq, 0),
> arizona->aod_irq_chip);
> +err_map_aod:
> + irq_dispose_mapping(irq_find_mapping(arizona->virq, 0));
> +err_domain:
> + irq_domain_remove(arizona->virq);
> err:
> return ret;
> }
>
> int arizona_irq_exit(struct arizona *arizona)
> {
> + unsigned int virq;
> +
> if (arizona->ctrlif_error)
> free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR),
> arizona);
> free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_BOOT_DONE), arizona);
> - regmap_del_irq_chip(irq_find_mapping(arizona->virq, 1),
> - arizona->irq_chip);
> - regmap_del_irq_chip(irq_find_mapping(arizona->virq, 0),
> - arizona->aod_irq_chip);
> +
> + virq = irq_find_mapping(arizona->virq, 1);
> + regmap_del_irq_chip(virq, arizona->irq_chip);
> + irq_dispose_mapping(virq);
> +
> + virq = irq_find_mapping(arizona->virq, 0);
> + regmap_del_irq_chip(virq, arizona->aod_irq_chip);
> + irq_dispose_mapping(virq);
> +
> + irq_domain_remove(arizona->virq);
> +
> free_irq(arizona->irq, arizona);
>
> return 0;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 14 Nov 2016, Charles Keepax wrote:
> We have arizona_map_irq we might as well use it rather than hard coding
> it in several places.
>
> Signed-off-by: Charles Keepax <[email protected]>
> ---
> drivers/mfd/arizona-irq.c | 21 +++++++++------------
> 1 file changed, 9 insertions(+), 12 deletions(-)
Applied, thanks.
> diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
> index bc3b342..22f96c9 100644
> --- a/drivers/mfd/arizona-irq.c
> +++ b/drivers/mfd/arizona-irq.c
> @@ -204,7 +204,7 @@ static const struct irq_domain_ops arizona_domain_ops = {
> int arizona_irq_init(struct arizona *arizona)
> {
> int flags = IRQF_ONESHOT;
> - int ret, i;
> + int ret;
> const struct regmap_irq_chip *aod, *irq;
> struct irq_data *irq_data;
> unsigned int virq;
> @@ -379,9 +379,8 @@ int arizona_irq_init(struct arizona *arizona)
> }
>
> /* Make sure the boot done IRQ is unmasked for resumes */
> - i = arizona_map_irq(arizona, ARIZONA_IRQ_BOOT_DONE);
> - ret = request_threaded_irq(i, NULL, arizona_boot_done, IRQF_ONESHOT,
> - "Boot done", arizona);
> + ret = arizona_request_irq(arizona, ARIZONA_IRQ_BOOT_DONE, "Boot done",
> + arizona_boot_done, arizona);
> if (ret != 0) {
> dev_err(arizona->dev, "Failed to request boot done %d: %d\n",
> arizona->irq, ret);
> @@ -390,10 +389,9 @@ int arizona_irq_init(struct arizona *arizona)
>
> /* Handle control interface errors in the core */
> if (arizona->ctrlif_error) {
> - i = arizona_map_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR);
> - ret = request_threaded_irq(i, NULL, arizona_ctrlif_err,
> - IRQF_ONESHOT,
> - "Control interface error", arizona);
> + ret = arizona_request_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR,
> + "Control interface error",
> + arizona_ctrlif_err, arizona);
> if (ret != 0) {
> dev_err(arizona->dev,
> "Failed to request CTRLIF_ERR %d: %d\n",
> @@ -405,7 +403,7 @@ int arizona_irq_init(struct arizona *arizona)
> return 0;
>
> err_ctrlif:
> - free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_BOOT_DONE), arizona);
> + arizona_free_irq(arizona, ARIZONA_IRQ_BOOT_DONE, arizona);
> err_boot_done:
> free_irq(arizona->irq, arizona);
> err_main_irq:
> @@ -429,9 +427,8 @@ int arizona_irq_exit(struct arizona *arizona)
> unsigned int virq;
>
> if (arizona->ctrlif_error)
> - free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR),
> - arizona);
> - free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_BOOT_DONE), arizona);
> + arizona_free_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR, arizona);
> + arizona_free_irq(arizona, ARIZONA_IRQ_BOOT_DONE, arizona);
>
> virq = irq_find_mapping(arizona->virq, 1);
> regmap_del_irq_chip(virq, arizona->irq_chip);
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 14 Nov 2016, Charles Keepax wrote:
> As DCVDD will often be supplied by a child node of the MFD, we
> can't call mfd_remove_devices as the first step in arizona_dev_exit
> as might be expected (tidy up the children before we tidy up the
> MFD). We need to disable and put the DCVDD regulator before we call
> mfd_remove_devices, to prevent PM runtime from turning this back on we
> also need to disable the PM runtime before we do this. Finally we can
> not clean up the IRQs until all the MFD children have been removed, as
> they may have registered IRQs themselves.
>
> This creates a window of time where the interrupts are enabled but
> the PM runtime, on which the IRQ handler depends, is not available,
> any interrupts in this window will go unhandled and fill the log with
> failed to resume device messages. To avoid this we simply disable the
> main IRQ at the start of arizona_dev_exit, we don't need to actually
> handle any IRQs in this window as we are removing the driver.
>
> Signed-off-by: Charles Keepax <[email protected]>
> ---
> drivers/mfd/arizona-core.c | 1 +
> 1 file changed, 1 insertion(+)
I'm convinced.
Applied, thanks.
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 41767f7..b6d4bc6 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -1553,6 +1553,7 @@ EXPORT_SYMBOL_GPL(arizona_dev_init);
>
> int arizona_dev_exit(struct arizona *arizona)
> {
> + disable_irq(arizona->irq);
> pm_runtime_disable(arizona->dev);
>
> regulator_disable(arizona->dcvdd);
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, 22 Nov 2016, Lee Jones wrote:
> On Mon, 14 Nov 2016, Charles Keepax wrote:
>
> > We have arizona_map_irq we might as well use it rather than hard coding
> > it in several places.
> >
> > Signed-off-by: Charles Keepax <[email protected]>
> > ---
> > drivers/mfd/arizona-irq.c | 21 +++++++++------------
> > 1 file changed, 9 insertions(+), 12 deletions(-)
>
> Applied, thanks.
Slight change of plan.
Patch doesn't seem to apply.
Please rebase it on top of my MFD tree and resubmit.
> > diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
> > index bc3b342..22f96c9 100644
> > --- a/drivers/mfd/arizona-irq.c
> > +++ b/drivers/mfd/arizona-irq.c
> > @@ -204,7 +204,7 @@ static const struct irq_domain_ops arizona_domain_ops = {
> > int arizona_irq_init(struct arizona *arizona)
> > {
> > int flags = IRQF_ONESHOT;
> > - int ret, i;
> > + int ret;
> > const struct regmap_irq_chip *aod, *irq;
> > struct irq_data *irq_data;
> > unsigned int virq;
> > @@ -379,9 +379,8 @@ int arizona_irq_init(struct arizona *arizona)
> > }
> >
> > /* Make sure the boot done IRQ is unmasked for resumes */
> > - i = arizona_map_irq(arizona, ARIZONA_IRQ_BOOT_DONE);
> > - ret = request_threaded_irq(i, NULL, arizona_boot_done, IRQF_ONESHOT,
> > - "Boot done", arizona);
> > + ret = arizona_request_irq(arizona, ARIZONA_IRQ_BOOT_DONE, "Boot done",
> > + arizona_boot_done, arizona);
> > if (ret != 0) {
> > dev_err(arizona->dev, "Failed to request boot done %d: %d\n",
> > arizona->irq, ret);
> > @@ -390,10 +389,9 @@ int arizona_irq_init(struct arizona *arizona)
> >
> > /* Handle control interface errors in the core */
> > if (arizona->ctrlif_error) {
> > - i = arizona_map_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR);
> > - ret = request_threaded_irq(i, NULL, arizona_ctrlif_err,
> > - IRQF_ONESHOT,
> > - "Control interface error", arizona);
> > + ret = arizona_request_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR,
> > + "Control interface error",
> > + arizona_ctrlif_err, arizona);
> > if (ret != 0) {
> > dev_err(arizona->dev,
> > "Failed to request CTRLIF_ERR %d: %d\n",
> > @@ -405,7 +403,7 @@ int arizona_irq_init(struct arizona *arizona)
> > return 0;
> >
> > err_ctrlif:
> > - free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_BOOT_DONE), arizona);
> > + arizona_free_irq(arizona, ARIZONA_IRQ_BOOT_DONE, arizona);
> > err_boot_done:
> > free_irq(arizona->irq, arizona);
> > err_main_irq:
> > @@ -429,9 +427,8 @@ int arizona_irq_exit(struct arizona *arizona)
> > unsigned int virq;
> >
> > if (arizona->ctrlif_error)
> > - free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR),
> > - arizona);
> > - free_irq(arizona_map_irq(arizona, ARIZONA_IRQ_BOOT_DONE), arizona);
> > + arizona_free_irq(arizona, ARIZONA_IRQ_CTRLIF_ERR, arizona);
> > + arizona_free_irq(arizona, ARIZONA_IRQ_BOOT_DONE, arizona);
> >
> > virq = irq_find_mapping(arizona->virq, 1);
> > regmap_del_irq_chip(virq, arizona->irq_chip);
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, Nov 22, 2016 at 03:54:36PM +0000, Lee Jones wrote:
> On Tue, 22 Nov 2016, Lee Jones wrote:
>
> > On Mon, 14 Nov 2016, Charles Keepax wrote:
> >
> > > We have arizona_map_irq we might as well use it rather than hard coding
> > > it in several places.
> > >
> > > Signed-off-by: Charles Keepax <[email protected]>
> > > ---
> > > drivers/mfd/arizona-irq.c | 21 +++++++++------------
> > > 1 file changed, 9 insertions(+), 12 deletions(-)
> >
> > Applied, thanks.
>
> Slight change of plan.
>
> Patch doesn't seem to apply.
>
> Please rebase it on top of my MFD tree and resubmit.
>
Yeah depends on the first patch, ignore the ones I just sent
(since I had assumed you fixed that up manually) and I will
resend the whole series.
Thanks,
Charles