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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxx>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>>  - Warning level 3 is now used: -Wimplicit-fallthrough=3
>>>>>>>    instead of warning level 2.
>>>>>>>  - All warnings in the switch statement are addressed now.
>>>>>>>
>>>>>>> Notice that these are the last remaining fall-through warnings
>>>>>>> in the USB subsystem. :)
>>>>>>
>>>>>>>  drivers/usb/serial/io_edgeport.c | 3 ++-
>>>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
>>>>>>> index 4ca31c0e4174..7ad10328f4e2 100644
>>>>>>> --- a/drivers/usb/serial/io_edgeport.c
>>>>>>> +++ b/drivers/usb/serial/io_edgeport.c
>>>>>>> @@ -1751,7 +1751,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
>>>>>>>  				edge_serial->rxState = EXPECT_HDR2;
>>>>>>>  				break;
>>>>>>>  			}
>>>>>>> -			/* otherwise, drop on through */
>>>>>>> +			/* Fall through - otherwise, drop on through */
>>>>>>>  		case EXPECT_HDR2:
>>>>>>>  			edge_serial->rxHeader2 = *buffer;
>>>>>>>  			++buffer;
>>>>>>> @@ -1813,6 +1813,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
>>>>>>>  				}
>>>>>>>  				/* Else, drop through */
>>>>>>>  			}
>>>>>>> +			/* Fall through */
>>>>>>>  		case EXPECT_DATA: /* Expect data */
>>>>>>
>>>>>> Looks like you forgot to take the original review feedback you got into
>>>>>> account:
>>>>>>
>>>>>> 	https://lkml.kernel.org/r/87k1zf4k24.fsf@xxxxxxxxxxxxxxxxx
>>>>>>
>>>>>
>>>>> Oh, the thing is that the fall-through comments have to be placed at
>>>>> the very bottom of the case. Also, based on that feedback, this time
>>>>> I left the "Else, drop through" comment in place, so people can be
>>>>> informed that such fall-through is conditional.
>>>>>
>>>>> What do you think about this:
>>>>>
>>>>> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
>>>>> index 4ca31c0e4174..52f27fc82563 100644
>>>>> --- a/drivers/usb/serial/io_edgeport.c
>>>>> +++ b/drivers/usb/serial/io_edgeport.c
>>>>> @@ -1751,7 +1751,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
>>>>>                                 edge_serial->rxState = EXPECT_HDR2;
>>>>>                                 break;
>>>>>                         }
>>>>> -                       /* otherwise, drop on through */
>>>>> +                       /* Fall through - otherwise, drop on through */
>>>>>                 case EXPECT_HDR2:
>>>>>                         edge_serial->rxHeader2 = *buffer;
>>>>>                         ++buffer;
>>>>> @@ -1813,6 +1813,11 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
>>>>>                                 }
>>>>>                                 /* Else, drop through */
>>>>>                         }
>>>>> +                       /* Beware that, currently, there are at least three
>>>>> +                        * break statements in this case block, so the
>>>>> +                        * fall-through marked below is NOT unconditional.
>>>>> +                        */
>>>>> +                       /* Fall through */
>>>>>                 case EXPECT_DATA: /* Expect data */
>>>>>                         if (bufferLength < edge_serial->rxBytesRemaining) {
>>>>>                                 rxLen = bufferLength;
>>>>
>>>> It's better than v2, but I thought you said you were gonna look into
>>>> restructuring the code to maintain (or even improve) readability?
>>>>
>>>
>>> At first, I thought about that, but now I don't think that's realistic.
>>> I'd turn the if-else into a switch, and based on the history of feedback
>>> on this patch, we will end up having the same complains about the break
>>> statements in that new switch and the possibility of a fall-through to
>>> case EXPECT_DATA. At the end I would still have to add a comment explaining
>>> that the last fall-through mark in unconditional.
>>
>> I love it how no one is blaming the original author of this code (i.e.
>> me...)
>>
>> Let me see if I can fix it up to be more "sane", this is my fault.
> 
> How about the following patch?  Johan, this look nicer to you?  It makes
> more sense to me.
> 

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;
> 



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux