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

[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

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

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

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