When I test the irqchip code of Loongson, I read the related code of other
chips in drivers/irqchip and I find some potential resource leaks in the
error path, I think it is better to fix them.
v2:
- Split the first patch into a new patch series which
includes small patches and add "Fixes" tag
- Use "goto" label to handle error path in some patches
v3:
- Add missed variable "ret" in the patch #5 and #13
v4:
- Modify the commit message of each patch suggested by Markus Elfring
- Make "irq_domain_remove(root_domain)" under CONFIG_SMP in patch #3
- Add a return statement before goto label in patch #4
Tiezhu Yang (14):
irqchip/ath79-misc: Fix potential resource leaks
irqchip/csky-apb-intc: Fix potential resource leaks
irqchip/csky-mpintc: Fix potential resource leaks
irqchip/davinci-aintc: Fix potential resource leaks
irqchip/davinci-cp-intc: Fix potential resource leaks
irqchip/digicolor: Fix potential resource leaks
irqchip/dw-apb-ictl: Fix potential resource leaks
irqchip/ls1x: Fix potential resource leaks
irqchip/mscc-ocelot: Fix potential resource leaks
irqchip/nvic: Fix potential resource leaks
irqchip/omap-intc: Fix potential resource leak
irqchip/riscv-intc: Fix potential resource leak
irqchip/s3c24xx: Fix potential resource leaks
irqchip/xilinx-intc: Fix potential resource leak
drivers/irqchip/irq-ath79-misc.c | 14 +++++++++++---
drivers/irqchip/irq-csky-apb-intc.c | 12 ++++++++++--
drivers/irqchip/irq-csky-mpintc.c | 34 +++++++++++++++++++++++++---------
drivers/irqchip/irq-davinci-aintc.c | 18 ++++++++++++++----
drivers/irqchip/irq-davinci-cp-intc.c | 18 +++++++++++++++---
drivers/irqchip/irq-digicolor.c | 14 +++++++++++---
drivers/irqchip/irq-dw-apb-ictl.c | 11 ++++++++---
drivers/irqchip/irq-ls1x.c | 4 +++-
drivers/irqchip/irq-mscc-ocelot.c | 6 ++++--
drivers/irqchip/irq-nvic.c | 12 +++++++++---
drivers/irqchip/irq-omap-intc.c | 4 +++-
drivers/irqchip/irq-riscv-intc.c | 1 +
drivers/irqchip/irq-s3c24xx.c | 23 +++++++++++++++++------
drivers/irqchip/irq-xilinx-intc.c | 4 +++-
14 files changed, 134 insertions(+), 41 deletions(-)
--
2.1.0
In the function omap_init_irq_of(), system resource "omap_irq_base"
was not released in the error case, fix it.
Fixes: 8598066cddd1 ("arm: omap: irq: move irq.c to drivers/irqchip/")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-omap-intc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-omap-intc.c b/drivers/irqchip/irq-omap-intc.c
index d360a6e..e711530 100644
--- a/drivers/irqchip/irq-omap-intc.c
+++ b/drivers/irqchip/irq-omap-intc.c
@@ -254,8 +254,10 @@ static int __init omap_init_irq_of(struct device_node *node)
omap_irq_soft_reset();
ret = omap_alloc_gc_of(domain, omap_irq_base);
- if (ret < 0)
+ if (ret < 0) {
irq_domain_remove(domain);
+ iounmap(omap_irq_base);
+ }
return ret;
}
--
2.1.0
In the function davinci_aintc_init(), system resources "config->reg.start",
"davinci_aintc_base", "irq_base" and "davinci_aintc_irq_domain" were not
released in a few error cases. Thus add jump targets for the completion of
the desired exception handling.
Fixes: 0145beed9d26 ("irqchip: davinci-aintc: move the driver to drivers/irqchip")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-davinci-aintc.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/irqchip/irq-davinci-aintc.c b/drivers/irqchip/irq-davinci-aintc.c
index 810ccc4..2a96dc9 100644
--- a/drivers/irqchip/irq-davinci-aintc.c
+++ b/drivers/irqchip/irq-davinci-aintc.c
@@ -96,7 +96,7 @@ void __init davinci_aintc_init(const struct davinci_aintc_config *config)
resource_size(&config->reg));
if (!davinci_aintc_base) {
pr_err("%s: unable to ioremap register range\n", __func__);
- return;
+ goto err_release;
}
/* Clear all interrupt requests */
@@ -133,7 +133,7 @@ void __init davinci_aintc_init(const struct davinci_aintc_config *config)
if (irq_base < 0) {
pr_err("%s: unable to allocate interrupt descriptors: %d\n",
__func__, irq_base);
- return;
+ goto err_iounmap;
}
davinci_aintc_irq_domain = irq_domain_add_legacy(NULL,
@@ -141,7 +141,7 @@ void __init davinci_aintc_init(const struct davinci_aintc_config *config)
&irq_domain_simple_ops, NULL);
if (!davinci_aintc_irq_domain) {
pr_err("%s: unable to create interrupt domain\n", __func__);
- return;
+ goto err_free_descs;
}
ret = irq_alloc_domain_generic_chips(davinci_aintc_irq_domain, 32, 1,
@@ -150,7 +150,7 @@ void __init davinci_aintc_init(const struct davinci_aintc_config *config)
if (ret) {
pr_err("%s: unable to allocate generic irq chips for domain\n",
__func__);
- return;
+ goto err_domain_remove;
}
for (irq_off = 0, reg_off = 0;
@@ -160,4 +160,14 @@ void __init davinci_aintc_init(const struct davinci_aintc_config *config)
irq_base + irq_off, 32);
set_handle_irq(davinci_aintc_handle_irq);
+ return;
+
+err_domain_remove:
+ irq_domain_remove(davinci_aintc_irq_domain);
+err_free_descs:
+ irq_free_descs(irq_base, config->num_irqs);
+err_iounmap:
+ iounmap(davinci_aintc_base);
+err_release:
+ release_mem_region(config->reg.start, resource_size(&config->reg));
}
--
2.1.0
In the function xilinx_intc_of_init(), system resource "irqc->root_domain"
was not released in the error case. Thus add jump target for the completion
of the desired exception handling.
Fixes: 9689c99e4950 ("irqchip/xilinx: Add support for parent intc")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-xilinx-intc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
index 1d3d273..dcc51e0 100644
--- a/drivers/irqchip/irq-xilinx-intc.c
+++ b/drivers/irqchip/irq-xilinx-intc.c
@@ -241,7 +241,7 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
} else {
pr_err("irq-xilinx: interrupts property not in DT\n");
ret = -EINVAL;
- goto error;
+ goto error_domain_remove;
}
} else {
primary_intc = irqc;
@@ -250,6 +250,8 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
return 0;
+error_domain_remove:
+ irq_domain_remove(irqc->root_domain);
error:
iounmap(irqc->base);
kfree(irqc);
--
2.1.0
In the function riscv_intc_init(), system resource "intc_domain"
was not released in the error case, fix it.
Fixes: 6b7ce8927b5a ("irqchip: RISC-V per-HART local interrupt controller driver")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-riscv-intc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
index a6f97fa..8d6286c 100644
--- a/drivers/irqchip/irq-riscv-intc.c
+++ b/drivers/irqchip/irq-riscv-intc.c
@@ -122,6 +122,7 @@ static int __init riscv_intc_init(struct device_node *node,
rc = set_handle_irq(&riscv_intc_irq);
if (rc) {
pr_err("failed to set irq handler\n");
+ irq_domain_remove(intc_domain);
return rc;
}
--
2.1.0
In the function ls1x_intc_of_init(), system resource "parent_irq"
was not released in a few error cases. Thus add jump target for
the completion of the desired exception handling.
Fixes: 9e543e22e204 ("irqchip: Add driver for Loongson-1 interrupt controller")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-ls1x.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-ls1x.c b/drivers/irqchip/irq-ls1x.c
index 353111a..409001b 100644
--- a/drivers/irqchip/irq-ls1x.c
+++ b/drivers/irqchip/irq-ls1x.c
@@ -131,7 +131,7 @@ static int __init ls1x_intc_of_init(struct device_node *node,
if (!priv->domain) {
pr_err("ls1x-irq: cannot add IRQ domain\n");
err = -ENOMEM;
- goto out_iounmap;
+ goto out_dispose_irq;
}
err = irq_alloc_domain_generic_chips(priv->domain, 32, 2,
@@ -182,6 +182,8 @@ static int __init ls1x_intc_of_init(struct device_node *node,
out_free_domain:
irq_domain_remove(priv->domain);
+out_dispose_irq:
+ irq_dispose_mapping(parent_irq);
out_iounmap:
iounmap(priv->intc_base);
out_free_priv:
--
2.1.0
In the function davinci_cp_intc_do_init(), system resources
"config->reg.start", "davinci_cp_intc_base" and "irq_base"
were not released in a few error cases. Thus add jump targets
for the completion of the desired exception handling.
Fixes: 0fc3d74cf946 ("irqchip: davinci-cp-intc: move the driver to drivers/irqchip")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-davinci-cp-intc.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-davinci-cp-intc.c b/drivers/irqchip/irq-davinci-cp-intc.c
index 276da277..2c2e115 100644
--- a/drivers/irqchip/irq-davinci-cp-intc.c
+++ b/drivers/irqchip/irq-davinci-cp-intc.c
@@ -162,6 +162,7 @@ davinci_cp_intc_do_init(const struct davinci_cp_intc_config *config,
unsigned int num_regs = BITS_TO_LONGS(config->num_irqs);
int offset, irq_base;
void __iomem *req;
+ int ret;
req = request_mem_region(config->reg.start,
resource_size(&config->reg),
@@ -175,7 +176,8 @@ davinci_cp_intc_do_init(const struct davinci_cp_intc_config *config,
resource_size(&config->reg));
if (!davinci_cp_intc_base) {
pr_err("%s: unable to ioremap register range\n", __func__);
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_release;
}
davinci_cp_intc_write(0, DAVINCI_CP_INTC_GLOBAL_ENABLE);
@@ -210,7 +212,8 @@ davinci_cp_intc_do_init(const struct davinci_cp_intc_config *config,
if (irq_base < 0) {
pr_err("%s: unable to allocate interrupt descriptors: %d\n",
__func__, irq_base);
- return irq_base;
+ ret = irq_base;
+ goto err_iounmap;
}
davinci_cp_intc_irq_domain = irq_domain_add_legacy(
@@ -219,7 +222,8 @@ davinci_cp_intc_do_init(const struct davinci_cp_intc_config *config,
if (!davinci_cp_intc_irq_domain) {
pr_err("%s: unable to create an interrupt domain\n", __func__);
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_free_descs;
}
set_handle_irq(davinci_cp_intc_handle_irq);
@@ -228,6 +232,14 @@ davinci_cp_intc_do_init(const struct davinci_cp_intc_config *config,
davinci_cp_intc_write(1, DAVINCI_CP_INTC_GLOBAL_ENABLE);
return 0;
+
+err_free_descs:
+ irq_free_descs(irq_base, config->num_irqs);
+err_iounmap:
+ iounmap(davinci_cp_intc_base);
+err_release:
+ release_mem_region(config->reg.start, resource_size(&config->reg));
+ return ret;
}
int __init davinci_cp_intc_init(const struct davinci_cp_intc_config *config)
--
2.1.0
In the function ocelot_irq_init(), system resource "parent_irq"
was not released in a few error cases. Thus add jump target for
the completion of the desired exception handling.
Fixes: 19d99164480a ("irqchip: Add a driver for the Microsemi Ocelot controller")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-mscc-ocelot.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-mscc-ocelot.c b/drivers/irqchip/irq-mscc-ocelot.c
index 88143c0..e676ae2 100644
--- a/drivers/irqchip/irq-mscc-ocelot.c
+++ b/drivers/irqchip/irq-mscc-ocelot.c
@@ -73,7 +73,8 @@ static int __init ocelot_irq_init(struct device_node *node,
&irq_generic_chip_ops, NULL);
if (!domain) {
pr_err("%pOFn: unable to add irq domain\n", node);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_irq_dispose;
}
ret = irq_alloc_domain_generic_chips(domain, OCELOT_NR_IRQ, 1,
@@ -109,9 +110,10 @@ static int __init ocelot_irq_init(struct device_node *node,
err_gc_free:
irq_free_generic_chip(gc);
-
err_domain_remove:
irq_domain_remove(domain);
+err_irq_dispose:
+ irq_dispose_mapping(parent_irq);
return ret;
}
--
2.1.0
In the function digicolor_of_init(), system resources "reg_base" and
"digicolor_irq_domain" were not released in a few error cases. Thus
add jump targets for the completion of the desired exception handling.
Fixes: 8041dfbd31cf ("irqchip: Conexant CX92755 interrupts controller driver")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-digicolor.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-digicolor.c b/drivers/irqchip/irq-digicolor.c
index fc38d2d..18c6e77 100644
--- a/drivers/irqchip/irq-digicolor.c
+++ b/drivers/irqchip/irq-digicolor.c
@@ -89,7 +89,8 @@ static int __init digicolor_of_init(struct device_node *node,
ucregs = syscon_regmap_lookup_by_phandle(node, "syscon");
if (IS_ERR(ucregs)) {
pr_err("%pOF: unable to map UC registers\n", node);
- return PTR_ERR(ucregs);
+ ret = PTR_ERR(ucregs);
+ goto err_iounmap;
}
/* channel 1, regular IRQs */
regmap_write(ucregs, UC_IRQ_CONTROL, 1);
@@ -98,7 +99,8 @@ static int __init digicolor_of_init(struct device_node *node,
irq_domain_add_linear(node, 64, &irq_generic_chip_ops, NULL);
if (!digicolor_irq_domain) {
pr_err("%pOF: unable to create IRQ domain\n", node);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_iounmap;
}
ret = irq_alloc_domain_generic_chips(digicolor_irq_domain, 32, 1,
@@ -106,7 +108,7 @@ static int __init digicolor_of_init(struct device_node *node,
clr, 0, 0);
if (ret) {
pr_err("%pOF: unable to allocate IRQ gc\n", node);
- return ret;
+ goto err_domain_remove;
}
digicolor_set_gc(reg_base, 0, IC_INT0ENABLE_LO, IC_FLAG_CLEAR_LO);
@@ -115,5 +117,11 @@ static int __init digicolor_of_init(struct device_node *node,
set_handle_irq(digicolor_handle_irq);
return 0;
+
+err_domain_remove:
+ irq_domain_remove(digicolor_irq_domain);
+err_iounmap:
+ iounmap(reg_base);
+ return ret;
}
IRQCHIP_DECLARE(conexant_digicolor_ic, "cnxt,cx92755-ic", digicolor_of_init);
--
2.1.0
In the function dw_apb_ictl_init(), system resources "irq" and "domain"
were not released in a few error cases. Thus add jump targets for the
completion of the desired exception handling.
Fixes: 350d71b94fc9 ("irqchip: add DesignWare APB ICTL interrupt controller")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-dw-apb-ictl.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
index e4550e9..bc9b750 100644
--- a/drivers/irqchip/irq-dw-apb-ictl.c
+++ b/drivers/irqchip/irq-dw-apb-ictl.c
@@ -86,12 +86,13 @@ static int __init dw_apb_ictl_init(struct device_node *np,
ret = of_address_to_resource(np, 0, &r);
if (ret) {
pr_err("%pOF: unable to get resource\n", np);
- return ret;
+ goto err_irq_dispose;
}
if (!request_mem_region(r.start, resource_size(&r), np->full_name)) {
pr_err("%pOF: unable to request mem region\n", np);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_irq_dispose;
}
iobase = ioremap(r.start, resource_size(&r));
@@ -133,7 +134,7 @@ static int __init dw_apb_ictl_init(struct device_node *np,
IRQ_GC_INIT_MASK_CACHE);
if (ret) {
pr_err("%pOF: unable to alloc irq domain gc\n", np);
- goto err_unmap;
+ goto err_domain_remove;
}
for (i = 0; i < DIV_ROUND_UP(nrirqs, 32); i++) {
@@ -150,10 +151,14 @@ static int __init dw_apb_ictl_init(struct device_node *np,
return 0;
+err_domain_remove:
+ irq_domain_remove(domain);
err_unmap:
iounmap(iobase);
err_release:
release_mem_region(r.start, resource_size(&r));
+err_irq_dispose:
+ irq_dispose_mapping(irq);
return ret;
}
IRQCHIP_DECLARE(dw_apb_ictl,
--
2.1.0
In the function nvic_of_init(), system resource "nvic_base" and
"nvic_irq_domain" were not released in a few error cases. Thus add
jump targets for the completion of the desired exception handling.
Fixes: 292ec080491d ("irqchip: Add support for ARMv7-M NVIC")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-nvic.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-nvic.c b/drivers/irqchip/irq-nvic.c
index f747e22..cd17f5d 100644
--- a/drivers/irqchip/irq-nvic.c
+++ b/drivers/irqchip/irq-nvic.c
@@ -94,7 +94,8 @@ static int __init nvic_of_init(struct device_node *node,
if (!nvic_irq_domain) {
pr_warn("Failed to allocate irq domain\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_iounmap;
}
ret = irq_alloc_domain_generic_chips(nvic_irq_domain, 32, 1,
@@ -102,8 +103,7 @@ static int __init nvic_of_init(struct device_node *node,
clr, 0, IRQ_GC_INIT_MASK_CACHE);
if (ret) {
pr_warn("Failed to allocate irq chips\n");
- irq_domain_remove(nvic_irq_domain);
- return ret;
+ goto err_domain_remove;
}
for (i = 0; i < numbanks; ++i) {
@@ -129,5 +129,11 @@ static int __init nvic_of_init(struct device_node *node,
writel_relaxed(0, nvic_base + NVIC_IPR + i);
return 0;
+
+err_domain_remove:
+ irq_domain_remove(nvic_irq_domain);
+err_iounmap:
+ iounmap(nvic_base);
+ return ret;
}
IRQCHIP_DECLARE(armv7m_nvic, "arm,armv7m-nvic", nvic_of_init);
--
2.1.0
In the function csky_mpintc_init(), system resources "__trigger",
"INTCG_base" and "root_domain" were not released in a few error
cases. Thus add jump targets for the completion of the desired
exception handling. By the way, do some coding-style cleanups
suggested by Markus.
Fixes: d8a5f5f79122 ("irqchip: add C-SKY SMP interrupt controller")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-csky-mpintc.c | 34 +++++++++++++++++++++++++---------
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/irqchip/irq-csky-mpintc.c b/drivers/irqchip/irq-csky-mpintc.c
index a1534ed..d7edc28 100644
--- a/drivers/irqchip/irq-csky-mpintc.c
+++ b/drivers/irqchip/irq-csky-mpintc.c
@@ -241,14 +241,16 @@ csky_mpintc_init(struct device_node *node, struct device_node *parent)
nr_irq = INTC_IRQS;
__trigger = kcalloc(nr_irq, sizeof(unsigned long), GFP_KERNEL);
- if (__trigger == NULL)
+ if (!__trigger)
return -ENXIO;
- if (INTCG_base == NULL) {
+ if (!INTCG_base) {
INTCG_base = ioremap(mfcr("cr<31, 14>"),
- INTCL_SIZE*nr_cpu_ids + INTCG_SIZE);
- if (INTCG_base == NULL)
- return -EIO;
+ INTCL_SIZE * nr_cpu_ids + INTCG_SIZE);
+ if (!INTCG_base) {
+ ret = -EIO;
+ goto err_free;
+ }
INTCL_base = INTCG_base + INTCG_SIZE;
@@ -257,8 +259,10 @@ csky_mpintc_init(struct device_node *node, struct device_node *parent)
root_domain = irq_domain_add_linear(node, nr_irq, &csky_irqdomain_ops,
NULL);
- if (!root_domain)
- return -ENXIO;
+ if (!root_domain) {
+ ret = -ENXIO;
+ goto err_iounmap;
+ }
/* for every cpu */
for_each_present_cpu(cpu) {
@@ -270,12 +274,24 @@ csky_mpintc_init(struct device_node *node, struct device_node *parent)
#ifdef CONFIG_SMP
ipi_irq = irq_create_mapping(root_domain, IPI_IRQ);
- if (!ipi_irq)
- return -EIO;
+ if (!ipi_irq) {
+ ret = -EIO;
+ goto err_domain_remove;
+ }
set_send_ipi(&csky_mpintc_send_ipi, ipi_irq);
#endif
return 0;
+
+#ifdef CONFIG_SMP
+err_domain_remove:
+ irq_domain_remove(root_domain);
+#endif
+err_iounmap:
+ iounmap(INTCG_base);
+err_free:
+ kfree(__trigger);
+ return ret;
}
IRQCHIP_DECLARE(csky_mpintc, "csky,mpintc", csky_mpintc_init);
--
2.1.0
In the function ck_intc_init_comm(), system resources "reg_base" and
"root_domain" were not released in a few error cases. Thus add jump
targets for the completion of the desired exception handling.
Fixes: edff1b4835b7 ("irqchip: add C-SKY APB bus interrupt controller")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-csky-apb-intc.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-csky-apb-intc.c b/drivers/irqchip/irq-csky-apb-intc.c
index 5a2ec43..11a35eb 100644
--- a/drivers/irqchip/irq-csky-apb-intc.c
+++ b/drivers/irqchip/irq-csky-apb-intc.c
@@ -118,7 +118,8 @@ ck_intc_init_comm(struct device_node *node, struct device_node *parent)
&irq_generic_chip_ops, NULL);
if (!root_domain) {
pr_err("C-SKY Intc irq_domain_add failed.\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_iounmap;
}
ret = irq_alloc_domain_generic_chips(root_domain, 32, 1,
@@ -126,10 +127,17 @@ ck_intc_init_comm(struct device_node *node, struct device_node *parent)
IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0);
if (ret) {
pr_err("C-SKY Intc irq_alloc_gc failed.\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_domain_remove;
}
return 0;
+
+err_domain_remove:
+ irq_domain_remove(root_domain);
+err_iounmap:
+ iounmap(reg_base);
+ return ret;
}
static inline bool handle_irq_perbit(struct pt_regs *regs, u32 hwirq,
--
2.1.0
In the function s3c_init_intc_of(), system resource "reg_base", "domain"
and "intc" were not released in a few error cases. Thus add jump targets
for the completion of the desired exception handling.
Fixes: f0774d41da0e ("irqchip: s3c24xx: add devicetree support")
Signed-off-by: Tiezhu Yang <[email protected]>
---
drivers/irqchip/irq-s3c24xx.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/irqchip/irq-s3c24xx.c b/drivers/irqchip/irq-s3c24xx.c
index d2031fe..166c27b 100644
--- a/drivers/irqchip/irq-s3c24xx.c
+++ b/drivers/irqchip/irq-s3c24xx.c
@@ -1227,7 +1227,7 @@ static int __init s3c_init_intc_of(struct device_node *np,
struct s3c24xx_irq_of_ctrl *ctrl;
struct irq_domain *domain;
void __iomem *reg_base;
- int i;
+ int i, ret;
reg_base = of_iomap(np, 0);
if (!reg_base) {
@@ -1239,7 +1239,8 @@ static int __init s3c_init_intc_of(struct device_node *np,
&s3c24xx_irq_ops_of, NULL);
if (!domain) {
pr_err("irq: could not create irq-domain\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_iounmap;
}
for (i = 0; i < num_ctrl; i++) {
@@ -1248,15 +1249,17 @@ static int __init s3c_init_intc_of(struct device_node *np,
pr_debug("irq: found controller %s\n", ctrl->name);
intc = kzalloc(sizeof(struct s3c_irq_intc), GFP_KERNEL);
- if (!intc)
- return -ENOMEM;
+ if (!intc) {
+ ret = -ENOMEM;
+ goto out_domain_remove;
+ }
intc->domain = domain;
intc->irqs = kcalloc(32, sizeof(struct s3c_irq_data),
GFP_KERNEL);
if (!intc->irqs) {
- kfree(intc);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out_free;
}
if (ctrl->parent) {
@@ -1285,6 +1288,14 @@ static int __init s3c_init_intc_of(struct device_node *np,
set_handle_irq(s3c24xx_handle_irq);
return 0;
+
+out_free:
+ kfree(intc);
+out_domain_remove:
+ irq_domain_remove(domain);
+out_iounmap:
+ iounmap(reg_base);
+ return ret;
}
static struct s3c24xx_irq_of_ctrl s3c2410_ctrl[] = {
--
2.1.0
> exception handling. By the way, do some coding-style cleanups
I propose to consider another bit of fine-tuning.
…
> +++ b/drivers/irqchip/irq-csky-mpintc.c
…
> @@ -270,12 +274,24 @@ csky_mpintc_init(struct device_node *node, struct device_node *parent)
>
> #ifdef CONFIG_SMP
> ipi_irq = irq_create_mapping(root_domain, IPI_IRQ);
> - if (!ipi_irq)
> - return -EIO;
> + if (!ipi_irq) {
> + ret = -EIO;
> + goto err_domain_remove;
How do you think about to use the following source code variant
at this place?
+ irq_domain_remove(root_domain);
+ ret = -EIO;
+ goto err_iounmap;
Would you like to avoid the repetition of the check “#ifdef CONFIG_SMP”?
Regards,
Markus
…
> +++ b/drivers/irqchip/irq-davinci-aintc.c
> @@ -96,7 +96,7 @@ void __init davinci_aintc_init(const struct davinci_aintc_config *config)
> resource_size(&config->reg));
> if (!davinci_aintc_base) {
> pr_err("%s: unable to ioremap register range\n", __func__);
> - return;
> + goto err_release;
> }
…
Can it help to return any error codes?
Would you like to reconsider the function return type?
Regards,
Markus
> … were not released in a few error cases. …
Another small wording adjustment:
… in two error cases. …
…
> +++ b/drivers/irqchip/irq-csky-apb-intc.c
…
> @@ -126,10 +127,17 @@ ck_intc_init_comm(struct device_node *node, struct device_node *parent)
…
> +err_iounmap:
> + iounmap(reg_base);
> + return ret;
> }
…
How do you think about to use the statement “return -ENOMEM;”?
Can the local variable “ret” be omitted in this function implementation?
Regards,
Markus
> In the function omap_init_irq_of(), system resource "omap_irq_base"
> was not released in the error case, fix it.
Another small wording adjustment:
… in an error case. Thus add a call of the function “iounmap”
in the if branch.
Regards,
Markus
On 07/01/2020 04:15 PM, Markus Elfring wrote:
> …
>> +++ b/drivers/irqchip/irq-davinci-aintc.c
>> @@ -96,7 +96,7 @@ void __init davinci_aintc_init(const struct davinci_aintc_config *config)
>> resource_size(&config->reg));
>> if (!davinci_aintc_base) {
>> pr_err("%s: unable to ioremap register range\n", __func__);
>> - return;
>> + goto err_release;
>> }
> …
>
> Can it help to return any error codes?
> Would you like to reconsider the function return type?
No, the initial aim of this patch is just to fix the potential resource
leaks,
so I think maybe no need to consider the function return type now.
>
> Regards,
> Markus
On 07/01/2020 03:49 PM, Markus Elfring wrote:
>> exception handling. By the way, do some coding-style cleanups
> I propose to consider another bit of fine-tuning.
>
>
> …
>> +++ b/drivers/irqchip/irq-csky-mpintc.c
> …
>> @@ -270,12 +274,24 @@ csky_mpintc_init(struct device_node *node, struct device_node *parent)
>>
>> #ifdef CONFIG_SMP
>> ipi_irq = irq_create_mapping(root_domain, IPI_IRQ);
>> - if (!ipi_irq)
>> - return -EIO;
>> + if (!ipi_irq) {
>> + ret = -EIO;
>> + goto err_domain_remove;
> How do you think about to use the following source code variant
> at this place?
>
> + irq_domain_remove(root_domain);
> + ret = -EIO;
> + goto err_iounmap;
>
>
> Would you like to avoid the repetition of the check “#ifdef CONFIG_SMP”?
OK, thank you, it looks good to me, I will do it in v5.
>
> Regards,
> Markus
> In the function riscv_intc_init(), system resource "intc_domain"
> was not released in the error case, fix it.
Another small wording adjustment:
… in an error case. Thus add a call of the function “irq_domain_remove”
in the if branch.
Regards,
Markus
On 07/01/2020 04:40 PM, Markus Elfring wrote:
>> … were not released in a few error cases. …
> Another small wording adjustment:
> … in two error cases. …
OK
>
>
> …
>> +++ b/drivers/irqchip/irq-csky-apb-intc.c
> …
>> @@ -126,10 +127,17 @@ ck_intc_init_comm(struct device_node *node, struct device_node *parent)
> …
>> +err_iounmap:
>> + iounmap(reg_base);
>> + return ret;
>> }
> …
>
> How do you think about to use the statement “return -ENOMEM;”?
OK
> Can the local variable “ret” be omitted in this function implementation?
If remove the local variable "ret", it will look like this:
diff --git a/drivers/irqchip/irq-csky-apb-intc.c
b/drivers/irqchip/irq-csky-apb-intc.c
index 5a2ec43..7e56657 100644
--- a/drivers/irqchip/irq-csky-apb-intc.c
+++ b/drivers/irqchip/irq-csky-apb-intc.c
@@ -101,8 +101,6 @@ static inline void setup_irq_channel(u32 magic, void
__iomem *reg_addr)
static int __init
ck_intc_init_comm(struct device_node *node, struct device_node *parent)
{
- int ret;
-
if (parent) {
pr_err("C-SKY Intc not a root irq controller\n");
return -EINVAL;
@@ -118,18 +116,23 @@ ck_intc_init_comm(struct device_node *node, struct
device_node *parent)
&irq_generic_chip_ops, NULL);
if (!root_domain) {
pr_err("C-SKY Intc irq_domain_add failed.\n");
- return -ENOMEM;
+ goto err_iounmap;
}
- ret = irq_alloc_domain_generic_chips(root_domain, 32, 1,
+ if (irq_alloc_domain_generic_chips(root_domain, 32, 1,
"csky_intc", handle_level_irq,
- IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0);
- if (ret) {
+ IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0)) {
pr_err("C-SKY Intc irq_alloc_gc failed.\n");
- return -ENOMEM;
+ goto err_domain_remove;
}
return 0;
+
+err_domain_remove:
+ irq_domain_remove(root_domain);
+err_iounmap:
+ iounmap(reg_base);
+ return -ENOMEM;
}
>
> Regards,
> Markus
On 07/01/2020 05:14 PM, Markus Elfring wrote:
>> In the function omap_init_irq_of(), system resource "omap_irq_base"
>> was not released in the error case, fix it.
> Another small wording adjustment:
> … in an error case. Thus add a call of the function “iounmap”
> in the if branch.
OK, thank you, I will do it in v5 of this patch #11 and next patch #12.
>
> Regards,
> Markus
> In the function xilinx_intc_of_init(), system resource "irqc->root_domain"
> was not released in the error case. Thus add jump target for the completion
> of the desired exception handling.
Another small wording adjustment:
… Thus add a jump target …
…
> +++ b/drivers/irqchip/irq-xilinx-intc.c
…
> @@ -250,6 +250,8 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
>
> return 0;
>
> +error_domain_remove:
> + irq_domain_remove(irqc->root_domain);
> error:
> iounmap(irqc->base);
…
Can labels like “remove_irq_domain” and “unmap_io” be nicer?
Regards,
Markus
On Wed, Jul 1, 2020 at 7:47 AM Tiezhu Yang <[email protected]> wrote:
>
> In the function riscv_intc_init(), system resource "intc_domain"
> was not released in the error case, fix it.
>
> Fixes: 6b7ce8927b5a ("irqchip: RISC-V per-HART local interrupt controller driver")
> Signed-off-by: Tiezhu Yang <[email protected]>
> ---
> drivers/irqchip/irq-riscv-intc.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> index a6f97fa..8d6286c 100644
> --- a/drivers/irqchip/irq-riscv-intc.c
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -122,6 +122,7 @@ static int __init riscv_intc_init(struct device_node *node,
> rc = set_handle_irq(&riscv_intc_irq);
> if (rc) {
> pr_err("failed to set irq handler\n");
> + irq_domain_remove(intc_domain);
> return rc;
> }
>
> --
> 2.1.0
>
Looks good to me.
Reviewed-by: Anup Patel <[email protected]>
Regards,
Anup
On 07/01/2020 05:42 PM, Markus Elfring wrote:
>> In the function xilinx_intc_of_init(), system resource "irqc->root_domain"
>> was not released in the error case. Thus add jump target for the completion
>> of the desired exception handling.
> Another small wording adjustment:
> … Thus add a jump target …
OK
>
>
> …
>> +++ b/drivers/irqchip/irq-xilinx-intc.c
> …
>> @@ -250,6 +250,8 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
>>
>> return 0;
>>
>> +error_domain_remove:
>> + irq_domain_remove(irqc->root_domain);
>> error:
>> iounmap(irqc->base);
> …
>
> Can labels like “remove_irq_domain” and “unmap_io” be nicer?
Thank you, I will use "err_domain_remove" and "err_iounmap"
to keep consistence with other patches.
>
> Regards,
> Markus
> If remove the local variable "ret", it will look like this:
…
> +++ b/drivers/irqchip/irq-csky-apb-intc.c
…
> @@ -118,18 +116,23 @@ ck_intc_init_comm(struct device_node *node, struct device_node *parent)
…
> - ret = irq_alloc_domain_generic_chips(root_domain, 32, 1,
> + if (irq_alloc_domain_generic_chips(root_domain, 32, 1,
> "csky_intc", handle_level_irq,
> - IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0);
> - if (ret) {
> + IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0)) {
> pr_err("C-SKY Intc irq_alloc_gc failed.\n");
…
I suggest to recheck the parameter alignment for such a function call.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=7c30b859a947535f2213277e827d7ac7dcff9c84#n93
Regards,
Markus
On 07/01/2020 09:04 PM, Markus Elfring wrote:
>> If remove the local variable "ret", it will look like this:
> …
>> +++ b/drivers/irqchip/irq-csky-apb-intc.c
> …
>> @@ -118,18 +116,23 @@ ck_intc_init_comm(struct device_node *node, struct device_node *parent)
> …
>> - ret = irq_alloc_domain_generic_chips(root_domain, 32, 1,
>> + if (irq_alloc_domain_generic_chips(root_domain, 32, 1,
>> "csky_intc", handle_level_irq,
>> - IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0);
>> - if (ret) {
>> + IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0)) {
>> pr_err("C-SKY Intc irq_alloc_gc failed.\n");
> …
>
> I suggest to recheck the parameter alignment for such a function call.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=7c30b859a947535f2213277e827d7ac7dcff9c84#n93
OK, thank you, like this:
- ret = irq_alloc_domain_generic_chips(root_domain, 32, 1,
- "csky_intc", handle_level_irq,
- IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0);
- if (ret) {
+ if (irq_alloc_domain_generic_chips(root_domain, 32, 1,
+ "csky_intc", handle_level_irq,
+ IRQ_NOREQUEST | IRQ_NOPROBE |
IRQ_NOAUTOEN, 0, 0)) {
pr_err("C-SKY Intc irq_alloc_gc failed.\n");
- return -ENOMEM;
+ goto err_domain_remove;
}
>
> Regards,
> Markus
>>> +++ b/drivers/irqchip/irq-csky-apb-intc.c
…
>> I suggest to recheck the parameter alignment for such a function call.
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=7c30b859a947535f2213277e827d7ac7dcff9c84#n93
>
> OK, thank you, like this:
>
> - ret = irq_alloc_domain_generic_chips(root_domain, 32, 1,
> - "csky_intc", handle_level_irq,
> - IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0);
> - if (ret) {
> + if (irq_alloc_domain_generic_chips(root_domain, 32, 1,
> + "csky_intc", handle_level_irq,
> + IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0)) {
> pr_err("C-SKY Intc irq_alloc_gc failed.\n");
…
Would you like to use also horizontal tab characters for the corresponding indentation?
Regards,
Markus
>>> +++ b/drivers/irqchip/irq-csky-apb-intc.c
…
>> I suggest to recheck the parameter alignment for such a function call.
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=7c30b859a947535f2213277e827d7ac7dcff9c84#n93
>
> OK, thank you, like this:
>
> - ret = irq_alloc_domain_generic_chips(root_domain, 32, 1,
> - "csky_intc", handle_level_irq,
> - IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0);
> - if (ret) {
> + if (irq_alloc_domain_generic_chips(root_domain, 32, 1,
> + "csky_intc", handle_level_irq,
> + IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0)) {
> pr_err("C-SKY Intc irq_alloc_gc failed.\n");
…
Would you like to use also horizontal tab characters for the corresponding indentation?
Regards,
Markus
On 07/02/2020 03:19 PM, Markus Elfring wrote:
>>>> +++ b/drivers/irqchip/irq-csky-apb-intc.c
> …
>>> I suggest to recheck the parameter alignment for such a function call.
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=7c30b859a947535f2213277e827d7ac7dcff9c84#n93
>> OK, thank you, like this:
>>
>> - ret = irq_alloc_domain_generic_chips(root_domain, 32, 1,
>> - "csky_intc", handle_level_irq,
>> - IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0);
>> - if (ret) {
>> + if (irq_alloc_domain_generic_chips(root_domain, 32, 1,
>> + "csky_intc", handle_level_irq,
>> + IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0)) {
>> pr_err("C-SKY Intc irq_alloc_gc failed.\n");
> …
>
> Would you like to use also horizontal tab characters for the corresponding indentation?
Sorry, I do not quite understanding what you mean, maybe like this?
if (irq_alloc_domain_generic_chips(root_domain, 32, 1,
"csky_intc", handle_level_irq,
IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0)) {
pr_err("C-SKY Intc irq_alloc_gc failed.\n");
goto err_domain_remove;
}
>
> Regards,
> Markus
On 07/02/2020 04:05 PM, Tiezhu Yang wrote:
> On 07/02/2020 03:19 PM, Markus Elfring wrote:
>>>>> +++ b/drivers/irqchip/irq-csky-apb-intc.c
>> …
>>>> I suggest to recheck the parameter alignment for such a function call.
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=7c30b859a947535f2213277e827d7ac7dcff9c84#n93
>>>>
>>> OK, thank you, like this:
>>>
>>> - ret = irq_alloc_domain_generic_chips(root_domain, 32, 1,
>>> - "csky_intc", handle_level_irq,
>>> - IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN,
>>> 0, 0);
>>> - if (ret) {
>>> + if (irq_alloc_domain_generic_chips(root_domain, 32, 1,
>>> + "csky_intc",
>>> handle_level_irq,
>>> + IRQ_NOREQUEST |
>>> IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0)) {
>>> pr_err("C-SKY Intc irq_alloc_gc failed.\n");
>> …
>>
>> Would you like to use also horizontal tab characters for the
>> corresponding indentation?
>
> Sorry, I do not quite understanding what you mean, maybe like this?
>
> if (irq_alloc_domain_generic_chips(root_domain, 32, 1,
> "csky_intc", handle_level_irq,
> IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN, 0, 0)) {
> pr_err("C-SKY Intc irq_alloc_gc failed.\n");
> goto err_domain_remove;
> }
>
Hi Markus,
Thank you very much for your review and suggestion.
Maybe still use "ret" variable is better, the following is another comment
which is only sent to me:
[I think that if one of the return values comes from a function call, then
you should use the value from the function call even if it is currently
always -ENOMEM. The return value of the function call could perhaps
change in the future.
In any case, ret = foo(); if (ret) is very common in kernel code, and
there is no reason not to do it, especially when the function call takes
up a lot of space.]
Let us keep it as it is to make the code clear and to avoid the
alignment issue:
ret = foo();
if (ret) {
ret = -ENOMEM;
goto ...
}
Thanks,
Tiezhu
>>
>> Regards,
>> Markus
>
>>>>>> +++ b/drivers/irqchip/irq-csky-apb-intc.c
…
> Let us keep it as it is
I propose to reconsider also this view.
> to make the code clear and to avoid the alignment issue:
>
> ret = foo();
> if (ret) {
> ret = -ENOMEM;
How do you think about to delete this assignment if you would like to
reuse the return value from a call of the function “irq_alloc_domain_generic_chips”?
> goto ...
> }
Please apply a known script also for the purpose to achieve consistent indentation.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=cd77006e01b3198c75fb7819b3d0ff89709539bb#n3301
Regards,
Markus
On 07/02/2020 08:24 PM, Markus Elfring wrote:
>>>>>>> +++ b/drivers/irqchip/irq-csky-apb-intc.c
> …
>> Let us keep it as it is
> I propose to reconsider also this view.
>
>
>> to make the code clear and to avoid the alignment issue:
>>
>> ret = foo();
>> if (ret) {
>> ret = -ENOMEM;
> How do you think about to delete this assignment if you would like to
> reuse the return value from a call of the function “irq_alloc_domain_generic_chips”?
OK, looks good to me, thank you.
>
>
>> goto ...
>> }
>
> Please apply a known script also for the purpose to achieve consistent indentation.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=cd77006e01b3198c75fb7819b3d0ff89709539bb#n3301
OK
>
> Regards,
> Markus
On Wed, Jul 01, 2020 at 05:35:35PM +0800, Tiezhu Yang wrote:
> On 07/01/2020 04:40 PM, Markus Elfring wrote:
> > > … were not released in a few error cases. …
> > Another small wording adjustment:
> > … in two error cases. …
>
> OK
A lot of people have told Marcus over and over not to comment on commit
messages. Greg has an automatic bot to respond to him.
https://lkml.org/lkml/2020/6/13/25
Marcus ignores us when we ask him to stop. Some new developers have
emailed me privately that they were confused and discouraged with his
feedback because they assumed he was a senior developer or something.
regards,
dan carpenter