* 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 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
[parent not found: <1593569786-11500-5-git-send-email-yangtiezhu@loongson.cn>]
* 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 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
[parent not found: <1593569786-11500-3-git-send-email-yangtiezhu@loongson.cn>]
* 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 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 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 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 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 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
[parent not found: <1593569786-11500-12-git-send-email-yangtiezhu@loongson.cn>]
* 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 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
[parent not found: <1593569786-11500-13-git-send-email-yangtiezhu@loongson.cn>]
* 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
[parent not found: <1593569786-11500-15-git-send-email-yangtiezhu@loongson.cn>]
* 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
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).