* [v2] USB: serial: io_edgeport: mark expected switch fall-throughs
@ 2019-05-01 21:33 Gustavo A. R. Silva
2019-05-01 21:33 ` [PATCH v2] " Gustavo A. R. Silva
2019-05-02 10:26 ` [v2] " Johan Hovold
0 siblings, 2 replies; 22+ messages in thread
From: Gustavo A. R. Silva @ 2019-05-01 21:33 UTC (permalink / raw)
To: Johan Hovold, 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 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(-)
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;
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs 2019-05-01 21:33 [v2] USB: serial: io_edgeport: mark expected switch fall-throughs Gustavo A. R. Silva @ 2019-05-01 21:33 ` Gustavo A. R. Silva 2019-05-02 10:26 ` [v2] " Johan Hovold 1 sibling, 0 replies; 22+ messages in thread From: Gustavo A. R. Silva @ 2019-05-01 21:33 UTC (permalink / raw) To: Johan Hovold, 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 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(-) 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; -- 2.21.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [v2] USB: serial: io_edgeport: mark expected switch fall-throughs @ 2019-05-02 10:26 ` Johan Hovold 2019-05-02 10:26 ` [PATCH v2] " Johan Hovold 2019-05-02 13:22 ` [v2] " Gustavo A. R. Silva 0 siblings, 2 replies; 22+ messages in thread From: Johan Hovold @ 2019-05-02 10:26 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs 2019-05-02 10:26 ` [v2] " Johan Hovold @ 2019-05-02 10:26 ` Johan Hovold 2019-05-02 13:22 ` [v2] " Gustavo A. R. Silva 1 sibling, 0 replies; 22+ messages in thread From: Johan Hovold @ 2019-05-02 10:26 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [v2] USB: serial: io_edgeport: mark expected switch fall-throughs @ 2019-05-02 13:22 ` Gustavo A. R. Silva 2019-05-02 13:22 ` [PATCH v2] " Gustavo A. R. Silva 2019-05-02 13:56 ` [v2] " Johan Hovold 0 siblings, 2 replies; 22+ messages in thread From: Gustavo A. R. Silva @ 2019-05-02 13:22 UTC (permalink / raw) To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Kees Cook 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: Thanks --- Gustavo 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; ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs 2019-05-02 13:22 ` [v2] " Gustavo A. R. Silva @ 2019-05-02 13:22 ` Gustavo A. R. Silva 2019-05-02 13:56 ` [v2] " Johan Hovold 1 sibling, 0 replies; 22+ messages in thread From: Gustavo A. R. Silva @ 2019-05-02 13:22 UTC (permalink / raw) To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [v2] USB: serial: io_edgeport: mark expected switch fall-throughs @ 2019-05-02 13:56 ` Johan Hovold 2019-05-02 13:56 ` [PATCH v2] " Johan Hovold 2019-05-02 14:28 ` [v2] " Gustavo A. R. Silva 0 siblings, 2 replies; 22+ messages in thread From: Johan Hovold @ 2019-05-02 13:56 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs 2019-05-02 13:56 ` [v2] " Johan Hovold @ 2019-05-02 13:56 ` Johan Hovold 2019-05-02 14:28 ` [v2] " Gustavo A. R. Silva 1 sibling, 0 replies; 22+ messages in thread From: Johan Hovold @ 2019-05-02 13:56 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [v2] USB: serial: io_edgeport: mark expected switch fall-throughs @ 2019-05-02 14:28 ` Gustavo A. R. Silva 2019-05-02 14:28 ` [PATCH v2] " Gustavo A. R. Silva 2019-05-02 14:40 ` [v2] " Greg Kroah-Hartman 0 siblings, 2 replies; 22+ messages in thread From: Gustavo A. R. Silva @ 2019-05-02 14:28 UTC (permalink / raw) To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs 2019-05-02 14:28 ` [v2] " Gustavo A. R. Silva @ 2019-05-02 14:28 ` Gustavo A. R. Silva 2019-05-02 14:40 ` [v2] " Greg Kroah-Hartman 1 sibling, 0 replies; 22+ messages in thread From: Gustavo A. R. Silva @ 2019-05-02 14:28 UTC (permalink / raw) To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [v2] USB: serial: io_edgeport: mark expected switch fall-throughs @ 2019-05-02 14:40 ` Greg Kroah-Hartman 2019-05-02 14:40 ` [PATCH v2] " Greg Kroah-Hartman ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Greg Kroah-Hartman @ 2019-05-02 14:40 UTC (permalink / raw) To: Gustavo A. R. Silva; +Cc: Johan Hovold, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs 2019-05-02 14:40 ` [v2] " Greg Kroah-Hartman @ 2019-05-02 14:40 ` Greg Kroah-Hartman 2019-05-02 14:47 ` [v2] " Greg Kroah-Hartman 2019-05-02 14:48 ` [v2] " Johan Hovold 2 siblings, 0 replies; 22+ messages in thread From: Greg Kroah-Hartman @ 2019-05-02 14:40 UTC (permalink / raw) To: Gustavo A. R. Silva; +Cc: Johan Hovold, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [v2] USB: serial: io_edgeport: mark expected switch fall-throughs @ 2019-05-02 14:47 ` Greg Kroah-Hartman 2019-05-02 14:47 ` [PATCH v2] " Greg Kroah-Hartman ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Greg Kroah-Hartman @ 2019-05-02 14:47 UTC (permalink / raw) To: Gustavo A. R. Silva; +Cc: Johan Hovold, linux-usb, linux-kernel, Kees Cook 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; ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs 2019-05-02 14:47 ` [v2] " Greg Kroah-Hartman @ 2019-05-02 14:47 ` Greg Kroah-Hartman 2019-05-02 14:57 ` [v2] " Gustavo A. R. Silva 2019-05-02 15:14 ` [v2] " Johan Hovold 2 siblings, 0 replies; 22+ messages in thread From: Greg Kroah-Hartman @ 2019-05-02 14:47 UTC (permalink / raw) To: Gustavo A. R. Silva; +Cc: Johan Hovold, linux-usb, linux-kernel, Kees Cook 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; ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [v2] USB: serial: io_edgeport: mark expected switch fall-throughs @ 2019-05-02 14:57 ` Gustavo A. R. Silva 2019-05-02 14:57 ` [PATCH v2] " Gustavo A. R. Silva 2019-05-02 15:04 ` [v2] " Greg Kroah-Hartman 0 siblings, 2 replies; 22+ messages in thread From: Gustavo A. R. Silva @ 2019-05-02 14:57 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Johan Hovold, linux-usb, linux-kernel, Kees Cook 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; > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs 2019-05-02 14:57 ` [v2] " Gustavo A. R. Silva @ 2019-05-02 14:57 ` Gustavo A. R. Silva 2019-05-02 15:04 ` [v2] " Greg Kroah-Hartman 1 sibling, 0 replies; 22+ messages in thread From: Gustavo A. R. Silva @ 2019-05-02 14:57 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Johan Hovold, linux-usb, linux-kernel, Kees Cook 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; > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [v2] USB: serial: io_edgeport: mark expected switch fall-throughs @ 2019-05-02 15:04 ` Greg Kroah-Hartman 2019-05-02 15:04 ` [PATCH v2] " Greg Kroah-Hartman 0 siblings, 1 reply; 22+ messages in thread From: Greg Kroah-Hartman @ 2019-05-02 15:04 UTC (permalink / raw) To: Gustavo A. R. Silva; +Cc: Johan Hovold, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs 2019-05-02 15:04 ` [v2] " Greg Kroah-Hartman @ 2019-05-02 15:04 ` Greg Kroah-Hartman 0 siblings, 0 replies; 22+ messages in thread From: Greg Kroah-Hartman @ 2019-05-02 15:04 UTC (permalink / raw) To: Gustavo A. R. Silva; +Cc: Johan Hovold, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [v2] USB: serial: io_edgeport: mark expected switch fall-throughs @ 2019-05-02 15:14 ` Johan Hovold 2019-05-02 15:14 ` [PATCH v2] " Johan Hovold 0 siblings, 1 reply; 22+ messages in thread From: Johan Hovold @ 2019-05-02 15:14 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Gustavo A. R. Silva, Johan Hovold, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs 2019-05-02 15:14 ` [v2] " Johan Hovold @ 2019-05-02 15:14 ` Johan Hovold 0 siblings, 0 replies; 22+ messages in thread From: Johan Hovold @ 2019-05-02 15:14 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Gustavo A. R. Silva, Johan Hovold, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [v2] USB: serial: io_edgeport: mark expected switch fall-throughs @ 2019-05-02 14:48 ` Johan Hovold 2019-05-02 14:48 ` [PATCH v2] " Johan Hovold 0 siblings, 1 reply; 22+ messages in thread From: Johan Hovold @ 2019-05-02 14:48 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Gustavo A. R. Silva, Johan Hovold, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs 2019-05-02 14:48 ` [v2] " Johan Hovold @ 2019-05-02 14:48 ` Johan Hovold 0 siblings, 0 replies; 22+ messages in thread From: Johan Hovold @ 2019-05-02 14:48 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Gustavo A. R. Silva, Johan Hovold, linux-usb, linux-kernel, Kees Cook 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-05-02 15:44 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-05-01 21:33 [v2] USB: serial: io_edgeport: mark expected switch fall-throughs Gustavo A. R. Silva 2019-05-01 21:33 ` [PATCH v2] " Gustavo A. R. Silva 2019-05-02 10:26 ` [v2] " Johan Hovold 2019-05-02 10:26 ` [PATCH v2] " Johan Hovold 2019-05-02 13:22 ` [v2] " Gustavo A. R. Silva 2019-05-02 13:22 ` [PATCH v2] " Gustavo A. R. Silva 2019-05-02 13:56 ` [v2] " Johan Hovold 2019-05-02 13:56 ` [PATCH v2] " Johan Hovold 2019-05-02 14:28 ` [v2] " Gustavo A. R. Silva 2019-05-02 14:28 ` [PATCH v2] " Gustavo A. R. Silva 2019-05-02 14:40 ` [v2] " Greg Kroah-Hartman 2019-05-02 14:40 ` [PATCH v2] " Greg Kroah-Hartman 2019-05-02 14:47 ` [v2] " Greg Kroah-Hartman 2019-05-02 14:47 ` [PATCH v2] " Greg Kroah-Hartman 2019-05-02 14:57 ` [v2] " Gustavo A. R. Silva 2019-05-02 14:57 ` [PATCH v2] " Gustavo A. R. Silva 2019-05-02 15:04 ` [v2] " Greg Kroah-Hartman 2019-05-02 15:04 ` [PATCH v2] " Greg Kroah-Hartman 2019-05-02 15:14 ` [v2] " Johan Hovold 2019-05-02 15:14 ` [PATCH v2] " Johan Hovold 2019-05-02 14:48 ` [v2] " Johan Hovold 2019-05-02 14:48 ` [PATCH v2] " Johan Hovold
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).