[PATCH v4 00/14] irqchip: Fix potential resource leaks

[PATCH v4 00/14] irqchip: Fix potential resource leaks

[Tiezhu Yang]

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

[PATCH v4 01/14] irqchip/ath79-misc: Fix potential resource leaks

[Tiezhu Yang]

In the function ath79_misc_intc_of_init(), system resources "irq" and
"base" were not released in a few error cases. Thus add jump targets
for the completion of the desired exception handling.

Fixes: 07ba4b061a79 ("irqchip/ath79-misc: Move the MISC driver from arch/mips/ath79/")
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 drivers/irqchip/irq-ath79-misc.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-ath79-misc.c b/drivers/irqchip/irq-ath79-misc.c
index 3d641bb..53e0c50 100644
--- a/drivers/irqchip/irq-ath79-misc.c
+++ b/drivers/irqchip/irq-ath79-misc.c
@@ -133,7 +133,7 @@ static int __init ath79_misc_intc_of_init(
 {
 	struct irq_domain *domain;
 	void __iomem *base;
-	int irq;
+	int irq, ret;
 
 	irq = irq_of_parse_and_map(node, 0);
 	if (!irq) {
@@ -144,18 +144,26 @@ static int __init ath79_misc_intc_of_init(
 	base = of_iomap(node, 0);
 	if (!base) {
 		pr_err("Failed to get MISC IRQ registers\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_irq_dispose;
 	}
 
 	domain = irq_domain_add_linear(node, ATH79_MISC_IRQ_COUNT,
 				&misc_irq_domain_ops, base);
 	if (!domain) {
 		pr_err("Failed to add MISC irqdomain\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_iounmap;
 	}
 
 	ath79_misc_intc_domain_init(domain, irq);
 	return 0;
+
+err_iounmap:
+	iounmap(base);
+err_irq_dispose:
+	irq_dispose_mapping(irq);
+	return ret;
 }
 
 static int __init ar7100_misc_intc_of_init(
-- 
2.1.0

[PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Markus Elfring]

> … 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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Markus Elfring]

> … 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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Tiezhu Yang]

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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Tiezhu Yang]

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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Markus Elfring]

> 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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Markus Elfring]

PiBJZiByZW1vdmUgdGhlIGxvY2FsIHZhcmlhYmxlICJyZXQiLMKgIGl0IHdpbGwgbG9vayBsaWtl
IHRoaXM6DQrigKYNCj4gKysrIGIvZHJpdmVycy9pcnFjaGlwL2lycS1jc2t5LWFwYi1pbnRjLmMN
CuKApg0KPiBAQCAtMTE4LDE4ICsxMTYsMjMgQEAgY2tfaW50Y19pbml0X2NvbW0oc3RydWN0IGRl
dmljZV9ub2RlICpub2RlLCBzdHJ1Y3QgZGV2aWNlX25vZGUgKnBhcmVudCkNCuKApg0KPiAtwqDC
oMKgwqDCoMKgIHJldCA9IGlycV9hbGxvY19kb21haW5fZ2VuZXJpY19jaGlwcyhyb290X2RvbWFp
biwgMzIsIDEsDQo+ICvCoMKgwqDCoMKgwqAgaWYgKGlycV9hbGxvY19kb21haW5fZ2VuZXJpY19j
aGlwcyhyb290X2RvbWFpbiwgMzIsIDEsDQo+IMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqAgImNza3lfaW50YyIsIGhhbmRsZV9sZXZlbF9pcnEsDQo+IC3CoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBJUlFfTk9SRVFVRVNUIHwg
SVJRX05PUFJPQkUgfCBJUlFfTk9BVVRPRU4sIDAsIDApOw0KPiAtwqDCoMKgwqDCoMKgIGlmIChy
ZXQpIHsNCj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIElS
UV9OT1JFUVVFU1QgfCBJUlFfTk9QUk9CRSB8IElSUV9OT0FVVE9FTiwgMCwgMCkpIHsNCj4gwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIHByX2VycigiQy1TS1kgSW50YyBpcnFfYWxsb2Nf
Z2MgZmFpbGVkLlxuIik7DQrigKYNCg0KSSBzdWdnZXN0IHRvIHJlY2hlY2sgdGhlIHBhcmFtZXRl
ciBhbGlnbm1lbnQgZm9yIHN1Y2ggYSBmdW5jdGlvbiBjYWxsLg0KaHR0cHM6Ly9naXQua2VybmVs
Lm9yZy9wdWIvc2NtL2xpbnV4L2tlcm5lbC9naXQvdG9ydmFsZHMvbGludXguZ2l0L3RyZWUvRG9j
dW1lbnRhdGlvbi9wcm9jZXNzL2NvZGluZy1zdHlsZS5yc3Q/aWQ9N2MzMGI4NTlhOTQ3NTM1ZjIy
MTMyNzdlODI3ZDdhYzdkY2ZmOWM4NCNuOTMNCg0KUmVnYXJkcywNCk1hcmt1cw0K

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Tiezhu Yang]

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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Tiezhu Yang]

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|30b859a947535f2213277e827d7ac7dcff9c84#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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Markus Elfring]

>>> +++ 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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Markus Elfring]

Pj4+ICsrKyBiL2RyaXZlcnMvaXJxY2hpcC9pcnEtY3NreS1hcGItaW50Yy5jDQrigKYNCj4+IEkg
c3VnZ2VzdCB0byByZWNoZWNrIHRoZSBwYXJhbWV0ZXIgYWxpZ25tZW50IGZvciBzdWNoIGEgZnVu
Y3Rpb24gY2FsbC4NCj4+IGh0dHBzOi8vZ2l0Lmtlcm5lbC5vcmcvcHViL3NjbS9saW51eC9rZXJu
ZWwvZ2l0L3RvcnZhbGRzL2xpbnV4LmdpdC90cmVlL0RvY3VtZW50YXRpb24vcHJvY2Vzcy9jb2Rp
bmctc3R5bGUucnN0P2lkPTdjMzBiODU5YTk0NzUzNWYyMjEzMjc3ZTgyN2Q3YWM3ZGNmZjljODQj
bjkzDQo+IA0KPiBPSywgdGhhbmsgeW91LCBsaWtlIHRoaXM6DQo+IA0KPiAtwqDCoMKgwqDCoMKg
IHJldCA9IGlycV9hbGxvY19kb21haW5fZ2VuZXJpY19jaGlwcyhyb290X2RvbWFpbiwgMzIsIDEs
DQo+IC3CoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCAiY3NreV9p
bnRjIiwgaGFuZGxlX2xldmVsX2lycSwNCj4gLcKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgIElSUV9OT1JFUVVFU1QgfCBJUlFfTk9QUk9CRSB8IElSUV9OT0FVVE9F
TiwgMCwgMCk7DQo+IC3CoMKgwqDCoMKgwqAgaWYgKHJldCkgew0KPiArwqDCoMKgwqDCoMKgIGlm
IChpcnFfYWxsb2NfZG9tYWluX2dlbmVyaWNfY2hpcHMocm9vdF9kb21haW4sIDMyLCAxLA0KPiAr
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCAiY3NreV9pbnRjIiwgaGFuZGxlX2xldmVsX2lycSwN
Cj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgSVJRX05PUkVRVUVTVCB8IElSUV9OT1BST0JF
IHwgSVJRX05PQVVUT0VOLCAwLCAwKSkgew0KPiDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqAgcHJfZXJyKCJDLVNLWSBJbnRjIGlycV9hbGxvY19nYyBmYWlsZWQuXG4iKTsNCuKApg0KDQpX
b3VsZCB5b3UgbGlrZSB0byB1c2UgYWxzbyBob3Jpem9udGFsIHRhYiBjaGFyYWN0ZXJzIGZvciB0
aGUgY29ycmVzcG9uZGluZyBpbmRlbnRhdGlvbj8NCg0KUmVnYXJkcywNCk1hcmt1cw0K

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Markus Elfring]

>>> +++ 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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Markus Elfring]

Pj4+ICsrKyBiL2RyaXZlcnMvaXJxY2hpcC9pcnEtY3NreS1hcGItaW50Yy5jDQrigKYNCj4+IEkg
c3VnZ2VzdCB0byByZWNoZWNrIHRoZSBwYXJhbWV0ZXIgYWxpZ25tZW50IGZvciBzdWNoIGEgZnVu
Y3Rpb24gY2FsbC4NCj4+IGh0dHBzOi8vZ2l0Lmtlcm5lbC5vcmcvcHViL3NjbS9saW51eC9rZXJu
ZWwvZ2l0L3RvcnZhbGRzL2xpbnV4LmdpdC90cmVlL0RvY3VtZW50YXRpb24vcHJvY2Vzcy9jb2Rp
bmctc3R5bGUucnN0P2lkPTdjMzBiODU5YTk0NzUzNWYyMjEzMjc3ZTgyN2Q3YWM3ZGNmZjljODQj
bjkzDQo+IA0KPiBPSywgdGhhbmsgeW91LCBsaWtlIHRoaXM6DQo+IA0KPiAtwqDCoMKgwqDCoMKg
IHJldCA9IGlycV9hbGxvY19kb21haW5fZ2VuZXJpY19jaGlwcyhyb290X2RvbWFpbiwgMzIsIDEs
DQo+IC3CoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCAiY3NreV9p
bnRjIiwgaGFuZGxlX2xldmVsX2lycSwNCj4gLcKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgIElSUV9OT1JFUVVFU1QgfCBJUlFfTk9QUk9CRSB8IElSUV9OT0FVVE9F
TiwgMCwgMCk7DQo+IC3CoMKgwqDCoMKgwqAgaWYgKHJldCkgew0KPiArwqDCoMKgwqDCoMKgIGlm
IChpcnFfYWxsb2NfZG9tYWluX2dlbmVyaWNfY2hpcHMocm9vdF9kb21haW4sIDMyLCAxLA0KPiAr
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCAiY3NreV9pbnRjIiwgaGFuZGxlX2xldmVsX2lycSwN
Cj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgSVJRX05PUkVRVUVTVCB8IElSUV9OT1BST0JF
IHwgSVJRX05PQVVUT0VOLCAwLCAwKSkgew0KPiDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqAgcHJfZXJyKCJDLVNLWSBJbnRjIGlycV9hbGxvY19nYyBmYWlsZWQuXG4iKTsNCuKApg0KDQpX
b3VsZCB5b3UgbGlrZSB0byB1c2UgYWxzbyBob3Jpem9udGFsIHRhYiBjaGFyYWN0ZXJzIGZvciB0
aGUgY29ycmVzcG9uZGluZyBpbmRlbnRhdGlvbj8NCg0KUmVnYXJkcywNCk1hcmt1cw0K

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Tiezhu Yang]

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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Tiezhu Yang]

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|30b859a947535f2213277e827d7ac7dcff9c84#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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Tiezhu Yang]

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
>

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Tiezhu Yang]

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|30b859a947535f2213277e827d7ac7dcff9c84#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
>

Re: [v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Markus Elfring]

>>>>>> +++ 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

Re: [v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Markus Elfring]

>>>>>> +++ 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

Re: [v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Tiezhu Yang]

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

Re: [v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Tiezhu Yang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 782 bytes --]

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Í77006e01b3198c75fb7819b3d0ff89709539bb#n3301

OK

>
> Regards,
> Markus

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Dan Carpenter]

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

Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks

[Dan Carpenter]

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

[PATCH v4 03/14] irqchip/csky-mpintc: Fix potential resource leaks

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

Re: [PATCH v4 03/14] irqchip/csky-mpintc: Fix potential resource leaks

[Markus Elfring]

> 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

Re: [PATCH v4 03/14] irqchip/csky-mpintc: Fix potential resource leaks

[Markus Elfring]

> 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

Re: [PATCH v4 03/14] irqchip/csky-mpintc: Fix potential resource leaks

[Tiezhu Yang]

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

Re: [PATCH v4 03/14] irqchip/csky-mpintc: Fix potential resource leaks

[Tiezhu Yang]

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

[PATCH v4 04/14] irqchip/davinci-aintc: Fix potential resource leaks

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

Re: [PATCH v4 04/14] irqchip/davinci-aintc: Fix potential resource leaks

[Markus Elfring]

…
> +++ 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

Re: [PATCH v4 04/14] irqchip/davinci-aintc: Fix potential resource leaks

[Markus Elfring]

…
> +++ 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

Re: [PATCH v4 04/14] irqchip/davinci-aintc: Fix potential resource leaks

[Tiezhu Yang]

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

Re: [PATCH v4 04/14] irqchip/davinci-aintc: Fix potential resource leaks

[Tiezhu Yang]

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

[PATCH v4 05/14] irqchip/davinci-cp-intc: Fix potential resource leaks

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

[PATCH v4 06/14] irqchip/digicolor: Fix potential resource leaks

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

[PATCH v4 07/14] irqchip/dw-apb-ictl: Fix potential resource leaks

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

[PATCH v4 08/14] irqchip/ls1x: Fix potential resource leaks

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

[PATCH v4 09/14] irqchip/mscc-ocelot: Fix potential resource leaks

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

[PATCH v4 10/14] irqchip/nvic: Fix potential resource leaks

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

[PATCH v4 11/14] irqchip/omap-intc: Fix potential resource leak

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

Re: [PATCH v4 11/14] irqchip/omap-intc: Fix potential resource leak

[Markus Elfring]

> 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

Re: [PATCH v4 11/14] irqchip/omap-intc: Fix potential resource leak

[Markus Elfring]

> 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

Re: [PATCH v4 11/14] irqchip/omap-intc: Fix potential resource leak

[Tiezhu Yang]

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

Re: [PATCH v4 11/14] irqchip/omap-intc: Fix potential resource leak

[Tiezhu Yang]

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

[PATCH v4 12/14] irqchip/riscv-intc: Fix potential resource leak

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

Re: [PATCH v4 12/14] irqchip/riscv-intc: Fix potential resource leak

[Markus Elfring]

> 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

Re: [PATCH v4 12/14] irqchip/riscv-intc: Fix potential resource leak

[Markus Elfring]

> 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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v4 12/14] irqchip/riscv-intc: Fix potential resource leak

[Markus Elfring]

> 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

Re: [PATCH v4 12/14] irqchip/riscv-intc: Fix potential resource leak

[Anup Patel]

On Wed, Jul 1, 2020 at 7:47 AM Tiezhu Yang <yangtiezhu@loongson.cn> 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 <yangtiezhu@loongson.cn>
> ---
>  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 <anup@brainfault.org>

Regards,
Anup

[PATCH v4 13/14] irqchip/s3c24xx: Fix potential resource leaks

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

[PATCH v4 14/14] irqchip/xilinx-intc: Fix potential resource leak

[Tiezhu Yang]

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 <yangtiezhu@loongson.cn>
---
 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

Re: [PATCH v4 14/14] irqchip/xilinx-intc: Fix potential resource leak

[Markus Elfring]

> 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

Re: [PATCH v4 14/14] irqchip/xilinx-intc: Fix potential resource leak

[Markus Elfring]

> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 14/14] irqchip/xilinx-intc: Fix potential resource leak

[Markus Elfring]

> 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

Re: [PATCH v4 14/14] irqchip/xilinx-intc: Fix potential resource leak

[Tiezhu Yang]

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

Re: [PATCH v4 14/14] irqchip/xilinx-intc: Fix potential resource leak

[Tiezhu Yang]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 14/14] irqchip/xilinx-intc: Fix potential resource leak

[Tiezhu Yang]

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