| Message ID | 20190429143957.GA6725@embeddedor (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | b063f04e0d2e7a808f6b5827bd1e39ad89617a22 |
| Headers | show |
| Series | usbip: vhci_hcd: Mark expected switch fall-through | expand |
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)
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
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
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
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");
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(+)