| Message ID | 20200707195214.GA3932@embeddedor (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 2da3b53c78be8a11800a3d5053a8ca22b2455c8e |
| Headers | show |
| Series | usbip: Use fallthrough pseudo-keyword | expand |
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
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.
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
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
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
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
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
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
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);
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(-)