* [PATCH] ravb: Use common error handling code in ravb_probe()
@ 2017-10-28 17:19 SF Markus Elfring
2017-10-29 11:00 ` Geert Uytterhoeven
2017-10-30 20:51 ` Sergei Shtylyov
0 siblings, 2 replies; 6+ messages in thread
From: SF Markus Elfring @ 2017-10-28 17:19 UTC (permalink / raw)
To: netdev, linux-renesas-soc, Dan Carpenter, David S. Miller,
Eugeniu Rosca, Kazuya Mizuguchi, Masaru Nagai,
Niklas Söderlund, Sergei Shtylyov, Simon Horman, Yuval Shaia
Cc: LKML, kernel-janitors
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;
+
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)
--
2.14.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ravb: Use common error handling code in ravb_probe()
2017-10-28 17:19 [PATCH] ravb: Use common error handling code in ravb_probe() SF Markus Elfring
@ 2017-10-29 11:00 ` Geert Uytterhoeven
2017-10-29 11:50 ` SF Markus Elfring
2017-10-30 21:08 ` Sergei Shtylyov
2017-10-30 20:51 ` Sergei Shtylyov
1 sibling, 2 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2017-10-29 11:00 UTC (permalink / raw)
To: SF Markus Elfring
Cc: netdev@vger.kernel.org, Linux-Renesas, Dan Carpenter,
David S. Miller, Eugeniu Rosca, Kazuya Mizuguchi, Masaru Nagai,
Niklas Söderlund, Sergei Shtylyov, Simon Horman, Yuval Shaia,
LKML, kernel-janitors@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ravb: Use common error handling code in ravb_probe()
2017-10-29 11:00 ` Geert Uytterhoeven
@ 2017-10-29 11:50 ` SF Markus Elfring
2017-10-30 21:08 ` Sergei Shtylyov
1 sibling, 0 replies; 6+ messages in thread
From: SF Markus Elfring @ 2017-10-29 11:50 UTC (permalink / raw)
To: Geert Uytterhoeven, netdev@vger.kernel.org, Linux-Renesas
Cc: Dan Carpenter, David S. Miller, Eugeniu Rosca, Kazuya Mizuguchi,
Masaru Nagai, Niklas Söderlund, Sergei Shtylyov,
Simon Horman, Yuval Shaia, LKML, kernel-janitors@vger.kernel.org
>> @@ -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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ravb: Use common error handling code in ravb_probe()
2017-10-29 11:00 ` Geert Uytterhoeven
2017-10-29 11:50 ` SF Markus Elfring
@ 2017-10-30 21:08 ` Sergei Shtylyov
1 sibling, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2017-10-30 21:08 UTC (permalink / raw)
To: Geert Uytterhoeven, SF Markus Elfring
Cc: netdev@vger.kernel.org, Linux-Renesas, Dan Carpenter,
David S. Miller, Eugeniu Rosca, Kazuya Mizuguchi, Masaru Nagai,
Niklas Söderlund, Simon Horman, Yuval Shaia, LKML,
kernel-janitors@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ravb: Use common error handling code in ravb_probe()
2017-10-28 17:19 [PATCH] ravb: Use common error handling code in ravb_probe() SF Markus Elfring
2017-10-29 11:00 ` Geert Uytterhoeven
@ 2017-10-30 20:51 ` Sergei Shtylyov
2017-10-30 21:19 ` Sergei Shtylyov
1 sibling, 1 reply; 6+ messages in thread
From: Sergei Shtylyov @ 2017-10-30 20:51 UTC (permalink / raw)
To: SF Markus Elfring, netdev, linux-renesas-soc, Dan Carpenter,
David S. Miller, Eugeniu Rosca, Kazuya Mizuguchi, Masaru Nagai,
Niklas Söderlund, Simon Horman, Yuval Shaia
Cc: LKML, kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ravb: Use common error handling code in ravb_probe()
2017-10-30 20:51 ` Sergei Shtylyov
@ 2017-10-30 21:19 ` Sergei Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2017-10-30 21:19 UTC (permalink / raw)
To: SF Markus Elfring, netdev, linux-renesas-soc, Dan Carpenter,
David S. Miller, Eugeniu Rosca, Kazuya Mizuguchi, Masaru Nagai,
Niklas Söderlund, Simon Horman, Yuval Shaia
Cc: LKML, kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-30 21:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-28 17:19 [PATCH] ravb: Use common error handling code in ravb_probe() SF Markus Elfring
2017-10-29 11:00 ` Geert Uytterhoeven
2017-10-29 11:50 ` SF Markus Elfring
2017-10-30 21:08 ` Sergei Shtylyov
2017-10-30 20:51 ` Sergei Shtylyov
2017-10-30 21:19 ` Sergei Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox