[PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Notice that in this particular case I replaced "...drop on through"
comments with a proper "fall through" comment on its own line, which
is what GCC is expecting to find.

Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 drivers/usb/serial/io_edgeport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index bdf8bd8..8d96e12 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -1759,7 +1759,7 @@ 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;
@@ -1819,8 +1819,8 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
 					edge_serial->rxState = EXPECT_DATA;
 					break;
 				}
-				/* Else, drop through */
 			}
+			/* fall through */
 		case EXPECT_DATA: /* Expect data */
 			if (bufferLength < edge_serial->rxBytesRemaining) {
 				rxLen = bufferLength;
-- 
2.7.4

Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[Bjørn Mork]

"Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:

> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Notice that in this particular case I replaced "...drop on through"
> comments with a proper "fall through" comment on its own line, which
> is what GCC is expecting to find.

Sounds to me like GCC is the wrong tool for this.  But I would of course
not mind if it was *just* the text.  However, as your patch cleary
shows, the simplified logic leads to real problems:

> @@ -1819,8 +1819,8 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
>  					edge_serial->rxState = EXPECT_DATA;
>  					break;
>  				}
> -				/* Else, drop through */
>  			}
> +			/* fall through */
>  		case EXPECT_DATA: /* Expect data */
>  			if (bufferLength < edge_serial->rxBytesRemaining) {
>  				rxLen = bufferLength;


The original comment clearly marked a *conditional* fall through at the
correct place.  Your patch makes it appear as if there is an
unconditional fall through here.  There is not.  The fallthrough only
applies to one of a number of nested if blocks. There are no less than
3 break statements in the same case block.

Not a big deal maybe, just as the lack of any "fall through" comment
isn't a big deal in the first place.  But this change is clearly making
this code harder to read, and the change is therefore harmful IMHO.

If you can't make -Wimplicit-fallthrough work without removing such
precise fallthrough markings, then my proposal is to drop it and use
some other tool.


Bjørn

Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[Gustavo A. R. Silva]

Quoting Bjørn Mork <bjorn@mork.no>:

> "Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
>
>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> where we are expecting to fall through.
>>
>> Notice that in this particular case I replaced "...drop on through"
>> comments with a proper "fall through" comment on its own line, which
>> is what GCC is expecting to find.
>
> Sounds to me like GCC is the wrong tool for this.  But I would of course
> not mind if it was *just* the text.  However, as your patch cleary
> shows, the simplified logic leads to real problems:
>
>> @@ -1819,8 +1819,8 @@ static void process_rcvd_data(struct  
>> edgeport_serial *edge_serial,
>>  					edge_serial->rxState = EXPECT_DATA;
>>  					break;
>>  				}
>> -				/* Else, drop through */
>>  			}
>> +			/* fall through */
>>  		case EXPECT_DATA: /* Expect data */
>>  			if (bufferLength < edge_serial->rxBytesRemaining) {
>>  				rxLen = bufferLength;
>
>
> The original comment clearly marked a *conditional* fall through at the
> correct place.  Your patch makes it appear as if there is an
> unconditional fall through here.  There is not.  The fallthrough only
> applies to one of a number of nested if blocks. There are no less than
> 3 break statements in the same case block.
>

I see.
You are right.

> Not a big deal maybe, just as the lack of any "fall through" comment
> isn't a big deal in the first place.  But this change is clearly making
> this code harder to read, and the change is therefore harmful IMHO.
>
> If you can't make -Wimplicit-fallthrough work without removing such
> precise fallthrough markings, then my proposal is to drop it and use
> some other tool.
>

I will talk with the hardening guys to see what we can do about this.

I appreciate for your comments.
Thanks
--
Gustavo A. R. Silva

RE: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[David Laight]

From: Bjørn Mork
> Sent: 28 October 2017 11:57
> > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > where we are expecting to fall through.
> >
> > Notice that in this particular case I replaced "...drop on through"
> > comments with a proper "fall through" comment on its own line, which
> > is what GCC is expecting to find.
> 
> Sounds to me like GCC is the wrong tool for this.  But I would of course
> not mind if it was *just* the text.  However, as your patch cleary
> shows, the simplified logic leads to real problems:
> 
> > @@ -1819,8 +1819,8 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> >  					edge_serial->rxState = EXPECT_DATA;
> >  					break;
> >  				}
> > -				/* Else, drop through */
> >  			}
> > +			/* fall through */
> >  		case EXPECT_DATA: /* Expect data */
> >  			if (bufferLength < edge_serial->rxBytesRemaining) {
> >  				rxLen = bufferLength;
> 
> 
> The original comment clearly marked a *conditional* fall through at the
> correct place.  Your patch makes it appear as if there is an
> unconditional fall through here.  There is not.  The fallthrough only
> applies to one of a number of nested if blocks. There are no less than
> 3 break statements in the same case block.
> 
> Not a big deal maybe, just as the lack of any "fall through" comment
> isn't a big deal in the first place.  But this change is clearly making
> this code harder to read, and the change is therefore harmful IMHO.
> 
> If you can't make -Wimplicit-fallthrough work without removing such
> precise fallthrough markings, then my proposal is to drop it and use
> some other tool.

Just remove the 'else' after the 'break' further up.

	David

Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[Johan Hovold]

On Mon, Oct 30, 2017 at 11:54:33AM +0000, David Laight wrote:
> From: Bjørn Mork
> > Sent: 28 October 2017 11:57
> > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > > where we are expecting to fall through.
> > >
> > > Notice that in this particular case I replaced "...drop on through"
> > > comments with a proper "fall through" comment on its own line, which
> > > is what GCC is expecting to find.
> > 
> > Sounds to me like GCC is the wrong tool for this.  But I would of course
> > not mind if it was *just* the text.  However, as your patch cleary
> > shows, the simplified logic leads to real problems:
> > 
> > > @@ -1819,8 +1819,8 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> > >  					edge_serial->rxState = EXPECT_DATA;
> > >  					break;
> > >  				}
> > > -				/* Else, drop through */
> > >  			}
> > > +			/* fall through */
> > >  		case EXPECT_DATA: /* Expect data */
> > >  			if (bufferLength < edge_serial->rxBytesRemaining) {
> > >  				rxLen = bufferLength;
> > 
> > 
> > The original comment clearly marked a *conditional* fall through at the
> > correct place.  Your patch makes it appear as if there is an
> > unconditional fall through here.  There is not.  The fallthrough only
> > applies to one of a number of nested if blocks. There are no less than
> > 3 break statements in the same case block.
> > 
> > Not a big deal maybe, just as the lack of any "fall through" comment
> > isn't a big deal in the first place.  But this change is clearly making
> > this code harder to read, and the change is therefore harmful IMHO.
> > 
> > If you can't make -Wimplicit-fallthrough work without removing such
> > precise fallthrough markings, then my proposal is to drop it and use
> > some other tool.
> 
> Just remove the 'else' after the 'break' further up.

Yeah, that might be a good way to resolve this. I was gonna suggest
adding the "fall though" comment before the case while keeping the
"Else, drop through" comment in the branch, but removing the else might
be better.

This code is pretty hard to read as is and could really use some clean
up...

Thanks,
Johan

Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[Gustavo A. R. Silva]

Hi David, Johan,

Quoting Johan Hovold <johan@kernel.org>:

> On Mon, Oct 30, 2017 at 11:54:33AM +0000, David Laight wrote:
>> From: Bjørn Mork
>> > Sent: 28 October 2017 11:57
>> > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> > > where we are expecting to fall through.
>> > >
>> > > Notice that in this particular case I replaced "...drop on through"
>> > > comments with a proper "fall through" comment on its own line, which
>> > > is what GCC is expecting to find.
>> >
>> > Sounds to me like GCC is the wrong tool for this.  But I would of course
>> > not mind if it was *just* the text.  However, as your patch cleary
>> > shows, the simplified logic leads to real problems:
>> >
>> > > @@ -1819,8 +1819,8 @@ static void process_rcvd_data(struct  
>> edgeport_serial *edge_serial,
>> > >  					edge_serial->rxState = EXPECT_DATA;
>> > >  					break;
>> > >  				}
>> > > -				/* Else, drop through */
>> > >  			}
>> > > +			/* fall through */
>> > >  		case EXPECT_DATA: /* Expect data */
>> > >  			if (bufferLength < edge_serial->rxBytesRemaining) {
>> > >  				rxLen = bufferLength;
>> >
>> >
>> > The original comment clearly marked a *conditional* fall through at the
>> > correct place.  Your patch makes it appear as if there is an
>> > unconditional fall through here.  There is not.  The fallthrough only
>> > applies to one of a number of nested if blocks. There are no less than
>> > 3 break statements in the same case block.
>> >
>> > Not a big deal maybe, just as the lack of any "fall through" comment
>> > isn't a big deal in the first place.  But this change is clearly making
>> > this code harder to read, and the change is therefore harmful IMHO.
>> >
>> > If you can't make -Wimplicit-fallthrough work without removing such
>> > precise fallthrough markings, then my proposal is to drop it and use
>> > some other tool.
>>
>> Just remove the 'else' after the 'break' further up.
>
> Yeah, that might be a good way to resolve this. I was gonna suggest
> adding the "fall though" comment before the case while keeping the
> "Else, drop through" comment in the branch, but removing the else might
> be better.
>

Thanks for the suggestion.

> This code is pretty hard to read as is and could really use some clean
> up...
>

I agree. I'll send a V2 of this patch and then let's see if I can help  
with some code refactoring.

Thank you
--
Gustavo A. R. Silva

Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[Johan Hovold]

On Mon, Oct 30, 2017 at 11:50:02PM -0500, Gustavo A. R. Silva wrote:

> Quoting Johan Hovold <johan@kernel.org>:

> > This code is pretty hard to read as is and could really use some clean
> > up...
> >
> 
> I agree. I'll send a V2 of this patch and then let's see if I can help  
> with some code refactoring.

Sounds good!

Thanks,
Johan

Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[Johan Hovold]

On Mon, Jul 02, 2018 at 08:00:43AM -0500, Gustavo A. R. Silva wrote:
> Hi Johan,
> 
> On 07/02/2018 03:51 AM, Johan Hovold wrote:
> > On Thu, Jun 28, 2018 at 01:40:30PM -0500, Gustavo A. R. Silva wrote:
> >> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> >> where we are expecting to fall through.
> >>
> >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> >> ---
> >>  drivers/usb/serial/io_edgeport.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> >> index 97c69d3..441dab6 100644
> >> --- a/drivers/usb/serial/io_edgeport.c
> >> +++ b/drivers/usb/serial/io_edgeport.c
> >> @@ -1760,7 +1760,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> >>  				edge_serial->rxState = EXPECT_HDR2;
> >>  				break;
> >>  			}
> >> -			/* otherwise, drop on through */
> >> +			/* else: fall through */
> > 
> > This doesn't silence the compiler warning with gcc 7.2.0 as the "else: "
> > pattern isn't recognised.
> > 
> 
> I'm using level 2:
> 
> -Wimplicit-fallthrough=2
> 
> The thing here is that some people have pointed out that it can be misleading to
> place a plain fall-through comment after an if-else code block containing a "break".
> So, the solution above has proved to be a good one.

I don't mind the "else", but I would expect you to mention in the commit
message that you're now relying on the non-default warning level (2).

> >>  		case EXPECT_HDR2:
> >>  			edge_serial->rxHeader2 = *buffer;
> >>  			++buffer;
> >> @@ -1820,7 +1820,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> >>  					edge_serial->rxState = EXPECT_DATA;
> >>  					break;
> >>  				}
> >> -				/* Else, drop through */
> >> +				/* else: fall through */
> >>  			}
> > 
> > And this doesn't work either due to the "else: " as well as the fact
> > that the compiler expects the fallthrough comment to precede the case
> > statement directly (e.g. it would need to be moved out of the else
> > block, but that isn't necessarily desirable as we discussed last year: 
> > 
> > 	lkml.kernel.org/r/20171027203906.GA7054@embeddedor.com
> > 
> 
> Yes. I'm aware of that. This certainly is still triggering a warning,
> so I just consider this
> as a temporal approach. I still need to define how are we going to
> manage cases like this.

Ok, so why did you not mention that in the commit message?

If this isn't even addressing the warning you get with the non-default
-Wimplicit-fallthrough=2, I don't see this as much of an improvement.

Might as well leave this unchanged, until all warnings in that switch
statement are addressed.

Johan

Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[Gustavo A. R. Silva]

Hi Johan,

On 07/02/2018 03:51 AM, Johan Hovold wrote:
> On Thu, Jun 28, 2018 at 01:40:30PM -0500, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> where we are expecting to fall through.
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>>  drivers/usb/serial/io_edgeport.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
>> index 97c69d3..441dab6 100644
>> --- a/drivers/usb/serial/io_edgeport.c
>> +++ b/drivers/usb/serial/io_edgeport.c
>> @@ -1760,7 +1760,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
>>  				edge_serial->rxState = EXPECT_HDR2;
>>  				break;
>>  			}
>> -			/* otherwise, drop on through */
>> +			/* else: fall through */
> 
> This doesn't silence the compiler warning with gcc 7.2.0 as the "else: "
> pattern isn't recognised.
> 

I'm using level 2:

-Wimplicit-fallthrough=2

The thing here is that some people have pointed out that it can be misleading to
place a plain fall-through comment after an if-else code block containing a "break".
So, the solution above has proved to be a good one.

>>  		case EXPECT_HDR2:
>>  			edge_serial->rxHeader2 = *buffer;
>>  			++buffer;
>> @@ -1820,7 +1820,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
>>  					edge_serial->rxState = EXPECT_DATA;
>>  					break;
>>  				}
>> -				/* Else, drop through */
>> +				/* else: fall through */
>>  			}
> 
> And this doesn't work either due to the "else: " as well as the fact
> that the compiler expects the fallthrough comment to precede the case
> statement directly (e.g. it would need to be moved out of the else
> block, but that isn't necessarily desirable as we discussed last year: 
> 
> 	lkml.kernel.org/r/20171027203906.GA7054@embeddedor.com
> 

Yes. I'm aware of that. This certainly is still triggering a warning, so I just consider this
as a temporal approach. I still need to define how are we going to manage cases like this.

> )
> 
>>  		case EXPECT_DATA: /* Expect data */
>>  			if (bufferLength < edge_serial->rxBytesRemaining) {
> 
> How do you compile test these these patches?
>

I already explained this above.

Thanks for your comments.
--
Gustavo

Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[Johan Hovold]

On Thu, Jun 28, 2018 at 01:40:30PM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
> 
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/usb/serial/io_edgeport.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> index 97c69d3..441dab6 100644
> --- a/drivers/usb/serial/io_edgeport.c
> +++ b/drivers/usb/serial/io_edgeport.c
> @@ -1760,7 +1760,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
>  				edge_serial->rxState = EXPECT_HDR2;
>  				break;
>  			}
> -			/* otherwise, drop on through */
> +			/* else: fall through */

This doesn't silence the compiler warning with gcc 7.2.0 as the "else: "
pattern isn't recognised.

>  		case EXPECT_HDR2:
>  			edge_serial->rxHeader2 = *buffer;
>  			++buffer;
> @@ -1820,7 +1820,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
>  					edge_serial->rxState = EXPECT_DATA;
>  					break;
>  				}
> -				/* Else, drop through */
> +				/* else: fall through */
>  			}

And this doesn't work either due to the "else: " as well as the fact
that the compiler expects the fallthrough comment to precede the case
statement directly (e.g. it would need to be moved out of the else
block, but that isn't necessarily desirable as we discussed last year: 

	lkml.kernel.org/r/20171027203906.GA7054@embeddedor.com

)

>  		case EXPECT_DATA: /* Expect data */
>  			if (bufferLength < edge_serial->rxBytesRemaining) {

How do you compile test these these patches?

Thanks,
Johan

[PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/usb/serial/io_edgeport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index 97c69d3..441dab6 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -1760,7 +1760,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
 				edge_serial->rxState = EXPECT_HDR2;
 				break;
 			}
-			/* otherwise, drop on through */
+			/* else: fall through */
 		case EXPECT_HDR2:
 			edge_serial->rxHeader2 = *buffer;
 			++buffer;
@@ -1820,7 +1820,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
 					edge_serial->rxState = EXPECT_DATA;
 					break;
 				}
-				/* Else, drop through */
+				/* else: fall through */
 			}
 		case EXPECT_DATA: /* Expect data */
 			if (bufferLength < edge_serial->rxBytesRemaining) {
-- 
2.7.4