| Message ID | 1402778536-31305-1-git-send-email-mihirsht@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Sat, Jun 14, 2014 at 1:42 PM, <mihirsht@gmail.com> wrote: > From: Mihir Shete <mihirsht@gmail.com> > > User hints are always ignored if cfg80211_regdom is intersected. > Add checks to make sure that user hints are processed if the > previous hints are done processing. > > Signed-off-by: Mihir Shete <mihirsht@gmail.com> > --- > net/wireless/reg.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/net/wireless/reg.c b/net/wireless/reg.c > index b4973bc..2c4e2ba 100644 > --- a/net/wireless/reg.c > +++ b/net/wireless/reg.c > @@ -396,6 +396,14 @@ static bool regdom_changes(const char *alpha2) > return !alpha2_equal(r->alpha2, alpha2); > } > > +static bool is_cfg80211_regdom_intersected(void) > +{ > + const struct ieee80211_regdomain *r = get_cfg80211_regdom(); > + > + if (!r) > + return false; > + return is_intersected_alpha2(r->alpha2); > +} Shouldn't this just be the last request->intersect too ? > /* > * The NL80211_REGDOM_SET_BY_USER regdom alpha2 is cached, this lets > * you know if a valid regulatory hint with NL80211_REGDOM_SET_BY_USER > @@ -1650,9 +1658,14 @@ __reg_process_hint_user(struct regulatory_request *user_request) > */ > if ((lr->initiator == NL80211_REGDOM_SET_BY_CORE || > lr->initiator == NL80211_REGDOM_SET_BY_DRIVER || > - lr->initiator == NL80211_REGDOM_SET_BY_USER) && > - regdom_changes(lr->alpha2)) > - return REG_REQ_IGNORE; > + lr->initiator == NL80211_REGDOM_SET_BY_USER)) { > + if (lr->intersect) { > + if (!is_cfg80211_regdom_intersected()) > + return REG_REQ_IGNORE; Let's keep this in mind: When processing requests on the queue get_last_request() will return the last requests which was accepted and already processed. When reg_update_last_request() is issued last_request becomes the newly accepted request that we are about to process. If this seems rather confusing we should split this up into two, but so far its not clear to me this is needed given we serialize processing requests and have a last_request->pending too. __reg_process_hint_user() is evaluating whether or not we should apply the passed hint. Given all this, when would lr->intersect be true but the set regdomain not be intersected? That would seem like an issue to me. Also note that your commit log seems to claim to want to make a change only for user type hints but the above folds all your desired changes to all the checks it was checking for. Lastly, we should be allowing through user changes if and only if a core or driver hint was not issued, otherwise the intersection makes sense to upkeep, however I do see it making sense to allow a user regulatory if an intersection was done but *iff* we want to clear the old userspace setting, but note that to do that cleanly we'd have to assume an original state. We can do this without much overhead by resetting the regulatory core to the original state given that core and driver hints will be cached and be set once the reset is called. You have to consider when a reset makes sense though and be very very careful about this though otherwise you might end up in inconsistent states that make no sense at run time. Consider being allowed to do certain things, being associated, and then all of a sudden driving the regulatory core which would prohibit that current state. If you really want to enable this regardless of the state you may have to trigger disassoc, etc. Please think of all this. NACK for now. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Let's keep this in mind: > > When processing requests on the queue get_last_request() will return > the last requests which was accepted and already processed. When > reg_update_last_request() is issued last_request becomes the newly > accepted request that we are about to process. If this seems rather > confusing we should split this up into two, but so far its not clear > to me this is needed given we serialize processing requests and have a > last_request->pending too. > > __reg_process_hint_user() is evaluating whether or not we should apply > the passed hint. Given all this, when would lr->intersect be true but > the set regdomain not be intersected? That would seem like an issue to > me. > hmm, yea but there can be cases when last_request is not processed and we have already called reg_update_last_request(). This happens because reg_update_last_request() is called before reg_call_crda() and so I thought we could ignore the user hints till we hear back from crda. > Also note that your commit log seems to claim to want to make a change > only for user type hints but the above folds all your desired changes > to all the checks it was checking for. > > Lastly, we should be allowing through user changes if and only if a > core or driver hint was not issued, User hints are for enhancing the compliance and they should be allowed even after the driver hints. Say, a device is configured to be sold in the US, and a User travels with it to some country where 5GHz operation is not allowed then the user hint will help ensuring that we comply with the new regulatory rules. >otherwise the intersection makes > sense to upkeep, however I do see it making sense to allow a user > regulatory if an intersection was done but *iff* we want to clear the > old userspace setting, Yes, agreed. and we have a check in __reg_process_hint_user() to override the intersection *iff* we have a *different* userspace setting. > but note that to do that cleanly we'd have to > assume an original state. We can do this without much overhead by > resetting the regulatory core to the original state given that core > and driver hints will be cached and be set once the reset is called. > You have to consider when a reset makes sense though and be very very > careful about this though otherwise you might end up in inconsistent > states that make no sense at run time. Consider being allowed to do > certain things, being associated, and then all of a sudden driving the > regulatory core which would prohibit that current state. > > If you really want to enable this regardless of the state you may have > to trigger disassoc, etc. Please think of all this. User hints can come at any time and they are always processed irrespective of the sme state, but I can see Arik is already working on taking care of this stuff.
Adding Ben and Janusz as I believe this is the core of the original issue Ben reported when a card has no regulatory domain programmed and his system ends up intersecting a user set regulatory domain and the cfg80211 regulatory domain was core. That should not happen and a proper fix would be entangled on this code section. Also adding Arik as he might be interested in the intersection case given that his new API would add a new type of regulatory setting we should consider for when then the user asks to update regulatory. On Thu, Jun 19, 2014 at 2:51 AM, Mihir Shete <mihirsht@gmail.com> wrote: >> Let's keep this in mind: >> >> When processing requests on the queue get_last_request() will return >> the last requests which was accepted and already processed. When >> reg_update_last_request() is issued last_request becomes the newly >> accepted request that we are about to process. If this seems rather >> confusing we should split this up into two, but so far its not clear >> to me this is needed given we serialize processing requests and have a >> last_request->pending too. >> >> __reg_process_hint_user() is evaluating whether or not we should apply >> the passed hint. Given all this, when would lr->intersect be true but >> the set regdomain not be intersected? That would seem like an issue to >> me. >> > hmm, yea but there can be cases when last_request is not processed Not within the context of processing requests on queue -- in this particular case when we are processing requests reg_process_pending_hints() will not allow the code to continue unless the last request is already processed. > and we have already called reg_update_last_request(). This happens because > reg_update_last_request() is called before reg_call_crda() and so I thought we > could ignore the user hints till we hear back from crda. We already do that on reg_process_pending_hints(), we won't allow hints through until the last request is processed, the requests then are serialized. >> Also note that your commit log seems to claim to want to make a change >> only for user type hints but the above folds all your desired changes >> to all the checks it was checking for. >> >> Lastly, we should be allowing through user changes if and only if a >> core or driver hint was not issued, > User hints are for enhancing the compliance and they should be allowed > even after the driver hints. Yeah good point, right now we're ignoring all hints from the user if the last request was set by core, driver or user and the regdom changes. I think it makes sense then to split up each of these into their own check just to make the code clear and document each case on top with the logic. Additionally we always are setting intersection as required regardless of what __reg_process_hint_user() told us to do, this is obviously a mistake for when the regulatory domain was set by core and the regulatory domain is world roaming for example so reg_process_hint_user() should check for the treatment instead of blindly always setting intersection. I think addressing these changes would fix the issue Ben reported on his custom test environment. > Say, a device is configured to be sold in the US, and a User travels with it > to some country where 5GHz operation is not allowed then the user > hint will help ensuring that we comply with the new regulatory rules. Agreed. That should be a specific case then. >>otherwise the intersection makes >> sense to upkeep, however I do see it making sense to allow a user >> regulatory if an intersection was done but *iff* we want to clear the >> old userspace setting, > Yes, agreed. and we have a check in __reg_process_hint_user() > to override the intersection *iff* we have a *different* userspace setting. Well but that code section right now asks to ignore the request: /* * If the user knows better the user should set the regdom * to their country before the IE is picked up */ if (lr->initiator == NL80211_REGDOM_SET_BY_USER && lr->intersect) return REG_REQ_IGNORE; That comment should be removed as its there from old times and adjusted to reflect current times. We already deal with cell base station hints above, this section deals with regular hints. This needs to be updated to ensure what you say is allowed but we have to consider the different cases for intersection of a regulatory domain that was hinted by user. We can have three hints that could lead to an intersection to user when a driver hint is involved, in this order: Core (world) --> driver --> user (intersection) Core (world) --> user --> driver (intersection) The first case is what we typically would see though but both are possible. Adding a check to ensure user hints are allowed to be set if the last request was a user hint would address the first case given that the wiphy->regdg would be cached but it wouldn't allow for setting the regulatory domain for the second case. Right now a driver hint is always intersected if the last request was not core. If the regulatory hints agree REG_REQ_ALREADY_SET is always returned. As reflected in the above code section right now we're ignoring all hints from the user if the last request was set by core, driver or user and the regdom changes. The change you make would tries to detangle the double user hint case, but I think it also makes sense to detangle and document clearly what happens when driver hint --> user hint happens, that should produce an intersection. >> but note that to do that cleanly we'd have to >> assume an original state. We can do this without much overhead by >> resetting the regulatory core to the original state given that core >> and driver hints will be cached and be set once the reset is called. >> You have to consider when a reset makes sense though and be very very >> careful about this though otherwise you might end up in inconsistent >> states that make no sense at run time. Consider being allowed to do >> certain things, being associated, and then all of a sudden driving the >> regulatory core which would prohibit that current state. >> >> If you really want to enable this regardless of the state you may have >> to trigger disassoc, etc. Please think of all this. > User hints can come at any time and they are always processed irrespective > of the sme state, but I can see Arik is already working on taking care of this > stuff. Indeed, all that work will be very useful, his change becomes more important given the nature of source of regulatory data though, since we process regulatory hints serially now and if a driver is only using wireless-regdb then there should be no major issues as the regulatory state changes, but his change is more important once we consider different regulatory sources. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/wireless/reg.c b/net/wireless/reg.c index b4973bc..2c4e2ba 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -396,6 +396,14 @@ static bool regdom_changes(const char *alpha2) return !alpha2_equal(r->alpha2, alpha2); } +static bool is_cfg80211_regdom_intersected(void) +{ + const struct ieee80211_regdomain *r = get_cfg80211_regdom(); + + if (!r) + return false; + return is_intersected_alpha2(r->alpha2); +} /* * The NL80211_REGDOM_SET_BY_USER regdom alpha2 is cached, this lets * you know if a valid regulatory hint with NL80211_REGDOM_SET_BY_USER @@ -1650,9 +1658,14 @@ __reg_process_hint_user(struct regulatory_request *user_request) */ if ((lr->initiator == NL80211_REGDOM_SET_BY_CORE || lr->initiator == NL80211_REGDOM_SET_BY_DRIVER || - lr->initiator == NL80211_REGDOM_SET_BY_USER) && - regdom_changes(lr->alpha2)) - return REG_REQ_IGNORE; + lr->initiator == NL80211_REGDOM_SET_BY_USER)) { + if (lr->intersect) { + if (!is_cfg80211_regdom_intersected()) + return REG_REQ_IGNORE; + } else if (regdom_changes(lr->alpha2)) { + return REG_REQ_IGNORE; + } + } if (!regdom_changes(user_request->alpha2)) return REG_REQ_ALREADY_SET;