Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH][next] greybus: Use fallthrough pseudo-keyword
From:   Alex Elder <elder@ieee.org>
To:     "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Johan Hovold <johan@kernel.org>, Alex Elder <elder@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     greybus-dev@lists.linaro.org, linux-kernel@vger.kernel.org
References: <20200727183258.GA28571@embeddedor>
 <63871608-e076-26b0-85df-cff11699c3a3@ieee.org>
Message-ID: <e36013ba-e19a-86c9-cb68-d7ad5559c292@ieee.org>
Date:   Wed, 5 Aug 2020 08:14:47 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <63871608-e076-26b0-85df-cff11699c3a3@ieee.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 7/28/20 5:37 PM, Alex Elder wrote:
> On 7/27/20 1:32 PM, Gustavo A. R. Silva wrote:
>> Replace the existing /* fall through */ comments and its variants with
>> the new pseudo-keyword macro fallthrough[1].
>>
>> [1] https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through
> 
> Thanks for the patch.  It looks good, but it raises
> another question I'd like discussion on.

It's been a week, and we heard back from Viresh (and Joe) on
this, but no one else.  Viresh left out the break statement on
the last case of the switch statement intentionally, arguing
that it is not needed (much like a return statement at the end
of a void function).  But he doesn't feel strongly enough
insist it should stay that way.  I'm sure the others omitted
the break statement intentionally as well.

Given no strong pushback, I'll ask you (Gustavo) to post a
second patch adding the missing break statements I described
(and look for any others I might have missed).  If you would
prefer not to do that, just say so, and I will send out such
a patch myself.

On your original patch, it looks good to me.  Thank you.

Reviewed-by: Alex Elder <elder@linaro.org>

> It seems that Johan likes default (or final) cases in
> switch statements without a "break" statement.  Viresh
> and Bryan appear to be fond of this too.
> 
> It's pedantic, but I don't like that.  Am I wrong?
>   --> Johan/Viresh/Bryan would you please comment?
> 
> If they aren't convincing (or don't care) I think break
> statements should also be added here:
> - In gb_interface_read_and_clear_init_status() for the
>   default case
> - In gb_interface_activate() for the default case.
> - In gb_svc_process_deferred_request() for the default
>   case
> 
> But let's wait to see what Johan (et al) says.  If you
> don't want to do that, say so and I'll do it later.
> 
> I looked at the code in drivers/staging/greybus/ and saw
> no need to add a "fallthrough" anywhere, but:
> - In fw_mgmt_backend_fw_version_operation() Viresh
>   seems to have skipped the break in the fault statement
> - In gb_uart_request_handler() Bryan did this too.
> 
> Depending on discussion, these could be fixed in a
> separate patch as well.
> 
> These cases aren't found by "checkpatch.pl" because it only
> looks at case "blocks" that are followed by another case.
> So the last case isn't checked.
> 
>                     -Alex
> 
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>>   drivers/greybus/es2.c       | 2 +-
>>   drivers/greybus/interface.c | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/greybus/es2.c b/drivers/greybus/es2.c
>> index 366716f11b1a..1df6ab5d339d 100644
>> --- a/drivers/greybus/es2.c
>> +++ b/drivers/greybus/es2.c
>> @@ -759,7 +759,7 @@ static int check_urb_status(struct urb *urb)
>>       case -EOVERFLOW:
>>           dev_err(dev, "%s: overflow actual length is %d\n",
>>               __func__, urb->actual_length);
>> -        /* fall through */
>> +        fallthrough;
>>       case -ECONNRESET:
>>       case -ENOENT:
>>       case -ESHUTDOWN:
>> diff --git a/drivers/greybus/interface.c b/drivers/greybus/interface.c
>> index 67dbe6fda9a1..58ea374d8aaa 100644
>> --- a/drivers/greybus/interface.c
>> +++ b/drivers/greybus/interface.c
>> @@ -1233,7 +1233,7 @@ int gb_interface_add(struct gb_interface *intf)
>>       case GB_INTERFACE_TYPE_GREYBUS:
>>           dev_info(&intf->dev, "GMP VID=0x%08x, PID=0x%08x\n",
>>                intf->vendor_id, intf->product_id);
>> -        /* fall-through */
>> +        fallthrough;
>>       case GB_INTERFACE_TYPE_UNIPRO:
>>           dev_info(&intf->dev, "DDBL1 Manufacturer=0x%08x, Product=0x%08x\n",
>>                intf->ddbl1_manufacturer_id,
>>
>