| Message ID | 20190501213329.GA26972@embeddedor (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] USB: serial: io_edgeport: mark expected switch fall-throughs | expand |
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@embeddedor.com> > --- > 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@miraculix.mork.no Johan
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@embeddedor.com> >> --- >> 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@miraculix.mork.no > 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; Thanks -- Gustavo
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@embeddedor.com> > >> --- > >> 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@miraculix.mork.no > > > > 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? Johan
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@embeddedor.com> >>>> --- >>>> 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@miraculix.mork.no >>> >> >> 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. Thanks -- Gustavo
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@embeddedor.com> > >>>> --- > >>>> 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@miraculix.mork.no > >>> > >> > >> 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. thanks, greg k-h
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@embeddedor.com> > > >>>> --- > > >>>> 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@miraculix.mork.no > > >>> > > >> > > >> 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;
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: > > >>>> @@ -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@miraculix.mork.no > > >>> > > >> > > >> 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. Thanks, that'd be great. I haven't looked at it myself in a long time, but judging from the old thread it did not seem impossible at least. Getting rid of some of that deep nesting would be good either way. :) Johan
On 5/2/19 9:47 AM, Greg Kroah-Hartman wrote: > 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@embeddedor.com> >>>>>>> --- >>>>>>> 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@miraculix.mork.no >>>>>> >>>>> >>>>> 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. > Thanks, Greg. Just notice that, unfortunately, the original complains are still applicable to your patch. :/ Thanks -- Gustavo > 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; >
On Thu, May 02, 2019 at 09:57:22AM -0500, Gustavo A. R. Silva wrote: > > > On 5/2/19 9:47 AM, Greg Kroah-Hartman wrote: > > 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@embeddedor.com> > >>>>>>> --- > >>>>>>> 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@miraculix.mork.no > >>>>>> > >>>>> > >>>>> 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. > > > > Thanks, Greg. Just notice that, unfortunately, the original complains are > still applicable to your patch. :/ Really? I got rid of the crazy nesting, it should be more obvious what is happening here now: > > 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 */ > > + "otherwise" makes no sense here, it's obvious that if the buffer isn't big enough we need to pass through and keep parsing. > > 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; The switch case is broken out here, so let's unindent the next block: > > - } 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 */ > > + This should make it more obvious. at least it is to me now, it's much easier to read, and gcc should also be happy about it. Parsers are a pain, you have to handle stuff like this in messy ways. Personally, I'm horrible at parsers, but this really looks to be the simplest way to write this code at the moment. Johan, any thoughts? thanks, greg k-h
On Thu, May 02, 2019 at 04:47:29PM +0200, Greg Kroah-Hartman wrote: > 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@embeddedor.com> > > > >>>> --- > > > >>>> 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@miraculix.mork.no > > > >>> > > > >> > > > >> 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. Hard to say... :) Less indentation is good, but the current flow seems more in line with the comment preceding the if-statement /* Process depending on whether this header is * data or status */ if (IS_CMD_STAT_HDR(edge_serial->rxHeader1)) { a; break; } else { b; } case EXPECT_DATA: /* Expect data */ which sort of gets lost if you just replace the else clause with b. But it still an improvement, let's go with it. > 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 :) Heh. > 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 */ > + I think the comment should go immediately before the case (no newline) here and below though. > case EXPECT_HDR2: Thanks, Johan
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 */ if (bufferLength < edge_serial->rxBytesRemaining) { rxLen = bufferLength;
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@embeddedor.com> --- 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. :) Thanks -- Gustavo drivers/usb/serial/io_edgeport.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)