Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs
Jes Sorensen writes: > On 10/11/2017 04:41 AM, Kalle Valo wrote: >> Jes Sorensen 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
On Wed, 2017-10-11 at 12:54 +, 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...
Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs
On Wed, Oct 11, 2017 at 7:32 AM, Gustavo A. R. Silva wrote: > Quoting Jes Sorensen : >> On 10/11/2017 04:41 AM, Kalle Valo wrote: >>> Jes Sorensen 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
Hi Jes, Quoting Jes Sorensen : On 10/11/2017 04:41 AM, Kalle Valo wrote: Jes Sorensen 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
On 10/11/2017 04:41 AM, Kalle Valo wrote: Jes Sorensen 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
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
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
Jes Sorensen 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
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
>> Cc: Kalle Valo
>> Cc: linux-wireless@vger.kernel.org
>> Cc: net...@vger.kernel.org
>> Signed-off-by: Gustavo A. R. Silva
>> ---
>> 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
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
Cc: Kalle Valo
Cc: linux-wireless@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva
---
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;
}
