* usbip: vhci_hcd: Mark expected switch fall-through
@ 2019-04-29 14:39 Gustavo A. R. Silva
2019-04-29 14:39 ` [PATCH] " Gustavo A. R. Silva
2019-04-29 14:44 ` David Laight
0 siblings, 2 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 usbip: vhci_hcd: Mark expected switch fall-through Gustavo A. R. Silva @ 2019-04-29 14:39 ` Gustavo A. R. Silva 2019-04-29 14:44 ` David Laight 1 sibling, 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 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 1 sibling, 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 2019-04-29 15:34 ` David Laight 2019-04-30 15:44 ` Greg Kroah-Hartman 0 siblings, 2 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 2019-04-29 15:34 ` [PATCH] " David Laight 0 siblings, 1 reply; 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 @ 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 2019-04-30 15:44 ` [PATCH] " Greg Kroah-Hartman ` (2 more replies) 0 siblings, 3 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 @ 2019-04-30 15:44 ` Greg Kroah-Hartman 2019-04-30 15:49 ` Shuah Khan 2019-04-30 17:38 ` Gustavo A. R. Silva 2 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 Khan 2019-04-30 15:49 ` [PATCH] " shuah 0 siblings, 1 reply; 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 Khan @ 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 2019-04-30 17:38 ` [PATCH] " Gustavo A. R. Silva 0 siblings, 1 reply; 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 @ 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 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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox