Re: [PATCH] ravb: Use common error handling code in ravb_probe()

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

On 10/29/2017 02:00 PM, Geert Uytterhoeven wrote:

>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Sat, 28 Oct 2017 19:10:08 +0200
>>
>> Add a jump target so that a bit of exception handling can be better reused
>> at the end of this function.
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>>   drivers/net/ethernet/renesas/ravb_main.c | 32 ++++++++++++++++----------------
>>   1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index a8822a756e08..62dbdf7de6cd 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2069,10 +2069,9 @@ static int ravb_probe(struct platform_device *pdev)
>>                  irq = platform_get_irq_byname(pdev, "ch22");
>>          else
>>                  irq = platform_get_irq(pdev, 0);
>> -       if (irq < 0) {
>> -               error = irq;
>> -               goto out_release;
>> -       }
>> +       if (irq < 0)
>> +               goto failure_indication;
> 
> IMHO, it's really confusing that "irq" contains the error code, not "error".

    I think it would have been equally confusing if 'error' was assigned to 
'ndev->irq', etc. It's just the duality of the result of these functions that 
makes them confusing...

> Especially when jumping to a meaningless label named "failure_indication"
> ("irq_failure" would be more intuitive).

    Yeah, the label sucks. :-)

> So I prefer the original code, regardless of the label name.

    On the 2nd thought, the patch can be fixed up and then merged.

> Gr{oetje,eeting}s,
> 
>                          Geert

MBR, Sergei