rtl8xxxu: mark expected switch fall-throughs
Subsystems (3)
PATCH 1/1 rtl8xxxu: mark expected switch fall-throughs
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
--- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +5 -0
@@ -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 ; }
Signature · 1 line
2.7.4
Jes Sorensen
maintainerQuoted 2 lines · Gustavo A. R. Silva
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
Quoted 50 lines
Cc Jes Sorensen <[email protected]>Cc Kalle Valo <[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(+)drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +5
-0@@ -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 ; }
Quoted 6 lines · Jes Sorensen
Quoted 2 lines · Gustavo A. R. Silva
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?
Quoted 60 lines
Jes
Quoted 55 lines
Cc Jes Sorensen <[email protected]>Cc Kalle Valo <[email protected]>Signed-off-by Gustavo A. R. Silva <garsilva-L1vi/[email protected]>--- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +1
-0@@ -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);drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +1
-0@@ -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;drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +2
-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++;drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +1
-0@@ -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 ;}
Signature · 1 line
Florian
Quoted 6 lines · Jes Sorensen
Quoted 2 lines · Gustavo A. R. Silva
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.
Signature · 1 line
Kalle Valo
Quoted 2 lines · Gustavo A. R. Silva
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
From: Joe Perches
Quoted 9 lines
Sent: 11 October 2017 11:21
Quoted 2 lines · Gustavo A. R. Silva
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
Jes Sorensen
maintainerQuoted 11 lines · Kalle Valo
Quoted 6 lines · Jes Sorensen
Quoted 2 lines · Gustavo A. R. Silva
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
Hi Jes,
Quoting Jes Sorensen <[email protected]>:
Quoted 10 lines · Kalle Valo
Quoted 9 lines
Quoted 6 lines · Jes Sorensen
Quoted 2 lines · Gustavo A. R. Silva
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.
Quoted 6 lines
Quoted 2 lines
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
Quoted 28 lines · Gustavo A. R. Silva
Quoting Jes Sorensen <[email protected]>:
Quoted 10 lines · Kalle Valo
Quoted 9 lines
Quoted 7 lines · Jes Sorensen
Quoted 3 lines · Gustavo A. R. Silva
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.
Quoted 7 lines
Quoted 2 lines
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?"
Quoted 8 lines
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
Signature · 2 lines
Kees Cook Pixel Security
Quoted 6 lines · David Laight
From: Joe Perches
Quoted 5 lines
Sent: 11 October 2017 11:21
Quoted 2 lines · Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
Quoted 7 lines
Quoted 4 lines
perhaps use Arnaldo's idea:
https://lkml.org/lkml/2017/2/9/845
https://lkml.org/lkml/2017/2/10/485gah, 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...
Quoted 10 lines · Jes Sorensen
Quoted 9 lines · Kalle Valo
Quoted 6 lines · Jes Sorensen
Quoted 2 lines · Gustavo A. R. Silva
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.
Quoted 7 lines
Quoted 2 lines
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.
Signature · 1 line
Kalle Valo