Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs
From: Greg Kroah-Hartman
Date: Thu May 02 2019 - 10:47:59 EST
On Thu, May 02, 2019 at 04:40:41PM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 02, 2019 at 09:28:37AM -0500, Gustavo A. R. Silva wrote:
> >
> >
> > On 5/2/19 8:56 AM, Johan Hovold wrote:
> > > On Thu, May 02, 2019 at 08:22:30AM -0500, Gustavo A. R. Silva wrote:
> > >>
> > >>
> > >> On 5/2/19 5:26 AM, Johan Hovold wrote:
> > >>> On Wed, May 01, 2019 at 04:33:29PM -0500, Gustavo A. R. Silva wrote:
> > >>>> In preparation to enabling -Wimplicit-fallthrough, mark switch
> > >>>> cases where we are expecting to fall through.
> > >>>>
> > >>>> This patch fixes the following warnings:
> > >>>>
> > >>>> drivers/usb/serial/io_edgeport.c: In function âprocess_rcvd_dataâ:
> > >>>> drivers/usb/serial/io_edgeport.c:1750:7: warning: this statement may fall through [-Wimplicit-fallthrough=]
> > >>>> if (bufferLength == 0) {
> > >>>> ^
> > >>>> drivers/usb/serial/io_edgeport.c:1755:3: note: here
> > >>>> case EXPECT_HDR2:
> > >>>> ^~~~
> > >>>> drivers/usb/serial/io_edgeport.c:1810:8: warning: this statement may fall through [-Wimplicit-fallthrough=]
> > >>>> if (bufferLength == 0) {
> > >>>> ^
> > >>>> drivers/usb/serial/io_edgeport.c:1816:3: note: here
> > >>>> case EXPECT_DATA: /* Expect data */
> > >>>> ^~~~
> > >>>>
> > >>>> Warning level 3 was used: -Wimplicit-fallthrough=3
> > >>>>
> > >>>> Notice that, in this particular case, the code comments are modified
> > >>>> in accordance with what GCC is expecting to find.
> > >>>>
> > >>>> This patch is part of the ongoing efforts to enable
> > >>>> -Wimplicit-fallthrough.
> > >>>>
> > >>>> Signed-off-by: Gustavo A. R. Silva <gustavo@xxxxxxxxxxxxxx>
> > >>>> ---
> > >>>> Changes in v2:
> > >>>> - Warning level 3 is now used: -Wimplicit-fallthrough=3
> > >>>> instead of warning level 2.
> > >>>> - All warnings in the switch statement are addressed now.
> > >>>>
> > >>>> Notice that these are the last remaining fall-through warnings
> > >>>> in the USB subsystem. :)
> > >>>
> > >>>> drivers/usb/serial/io_edgeport.c | 3 ++-
> > >>>> 1 file changed, 2 insertions(+), 1 deletion(-)
> > >>>>
> > >>>> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> > >>>> index 4ca31c0e4174..7ad10328f4e2 100644
> > >>>> --- a/drivers/usb/serial/io_edgeport.c
> > >>>> +++ b/drivers/usb/serial/io_edgeport.c
> > >>>> @@ -1751,7 +1751,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> > >>>> edge_serial->rxState = EXPECT_HDR2;
> > >>>> break;
> > >>>> }
> > >>>> - /* otherwise, drop on through */
> > >>>> + /* Fall through - otherwise, drop on through */
> > >>>> case EXPECT_HDR2:
> > >>>> edge_serial->rxHeader2 = *buffer;
> > >>>> ++buffer;
> > >>>> @@ -1813,6 +1813,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> > >>>> }
> > >>>> /* Else, drop through */
> > >>>> }
> > >>>> + /* Fall through */
> > >>>> case EXPECT_DATA: /* Expect data */
> > >>>
> > >>> Looks like you forgot to take the original review feedback you got into
> > >>> account:
> > >>>
> > >>> https://lkml.kernel.org/r/87k1zf4k24.fsf@xxxxxxxxxxxxxxxxx
> > >>>
> > >>
> > >> Oh, the thing is that the fall-through comments have to be placed at
> > >> the very bottom of the case. Also, based on that feedback, this time
> > >> I left the "Else, drop through" comment in place, so people can be
> > >> informed that such fall-through is conditional.
> > >>
> > >> What do you think about this:
> > >>
> > >> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> > >> index 4ca31c0e4174..52f27fc82563 100644
> > >> --- a/drivers/usb/serial/io_edgeport.c
> > >> +++ b/drivers/usb/serial/io_edgeport.c
> > >> @@ -1751,7 +1751,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> > >> edge_serial->rxState = EXPECT_HDR2;
> > >> break;
> > >> }
> > >> - /* otherwise, drop on through */
> > >> + /* Fall through - otherwise, drop on through */
> > >> case EXPECT_HDR2:
> > >> edge_serial->rxHeader2 = *buffer;
> > >> ++buffer;
> > >> @@ -1813,6 +1813,11 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> > >> }
> > >> /* Else, drop through */
> > >> }
> > >> + /* Beware that, currently, there are at least three
> > >> + * break statements in this case block, so the
> > >> + * fall-through marked below is NOT unconditional.
> > >> + */
> > >> + /* Fall through */
> > >> case EXPECT_DATA: /* Expect data */
> > >> if (bufferLength < edge_serial->rxBytesRemaining) {
> > >> rxLen = bufferLength;
> > >
> > > It's better than v2, but I thought you said you were gonna look into
> > > restructuring the code to maintain (or even improve) readability?
> > >
> >
> > At first, I thought about that, but now I don't think that's realistic.
> > I'd turn the if-else into a switch, and based on the history of feedback
> > on this patch, we will end up having the same complains about the break
> > statements in that new switch and the possibility of a fall-through to
> > case EXPECT_DATA. At the end I would still have to add a comment explaining
> > that the last fall-through mark in unconditional.
>
> I love it how no one is blaming the original author of this code (i.e.
> me...)
>
> Let me see if I can fix it up to be more "sane", this is my fault.
How about the following patch? Johan, this look nicer to you? It makes
more sense to me.
And in looking at the history, I can't claim total credit for this
monstrosity, it was originally written by someone else, I just "cleaned
it up" back in 2001, to get it into mergable shape. Clearly "mergable
shape" was much looser back then :)
thanks,
greg k-h
----------------
diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index 4ca31c0e4174..732081b3718f 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -1751,7 +1751,8 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
edge_serial->rxState = EXPECT_HDR2;
break;
}
- /* otherwise, drop on through */
+ /* Fall through */
+
case EXPECT_HDR2:
edge_serial->rxHeader2 = *buffer;
++buffer;
@@ -1790,29 +1791,21 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
edge_serial->rxHeader2, 0);
edge_serial->rxState = EXPECT_HDR1;
break;
- } else {
- edge_serial->rxPort =
- IOSP_GET_HDR_PORT(edge_serial->rxHeader1);
- edge_serial->rxBytesRemaining =
- IOSP_GET_HDR_DATA_LEN(
- edge_serial->rxHeader1,
- edge_serial->rxHeader2);
- dev_dbg(dev, "%s - Data for Port %u Len %u\n",
- __func__,
- edge_serial->rxPort,
- edge_serial->rxBytesRemaining);
-
- /* ASSERT(DevExt->RxPort < DevExt->NumPorts);
- * ASSERT(DevExt->RxBytesRemaining <
- * IOSP_MAX_DATA_LENGTH);
- */
-
- if (bufferLength == 0) {
- edge_serial->rxState = EXPECT_DATA;
- break;
- }
- /* Else, drop through */
}
+
+ edge_serial->rxPort = IOSP_GET_HDR_PORT(edge_serial->rxHeader1);
+ edge_serial->rxBytesRemaining = IOSP_GET_HDR_DATA_LEN(edge_serial->rxHeader1,
+ edge_serial->rxHeader2);
+ dev_dbg(dev, "%s - Data for Port %u Len %u\n", __func__,
+ edge_serial->rxPort,
+ edge_serial->rxBytesRemaining);
+
+ if (bufferLength == 0) {
+ edge_serial->rxState = EXPECT_DATA;
+ break;
+ }
+ /* Fall through */
+
case EXPECT_DATA: /* Expect data */
if (bufferLength < edge_serial->rxBytesRemaining) {
rxLen = bufferLength;