In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Cc: Jes Sorensen <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
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
Hi Jes,
Quoting Jes Sorensen <[email protected]>:
> On 10/11/2017 04:41 AM, Kalle Valo wrote:
>> Jes Sorensen <[email protected]> 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
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 <[email protected]>
>> Cc: Kalle Valo <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
>> ---
>> 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
Jes Sorensen <[email protected]> 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
On 10/11/2017 04:41 AM, Kalle Valo wrote:
> Jes Sorensen <[email protected]> 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
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
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 <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Gustavo A. R. Silva <[email protected]>
> ---
> 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;
> }
>
Jes Sorensen <[email protected]> writes:
> On 10/11/2017 04:41 AM, Kalle Valo wrote:
>> Jes Sorensen <[email protected]> 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
On Wed, Oct 11, 2017 at 7:32 AM, Gustavo A. R. Silva
<[email protected]> wrote:
> Quoting Jes Sorensen <[email protected]>:
>> On 10/11/2017 04:41 AM, Kalle Valo wrote:
>>> Jes Sorensen <[email protected]> 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
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
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...