| Message ID | 2839c3c2-0116-7549-6ff4-a49eb0a52298@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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
>> @@ -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
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
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
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
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)