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

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

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]

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

[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