| Message ID | 10f2b5a4-1dbd-2c4f-caaa-4225dff35296@users.sourceforge.net |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | sr9800: Use common error handling code in sr9800_phy_powerup() | expand |
Hi Markus, On Sun, Oct 29, 2017 at 11:45 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sun, 29 Oct 2017 11:33:14 +0100 > > Add a jump target so that a specific error message is stored only once > at the end of this function implementation. > Replace two calls of the function "netdev_err" by goto statements. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/net/usb/sr9800.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/usb/sr9800.c b/drivers/net/usb/sr9800.c > index 9277a0f228df..79cfa72c68ba 100644 > --- a/drivers/net/usb/sr9800.c > +++ b/drivers/net/usb/sr9800.c > @@ -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... Hence I prefer the original version, which had _less_ lines of code... > + > msleep(600); > > /* set the embedded Ethernet PHY in reset state */ > @@ -716,12 +715,14 @@ 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); Gcc is smart enough to optimize away the second identical string printed. > - return ret; > - } > + if (ret < 0) > + goto report_reset_failure; > > return 0; > + > +report_reset_failure: > + netdev_err(dev->net, "Failed to reset PHY: %d\n", ret); > + return ret; > } > > static int sr9800_bind(struct usbnet *dev, struct usb_interface *intf) 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
>> @@ -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? > 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? Regards, Markus
Hi Markus, On Sun, Oct 29, 2017 at 1:03 PM, SF Markus Elfring <elfring@users.sourceforge.net> 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@linux-m68k.org> >> 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@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
> If you play the "smaller executable object code" card, people expect that > you provide the actual numbers, too. I can offer another bit of information for this software development discussion. The affected source file can be compiled for the processor architecture “x86_64” by a tool like “GCC 6.4.1+r251631-1.3” from the software distribution “openSUSE Tumbleweed” with the following command example. my_cc=/usr/bin/gcc-6 \ && my_module=drivers/net/usb/sr9800.ko \ && git checkout next-20171009 \ && make -j4 CC="${my_cc}" HOSTCC="${my_cc}" allmodconfig "${my_module}" \ && size "${my_module}" \ && git checkout ':/^sr9800: Use common error handling code in sr9800_phy_powerup' \ && make -j4 CC="${my_cc}" HOSTCC="${my_cc}" allmodconfig "${my_module}" \ && size "${my_module}" Do you find the following details useful for further clarification? text: -47 data: 0 bss: 0 Regards, Markus
diff --git a/drivers/net/usb/sr9800.c b/drivers/net/usb/sr9800.c index 9277a0f228df..79cfa72c68ba 100644 --- a/drivers/net/usb/sr9800.c +++ b/drivers/net/usb/sr9800.c @@ -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; + msleep(600); /* set the embedded Ethernet PHY in reset state */ @@ -716,12 +715,14 @@ 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; return 0; + +report_reset_failure: + netdev_err(dev->net, "Failed to reset PHY: %d\n", ret); + return ret; } static int sr9800_bind(struct usbnet *dev, struct usb_interface *intf)