Re: [PATCH] sr9800: Use common error handling code in sr9800_phy_powerup()

[Geert Uytterhoeven]

Hi Markus,

On Sun, Oct 29, 2017 at 1:03 PM, SF Markus Elfring
<elfring@xxxxxxxxxxxxxxxxxxxxx> wrote:
>>> @@ -700,10 +700,9 @@ static int sr9800_phy_powerup(struct usbnet *dev)
>>>
>>>         /* set the embedded Ethernet PHY in power-up state */
>>>         ret = sr_sw_reset(dev, SR_SWRESET_IPRL);
>>> -       if (ret < 0) {
>>> -               netdev_err(dev->net, "Failed to reset PHY: %d\n", ret);
>>> -               return ret;
>>> -       }
>>> +       if (ret < 0)
>>> +               goto report_reset_failure;
>>
>> So now I have to look below to see what error handling it does...
>
> Yes. - Can this be an usual consequence if you apply information from
> the section â7) Centralized exiting of functionsâ in the document
> âcoding-style.rstâ a bit more?

That section is useful if several cleanups steps have to be performed.
In this case, the only common part is the printing of the error message.
Printing error messages is not a cleanup step.

What's also bad in this case, is that after the first "goto", there are other
error cases that don't use goto, but just return, because there's no cleanup
to do there.

Hence:
NAKed-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>

>> Hence I prefer the original version, which had _less_ lines of code...
>
> My update suggestion is only one line âbiggerâ in this case, isn't it?
>
> I propose an other source code layout so that a bit smaller executable
> object code could be achieved.
> Do find such a software design direction feasible?

If you play the "smaller executable object code" card, people expect that
you provide the actual numbers, too.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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