[PATCH] rtl8xxxu: mark expected switch fall-throughs

[PATCH] rtl8xxxu: 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.

Cc: Jes Sorensen <Jes.Sorensen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Gustavo A. R. Silva <garsilva-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org>
---
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 7806a4d..e66be05 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -1153,6 +1153,7 @@ void rtl8xxxu_gen1_config_channel(struct ieee80211_hw *hw)
 	switch (hw->conf.chandef.width) {
 	case NL80211_CHAN_WIDTH_20_NOHT:
 		ht = false;
+		/* fall through */
 	case NL80211_CHAN_WIDTH_20:
 		opmode |= BW_OPMODE_20MHZ;
 		rtl8xxxu_write8(priv, REG_BW_OPMODE, opmode);
@@ -1280,6 +1281,7 @@ void rtl8xxxu_gen2_config_channel(struct ieee80211_hw *hw)
 	switch (hw->conf.chandef.width) {
 	case NL80211_CHAN_WIDTH_20_NOHT:
 		ht = false;
+		/* fall through */
 	case NL80211_CHAN_WIDTH_20:
 		rf_mode_bw |= WMAC_TRXPTCL_CTL_BW_20;
 		subchannel = 0;
@@ -1748,9 +1750,11 @@ static int rtl8xxxu_identify_chip(struct rtl8xxxu_priv *priv)
 		case 3:
 			priv->ep_tx_low_queue = 1;
 			priv->ep_tx_count++;
+			/* fall through */
 		case 2:
 			priv->ep_tx_normal_queue = 1;
 			priv->ep_tx_count++;
+			/* fall through */
 		case 1:
 			priv->ep_tx_high_queue = 1;
 			priv->ep_tx_count++;
@@ -5691,6 +5695,7 @@ static int rtl8xxxu_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
 		break;
 	case WLAN_CIPHER_SUITE_TKIP:
 		key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
+		/* fall through */
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
2.7.4

Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs

[Jes Sorensen]

On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.

While this isn't harmful, to me this looks like pointless patch churn 
for zero gain and it's just ugly.

Jes


> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: linux-wireless@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
>   drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 7806a4d..e66be05 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -1153,6 +1153,7 @@ void rtl8xxxu_gen1_config_channel(struct ieee80211_hw *hw)
>   	switch (hw->conf.chandef.width) {
>   	case NL80211_CHAN_WIDTH_20_NOHT:
>   		ht = false;
> +		/* fall through */
>   	case NL80211_CHAN_WIDTH_20:
>   		opmode |= BW_OPMODE_20MHZ;
>   		rtl8xxxu_write8(priv, REG_BW_OPMODE, opmode);
> @@ -1280,6 +1281,7 @@ void rtl8xxxu_gen2_config_channel(struct ieee80211_hw *hw)
>   	switch (hw->conf.chandef.width) {
>   	case NL80211_CHAN_WIDTH_20_NOHT:
>   		ht = false;
> +		/* fall through */
>   	case NL80211_CHAN_WIDTH_20:
>   		rf_mode_bw |= WMAC_TRXPTCL_CTL_BW_20;
>   		subchannel = 0;
> @@ -1748,9 +1750,11 @@ static int rtl8xxxu_identify_chip(struct rtl8xxxu_priv *priv)
>   		case 3:
>   			priv->ep_tx_low_queue = 1;
>   			priv->ep_tx_count++;
> +			/* fall through */
>   		case 2:
>   			priv->ep_tx_normal_queue = 1;
>   			priv->ep_tx_count++;
> +			/* fall through */
>   		case 1:
>   			priv->ep_tx_high_queue = 1;
>   			priv->ep_tx_count++;
> @@ -5691,6 +5695,7 @@ static int rtl8xxxu_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>   		break;
>   	case WLAN_CIPHER_SUITE_TKIP:
>   		key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
> +		/* fall through */
>   	default:
>   		return -EOPNOTSUPP;
>   	}
> 

Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs

[Florian Fainelli]

On 10/10/2017 12:35 PM, Jes Sorensen wrote:
> On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> where we are expecting to fall through.
> 
> While this isn't harmful, to me this looks like pointless patch churn
> for zero gain and it's just ugly.

That is the canonical way to tell static analyzers and compilers that
fall throughs are wanted and not accidental mistakes in the code. For
people that deal with these kinds of errors, it's quite helpful, unless
you suggest disabling that particular GCC warning specific for that
file/directory?

> 
> Jes
> 
> 
>> Cc: Jes Sorensen <Jes.Sorensen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Gustavo A. R. Silva <garsilva-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org>
>> ---
>>   drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> index 7806a4d..e66be05 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> @@ -1153,6 +1153,7 @@ void rtl8xxxu_gen1_config_channel(struct
>> ieee80211_hw *hw)
>>       switch (hw->conf.chandef.width) {
>>       case NL80211_CHAN_WIDTH_20_NOHT:
>>           ht = false;
>> +        /* fall through */
>>       case NL80211_CHAN_WIDTH_20:
>>           opmode |= BW_OPMODE_20MHZ;
>>           rtl8xxxu_write8(priv, REG_BW_OPMODE, opmode);
>> @@ -1280,6 +1281,7 @@ void rtl8xxxu_gen2_config_channel(struct
>> ieee80211_hw *hw)
>>       switch (hw->conf.chandef.width) {
>>       case NL80211_CHAN_WIDTH_20_NOHT:
>>           ht = false;
>> +        /* fall through */
>>       case NL80211_CHAN_WIDTH_20:
>>           rf_mode_bw |= WMAC_TRXPTCL_CTL_BW_20;
>>           subchannel = 0;
>> @@ -1748,9 +1750,11 @@ static int rtl8xxxu_identify_chip(struct
>> rtl8xxxu_priv *priv)
>>           case 3:
>>               priv->ep_tx_low_queue = 1;
>>               priv->ep_tx_count++;
>> +            /* fall through */
>>           case 2:
>>               priv->ep_tx_normal_queue = 1;
>>               priv->ep_tx_count++;
>> +            /* fall through */
>>           case 1:
>>               priv->ep_tx_high_queue = 1;
>>               priv->ep_tx_count++;
>> @@ -5691,6 +5695,7 @@ static int rtl8xxxu_set_key(struct ieee80211_hw
>> *hw, enum set_key_cmd cmd,
>>           break;
>>       case WLAN_CIPHER_SUITE_TKIP:
>>           key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
>> +        /* fall through */
>>       default:
>>           return -EOPNOTSUPP;
>>       }
>>
> 


-- 
Florian

Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs

[Kalle Valo]

Jes Sorensen <jes.sorensen@gmail.com> writes:

> On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> where we are expecting to fall through.
>
> While this isn't harmful, to me this looks like pointless patch churn
> for zero gain and it's just ugly.

In general I find it useful to mark fall through cases. And it's just a
comment with two words, so they cannot hurt your eyes that much.

-- 
Kalle Valo

Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs

[Jes Sorensen]

On 10/11/2017 04:41 AM, Kalle Valo wrote:
> Jes Sorensen <jes.sorensen@gmail.com> writes:
> 
>> On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:
>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>> where we are expecting to fall through.
>>
>> While this isn't harmful, to me this looks like pointless patch churn
>> for zero gain and it's just ugly.
> 
> In general I find it useful to mark fall through cases. And it's just a
> comment with two words, so they cannot hurt your eyes that much.

I don't see them being harmful in the code, but I don't see them of much 
use either. If it happened as part of natural code development, fine. My 
objection is to people running around doing this systematically causing 
patch churn for little to zero gain.

Jes

Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs

[Gustavo A. R. Silva]

Hi Jes,

Quoting Jes Sorensen <jes.sorensen@gmail.com>:

> On 10/11/2017 04:41 AM, Kalle Valo wrote:
>> Jes Sorensen <jes.sorensen@gmail.com> writes:
>>
>>> On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:
>>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>>> where we are expecting to fall through.
>>>
>>> While this isn't harmful, to me this looks like pointless patch churn
>>> for zero gain and it's just ugly.
>>
>> In general I find it useful to mark fall through cases. And it's just a
>> comment with two words, so they cannot hurt your eyes that much.
>
> I don't see them being harmful in the code, but I don't see them of  
> much use either. If it happened as part of natural code development,  
> fine. My objection is to people running around doing this  
> systematically causing patch churn for little to zero gain.
>
> Jes


I understand that you think this is of zero gain for you, but as  
Florian Fainelli pointed out:

"That is the canonical way to tell static analyzers and compilers that
fall throughs are wanted and not accidental mistakes in the code. For
people that deal with these kinds of errors, it's quite helpful, unless
you suggest disabling that particular GCC warning specific for that
file/directory?"

this is very helpful for people working on fixing issues reported by  
static analyzers. It saves a huge amount of time when dealing with  
False Positives. Also, there are cases when an apparently intentional  
fall-through turns out to be an actual missing break or continue.

So there is an ongoing effort to detect such cases and avoid them to  
show up in the future by at least warning people about a potential  
issue in their code. And this is helpful for everybody.

Thanks
--
Gustavo A. R. Silva

Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs

[Kees Cook]

On Wed, Oct 11, 2017 at 7:32 AM, Gustavo A. R. Silva
<garsilva@embeddedor.com> wrote:
> Quoting Jes Sorensen <jes.sorensen@gmail.com>:
>> On 10/11/2017 04:41 AM, Kalle Valo wrote:
>>> Jes Sorensen <jes.sorensen@gmail.com> writes:
>>>> On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:
>>>>>
>>>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>>>> where we are expecting to fall through.
>>>>
>>>> While this isn't harmful, to me this looks like pointless patch churn
>>>> for zero gain and it's just ugly.
>>>
>>> In general I find it useful to mark fall through cases. And it's just a
>>> comment with two words, so they cannot hurt your eyes that much.
>>
>> I don't see them being harmful in the code, but I don't see them of much
>> use either. If it happened as part of natural code development, fine. My
>> objection is to people running around doing this systematically causing
>> patch churn for little to zero gain.
>
> I understand that you think this is of zero gain for you, but as Florian
> Fainelli pointed out:
>
> "That is the canonical way to tell static analyzers and compilers that
> fall throughs are wanted and not accidental mistakes in the code. For
> people that deal with these kinds of errors, it's quite helpful, unless
> you suggest disabling that particular GCC warning specific for that
> file/directory?"
>
> this is very helpful for people working on fixing issues reported by static
> analyzers. It saves a huge amount of time when dealing with False Positives.
> Also, there are cases when an apparently intentional fall-through turns out
> to be an actual missing break or continue.
>
> So there is an ongoing effort to detect such cases and avoid them to show up
> in the future by at least warning people about a potential issue in their
> code. And this is helpful for everybody.

This is an unfortunate omission in the C language, and thankfully both
gcc and clang have stepped up to solve this the same way static
analyzers have solved it. It's not exactly pretty, but it does both
document the intention for humans and provide a way for analyzers to
report issues. Having the compiler help us not make mistakes is quite
handy, and with Gustavo grinding through all the Coverity warnings,
he's found actual bugs with missing "break"s, so I think this has a
demonstrable benefit to the code-base as a whole. It makes things
unambiguous to someone else reviewing the code.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs

[Kalle Valo]

Jes Sorensen <jes.sorensen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> On 10/11/2017 04:41 AM, Kalle Valo wrote:
>> Jes Sorensen <jes.sorensen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>
>>> On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:
>>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>>> where we are expecting to fall through.
>>>
>>> While this isn't harmful, to me this looks like pointless patch churn
>>> for zero gain and it's just ugly.
>>
>> In general I find it useful to mark fall through cases. And it's just a
>> comment with two words, so they cannot hurt your eyes that much.
>
> I don't see them being harmful in the code, but I don't see them of
> much use either. If it happened as part of natural code development,
> fine. My objection is to people running around doing this
> systematically causing patch churn for little to zero gain.

We do receive quite a lot these kind of cleanup patches found with
various analysers and tools. I guess one could classify those as churn
but I think the net result is still very much on the positive side. And
this patch in particular seems useful for me and I think we should take
it.

-- 
Kalle Valo

Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs

[Joe Perches]

On Tue, 2017-10-10 at 14:30 -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.

perhaps use Arnaldo's idea:

https://lkml.org/lkml/2017/2/9/845
https://lkml.org/lkml/2017/2/10/485

RE: [PATCH] rtl8xxxu: mark expected switch fall-throughs

[David Laight]

From: Joe Perches
> Sent: 11 October 2017 11:21
> On Tue, 2017-10-10 at 14:30 -0500, Gustavo A. R. Silva wrote:
> > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > where we are expecting to fall through.
> 
> perhaps use Arnaldo's idea:
> 
> https://lkml.org/lkml/2017/2/9/845
> https://lkml.org/lkml/2017/2/10/485

gah, that is even uglier and requires a chase through
headers to find out what it means.

	David

Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs

[Joe Perches]

On Wed, 2017-10-11 at 12:54 +0000, David Laight wrote:
> From: Joe Perches
> > Sent: 11 October 2017 11:21
> > On Tue, 2017-10-10 at 14:30 -0500, Gustavo A. R. Silva wrote:
> > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > > where we are expecting to fall through.
> > 
> > perhaps use Arnaldo's idea:
> > 
> > https://lkml.org/lkml/2017/2/9/845
> > https://lkml.org/lkml/2017/2/10/485
> 
> gah, that is even uglier and requires a chase through
> headers to find out what it means.

Sure, if you think __fallthrough; isn't self-documenting.

	case foo;
		bar;
		__fallthrough;
	case baz;
		etc...