ravb: Use common error handling code in ravb_probe()
diff mbox

Message ID 2839c3c2-0116-7549-6ff4-a49eb0a52298@users.sourceforge.net
State Under Review
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

SF Markus Elfring Oct. 28, 2017, 5:19 p.m. UTC
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(-)

Comments

Geert Uytterhoeven Oct. 29, 2017, 11 a.m. UTC | #1
Hi Markus,

On Sat, Oct 28, 2017 at 7:19 PM, SF Markus Elfring
<elfring@users.sourceforge.net> 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".
Especially when jumping to a meaningless label named "failure_indication"
("irq_failure" would be more intuitive).

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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
SF Markus Elfring Oct. 29, 2017, 11:50 a.m. UTC | #2
>> @@ -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".
> Especially when jumping to a meaningless label named "failure_indication"
> ("irq_failure" would be more intuitive).

Thanks for your constructive feedback.


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

Can another attempt make sense to concentrate the setting of a variable
at the end of this function with more pleasing identifiers?

Regards,
Markus
Sergei Shtylyov Oct. 30, 2017, 8:51 p.m. UTC | #3
Hello!

On 10/28/2017 08:19 PM, SF Markus Elfring 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.

    Have you actually tried to see if there's any change in the resulting 
object code? I've looked at ARM gcc 4.8.5's output and it didn't really change 
-- gcc was able to optimize these repetitive assignments pretty well...

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

    Diffstat also speaks about the futility of this patch.

> 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

> @@ -2226,6 +2222,10 @@ static int ravb_probe(struct platform_device *pdev)
>   	pm_runtime_put(&pdev->dev);
>   	pm_runtime_disable(&pdev->dev);
>   	return error;
> +
> +failure_indication:

    Pretty poor label name -- I would've preferred 'no_irq' or smth...

> +	error = irq;
> +	goto out_release;

    The only good thing is that looking at the assembly, I was able to figure 
out that 'ndev' check at 'out_release' is futile... :-)

[...]

MBR, Sergei
Sergei Shtylyov Oct. 30, 2017, 9:08 p.m. UTC | #4
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
Sergei Shtylyov Oct. 30, 2017, 9:19 p.m. UTC | #5
On 10/30/2017 11:51 PM, Sergei Shtylyov 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.
> 
>     Have you actually tried to see if there's any change in the resulting 
> object code? I've looked at ARM gcc 4.8.5's output and it didn't really change 
> -- gcc was able to optimize these repetitive assignments pretty well...
> 
>> 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(-)
> 
>     Diffstat also speaks about the futility of this patch.

    Ah, you've added empty lines where there was none... Please don't do this.
I'll ACK the v2 patch, given my comments are addressed.

[...]

MBR, Sergei

Patch
diff mbox

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;
+
 	ndev->irq = irq;
 
 	SET_NETDEV_DEV(ndev, &pdev->dev);
@@ -2101,25 +2100,22 @@  static int ravb_probe(struct platform_device *pdev)
 
 	if (chip_id == RCAR_GEN3) {
 		irq = platform_get_irq_byname(pdev, "ch24");
-		if (irq < 0) {
-			error = irq;
-			goto out_release;
-		}
+		if (irq < 0)
+			goto failure_indication;
+
 		priv->emac_irq = irq;
 		for (i = 0; i < NUM_RX_QUEUE; i++) {
 			irq = platform_get_irq_byname(pdev, ravb_rx_irqs[i]);
-			if (irq < 0) {
-				error = irq;
-				goto out_release;
-			}
+			if (irq < 0)
+				goto failure_indication;
+
 			priv->rx_irqs[i] = irq;
 		}
 		for (i = 0; i < NUM_TX_QUEUE; i++) {
 			irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
-			if (irq < 0) {
-				error = irq;
-				goto out_release;
-			}
+			if (irq < 0)
+				goto failure_indication;
+
 			priv->tx_irqs[i] = irq;
 		}
 	}
@@ -2226,6 +2222,10 @@  static int ravb_probe(struct platform_device *pdev)
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	return error;
+
+failure_indication:
+	error = irq;
+	goto out_release;
 }
 
 static int ravb_remove(struct platform_device *pdev)