| Message ID | 20180419091738.9068-1-toke@toke.dk (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
Hello Toke & all, looking at the patch, I see more occurences of the outdated "IE" in a constant name and function name: case NL80211_REGDOM_SET_BY_COUNTRY_IE: or reg_process_hint_country_ie() Would it make sense to likewise fix these (and possibly more) occurrences too, to match the spec from 2016? For the sake of readability and consistency of code ... Bernhard Am 19.04.2018 um 11:17 schrieb Toke Høiland-Jørgensen: > The 'country IE' messages in the log can be confusing and make people think > that the country code has been set to Ireland. Fix this by changing the > log messages to use 'country element' instead (as they are no longer called > 'information element' in the spec anyway). > > Reported-by: Bernhard Gabler <Bernhard_Gabler@web.de> > Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> > --- > net/wireless/reg.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/wireless/reg.c b/net/wireless/reg.c > index 16c7e4ef5820..ecfee5f06c76 100644 > --- a/net/wireless/reg.c > +++ b/net/wireless/reg.c > @@ -1652,7 +1652,7 @@ const char *reg_initiator_name(enum nl80211_reg_initiator initiator) > case NL80211_REGDOM_SET_BY_DRIVER: > return "driver"; > case NL80211_REGDOM_SET_BY_COUNTRY_IE: > - return "country IE"; > + return "country element"; > default: > WARN_ON(1); > return "bug"; > @@ -2618,7 +2618,7 @@ reg_process_hint_country_ie(struct wiphy *wiphy, > * This doesn't happen yet, not sure we > * ever want to support it for this case. > */ > - WARN_ONCE(1, "Unexpected intersection for country IEs"); > + WARN_ONCE(1, "Unexpected intersection for country elements"); > return REG_REQ_IGNORE; > } >
On Fri, 2018-04-20 at 00:32 +0200, Bernhard Gabler wrote: > looking at the patch, I see more occurences of the outdated "IE" in a > constant name and function name: > > case NL80211_REGDOM_SET_BY_COUNTRY_IE: > or > reg_process_hint_country_ie() Right. I searched for further strings that would show up to the user, but didn't see any, but the code still refers to it in many places. > Would it make sense to likewise fix these (and possibly more) > occurrences too, to match the spec from 2016? > For the sake of readability and consistency of code ... I dunno. Country IE isn't the only place where "IE" is used, it's even used in struct names (struct ieee80211_tim_ie, and many others), so I would think that people who actually work on the code will have to be aware of the previous wording of "Information Element" (and the abbreviation IE) anyway. Changing *everything* here to be just _element instead of _ie or then probably the shorter _elem would IMHO be too much code churn. johannes
Johannes Berg <johannes@sipsolutions.net> writes: > On Fri, 2018-04-20 at 00:32 +0200, Bernhard Gabler wrote: > >> looking at the patch, I see more occurences of the outdated "IE" in a >> constant name and function name: >> >> case NL80211_REGDOM_SET_BY_COUNTRY_IE: >> or >> reg_process_hint_country_ie() > > > Right. I searched for further strings that would show up to the user, > but didn't see any, but the code still refers to it in many places. > >> Would it make sense to likewise fix these (and possibly more) >> occurrences too, to match the spec from 2016? >> For the sake of readability and consistency of code ... > > I dunno. Country IE isn't the only place where "IE" is used, it's even > used in struct names (struct ieee80211_tim_ie, and many others), so I > would think that people who actually work on the code will have to be > aware of the previous wording of "Information Element" (and the > abbreviation IE) anyway. Changing *everything* here to be just _element > instead of _ie or then probably the shorter _elem would IMHO be too much > code churn. Also, no one is going to think that reg_process_hint_country_ie() has anything to do with Ireland. So it's just a matter of outdated terminology... -Toke
just for your information. while testing your latest mac80211 tree i found out that db3bdcb9c3ffc628c5284d7ed03a704295ba1214 will break wds sta / ap support (tested in ath10k) i understand what this patch does. but this patch should not find its way into mac80211 without a patch to all affected drivers i'm pretty sure that problem will not just belong to ath10k. basicly all SW_CRYPTO_CONTROL enabled devices wont support wds anymore with that patch beside this your patch description is wrong. ap_vlan which is used for wds sta / ap operation works with encryption in ath10k and is not related in any way to sw_crypto support Sebastian Am 20.04.2018 um 08:55 schrieb Johannes Berg: > On Fri, 2018-04-20 at 00:32 +0200, Bernhard Gabler wrote: > >> looking at the patch, I see more occurences of the outdated "IE" in a >> constant name and function name: >> >> case NL80211_REGDOM_SET_BY_COUNTRY_IE: >> or >> reg_process_hint_country_ie() > > Right. I searched for further strings that would show up to the user, > but didn't see any, but the code still refers to it in many places. > >> Would it make sense to likewise fix these (and possibly more) >> occurrences too, to match the spec from 2016? >> For the sake of readability and consistency of code ... > I dunno. Country IE isn't the only place where "IE" is used, it's even > used in struct names (struct ieee80211_tim_ie, and many others), so I > would think that people who actually work on the code will have to be > aware of the previous wording of "Information Element" (and the > abbreviation IE) anyway. Changing *everything* here to be just _element > instead of _ie or then probably the shorter _elem would IMHO be too much > code churn. > > johannes >
diff --git a/net/wireless/reg.c b/net/wireless/reg.c index 16c7e4ef5820..ecfee5f06c76 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -1652,7 +1652,7 @@ const char *reg_initiator_name(enum nl80211_reg_initiator initiator) case NL80211_REGDOM_SET_BY_DRIVER: return "driver"; case NL80211_REGDOM_SET_BY_COUNTRY_IE: - return "country IE"; + return "country element"; default: WARN_ON(1); return "bug"; @@ -2618,7 +2618,7 @@ reg_process_hint_country_ie(struct wiphy *wiphy, * This doesn't happen yet, not sure we * ever want to support it for this case. */ - WARN_ONCE(1, "Unexpected intersection for country IEs"); + WARN_ONCE(1, "Unexpected intersection for country elements"); return REG_REQ_IGNORE; }
The 'country IE' messages in the log can be confusing and make people think that the country code has been set to Ireland. Fix this by changing the log messages to use 'country element' instead (as they are no longer called 'information element' in the spec anyway). Reported-by: Bernhard Gabler <Bernhard_Gabler@web.de> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk> --- net/wireless/reg.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)