* Re: [PATCH v4 03/14] irqchip/csky-mpintc: Fix potential resource leaks
[not found] ` <1593569786-11500-4-git-send-email-yangtiezhu@loongson.cn>
@ 2020-07-01 7:49 ` Markus Elfring
2020-07-01 9:23 ` Tiezhu Yang
0 siblings, 1 reply; 20+ messages in thread
From: Markus Elfring @ 2020-07-01 7:49 UTC (permalink / raw)
To: Tiezhu Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky
Cc: linux-kernel, kernel-janitors
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 04/14] irqchip/davinci-aintc: Fix potential resource leaks
[not found] ` <1593569786-11500-5-git-send-email-yangtiezhu@loongson.cn>
@ 2020-07-01 8:15 ` Markus Elfring
2020-07-01 9:19 ` Tiezhu Yang
0 siblings, 1 reply; 20+ messages in thread
From: Markus Elfring @ 2020-07-01 8:15 UTC (permalink / raw)
To: Tiezhu Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier
Cc: linux-kernel, kernel-janitors
…
> +++ 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks
[not found] ` <1593569786-11500-3-git-send-email-yangtiezhu@loongson.cn>
@ 2020-07-01 8:40 ` Markus Elfring
2020-07-01 9:35 ` Tiezhu Yang
0 siblings, 1 reply; 20+ messages in thread
From: Markus Elfring @ 2020-07-01 8:40 UTC (permalink / raw)
To: Tiezhu Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky
Cc: linux-kernel, kernel-janitors
> … 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 11/14] irqchip/omap-intc: Fix potential resource leak
[not found] ` <1593569786-11500-12-git-send-email-yangtiezhu@loongson.cn>
@ 2020-07-01 9:14 ` Markus Elfring
2020-07-01 9:40 ` Tiezhu Yang
0 siblings, 1 reply; 20+ messages in thread
From: Markus Elfring @ 2020-07-01 9:14 UTC (permalink / raw)
To: Tiezhu Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Tony Lindgren, linux-omap
Cc: linux-kernel, kernel-janitors
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 04/14] irqchip/davinci-aintc: Fix potential resource leaks
2020-07-01 8:15 ` [PATCH v4 04/14] irqchip/davinci-aintc: " Markus Elfring
@ 2020-07-01 9:19 ` Tiezhu Yang
0 siblings, 0 replies; 20+ messages in thread
From: Tiezhu Yang @ 2020-07-01 9:19 UTC (permalink / raw)
To: Markus Elfring, Thomas Gleixner, Jason Cooper, Marc Zyngier
Cc: linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 03/14] irqchip/csky-mpintc: Fix potential resource leaks
2020-07-01 7:49 ` [PATCH v4 03/14] irqchip/csky-mpintc: Fix potential resource leaks Markus Elfring
@ 2020-07-01 9:23 ` Tiezhu Yang
0 siblings, 0 replies; 20+ messages in thread
From: Tiezhu Yang @ 2020-07-01 9:23 UTC (permalink / raw)
To: Markus Elfring, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky
Cc: linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 12/14] irqchip/riscv-intc: Fix potential resource leak
[not found] ` <1593569786-11500-13-git-send-email-yangtiezhu@loongson.cn>
@ 2020-07-01 9:30 ` Markus Elfring
0 siblings, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2020-07-01 9:30 UTC (permalink / raw)
To: Tiezhu Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Albert Ou, Palmer Dabbelt, Paul Walmsley, linux-riscv
Cc: kernel-janitors, linux-kernel
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks
2020-07-01 8:40 ` [PATCH v4 02/14] irqchip/csky-apb-intc: " Markus Elfring
@ 2020-07-01 9:35 ` Tiezhu Yang
2020-07-01 13:04 ` Markus Elfring
2020-07-02 15:00 ` [PATCH v4 " Dan Carpenter
0 siblings, 2 replies; 20+ messages in thread
From: Tiezhu Yang @ 2020-07-01 9:35 UTC (permalink / raw)
To: Markus Elfring, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky
Cc: linux-kernel, kernel-janitors
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4 11/14] irqchip/omap-intc: Fix potential resource leak
2020-07-01 9:14 ` [PATCH v4 11/14] irqchip/omap-intc: Fix potential resource leak Markus Elfring
@ 2020-07-01 9:40 ` Tiezhu Yang
0 siblings, 0 replies; 20+ messages in thread
From: Tiezhu Yang @ 2020-07-01 9:40 UTC (permalink / raw)
To: Markus Elfring, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Tony Lindgren, linux-omap
Cc: linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 14/14] irqchip/xilinx-intc: Fix potential resource leak
[not found] ` <1593569786-11500-15-git-send-email-yangtiezhu@loongson.cn>
@ 2020-07-01 9:42 ` Markus Elfring
2020-07-01 9:58 ` Tiezhu Yang
0 siblings, 1 reply; 20+ messages in thread
From: Markus Elfring @ 2020-07-01 9:42 UTC (permalink / raw)
To: Tiezhu Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Michal Simek, linux-arm-kernel
Cc: kernel-janitors, linux-kernel
> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 14/14] irqchip/xilinx-intc: Fix potential resource leak
2020-07-01 9:42 ` [PATCH v4 14/14] irqchip/xilinx-intc: " Markus Elfring
@ 2020-07-01 9:58 ` Tiezhu Yang
0 siblings, 0 replies; 20+ messages in thread
From: Tiezhu Yang @ 2020-07-01 9:58 UTC (permalink / raw)
To: Markus Elfring, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Michal Simek, linux-arm-kernel
Cc: kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks
2020-07-01 9:35 ` Tiezhu Yang
@ 2020-07-01 13:04 ` Markus Elfring
2020-07-02 1:18 ` Tiezhu Yang
2020-07-02 15:00 ` [PATCH v4 " Dan Carpenter
1 sibling, 1 reply; 20+ messages in thread
From: Markus Elfring @ 2020-07-01 13:04 UTC (permalink / raw)
To: Tiezhu Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky
Cc: linux-kernel, kernel-janitors
PiBJZiByZW1vdmUgdGhlIGxvY2FsIHZhcmlhYmxlICJyZXQiLMKgIGl0IHdpbGwgbG9vayBsaWtl
IHRoaXM6DQrigKYNCj4gKysrIGIvZHJpdmVycy9pcnFjaGlwL2lycS1jc2t5LWFwYi1pbnRjLmMN
CuKApg0KPiBAQCAtMTE4LDE4ICsxMTYsMjMgQEAgY2tfaW50Y19pbml0X2NvbW0oc3RydWN0IGRl
dmljZV9ub2RlICpub2RlLCBzdHJ1Y3QgZGV2aWNlX25vZGUgKnBhcmVudCkNCuKApg0KPiAtwqDC
oMKgwqDCoMKgIHJldCA9IGlycV9hbGxvY19kb21haW5fZ2VuZXJpY19jaGlwcyhyb290X2RvbWFp
biwgMzIsIDEsDQo+ICvCoMKgwqDCoMKgwqAgaWYgKGlycV9hbGxvY19kb21haW5fZ2VuZXJpY19j
aGlwcyhyb290X2RvbWFpbiwgMzIsIDEsDQo+IMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqAgImNza3lfaW50YyIsIGhhbmRsZV9sZXZlbF9pcnEsDQo+IC3CoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBJUlFfTk9SRVFVRVNUIHwg
SVJRX05PUFJPQkUgfCBJUlFfTk9BVVRPRU4sIDAsIDApOw0KPiAtwqDCoMKgwqDCoMKgIGlmIChy
ZXQpIHsNCj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIElS
UV9OT1JFUVVFU1QgfCBJUlFfTk9QUk9CRSB8IElSUV9OT0FVVE9FTiwgMCwgMCkpIHsNCj4gwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIHByX2VycigiQy1TS1kgSW50YyBpcnFfYWxsb2Nf
Z2MgZmFpbGVkLlxuIik7DQrigKYNCg0KSSBzdWdnZXN0IHRvIHJlY2hlY2sgdGhlIHBhcmFtZXRl
ciBhbGlnbm1lbnQgZm9yIHN1Y2ggYSBmdW5jdGlvbiBjYWxsLg0KaHR0cHM6Ly9naXQua2VybmVs
Lm9yZy9wdWIvc2NtL2xpbnV4L2tlcm5lbC9naXQvdG9ydmFsZHMvbGludXguZ2l0L3RyZWUvRG9j
dW1lbnRhdGlvbi9wcm9jZXNzL2NvZGluZy1zdHlsZS5yc3Q/aWQ9N2MzMGI4NTlhOTQ3NTM1ZjIy
MTMyNzdlODI3ZDdhYzdkY2ZmOWM4NCNuOTMNCg0KUmVnYXJkcywNCk1hcmt1cw0K
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks
2020-07-01 13:04 ` Markus Elfring
@ 2020-07-02 1:18 ` Tiezhu Yang
2020-07-02 7:19 ` Markus Elfring
2020-07-02 7:19 ` Markus Elfring
0 siblings, 2 replies; 20+ messages in thread
From: Tiezhu Yang @ 2020-07-02 1:18 UTC (permalink / raw)
To: Markus Elfring, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky
Cc: linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks
2020-07-02 1:18 ` Tiezhu Yang
2020-07-02 7:19 ` Markus Elfring
@ 2020-07-02 7:19 ` Markus Elfring
2020-07-02 8:05 ` Tiezhu Yang
1 sibling, 1 reply; 20+ messages in thread
From: Markus Elfring @ 2020-07-02 7:19 UTC (permalink / raw)
To: Tiezhu Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky
Cc: linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks
2020-07-02 1:18 ` Tiezhu Yang
@ 2020-07-02 7:19 ` Markus Elfring
2020-07-02 7:19 ` Markus Elfring
1 sibling, 0 replies; 20+ messages in thread
From: Markus Elfring @ 2020-07-02 7:19 UTC (permalink / raw)
To: Tiezhu Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky
Cc: linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks
2020-07-02 7:19 ` Markus Elfring
@ 2020-07-02 8:05 ` Tiezhu Yang
2020-07-02 11:53 ` Tiezhu Yang
0 siblings, 1 reply; 20+ messages in thread
From: Tiezhu Yang @ 2020-07-02 8:05 UTC (permalink / raw)
To: Markus Elfring, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky
Cc: linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks
2020-07-02 8:05 ` Tiezhu Yang
@ 2020-07-02 11:53 ` Tiezhu Yang
2020-07-02 12:24 ` [v4 " Markus Elfring
0 siblings, 1 reply; 20+ messages in thread
From: Tiezhu Yang @ 2020-07-02 11:53 UTC (permalink / raw)
To: Markus Elfring, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky
Cc: linux-kernel, kernel-janitors
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks
2020-07-02 11:53 ` Tiezhu Yang
@ 2020-07-02 12:24 ` Markus Elfring
2020-07-02 12:35 ` Tiezhu Yang
0 siblings, 1 reply; 20+ messages in thread
From: Markus Elfring @ 2020-07-02 12:24 UTC (permalink / raw)
To: Tiezhu Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky
Cc: linux-kernel, kernel-janitors
>>>>>> +++ 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks
2020-07-02 12:24 ` [v4 " Markus Elfring
@ 2020-07-02 12:35 ` Tiezhu Yang
0 siblings, 0 replies; 20+ messages in thread
From: Tiezhu Yang @ 2020-07-02 12:35 UTC (permalink / raw)
To: Markus Elfring, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky
Cc: linux-kernel, kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 02/14] irqchip/csky-apb-intc: Fix potential resource leaks
2020-07-01 9:35 ` Tiezhu Yang
2020-07-01 13:04 ` Markus Elfring
@ 2020-07-02 15:00 ` Dan Carpenter
1 sibling, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2020-07-02 15:00 UTC (permalink / raw)
To: Tiezhu Yang
Cc: Markus Elfring, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Guo Ren, linux-csky, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-07-02 15:00 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1593569786-11500-1-git-send-email-yangtiezhu@loongson.cn>
[not found] ` <1593569786-11500-4-git-send-email-yangtiezhu@loongson.cn>
2020-07-01 7:49 ` [PATCH v4 03/14] irqchip/csky-mpintc: Fix potential resource leaks Markus Elfring
2020-07-01 9:23 ` Tiezhu Yang
[not found] ` <1593569786-11500-5-git-send-email-yangtiezhu@loongson.cn>
2020-07-01 8:15 ` [PATCH v4 04/14] irqchip/davinci-aintc: " Markus Elfring
2020-07-01 9:19 ` Tiezhu Yang
[not found] ` <1593569786-11500-3-git-send-email-yangtiezhu@loongson.cn>
2020-07-01 8:40 ` [PATCH v4 02/14] irqchip/csky-apb-intc: " Markus Elfring
2020-07-01 9:35 ` Tiezhu Yang
2020-07-01 13:04 ` Markus Elfring
2020-07-02 1:18 ` Tiezhu Yang
2020-07-02 7:19 ` Markus Elfring
2020-07-02 7:19 ` Markus Elfring
2020-07-02 8:05 ` Tiezhu Yang
2020-07-02 11:53 ` Tiezhu Yang
2020-07-02 12:24 ` [v4 " Markus Elfring
2020-07-02 12:35 ` Tiezhu Yang
2020-07-02 15:00 ` [PATCH v4 " Dan Carpenter
[not found] ` <1593569786-11500-12-git-send-email-yangtiezhu@loongson.cn>
2020-07-01 9:14 ` [PATCH v4 11/14] irqchip/omap-intc: Fix potential resource leak Markus Elfring
2020-07-01 9:40 ` Tiezhu Yang
[not found] ` <1593569786-11500-13-git-send-email-yangtiezhu@loongson.cn>
2020-07-01 9:30 ` [PATCH v4 12/14] irqchip/riscv-intc: " Markus Elfring
[not found] ` <1593569786-11500-15-git-send-email-yangtiezhu@loongson.cn>
2020-07-01 9:42 ` [PATCH v4 14/14] irqchip/xilinx-intc: " Markus Elfring
2020-07-01 9:58 ` Tiezhu Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).