Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch fall-throughs
From: Gustavo A. R. Silva
Date: Thu May 02 2019 - 10:57:48 EST
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;
>