| Message ID | 20181023001308.GA4150@embeddedor.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v2] wireless: mark expected switch fall-throughs | expand |
On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote: > In preparation to enabling -Wimplicit-fallthrough, mark switch cases > where we are expecting to fall through. > > Warning level 3 was used: -Wimplicit-fallthrough=3 > > This code was not tested and GCC 7.2.0 was used to compile it. Look, I'm not going to make this any clearer: I'm not applying patches like that where you've invested no effort whatsoever on verifying that they're correct. johannes
On 10/23/18 9:01 AM, Johannes Berg wrote: > On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote: >> In preparation to enabling -Wimplicit-fallthrough, mark switch cases >> where we are expecting to fall through. >> >> Warning level 3 was used: -Wimplicit-fallthrough=3 >> >> This code was not tested and GCC 7.2.0 was used to compile it. > > Look, I'm not going to make this any clearer: I'm not applying patches > like that where you've invested no effort whatsoever on verifying that > they're correct. > How do you suggest me to verify that every part is correct in this type of patches? Thanks -- Gustavo
On 10/23/18 10:59 AM, Gustavo A. R. Silva wrote: > > On 10/23/18 9:01 AM, Johannes Berg wrote: >> On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote: >>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases >>> where we are expecting to fall through. >>> >>> Warning level 3 was used: -Wimplicit-fallthrough=3 >>> >>> This code was not tested and GCC 7.2.0 was used to compile it. >> >> Look, I'm not going to make this any clearer: I'm not applying patches >> like that where you've invested no effort whatsoever on verifying that >> they're correct. >> > > How do you suggest me to verify that every part is correct in this type > of patches? > BTW... I'm under the impression you think that I don't even look at the code. Is that correct? I've been working on this for quite a while, and in every case I try to understand the code in terms of the context in which every warning is reported. I look for dependencies between variables in adjacent switch cases, that could make think it might be OK for some of those cases to fall through to the one below. I check the names of the case labels to see if there might be any relation between them, or if they are totally diferent as it might be the case for labels that indicate the transmision(FOO_TX) or reception of data(FOO_RX). Something similar to the latter is the case with on/off logic (FOO_ON/FOO_OFF). These are some of the things I review in the code, so I can have an idea if the warning is a false positive or an actual bug. Here is a bug I found yesterday at drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c 5690 case WLAN_CIPHER_SUITE_CCMP: 5691 key->flags |= IEEE80211_KEY_FLAG_SW_MGMT_TX; 5692 break; 5693 case WLAN_CIPHER_SUITE_TKIP: 5694 key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC; 5695 default: 5696 return -EOPNOTSUPP; 5697 } Notice how the absence of a break statement is very suspicious in case WLAN_CIPHER_SUITE_TKIP, because the code for case WLAN_CIPHER_SUITE_CCMP is pretty similar and in that case there is a break at the bottom. Now, that's not the only thing that looks supicious: in the absence of a break, the code falls through to the default case, which returns the error value -EOPNOTSUPP. So, even when key->flags is updated, the code always returns an error for case WLAN_CIPHER_SUITE_TKIP. This analysis led me to think that this is an actual bug, so I sent a patch to fix it: https://lore.kernel.org/patchwork/patch/1002440/ Notice that this bug has been there since 2015: commit 26f1fad29ad973b0fb26a9ca3dcb2a73dde781aa Now, let's take a look at the following warning in net/wireless/chan.c: net/wireless/chan.c: In function ‘cfg80211_chandef_usable’: net/wireless/chan.c:748:6: warning: this statement may fall through [-Wimplicit-fallthrough=] if (!ht_cap->ht_supported) ^ net/wireless/chan.c:750:2: note: here case NL80211_CHAN_WIDTH_20_NOHT: ^~~~ This is the piece of code at net/wireless/chan.c:740: 740 case NL80211_CHAN_WIDTH_5: 741 width = 5; 742 break; 743 case NL80211_CHAN_WIDTH_10: 744 prohibited_flags |= IEEE80211_CHAN_NO_10MHZ; 745 width = 10; 746 break; 747 case NL80211_CHAN_WIDTH_20: 748 if (!ht_cap->ht_supported) 749 return false; 750 case NL80211_CHAN_WIDTH_20_NOHT: 751 prohibited_flags |= IEEE80211_CHAN_NO_20MHZ; 752 width = 20; 753 break; 754 case NL80211_CHAN_WIDTH_40: 755 width = 40; 756 if (!ht_cap->ht_supported) 757 return false; 758 if (!(ht_cap->cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40) || 759 ht_cap->cap & IEEE80211_HT_CAP_40MHZ_INTOLERANT) 760 return false; 761 if (chandef->center_freq1 < control_freq && 762 chandef->chan->flags & IEEE80211_CHAN_NO_HT40MINUS) 763 return false; 764 if (chandef->center_freq1 > control_freq && 765 chandef->chan->flags & IEEE80211_CHAN_NO_HT40PLUS) 766 return false; 767 break; Notice that the warning was reported at line 748, but I'm including more code here to make it explicitly clear that I not only focus my atention in a very narrowed piece of code around the warning. Now, I don't see anything supicious. The labels NL80211_CHAN_WIDTH_20 and NL80211_CHAN_WIDTH_20_NOHT seem to be related, and it's even less supicious when there is an explicit line of code that breaks the switch under certain conditions, just at the bottom of the "case", as is the case with lines 748 and 749: 748 if (!ht_cap->ht_supported) 749 return false; Also, no default case returning an error every time if the code falls through to the next case. Lastly, I also check the age of the code, but this only after I have analyzed it as explained above. I do this analysis for every warning. Now, when I say I haven't tested the code is because I don't have any log as evidence for anything. Not that I haven't put any effort on trying to understand it and its context. When I started working on this task there were more than 2000 of these issues, now there are around 600 left. I have fixed many bugs on the way, so a good amount of work is being invested on this, and it's paying off. :) Now, let me ask you this question: It would be easier for you to review this patch if I turn it into a series? I can do that without a problem. Thanks! -- Gustavo
On Tue, 2018-10-23 at 12:58 +0200, Gustavo A. R. Silva wrote: > On 10/23/18 10:59 AM, Gustavo A. R. Silva wrote: > > > > On 10/23/18 9:01 AM, Johannes Berg wrote: > > > On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote: > > > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases > > > > where we are expecting to fall through. > > > > > > > > Warning level 3 was used: -Wimplicit-fallthrough=3 > > > > > > > > This code was not tested and GCC 7.2.0 was used to compile it. > > > > > > Look, I'm not going to make this any clearer: I'm not applying patches > > > like that where you've invested no effort whatsoever on verifying that > > > they're correct. > > > > > > > How do you suggest me to verify that every part is correct in this type > > of patches? > > > > BTW... I'm under the impression you think that I don't even look at > the code. Is that correct? That's what your commit log looks like, yes. This is your full commit log: In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Warning level 3 was used: -Wimplicit-fallthrough=3 This code was not tested and GCC 7.2.0 was used to compile it. For all I know, you could've run spatch to add the comments wherever there was no break, and then compiled it once. > I've been working on this for quite a while, and in every case I try to > understand the code in terms of the context in which every warning is > reported. That's great. > Here is a bug I found yesterday at drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > > 5690 case WLAN_CIPHER_SUITE_CCMP: > 5691 key->flags |= IEEE80211_KEY_FLAG_SW_MGMT_TX; > 5692 break; > 5693 case WLAN_CIPHER_SUITE_TKIP: > 5694 key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC; > 5695 default: > 5696 return -EOPNOTSUPP; > 5697 } Indeed, that looks like a bug, although kinda benign since it just means TKIP will always use software crypto and TKIP is slow anyway ;-) > I do this analysis for every warning. Now, when I say I haven't tested the code > is because I don't have any log as evidence for anything. Not that I haven't put > any effort on trying to understand it and its context. When I started working on > this task there were more than 2000 of these issues, now there are around 600 left. > > I have fixed many bugs on the way, so a good amount of work is being invested on > this, and it's paying off. :) :-) > Now, let me ask you this question: > > It would be easier for you to review this patch if I turn it into a series? > > I can do that without a problem. I'd be happy if you were to actually just mention that you've looked at them, and found them to be expected fall throughs. I'll still review them, but without that information I feel like I'm doing the first round of reviews this code ever got. johannes
Hi Gustavo, On Wed, Oct 24, 2018 at 7:36 AM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Tue, 2018-10-23 at 12:58 +0200, Gustavo A. R. Silva wrote: > > On 10/23/18 10:59 AM, Gustavo A. R. Silva wrote: > > > > > > On 10/23/18 9:01 AM, Johannes Berg wrote: > > > > On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote: > > > > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases > > > > > where we are expecting to fall through. > > > > > > > > > > Warning level 3 was used: -Wimplicit-fallthrough=3 > > > > > > > > > > This code was not tested and GCC 7.2.0 was used to compile it. > > > > > > > > Look, I'm not going to make this any clearer: I'm not applying patches > > > > like that where you've invested no effort whatsoever on verifying that > > > > they're correct. > > > > > > > > > > How do you suggest me to verify that every part is correct in this type > > > of patches? > > > > > > > BTW... I'm under the impression you think that I don't even look at > > the code. Is that correct? > > That's what your commit log looks like, yes. This is your full commit > log: > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases > where we are expecting to fall through. > > Warning level 3 was used: -Wimplicit-fallthrough=3 > > This code was not tested and GCC 7.2.0 was used to compile it. > > For all I know, you could've run spatch to add the comments wherever > there was no break, and then compiled it once. It might also help to split these up by the "type" of change. For example this patch contains: 1. A whole lot of places where there's a "if (condition) { break; }" just before the fall through You could add a line describing the logic there to the commit message, maybe something like: As there's a conditional break at the end of the case, the fall through appears to be intentional. 2. Reformatting an existing comment These should definitely be split out as they're just reformatting comments to match gcc's expectations. You could describe this like: Reformat existing fall through messages into the format expected by gcc. Thanks,
diff --git a/net/wireless/chan.c b/net/wireless/chan.c index 2db713d..0a45265 100644 --- a/net/wireless/chan.c +++ b/net/wireless/chan.c @@ -747,6 +747,7 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy, case NL80211_CHAN_WIDTH_20: if (!ht_cap->ht_supported) return false; + /* fall through */ case NL80211_CHAN_WIDTH_20_NOHT: prohibited_flags |= IEEE80211_CHAN_NO_20MHZ; width = 20; @@ -769,6 +770,7 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy, cap = vht_cap->cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK; if (cap != IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ) return false; + /* fall through */ case NL80211_CHAN_WIDTH_80: if (!vht_cap->vht_supported) return false; diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 744b585..0276370 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -1706,6 +1706,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, state->split_start++; if (state->split) break; + /* fall through */ case 1: if (nla_put(msg, NL80211_ATTR_CIPHER_SUITES, sizeof(u32) * rdev->wiphy.n_cipher_suites, @@ -1752,6 +1753,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, state->split_start++; if (state->split) break; + /* fall through */ case 2: if (nl80211_put_iftypes(msg, NL80211_ATTR_SUPPORTED_IFTYPES, rdev->wiphy.interface_modes)) @@ -1759,6 +1761,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, state->split_start++; if (state->split) break; + /* fall through */ case 3: nl_bands = nla_nest_start(msg, NL80211_ATTR_WIPHY_BANDS); if (!nl_bands) @@ -1784,6 +1787,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, state->chan_start++; if (state->split) break; + /* fall through */ default: /* add frequencies */ nl_freqs = nla_nest_start( @@ -1837,6 +1841,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, state->split_start++; if (state->split) break; + /* fall through */ case 4: nl_cmds = nla_nest_start(msg, NL80211_ATTR_SUPPORTED_COMMANDS); if (!nl_cmds) @@ -1863,6 +1868,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, state->split_start++; if (state->split) break; + /* fall through */ case 5: if (rdev->ops->remain_on_channel && (rdev->wiphy.flags & WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL) && @@ -1880,6 +1886,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, state->split_start++; if (state->split) break; + /* fall through */ case 6: #ifdef CONFIG_PM if (nl80211_send_wowlan(msg, rdev, state->split)) @@ -1890,6 +1897,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, #else state->split_start++; #endif + /* fall through */ case 7: if (nl80211_put_iftypes(msg, NL80211_ATTR_SOFTWARE_IFTYPES, rdev->wiphy.software_iftypes)) @@ -1902,6 +1910,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev, state->split_start++; if (state->split) break; + /* fall through */ case 8: if ((rdev->wiphy.flags & WIPHY_FLAG_HAVE_AP_SME) && nla_put_u32(msg, NL80211_ATTR_DEVICE_AP_SME, diff --git a/net/wireless/scan.c b/net/wireless/scan.c index d0e7472..0f0b519 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -1183,7 +1183,7 @@ cfg80211_inform_bss_data(struct wiphy *wiphy, switch (ftype) { case CFG80211_BSS_FTYPE_BEACON: ies->from_beacon = true; - /* fall through to assign */ + /* fall through - to assign */ case CFG80211_BSS_FTYPE_UNKNOWN: rcu_assign_pointer(tmp.pub.beacon_ies, ies); break; diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c index 06943d9..d522787 100644 --- a/net/wireless/wext-compat.c +++ b/net/wireless/wext-compat.c @@ -1337,6 +1337,7 @@ static struct iw_statistics *cfg80211_wireless_stats(struct net_device *dev) wstats.qual.qual = sig + 110; break; } + /* fall through */ case CFG80211_SIGNAL_TYPE_UNSPEC: if (sinfo.filled & BIT_ULL(NL80211_STA_INFO_SIGNAL)) { wstats.qual.updated |= IW_QUAL_LEVEL_UPDATED; @@ -1345,6 +1346,7 @@ static struct iw_statistics *cfg80211_wireless_stats(struct net_device *dev) wstats.qual.qual = sinfo.signal; break; } + /* fall through */ default: wstats.qual.updated |= IW_QUAL_LEVEL_INVALID; wstats.qual.updated |= IW_QUAL_QUAL_INVALID;
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Warning level 3 was used: -Wimplicit-fallthrough=3 This code was not tested and GCC 7.2.0 was used to compile it. Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- Changes in v2: - Explicity mention in the commit log that this the code was not tested. - Use warning level 3 instead of 2. - Change the form "else: fall through" to "fall through" instead. - Address -Wimplicit-fallthrough warnings in net/wireless/scan.c net/wireless/chan.c | 2 ++ net/wireless/nl80211.c | 9 +++++++++ net/wireless/scan.c | 2 +- net/wireless/wext-compat.c | 2 ++ 4 files changed, 14 insertions(+), 1 deletion(-)