USB: serial: io_edgeport: mark expected switch fall-throughs

PATCH In review Gustavo A. R. Silva 3 participants 11 messages May 1, 2019 – May 2, 2019

Subsystems (2)

USB USB serial

lore

Share & tools

lore

Gustavo A. R. Silva

author

2019-05-01 21:33 UTC #0 lore raw

PATCH v2 1/1 USB: serial: io_edgeport: mark expected switch fall-throughs

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 <[email protected]>

---
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(-)

drivers/usb/serial/io_edgeport.c +2 -1

index 4ca31c0e4174..7ad10328f4e2 100644
@@ -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;

Gustavo A. R. Silva

author

2019-05-02 13:22 UTC #2 lore raw

Quoted 64 lines · Johan Hovold

Quoted 54 lines · Gustavo A. R. Silva
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 <[email protected]>
---
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(-)
drivers/usb/serial/io_edgeport.c +1 -1
index 4ca31c0e4174..7ad10328f4e2 100644
@@ -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;
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1817
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +1 -0
@@ -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:

Quoted 2 lines

	https://lkml.kernel.org/r/[email protected]

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

drivers/usb/serial/io_edgeport.c +6 -1

index 4ca31c0e4174..52f27fc82563 100644
@@ -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;

Johan Hovold

maintainer

2019-05-02 13:56 UTC #3 lore raw

Quoted 92 lines · Gustavo A. R. Silva

Quoted 64 lines · Johan Hovold
Quoted 54 lines · Gustavo A. R. Silva
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 <[email protected]>
---
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(-)
drivers/usb/serial/io_edgeport.c +1 -1
index 4ca31c0e4174..7ad10328f4e2 100644
@@ -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;
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1817
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +1 -0
@@ -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:
Quoted 2 lines
	https://lkml.kernel.org/r/[email protected]
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:
drivers/usb/serial/io_edgeport.c +1 -1
index 4ca31c0e4174..52f27fc82563 100644
@@ -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,
                                }

drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1823

Quoted 10 lines

drivers/usb/serial/io_edgeport.c +5 -0

                                /* 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

Gustavo A. R. Silva

author

2019-05-02 14:28 UTC #4 lore raw

Quoted 98 lines · Johan Hovold

Quoted 97 lines · Gustavo A. R. Silva
Quoted 64 lines · Johan Hovold
Quoted 54 lines · Gustavo A. R. Silva
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 <[email protected]>
---
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(-)
drivers/usb/serial/io_edgeport.c +1 -1
index 4ca31c0e4174..7ad10328f4e2 100644
@@ -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;
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1817
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +1 -0
@@ -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:
Quoted 2 lines
	https://lkml.kernel.org/r/[email protected]
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:
drivers/usb/serial/io_edgeport.c +4 -1
index 4ca31c0e4174..52f27fc82563 100644
@@ -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.

Quoted 9 lines

drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1823
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +2 -0
+                        */
+                       /* 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

Greg Kroah-Hartman

maintainer

2019-05-02 14:40 UTC #5 lore raw

Quoted 111 lines · Gustavo A. R. Silva

Quoted 98 lines · Johan Hovold
Quoted 97 lines · Gustavo A. R. Silva
Quoted 64 lines · Johan Hovold
Quoted 54 lines · Gustavo A. R. Silva
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 <[email protected]>
---
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(-)
drivers/usb/serial/io_edgeport.c +1 -1
index 4ca31c0e4174..7ad10328f4e2 100644
@@ -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;
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1817
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +1 -0
@@ -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:
Quoted 2 lines
	https://lkml.kernel.org/r/[email protected]
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:
drivers/usb/serial/io_edgeport.c +4 -1
index 4ca31c0e4174..52f27fc82563 100644
@@ -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.
Quoted 9 lines
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1823
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +2 -0
+                        */
+                       /* 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?

Quoted 6 lines

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

Greg Kroah-Hartman

maintainer

2019-05-02 14:47 UTC #6 lore raw

Quoted 119 lines · Greg Kroah-Hartman

Quoted 111 lines · Gustavo A. R. Silva
Quoted 98 lines · Johan Hovold
Quoted 97 lines · Gustavo A. R. Silva
Quoted 64 lines · Johan Hovold
Quoted 54 lines · Gustavo A. R. Silva
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 <[email protected]>
---
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(-)
drivers/usb/serial/io_edgeport.c +1 -1
index 4ca31c0e4174..7ad10328f4e2 100644
@@ -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;
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1817
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +1 -0
@@ -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:
Quoted 2 lines
	https://lkml.kernel.org/r/[email protected]
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:
drivers/usb/serial/io_edgeport.c +4 -1
index 4ca31c0e4174..52f27fc82563 100644
@@ -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.
Quoted 9 lines
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1823
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +2 -0
+                        */
+                       /* 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?
Quoted 6 lines
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.

Quoted 4 lines

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

----------------

drivers/usb/serial/io_edgeport.c +16 ~~-23~~

index 4ca31c0e4174..732081b3718f 100644
@@ -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;

Gustavo A. R. Silva

author

2019-05-02 14:57 UTC #7 lore raw

Quoted 120 lines · Greg Kroah-Hartman

Quoted 119 lines · Greg Kroah-Hartman
Quoted 117 lines · Gustavo A. R. Silva
Quoted 98 lines · Johan Hovold
Quoted 97 lines · Gustavo A. R. Silva
Quoted 64 lines · Johan Hovold
Quoted 54 lines · Gustavo A. R. Silva
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 <[email protected]>
---
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(-)
drivers/usb/serial/io_edgeport.c +1 -1
index 4ca31c0e4174..7ad10328f4e2 100644
@@ -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;
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1817
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +1 -0
@@ -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:
Quoted 2 lines
	https://lkml.kernel.org/r/[email protected]
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:
drivers/usb/serial/io_edgeport.c +4 -1
index 4ca31c0e4174..52f27fc82563 100644
@@ -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.
Quoted 9 lines
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1811
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +2 -0
+                        */
+                       /* 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.

Quoted 8 lines

Quoted 4 lines
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

Quoted 70 lines

thanks,

greg k-h

----------------

drivers/usb/serial/io_edgeport.c +16 ~~-23~~

index 4ca31c0e4174..732081b3718f 100644
@@ -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;

Greg Kroah-Hartman

maintainer

2019-05-02 15:04 UTC #8 lore raw

Quoted 126 lines · Gustavo A. R. Silva

Quoted 123 lines · Greg Kroah-Hartman
Quoted 122 lines · Greg Kroah-Hartman
Quoted 111 lines · Gustavo A. R. Silva
Quoted 98 lines · Johan Hovold
Quoted 97 lines · Gustavo A. R. Silva
Quoted 64 lines · Johan Hovold
Quoted 54 lines · Gustavo A. R. Silva
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 <[email protected]>
---
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(-)
drivers/usb/serial/io_edgeport.c +1 -1
index 4ca31c0e4174..7ad10328f4e2 100644
@@ -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;
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1817
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +1 -0
@@ -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:
Quoted 2 lines
	https://lkml.kernel.org/r/[email protected]
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:
drivers/usb/serial/io_edgeport.c +4 -1
index 4ca31c0e4174..52f27fc82563 100644
@@ -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.
Quoted 9 lines
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1811
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +2 -0
+                        */
+                       /* 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?
Quoted 6 lines
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...)

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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:

Quoted 11 lines

drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1755
Quoted 11 lines
drivers/usb/serial/io_edgeport.c +2 -1
index 4ca31c0e4174..732081b3718f 100644
@@ -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.

Quoted 7 lines

drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1793
Quoted 7 lines
drivers/usb/serial/io_edgeport.c +0 -0
 		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:

Quoted 29 lines

drivers/usb/serial/io_edgeport.c +6 ~~-22~~

-			} 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,

Quoted 8 lines

drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1808
Quoted 8 lines
drivers/usb/serial/io_edgeport.c +8 -0
+				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

Johan Hovold

maintainer

2019-05-02 15:14 UTC #9 lore raw

In reply to Greg Kroah-Hartman

Quoted 120 lines · Greg Kroah-Hartman

Quoted 119 lines · Greg Kroah-Hartman
Quoted 117 lines · Gustavo A. R. Silva
Quoted 98 lines · Johan Hovold
Quoted 97 lines · Gustavo A. R. Silva
Quoted 64 lines · Johan Hovold
Quoted 54 lines · Gustavo A. R. Silva
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 <[email protected]>
---
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(-)
drivers/usb/serial/io_edgeport.c +1 -1
index 4ca31c0e4174..7ad10328f4e2 100644
@@ -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;
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1817
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +1 -0
@@ -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:
Quoted 2 lines
	https://lkml.kernel.org/r/[email protected]
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:
drivers/usb/serial/io_edgeport.c +4 -1
index 4ca31c0e4174..52f27fc82563 100644
@@ -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.
Quoted 9 lines
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1811
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +2 -0
+                        */
+                       /* 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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 4 lines

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.

drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1755

Quoted 11 lines

drivers/usb/serial/io_edgeport.c +2 -1

index 4ca31c0e4174..732081b3718f 100644
@@ -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.

drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1756

Quoted 1 line

drivers/usb/serial/io_edgeport.c +0 -0
 		case EXPECT_HDR2:

Thanks,
Johan

Johan Hovold

maintainer

2019-05-02 14:48 UTC #10 lore raw

In reply to Greg Kroah-Hartman

Quoted 59 lines · Greg Kroah-Hartman

Quoted 50 lines · Gustavo A. R. Silva
Quoted 40 lines
Quoted 40 lines
Quoted 10 lines
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1817
Quoted 6 lines
drivers/usb/serial/io_edgeport.c +1 -0
@@ -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:
Quoted 2 lines
	https://lkml.kernel.org/r/[email protected]
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:
drivers/usb/serial/io_edgeport.c +4 -1
index 4ca31c0e4174..52f27fc82563 100644
@@ -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.
Quoted 9 lines
drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1823
Quoted 5 lines
drivers/usb/serial/io_edgeport.c +2 -0
+                        */
+                       /* 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?
Quoted 6 lines
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.

Quoted 4 lines

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