[PATCH] USBIP: Use fallthrough pseudo-keyword

[PATCH] USBIP: Use fallthrough pseudo-keyword

[Pairman Guo]

Hi maintainers,

There is a usage of ``/* FALLTHRU */`` in a switch statement in main()
that have long been untouched. This patch replaced it with the
better and proper pseudo-keyword ``fallthrough;``.

Please merge if it is the case. Thank you in advance.

Signed-off-by: Pairman Guo <pairmanxlr@gmail.com>
---
 tools/usb/usbip/src/usbip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/usb/usbip/src/usbip.c b/tools/usb/usbip/src/usbip.c
index f7c7220d9..ddcafb5c7 100644
--- a/tools/usb/usbip/src/usbip.c
+++ b/tools/usb/usbip/src/usbip.c
@@ -165,7 +165,7 @@ int main(int argc, char *argv[])
 		case '?':
 			printf("usbip: invalid option\n");
 			/* Terminate after printing error */
-			/* FALLTHRU */
+			fallthrough;
 		default:
 			usbip_usage();
 			goto out;
-- 
2.43.1

Re: [PATCH] USBIP: Use fallthrough pseudo-keyword

[Greg KH]

On Thu, Feb 15, 2024 at 05:26:30PM +0800, Pairman Guo wrote:
> Hi maintainers,
> 
> There is a usage of ``/* FALLTHRU */`` in a switch statement in main()
> that have long been untouched. This patch replaced it with the
> better and proper pseudo-keyword ``fallthrough;``.
> 
> Please merge if it is the case. Thank you in advance.

This is not needed in a changelog text.

> Signed-off-by: Pairman Guo <pairmanxlr@gmail.com>
> ---
>  tools/usb/usbip/src/usbip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/usb/usbip/src/usbip.c b/tools/usb/usbip/src/usbip.c
> index f7c7220d9..ddcafb5c7 100644
> --- a/tools/usb/usbip/src/usbip.c
> +++ b/tools/usb/usbip/src/usbip.c
> @@ -165,7 +165,7 @@ int main(int argc, char *argv[])
>  		case '?':
>  			printf("usbip: invalid option\n");
>  			/* Terminate after printing error */
> -			/* FALLTHRU */
> +			fallthrough;

Did you compile this?  This is userspace code, and as-such, I don't
think it has this keyword, does it?

thanks,

greg k-h

Re: Re: [PATCH] USBIP: Use fallthrough pseudo-keyword

[Pairman Guo]

On Thu, Feb 15, 2024 at 11:14:49 +0800, Greg KH wrote:
> This is not needed in a changelog text.

Thanks for telling. I'm new to lkml and still learning about the format.

> >  tools/usb/usbip/src/usbip.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/usb/usbip/src/usbip.c b/tools/usb/usbip/src/usbip.c
> > index f7c7220d9..ddcafb5c7 100644
> > --- a/tools/usb/usbip/src/usbip.c
> > +++ b/tools/usb/usbip/src/usbip.c
> > @@ -165,7 +165,7 @@ int main(int argc, char *argv[])
> >  		case '?':
> >  			printf("usbip: invalid option\n");
> >  			/* Terminate after printing error */
> > -			/* FALLTHRU */
> > +			fallthrough;
> 
> Did you compile this?  This is userspace code, and as-such, I don't
> think it has this keyword, does it?

I'm sorry for my hurrying. I ran ``make tools/usb`` with no warning or error,
so I sent this patch. Now it turns out ``make tools/usb`` doesn't cover
tools/usb/usbip. I tried again with ``./autogen.sh && ./configure && make``
and it indeed failed. Since usbip.c doesn't have any reference to
include/linux/compiler.h, it cannot use ``fallthrough``. Thanks again for
pointing out this error.

Thanks,

Pairman

[PATCH] usbip: Use fallthrough pseudo-keyword

[Gustavo A. R. Silva]

Replace the existing /* fall through */ comments and its variants with
the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary
fall-through markings when it is the case.

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/usb/usbip/stub_rx.c       |    2 +-
 drivers/usb/usbip/vhci_hcd.c      |    7 +++----
 drivers/usb/usbip/vhci_rx.c       |    2 +-
 drivers/usb/usbip/vudc_transfer.c |    4 ++--
 4 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index e2b019532234..325c22008e53 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -424,7 +424,7 @@ static void masking_bogus_flags(struct urb *urb)
 	case USB_ENDPOINT_XFER_BULK:
 		if (is_out)
 			allowed |= URB_ZERO_PACKET;
-		/* FALLTHROUGH */
+		fallthrough;
 	default:			/* all non-iso endpoints */
 		if (!is_out)
 			allowed |= URB_SHORT_NOT_OK;
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 65850e9c7190..1b598db5d8b9 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -508,7 +508,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		case USB_PORT_FEAT_U1_TIMEOUT:
 			usbip_dbg_vhci_rh(
 				" SetPortFeature: USB_PORT_FEAT_U1_TIMEOUT\n");
-			/* Fall through */
+			fallthrough;
 		case USB_PORT_FEAT_U2_TIMEOUT:
 			usbip_dbg_vhci_rh(
 				" SetPortFeature: USB_PORT_FEAT_U2_TIMEOUT\n");
@@ -561,7 +561,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				       "supported for USB 2.0 roothub\n");
 				goto error;
 			}
-			/* FALLS THROUGH */
+			fallthrough;
 		case USB_PORT_FEAT_RESET:
 			usbip_dbg_vhci_rh(
 				" SetPortFeature: USB_PORT_FEAT_RESET\n");
@@ -584,8 +584,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 
 			/* 50msec reset signaling */
 			vhci_hcd->re_timeout = jiffies + msecs_to_jiffies(50);
-
-			/* FALLS THROUGH */
+			fallthrough;
 		default:
 			usbip_dbg_vhci_rh(" SetPortFeature: default %d\n",
 					  wValue);
diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
index 00fc98741c5d..266024cbb64f 100644
--- a/drivers/usb/usbip/vhci_rx.c
+++ b/drivers/usb/usbip/vhci_rx.c
@@ -27,7 +27,7 @@ struct urb *pickup_urb_and_free_priv(struct vhci_device *vdev, __u32 seqnum)
 
 		switch (status) {
 		case -ENOENT:
-			/* fall through */
+			fallthrough;
 		case -ECONNRESET:
 			dev_dbg(&urb->dev->dev,
 				 "urb seq# %u was unlinked %ssynchronously\n",
diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
index c9db846ee4f6..7e801fee33bf 100644
--- a/drivers/usb/usbip/vudc_transfer.c
+++ b/drivers/usb/usbip/vudc_transfer.c
@@ -404,7 +404,7 @@ static void v_timer(struct timer_list *t)
 			 * for now, give unlimited bandwidth
 			 */
 			limit += urb->transfer_buffer_length;
-			/* fallthrough */
+			fallthrough;
 		default:
 treat_control_like_bulk:
 			total -= transfer(udc, urb, ep, limit);
@@ -479,7 +479,7 @@ void v_kick_timer(struct vudc *udc, unsigned long time)
 		return;
 	case VUDC_TR_IDLE:
 		t->state = VUDC_TR_RUNNING;
-		/* fallthrough */
+		fallthrough;
 	case VUDC_TR_STOPPED:
 		/* we may want to kick timer to unqueue urbs */
 		mod_timer(&t->timer, time);

Re: [PATCH] usbip: Use fallthrough pseudo-keyword

[Shuah Khan]

On 7/7/20 1:52 PM, Gustavo A. R. Silva wrote:
> Replace the existing /* fall through */ comments and its variants with
> the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary
> fall-through markings when it is the case.
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
> 

Is fallthrough syntax supported on our min gcc version?

Does checkpatch or coccicheck catch these cases?

The patch looks good.

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [PATCH] usbip: Use fallthrough pseudo-keyword

[Joe Perches]

On Tue, 2020-07-07 at 14:06 -0600, Shuah Khan wrote:
> On 7/7/20 1:52 PM, Gustavo A. R. Silva wrote:
> > Replace the existing /* fall through */ comments and its variants with
> > the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary
> > fall-through markings when it is the case.
> > 
> > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
> > 
> 
> Is fallthrough syntax supported on our min gcc version?

No.  Introduced in gcc 7.

> Does checkpatch or coccicheck catch these cases?

Kinda.  checkpatch isn't very good at it.
I _believe_, though I'm not at all sure,
that coccinelle can find these.

Re: [PATCH] usbip: Use fallthrough pseudo-keyword

[Julia Lawall]

On Tue, 7 Jul 2020, Joe Perches wrote:

> On Tue, 2020-07-07 at 14:06 -0600, Shuah Khan wrote:
> > On 7/7/20 1:52 PM, Gustavo A. R. Silva wrote:
> > > Replace the existing /* fall through */ comments and its variants with
> > > the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary
> > > fall-through markings when it is the case.
> > >
> > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
> > >
> >
> > Is fallthrough syntax supported on our min gcc version?
>
> No.  Introduced in gcc 7.
>
> > Does checkpatch or coccicheck catch these cases?
>
> Kinda.  checkpatch isn't very good at it.
> I _believe_, though I'm not at all sure,
> that coccinelle can find these.

I would not guarantee anything about the support of Coccinelle for switch.
Coccinelle does now have the ability to match on comments.  So since there
is a distinct comment that it is to be removed, it might be possible to do
that part automatically.

Maybe it would have to look something like this:

@r1@
comments c : script:python() { code to recognize the comment };
statement S;
@@

S@c
+ fallthrough(); //or whatever is wanted

@@
statement r1.S;
@@

- S
- fallthrough();
+ S
+ fallthrough();

The second rule probably looks pretty strange, but the goal is to remove
the comments between S and fallthrough();

There is an example demos/comments.cocci that shows how to access the
comment information using both ocaml and python.

julia

Re: [PATCH] usbip: Use fallthrough pseudo-keyword

[Shuah Khan]

On 7/8/20 4:16 AM, Julia Lawall wrote:
> 
> 
> On Tue, 7 Jul 2020, Joe Perches wrote:
> 
>> On Tue, 2020-07-07 at 14:06 -0600, Shuah Khan wrote:
>>> On 7/7/20 1:52 PM, Gustavo A. R. Silva wrote:
>>>> Replace the existing /* fall through */ comments and its variants with
>>>> the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary
>>>> fall-through markings when it is the case.
>>>>
>>>> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
>>>>
>>>
>>> Is fallthrough syntax supported on our min gcc version?
>>
>> No.  Introduced in gcc 7.
>>
>>> Does checkpatch or coccicheck catch these cases?
>>
>> Kinda.  checkpatch isn't very good at it.
>> I _believe_, though I'm not at all sure,
>> that coccinelle can find these.
> 
> I would not guarantee anything about the support of Coccinelle for switch.
> Coccinelle does now have the ability to match on comments.  So since there
> is a distinct comment that it is to be removed, it might be possible to do
> that part automatically.
> 
> Maybe it would have to look something like this:
> 
> @r1@
> comments c : script:python() { code to recognize the comment };
> statement S;
> @@
> 
> S@c
> + fallthrough(); //or whatever is wanted
> 
> @@
> statement r1.S;
> @@
> 
> - S
> - fallthrough();
> + S
> + fallthrough();
> 
> The second rule probably looks pretty strange, but the goal is to remove
> the comments between S and fallthrough();
> 
> There is an example demos/comments.cocci that shows how to access the
> comment information using both ocaml and python.
> 

Thanks Julia. Maybe this is a way to address all of the cases. I am a
bit concerned about min gcc which is 4.8 and the fallthrough syntax
support is in gcc 7

-- Shuah

Re: [PATCH] usbip: Use fallthrough pseudo-keyword

[Shuah Khan]

On 7/7/20 7:56 PM, Joe Perches wrote:
> On Tue, 2020-07-07 at 14:06 -0600, Shuah Khan wrote:
>> On 7/7/20 1:52 PM, Gustavo A. R. Silva wrote:
>>> Replace the existing /* fall through */ comments and its variants with
>>> the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary
>>> fall-through markings when it is the case.
>>>
>>> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
>>>
>>
>> Is fallthrough syntax supported on our min gcc version?
> 
> No.  Introduced in gcc 7.
> 

Gustavo,

In which case, this patch would break usbip build on older gcc
revisions.

thanks,
-- Shuah

Re: [PATCH] usbip: Use fallthrough pseudo-keyword

[Joe Perches]

On 2020-07-08 07:35, Shuah Khan wrote:
> On 7/7/20 7:56 PM, Joe Perches wrote:
>> On Tue, 2020-07-07 at 14:06 -0600, Shuah Khan wrote:
>>> On 7/7/20 1:52 PM, Gustavo A. R. Silva wrote:
>>>> Replace the existing /* fall through */ comments and its variants 
>>>> with
>>>> the new pseudo-keyword macro fallthrough[1]. Also, remove 
>>>> unnecessary
>>>> fall-through markings when it is the case.
>>>> 
>>>> [1] 
>>>> https://www.kernel.org/doc/html/latest/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
>>>> 
>>> 
>>> Is fallthrough syntax supported on our min gcc version?
>> 
>> No.  Introduced in gcc 7.
>> 
> 
> Gustavo,
> 
> In which case, this patch would break usbip build on older gcc
> revisions.

No it will not.  Comment fall through was introduced in gcc 7 and this 
devolved into a do while 0
> thanks,
> -- Shuah

Re: [PATCH] usbip: Use fallthrough pseudo-keyword

[Shuah Khan]

On 7/8/20 9:39 AM, Joe Perches wrote:
> On 2020-07-08 07:35, Shuah Khan wrote:
>> On 7/7/20 7:56 PM, Joe Perches wrote:
>>> On Tue, 2020-07-07 at 14:06 -0600, Shuah Khan wrote:
>>>> On 7/7/20 1:52 PM, Gustavo A. R. Silva wrote:
>>>>> Replace the existing /* fall through */ comments and its variants with
>>>>> the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary
>>>>> fall-through markings when it is the case.
>>>>>
>>>>> [1] 
>>>>> https://www.kernel.org/doc/html/latest/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through 
>>>>>
>>>>>
>>>>
>>>> Is fallthrough syntax supported on our min gcc version?
>>>
>>> No.  Introduced in gcc 7.
>>>
>>
>> Gustavo,
>>
>> In which case, this patch would break usbip build on older gcc
>> revisions.
> 
> No it will not.  Comment fall through was introduced in gcc 7 and this 
> devolved into a do while 0

Ah thanks. I didn't know that. No concerns of patch to go in.

thanks,
--- Shuah

Re: [PATCH] usbip: Use fallthrough pseudo-keyword

[Gustavo A. R. Silva]

On Tue, Jul 07, 2020 at 02:06:26PM -0600, Shuah Khan wrote:
> On 7/7/20 1:52 PM, Gustavo A. R. Silva wrote:
> > Replace the existing /* fall through */ comments and its variants with
> > the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary
> > fall-through markings when it is the case.
> > 
> > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
> > 
> 
> Is fallthrough syntax supported on our min gcc version?
> 

The __attribute__((__fallthrough__)) has been supported since GCC 7.1
and it should be no problem for early versions because fallthrough is
a macro that also expands to: do {} while (0)  /* fallthrough */

> Does checkpatch or coccicheck catch these cases?
> 

checkpatch does:

https://git.kernel.org/linus/f36d3eb89a43047d3eba222a8132585da25cebfd

> The patch looks good.
> 
> Acked-by: Shuah Khan <skhan@linuxfoundation.org>
> 

Thanks
--
Gustavo