[PATCH 0/2] drivers/tty: delete break after return or goto

[PATCH 0/2] drivers/tty: delete break after return or goto

[Bernard Zhao]

Hi, Greg:

This patch sereies delete code which never run:
{
case XXX:
	return XXX;
	break; //The break is meanless, so just delete it.
case YYY:
	goto YYY;
	break; //The break is meanless, so just delete it.
......
}

Bernard Zhao (2):
  tty/serial: delete break after return
  drivers/tty: delete break after goto/return

 drivers/tty/nozomi.c     | 4 ----
 drivers/tty/serial/imx.c | 5 -----
 2 files changed, 9 deletions(-)

-- 
2.29.0

[PATCH 1/2] tty/serial: delete break after return

[Bernard Zhao]

Delete break after return, which will never run.

Signed-off-by: Bernard Zhao <bernard@vivo.com>
---
 drivers/tty/serial/imx.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 1731d9728865..09703079db7b 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -320,7 +320,6 @@ static u32 imx_uart_readl(struct imx_port *sport, u32 offset)
 	switch (offset) {
 	case UCR1:
 		return sport->ucr1;
-		break;
 	case UCR2:
 		/*
 		 * UCR2_SRST is the only bit in the cached registers that might
@@ -331,16 +330,12 @@ static u32 imx_uart_readl(struct imx_port *sport, u32 offset)
 		if (!(sport->ucr2 & UCR2_SRST))
 			sport->ucr2 = readl(sport->port.membase + offset);
 		return sport->ucr2;
-		break;
 	case UCR3:
 		return sport->ucr3;
-		break;
 	case UCR4:
 		return sport->ucr4;
-		break;
 	case UFCR:
 		return sport->ufcr;
-		break;
 	default:
 		return readl(sport->port.membase + offset);
 	}
-- 
2.29.0

Re: [PATCH 1/2] tty/serial: delete break after return

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2191 bytes --]

Hello,

the Subject is wrong, it should use a prefix similar to "serial: imx:".
It's a good idea to check previous patches to the same file to pick a
suitable prefix. (E.g. git log --oneline drivers/tty/serial/imx.c)

On Fri, Nov 06, 2020 at 07:29:23PM -0800, Bernard Zhao wrote:
> Delete break after return, which will never run.
> 
> Signed-off-by: Bernard Zhao <bernard@vivo.com>
> ---
>  drivers/tty/serial/imx.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 1731d9728865..09703079db7b 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -320,7 +320,6 @@ static u32 imx_uart_readl(struct imx_port *sport, u32 offset)
>  	switch (offset) {
>  	case UCR1:
>  		return sport->ucr1;
> -		break;
>  	case UCR2:
>  		/*
>  		 * UCR2_SRST is the only bit in the cached registers that might
> @@ -331,16 +330,12 @@ static u32 imx_uart_readl(struct imx_port *sport, u32 offset)
>  		if (!(sport->ucr2 & UCR2_SRST))
>  			sport->ucr2 = readl(sport->port.membase + offset);
>  		return sport->ucr2;
> -		break;
>  	case UCR3:
>  		return sport->ucr3;
> -		break;
>  	case UCR4:
>  		return sport->ucr4;
> -		break;
>  	case UFCR:
>  		return sport->ufcr;
> -		break;

you're the third to send this patch since October 20:

	https://lore.kernel.org/r/20201026125142.21105-1-zhangqilong3@huawei.com
	https://lore.kernel.org/r/20201020130709.28096-1-trix@redhat.com

My comment for these was:

> this might be subjective, but I like the break being there for clearity.
> So I object to make a patch to remove them. In case I'm outvoted I'd at
> least want empty lines instead.

Zhang Qilong wrote he found the patch opportunity by manual code
inspection, I would have expected that there is a tool that identifies a
break after a return. If you had tool support, please mention the tool
in the commit log (if you really want to keep following the patch's
idea).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re:Re: [PATCH 1/2] tty/serial: delete break after return

[Bernard]

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Date: 2020-11-07 22:01:29
To:  Bernard Zhao <bernard@vivo.com>
Cc:  Greg Kroah-Hartman <gregkh@linuxfoundation.org>,Jiri Slaby <jirislaby@kernel.org>,Shawn Guo <shawnguo@kernel.org>,Sascha Hauer <s.hauer@pengutronix.de>,Pengutronix Kernel Team <kernel@pengutronix.de>,Fabio Estevam <festevam@gmail.com>,NXP Linux Team <linux-imx@nxp.com>,linux-kernel@vger.kernel.org,linux-serial@vger.kernel.org,linux-arm-kernel@lists.infradead.org,opensource.kernel@vivo.com
Subject: Re: [PATCH 1/2] tty/serial: delete break after return>Hello,
>
>the Subject is wrong, it should use a prefix similar to "serial: imx:".
>It's a good idea to check previous patches to the same file to pick a
>suitable prefix. (E.g. git log --oneline drivers/tty/serial/imx.c)

Hi, Uwe:

Thank you for your suggestion, I will make a more accurate subject in my future patches.

>On Fri, Nov 06, 2020 at 07:29:23PM -0800, Bernard Zhao wrote:
>> Delete break after return, which will never run.
>> 
>> Signed-off-by: Bernard Zhao <bernard@vivo.com>
>> ---
>>  drivers/tty/serial/imx.c | 5 -----
>>  1 file changed, 5 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 1731d9728865..09703079db7b 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -320,7 +320,6 @@ static u32 imx_uart_readl(struct imx_port *sport, u32 offset)
>>  	switch (offset) {
>>  	case UCR1:
>>  		return sport->ucr1;
>> -		break;
>>  	case UCR2:
>>  		/*
>>  		 * UCR2_SRST is the only bit in the cached registers that might
>> @@ -331,16 +330,12 @@ static u32 imx_uart_readl(struct imx_port *sport, u32 offset)
>>  		if (!(sport->ucr2 & UCR2_SRST))
>>  			sport->ucr2 = readl(sport->port.membase + offset);
>>  		return sport->ucr2;
>> -		break;
>>  	case UCR3:
>>  		return sport->ucr3;
>> -		break;
>>  	case UCR4:
>>  		return sport->ucr4;
>> -		break;
>>  	case UFCR:
>>  		return sport->ufcr;
>> -		break;
>
>you're the third to send this patch since October 20:
>
>	https://lore.kernel.org/r/20201026125142.21105-1-zhangqilong3@huawei.com
>	https://lore.kernel.org/r/20201020130709.28096-1-trix@redhat.com
>
>My comment for these was:
>
>> this might be subjective, but I like the break being there for clearity.
>> So I object to make a patch to remove them. In case I'm outvoted I'd at
>> least want empty lines instead.
>
>Zhang Qilong wrote he found the patch opportunity by manual code
>inspection, I would have expected that there is a tool that identifies a
>break after a return. If you had tool support, please mention the tool
>in the commit log (if you really want to keep following the patch's
>idea).

For this part:
I wrote a python script to check if there is a break after a return or goto.
The script currently has some issues in its handling of special characters, the search result is not precise enough. It is still under debugging. 
These are the only places which  the script checks out in path: driver/tty.
I also checked other path(like: driver/vme/, driver/video, driver/usb), these paths also have a certain number of similar issues. And I will try to submit these patch later.
Thanks!

BR//Bernard

>Best regards
>Uwe
>
>-- 
>Pengutronix e.K.                           | Uwe Kleine-König            |
>Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[PATCH 2/2] drivers/tty: delete break after goto/return

[Bernard Zhao]

Delete break after goto/return, which will never run.

Signed-off-by: Bernard Zhao <bernard@vivo.com>
---
 drivers/tty/nozomi.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/tty/nozomi.c b/drivers/tty/nozomi.c
index d42b854cb7df..946cc16827aa 100644
--- a/drivers/tty/nozomi.c
+++ b/drivers/tty/nozomi.c
@@ -414,11 +414,9 @@ static void read_mem32(u32 *buf, const void __iomem *mem_addr_start,
 		buf16 = (u16 *) buf;
 		*buf16 = __le16_to_cpu(readw(ptr));
 		goto out;
-		break;
 	case 4:	/* 4 bytes */
 		*(buf) = __le32_to_cpu(readl(ptr));
 		goto out;
-		break;
 	}
 
 	while (i < size_bytes) {
@@ -460,7 +458,6 @@ static u32 write_mem32(void __iomem *mem_addr_start, const u32 *buf,
 		buf16 = (const u16 *)buf;
 		writew(__cpu_to_le16(*buf16), ptr);
 		return 2;
-		break;
 	case 1: /*
 		 * also needs to write 4 bytes in this case
 		 * so falling through..
@@ -468,7 +465,6 @@ static u32 write_mem32(void __iomem *mem_addr_start, const u32 *buf,
 	case 4: /* 4 bytes */
 		writel(__cpu_to_le32(*buf), ptr);
 		return 4;
-		break;
 	}
 
 	while (i < size_bytes) {
-- 
2.29.0

Re: [PATCH 2/2] drivers/tty: delete break after goto/return

[Greg Kroah-Hartman]

On Fri, Nov 06, 2020 at 07:29:24PM -0800, Bernard Zhao wrote:
> Delete break after goto/return, which will never run.
> 
> Signed-off-by: Bernard Zhao <bernard@vivo.com>
> ---
>  drivers/tty/nozomi.c | 4 ----
>  1 file changed, 4 deletions(-)

If you look at the commits for this file:

	$ git log --oneline drivers/tty/nozomi.c | head -n 5
	1a460c36078e tty: nozomi: remove unneeded break
	caa47cc63947 tty: nozomi: Use scnprintf() for avoiding potential buffer overflow
	e2c2e7987106 tty: nozomi: fix spelling mistake "reserverd" -> "reserved"
	18b1345e60ae tty: nozomi: Use dev_get_drvdata
	c392ed464205 tty/nozomi: use pci_iomap instead of ioremap_nocache

You will notice that you should probably put the driver name in the
subject line.  Otherwise this patch really looks like you are doing this
action on all of drivers/tty/ right?

Same for patch 1/2 as was pointed out by others.

thanks,

greg k-h