[PATCH] usbip: vhci_hcd: Mark expected switch fall-through

[PATCH] usbip: vhci_hcd: Mark expected switch fall-through

[Gustavo A. R. Silva]

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 <gustavo@embeddedor.com>
---
 drivers/usb/usbip/vhci_hcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 667d9c0ec905..000ab7225717 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -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");
-- 
2.21.0

RE: [PATCH] usbip: vhci_hcd: Mark expected switch fall-through

[David Laight]

From: Gustavo A. R. Silva
> Sent: 29 April 2019 15:40
> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
...
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 667d9c0ec905..000ab7225717 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -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)

Re: [PATCH] usbip: vhci_hcd: Mark expected switch fall-through

[Gustavo A. R. Silva]

On 4/29/19 9:44 AM, David Laight wrote:
> From: Gustavo A. R. Silva
>> Sent: 29 April 2019 15:40
>> In preparation to enabling -Wimplicit-fallthrough, mark switch
>> cases where we are expecting to fall through.
> ...
>> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
>> index 667d9c0ec905..000ab7225717 100644
>> --- a/drivers/usb/usbip/vhci_hcd.c
>> +++ b/drivers/usb/usbip/vhci_hcd.c
>> @@ -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

RE: [PATCH] usbip: vhci_hcd: Mark expected switch fall-through

[David Laight]

From: Gustavo A. R. Silva
> Sent: 29 April 2019 16:06
> On 4/29/19 9:44 AM, David Laight wrote:
> > From: Gustavo A. R. Silva
> >> Sent: 29 April 2019 15:40
> >> In preparation to enabling -Wimplicit-fallthrough, mark switch
> >> cases where we are expecting to fall through.
> > ...
> >> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> >> index 667d9c0ec905..000ab7225717 100644
> >> --- a/drivers/usb/usbip/vhci_hcd.c
> >> +++ b/drivers/usb/usbip/vhci_hcd.c
> >> @@ -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;
> 	}

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)

Re: [PATCH] usbip: vhci_hcd: Mark expected switch fall-through

[Greg Kroah-Hartman]

On Mon, Apr 29, 2019 at 10:05:51AM -0500, Gustavo A. R. Silva wrote:
> 
> 
> On 4/29/19 9:44 AM, David Laight wrote:
> > From: Gustavo A. R. Silva
> >> Sent: 29 April 2019 15:40
> >> In preparation to enabling -Wimplicit-fallthrough, mark switch
> >> cases where we are expecting to fall through.
> > ...
> >> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> >> index 667d9c0ec905..000ab7225717 100644
> >> --- a/drivers/usb/usbip/vhci_hcd.c
> >> +++ b/drivers/usb/usbip/vhci_hcd.c
> >> @@ -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;
> 	}

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

Will go queue this up, thanks.

greg k-h

Re: [PATCH] usbip: vhci_hcd: Mark expected switch fall-through

[shuah]

On 4/30/19 9:44 AM, Greg Kroah-Hartman wrote:
> On Mon, Apr 29, 2019 at 10:05:51AM -0500, Gustavo A. R. Silva wrote:
>>
>>
>> On 4/29/19 9:44 AM, David Laight wrote:
>>> From: Gustavo A. R. Silva
>>>> Sent: 29 April 2019 15:40
>>>> In preparation to enabling -Wimplicit-fallthrough, mark switch
>>>> cases where we are expecting to fall through.
>>> ...
>>>> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
>>>> index 667d9c0ec905..000ab7225717 100644
>>>> --- a/drivers/usb/usbip/vhci_hcd.c
>>>> +++ b/drivers/usb/usbip/vhci_hcd.c
>>>> @@ -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;
>> 	}
> 
> I think you are right, that's horrid, but correct :(

Yes. This hub_control is poorly organized and could use cleanup.
> 
> Will go queue this up, thanks.
> 

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

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

-- Shuah

Re: [PATCH] usbip: vhci_hcd: Mark expected switch fall-through

[Gustavo A. R. Silva]

On 4/30/19 10:44 AM, Greg Kroah-Hartman wrote:

>>
>> 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 :(
> 

Yep. :/

> Will go queue this up, thanks.
> 

Thanks, Greg.
--
Gustavo