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

[Sergei Shtylyov]

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

> > From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >   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