All of lore.kernel.org
 help / color / mirror / Atom feed
* usbip: vhci_hcd: Mark expected switch fall-through
@ 2019-04-29 14:39 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 12+ messages in thread
From: Gustavo A. R. Silva @ 2019-04-29 14:39 UTC (permalink / raw)
  To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman
  Cc: linux-usb, linux-kernel, Gustavo A. R. Silva, Kees Cook

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");

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH] usbip: vhci_hcd: Mark expected switch fall-through
@ 2019-04-29 14:39 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 12+ messages in thread
From: Gustavo A. R. Silva @ 2019-04-29 14:39 UTC (permalink / raw)
  To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman
  Cc: linux-usb, linux-kernel, Gustavo A. R. Silva, Kees Cook

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* RE: [PATCH] usbip: vhci_hcd: Mark expected switch fall-through
  2019-04-29 14:39 ` [PATCH] " Gustavo A. R. Silva
  (?)
@ 2019-04-29 14:44 ` David Laight
  2019-04-29 15:05   ` Gustavo A. R. Silva
  -1 siblings, 1 reply; 12+ messages in thread
From: David Laight @ 2019-04-29 14:44 UTC (permalink / raw)
  To: 'Gustavo A. R. Silva', Valentina Manea, Shuah Khan,
	Greg Kroah-Hartman
  Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Kees Cook

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)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usbip: vhci_hcd: Mark expected switch fall-through
  2019-04-29 14:44 ` David Laight
@ 2019-04-29 15:05   ` Gustavo A. R. Silva
  0 siblings, 0 replies; 12+ messages in thread
From: Gustavo A. R. Silva @ 2019-04-29 15:05 UTC (permalink / raw)
  To: David Laight, Valentina Manea, Shuah Khan, Greg Kroah-Hartman
  Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Kees Cook



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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* usbip: vhci_hcd: Mark expected switch fall-through
@ 2019-04-29 15:34 ` David Laight
  0 siblings, 0 replies; 12+ messages in thread
From: David Laight @ 2019-04-29 15:34 UTC (permalink / raw)
  To: 'Gustavo A. R. Silva', Valentina Manea, Shuah Khan,
	Greg Kroah-Hartman
  Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Kees Cook

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)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] usbip: vhci_hcd: Mark expected switch fall-through
@ 2019-04-29 15:34 ` David Laight
  0 siblings, 0 replies; 12+ messages in thread
From: David Laight @ 2019-04-29 15:34 UTC (permalink / raw)
  To: 'Gustavo A. R. Silva', Valentina Manea, Shuah Khan,
	Greg Kroah-Hartman
  Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Kees Cook

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)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* usbip: vhci_hcd: Mark expected switch fall-through
@ 2019-04-30 15:44 ` Greg Kroah-Hartman
  0 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2019-04-30 15:44 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: David Laight, Valentina Manea, Shuah Khan,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Kees Cook

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usbip: vhci_hcd: Mark expected switch fall-through
@ 2019-04-30 15:44 ` Greg Kroah-Hartman
  0 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2019-04-30 15:44 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: David Laight, Valentina Manea, Shuah Khan,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Kees Cook

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* usbip: vhci_hcd: Mark expected switch fall-through
@ 2019-04-30 15:49 ` shuah
  0 siblings, 0 replies; 12+ messages in thread
From: Shuah Khan @ 2019-04-30 15:49 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Gustavo A. R. Silva
  Cc: David Laight, Valentina Manea, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, Kees Cook, 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usbip: vhci_hcd: Mark expected switch fall-through
@ 2019-04-30 15:49 ` shuah
  0 siblings, 0 replies; 12+ messages in thread
From: shuah @ 2019-04-30 15:49 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Gustavo A. R. Silva
  Cc: David Laight, Valentina Manea, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, Kees Cook, 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* usbip: vhci_hcd: Mark expected switch fall-through
@ 2019-04-30 17:38 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 12+ messages in thread
From: Gustavo A. R. Silva @ 2019-04-30 17:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: David Laight, Valentina Manea, Shuah Khan,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Kees Cook

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] usbip: vhci_hcd: Mark expected switch fall-through
@ 2019-04-30 17:38 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 12+ messages in thread
From: Gustavo A. R. Silva @ 2019-04-30 17:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: David Laight, Valentina Manea, Shuah Khan,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	Kees Cook



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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-04-30 17:59 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-04-29 14:39 usbip: vhci_hcd: Mark expected switch fall-through Gustavo A. R. Silva
2019-04-29 14:39 ` [PATCH] " Gustavo A. R. Silva
2019-04-29 14:44 ` David Laight
2019-04-29 15:05   ` Gustavo A. R. Silva
  -- strict thread matches above, loose matches on Subject: below --
2019-04-29 15:34 David Laight
2019-04-29 15:34 ` [PATCH] " David Laight
2019-04-30 15:44 Greg Kroah-Hartman
2019-04-30 15:44 ` [PATCH] " Greg Kroah-Hartman
2019-04-30 15:49 Shuah Khan
2019-04-30 15:49 ` [PATCH] " shuah
2019-04-30 17:38 Gustavo A. R. Silva
2019-04-30 17:38 ` [PATCH] " Gustavo A. R. Silva

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.