usbip: vhci_hcd: Mark expected switch fall-through

PATCH In review Gustavo A. R. Silva 4 participants 7 messages Apr 29, 2019 – Apr 30, 2019

Subsystems (2)

USB USB over IP driver

lore

Share & tools

lore

Gustavo A. R. Silva

author

2019-04-29 14:39 UTC #0 lore raw

PATCH 1/1 usbip: vhci_hcd: Mark expected switch fall-through

In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.

This patch fixes the following warning:

In file included from drivers/usb/usbip/vhci_hcd.c:15:
drivers/usb/usbip/vhci_hcd.c: In function ‘vhci_hub_control’:
drivers/usb/usbip/usbip_common.h:63:6: warning: this statement may fall through [-Wimplicit-fallthrough=]

   if (flag & usbip_debug_flag)  \
      ^

drivers/usb/usbip/usbip_common.h:77:2: note: in expansion of macro ‘usbip_dbg_with_flag’
usbip_dbg_with_flag(usbip_debug_vhci_rh, fmt , ##args) ^~~~~~~~~~~~~~~~~~~
drivers/usb/usbip/vhci_hcd.c:509:4: note: in expansion of macro ‘usbip_dbg_vhci_rh’

    usbip_dbg_vhci_rh(
    ^~~~~~~~~~~~~~~~~

drivers/usb/usbip/vhci_hcd.c:511:3: note: here
case USB_PORT_FEAT_U2_TIMEOUT:
^~~~

Warning level 3 was used: -Wimplicit-fallthrough=3

This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough.

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/usb/usbip/vhci_hcd.c | 1 +
 1 file changed, 1 insertion(+)

drivers/usb/usbip/vhci_hcd.c +1 -0

index 667d9c0ec905..000ab7225717 100644
@@ -508,6 +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 */
 		case USB_PORT_FEAT_U2_TIMEOUT:
 			usbip_dbg_vhci_rh(
 				" SetPortFeature: USB_PORT_FEAT_U2_TIMEOUT\n");

David Laight

2019-04-29 14:44 UTC #1 lore raw

From: Gustavo A. R. Silva

Quoted 3 lines

Sent: 29 April 2019 15:40
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.

...

drivers/usb/usbip/vhci_hcd.c static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, · line 514

Quoted 12 lines

drivers/usb/usbip/vhci_hcd.c +1 -0

index 667d9c0ec905..000ab7225717 100644
@@ -508,6 +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 */
 		case USB_PORT_FEAT_U2_TIMEOUT:
 			usbip_dbg_vhci_rh(
 				" SetPortFeature: USB_PORT_FEAT_U2_TIMEOUT\n");

That doesn't look right, both debug messages seem to get printed.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Gustavo A. R. Silva

author

2019-04-29 15:05 UTC #2 lore raw

Quoted 5 lines · David Laight

From: Gustavo A. R. Silva
Quoted 3 lines
Sent: 29 April 2019 15:40
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
...

Quoted 15 lines

drivers/usb/usbip/vhci_hcd.c static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, · line 514
Quoted 12 lines
drivers/usb/usbip/vhci_hcd.c +1 -0
index 667d9c0ec905..000ab7225717 100644
@@ -508,6 +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 */
 		case USB_PORT_FEAT_U2_TIMEOUT:
 			usbip_dbg_vhci_rh(
 				" SetPortFeature: USB_PORT_FEAT_U2_TIMEOUT\n");
That doesn't look right, both debug messages seem to get printed.

At first sight, I thought the same way, then I took a look into commit:

1c9de5bf428612458427943b724bea51abde520a

and noticed that the original developer properly added fall-through comments in other places in the same switch() code, that gave me the impression he knew what he was doing; then I noticed the following error message in case USB_PORT_FEAT_U2_TIMEOUT:

	if (hcd->speed != HCD_USB3) {
		pr_err("USB_PORT_FEAT_U1/2_TIMEOUT req not "
		       "supported for USB 2.0 roothub\n");
		goto error;
	}

this error message is what makes me think the fall-through is intentional; otherwise I think it would look like this instead:

	if (hcd->speed != HCD_USB3) {
		pr_err("USB_PORT_FEAT_U2_TIMEOUT req not "
		       "supported for USB 2.0 roothub\n");
		goto error;
	}

Thanks
--
Gustavo

David Laight

2019-04-29 15:34 UTC #3 lore raw

From: Gustavo A. R. Silva

Quoted 39 lines

Sent: 29 April 2019 16:06
Quoted 5 lines · David Laight
From: Gustavo A. R. Silva
Quoted 3 lines
Sent: 29 April 2019 15:40
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
...
Quoted 15 lines
drivers/usb/usbip/vhci_hcd.c static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, · line 514
Quoted 12 lines
drivers/usb/usbip/vhci_hcd.c +1 -0
index 667d9c0ec905..000ab7225717 100644
@@ -508,6 +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 */
 		case USB_PORT_FEAT_U2_TIMEOUT:
 			usbip_dbg_vhci_rh(
 				" SetPortFeature: USB_PORT_FEAT_U2_TIMEOUT\n");
That doesn't look right, both debug messages seem to get printed.
At first sight, I thought the same way, then I took a look into commit:
1c9de5bf428612458427943b724bea51abde520a
and noticed that the original developer properly added fall-through comments in other places in the same switch() code, that gave me the impression he knew what he was doing; then I noticed the following error message in case USB_PORT_FEAT_U2_TIMEOUT:
	if (hcd->speed != HCD_USB3) {
		pr_err("USB_PORT_FEAT_U1/2_TIMEOUT req not "
		       "supported for USB 2.0 roothub\n");
		goto error;
	}

Quoted 8 lines

this error message is what makes me think the fall-through is intentional; otherwise I think it would look like this instead:
	if (hcd->speed != HCD_USB3) {
		pr_err("USB_PORT_FEAT_U2_TIMEOUT req not "
		       "supported for USB 2.0 roothub\n");
		goto error;
	}

God, that code is truly ugly :-(

It starts off bad with all those 'u16' parameters - they'll require a mask operation somewhere.

Then we have the classic:

	wIndex = ((__u8)(wIndex & 0xff));

Some compilers have been known to and with 0xff twice for code like that. Since the target is u16 there could be a 3rd mask with 0xffff on non-x86.

I like to put a blank line before 'case' lines - the only ones in that function are in the middle of nested case blocks!

If you have to rely on the usbip_dbg_vhci_rh() debug lines you'll wish they contained more context!
vhci_hub_control() has one early on; the rest could be killed - they contain no more information.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Greg Kroah-Hartman

maintainer

2019-04-30 15:44 UTC #4 lore raw

In reply to Gustavo A. R. Silva

Quoted 40 lines · Gustavo A. R. Silva

Quoted 5 lines · David Laight
From: Gustavo A. R. Silva
Quoted 3 lines
Sent: 29 April 2019 15:40
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
...
Quoted 15 lines
drivers/usb/usbip/vhci_hcd.c static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, · line 514
Quoted 12 lines
drivers/usb/usbip/vhci_hcd.c +1 -0
index 667d9c0ec905..000ab7225717 100644
@@ -508,6 +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 */
 		case USB_PORT_FEAT_U2_TIMEOUT:
 			usbip_dbg_vhci_rh(
 				" SetPortFeature: USB_PORT_FEAT_U2_TIMEOUT\n");
That doesn't look right, both debug messages seem to get printed.
At first sight, I thought the same way, then I took a look into commit:
1c9de5bf428612458427943b724bea51abde520a
and noticed that the original developer properly added fall-through comments in other places in the same switch() code, that gave me the impression he knew what he was doing; then I noticed the following error message in case USB_PORT_FEAT_U2_TIMEOUT:
	if (hcd->speed != HCD_USB3) {
		pr_err("USB_PORT_FEAT_U1/2_TIMEOUT req not "
		       "supported for USB 2.0 roothub\n");
		goto error;
	}

Quoted 8 lines

this error message is what makes me think the fall-through is intentional; otherwise I think it would look like this instead:
	if (hcd->speed != HCD_USB3) {
		pr_err("USB_PORT_FEAT_U2_TIMEOUT req not "
		       "supported for USB 2.0 roothub\n");
		goto error;
	}

I think you are right, that's horrid, but correct :(

Will go queue this up, thanks.

greg k-h

shuah

maintainer Acked-by Shuah Khan

2019-04-30 15:49 UTC #5 lore raw

Quoted 44 lines · Greg Kroah-Hartman

Quoted 43 lines · Gustavo A. R. Silva
Quoted 5 lines · David Laight
From: Gustavo A. R. Silva
Quoted 3 lines
Sent: 29 April 2019 15:40
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
...
Quoted 15 lines
drivers/usb/usbip/vhci_hcd.c static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, · line 514
Quoted 12 lines
drivers/usb/usbip/vhci_hcd.c +1 -0
index 667d9c0ec905..000ab7225717 100644
@@ -508,6 +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 */
  		case USB_PORT_FEAT_U2_TIMEOUT:
  			usbip_dbg_vhci_rh(
  				" SetPortFeature: USB_PORT_FEAT_U2_TIMEOUT\n");
That doesn't look right, both debug messages seem to get printed.
At first sight, I thought the same way, then I took a look into commit:
1c9de5bf428612458427943b724bea51abde520a
and noticed that the original developer properly added fall-through comments in other places in the same switch() code, that gave me the impression he knew what he was doing; then I noticed the following error message in case USB_PORT_FEAT_U2_TIMEOUT:
	if (hcd->speed != HCD_USB3) {
		pr_err("USB_PORT_FEAT_U1/2_TIMEOUT req not "
		       "supported for USB 2.0 roothub\n");
		goto error;
	}
this error message is what makes me think the fall-through is intentional; otherwise I think it would look like this instead:

Quoted 7 lines

Quoted 5 lines
	if (hcd->speed != HCD_USB3) {
		pr_err("USB_PORT_FEAT_U2_TIMEOUT req not "
		       "supported for USB 2.0 roothub\n");
		goto error;
	}
I think you are right, that's horrid, but correct :(

Yes. This hub_control is poorly organized and could use cleanup.

Quoted 3 lines

Will go queue this up, thanks.

Acked-by Shuah Khan <[email protected]>

Thanks Greg!. It is on my list of things to Ack today.

-- Shuah

Gustavo A. R. Silva

author

2019-04-30 17:38 UTC #6 lore raw

In reply to Greg Kroah-Hartman

Quoted 4 lines · Greg Kroah-Hartman

Quoted 4 lines
this error message is what makes me think the fall-through is intentional; otherwise I think it would look like this instead:

Quoted 8 lines

Quoted 5 lines
	if (hcd->speed != HCD_USB3) {
		pr_err("USB_PORT_FEAT_U2_TIMEOUT req not "
		       "supported for USB 2.0 roothub\n");
		goto error;
	}
I think you are right, that's horrid, but correct :(

Yep. :/

Quoted 2 lines

Will go queue this up, thanks.

Thanks, Greg.
---
Gustavo