On Fri, Nov 12, 2010 at 12:27:32PM -0800, Luis Rodriguez wrote:
> On Thu, Nov 11, 2010 at 10:05:21PM -0800, Mark Mentovai wrote:
> > Luis R. Rodriguez wrote:
> > > When two cards are connected with the same regulatory domain
> > > if CRDA had a delayed response then cfg80211's own set regulatory
> > > domain would still be the world regulatory domain. There was a bug
> > > on cfg80211's ignore_request() when analyzing incoming driver
> > > regulatory hints as it was only checking against the currently
> > > set cfg80211 regulatory domain and not for any other new
> > > pending requests. This is easily fixed by also checking against
> > > the pending request.
> >
> > Luis, thanks for taking a look at this bug.
> >
> > Capsule summary: youâve overloaded -EALREADY, which until now seemed
> > to mean âthatâs the regdomain Iâm already using,â to now also mean
> > âIâm going to be using that regdomain soon but Iâm waiting on CRDA.â
> > The two cases need to be handled differently.
> >
> > In my case, this patch makes things a little bit worse. I tested it
> > with compat_wireless 20101110. Iâve included what I see after boot
> > below the explanation.
> >
> > Your patch changes things so that, according to the steps in my
> > original message, steps 3 and 4 become:
> >
> > 3. The second cardâs driver calls regulatory_hint to provide US as a
> > driver hint. ignore_request sees that the last request came from a
> > driver and that the current and last request sought to set the same
> > hint, so it returns -EALREADY. This triggers the âif the regulatory
> > domain being requested by the driverâ block, which calls reg_copy_regd
> > on the apparent assumption that cfg80211_regdomain contains the
> > regdomain that the wiphy actually wants, although it doesnât, and itâs
> > very wrong to do this copy. cfg80211_regdomain is still on 00, because
> > CRDA hasnât responded to the first cardâs request yet.
> >
> > 4. When CRDA finally responds to the first cardâs request from #2, it
> > gets plumbed to set_regdom, which calls __set_regdom. __set_regdom
> > sees that itâs not intersecting, and enters the block where it would
> > normally copy reg_copy_regd to set the wiphyâs regd, but the wiphy it
> > uses is the one saved from last_request (in step 3), and it already
> > had something copied to it (also in step 3). Since __set_regdom checks
> > for this and bails out with -EALREADY in the âuserspace could have
> > sent two replies with only one kernel requestâ case. Because
> > __set_regdom fails, set_regdom returns early and never makes it to the
> > update_all_wiphy_regulatory or print_regdomain steps. The regdomain
> > remains unchanged. Iâm still stuck at 00.
> >
> > What about using something other than -EALREADY to signal âthat
> > request is already pending?â On its face, this works, but I think
> > thereâs a deeper flaw that also needs to be addressed. Iâm concerned
> > that the wiphys that fall into this state wonât see a reg_copy_regd
> > call at all. The idea is that any such wiphy shouldnât really be
> > ignored, but it should be joined to a group of wiphys waiting on the
> > pending request, and when the request is satisfied, the regd field
> > should be populated for each of them.
>
> Thanks for testing and your review.
>
> We need to address a queue of requests with the associated wiphys,
> as I originally had developed, I'll go ahead and add this and
> treat these, it should take care of even multiple cards with
> different regulatory hints. I expect the amount of code will be
> a bit to large for stable though so likely we may only be able
> to fix this for new kernels.
>
> I'll take a stab at this now.
OK we did have the queuing mechanism in place but we were not processing
the requests atomically. Please give the patch below a shot.
diff --git a/include/net/regulatory.h b/include/net/regulatory.h
index 9e103a4..356d6e3 100644
--- a/include/net/regulatory.h
+++ b/include/net/regulatory.h
@@ -43,6 +43,12 @@ enum environment_cap {
* @intersect: indicates whether the wireless core should intersect
* the requested regulatory domain with the presently set regulatory
* domain.
+ * @processed: indicates whether or not this requests has already been
+ * processed. When the last request is processed it means that the
+ * currently regulatory domain set on cfg80211 is updated from
+ * CRDA and can be used by other regulatory requests. When a
+ * the last request is not yet processed we must yield until it
+ * is processed before processing any new requests.
* @country_ie_checksum: checksum of the last processed and accepted
* country IE
* @country_ie_env: lets us know if the AP is telling us we are outdoor,
@@ -54,6 +60,7 @@ struct regulatory_request {
enum nl80211_reg_initiator initiator;
char alpha2[2];
bool intersect;
+ bool processed;
enum environment_cap country_ie_env;
struct list_head list;
};
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 3be18d9..570df1e 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1392,8 +1392,10 @@ new_request:
* have applied the requested regulatory domain before we just
* inform userspace we have processed the request
*/
- if (r == -EALREADY)
+ if (r == -EALREADY) {
nl80211_send_reg_change_event(last_request);
+ last_request->processed = true;
+ }
return r;
}
@@ -1409,16 +1411,13 @@ static void reg_process_hint(struct regulatory_request *reg_request)
BUG_ON(!reg_request->alpha2);
- mutex_lock(&cfg80211_mutex);
- mutex_lock(®_mutex);
-
if (wiphy_idx_valid(reg_request->wiphy_idx))
wiphy = wiphy_idx_to_wiphy(reg_request->wiphy_idx);
if (reg_request->initiator == NL80211_REGDOM_SET_BY_DRIVER &&
!wiphy) {
kfree(reg_request);
- goto out;
+ return;
}
r = __regulatory_hint(wiphy, reg_request);
@@ -1426,28 +1425,52 @@ static void reg_process_hint(struct regulatory_request *reg_request)
if (r == -EALREADY && wiphy &&
wiphy->flags & WIPHY_FLAG_STRICT_REGULATORY)
wiphy_update_regulatory(wiphy, initiator);
-out:
- mutex_unlock(®_mutex);
- mutex_unlock(&cfg80211_mutex);
}
-/* Processes regulatory hints, this is all the NL80211_REGDOM_SET_BY_* */
+static void reg_delayed_todo(struct work_struct *work);
+static DECLARE_DELAYED_WORK(reg_delayed_work, reg_delayed_todo);
+
+/*
+ * Processes regulatory hints, this is all the NL80211_REGDOM_SET_BY_*
+ * Regulatory hints come on a first come first serve basis and we
+ * must process each one atomically.
+ */
static void reg_process_pending_hints(void)
{
struct regulatory_request *reg_request;
+ mutex_lock(&cfg80211_mutex);
+ mutex_lock(®_mutex);
+
+ if (!last_request->processed) {
+ REG_DBG_PRINT("Pending regulatory request, waiting "
+ "for it to be processed...");
+ schedule_delayed_work(®_delayed_work, HZ / 20);
+ goto unlock;
+ }
+
spin_lock(®_requests_lock);
- while (!list_empty(®_requests_list)) {
- reg_request = list_first_entry(®_requests_list,
- struct regulatory_request,
- list);
- list_del_init(®_request->list);
+ if (list_empty(®_requests_list)) {
spin_unlock(®_requests_lock);
- reg_process_hint(reg_request);
- spin_lock(®_requests_lock);
+ goto unlock;
}
+
+ reg_request = list_first_entry(®_requests_list,
+ struct regulatory_request,
+ list);
+ list_del_init(®_request->list);
+
+ reg_process_hint(reg_request);
+
+ if (!list_empty(®_requests_list))
+ schedule_delayed_work(®_delayed_work, HZ / 20);
+
spin_unlock(®_requests_lock);
+
+unlock:
+ mutex_unlock(®_mutex);
+ mutex_unlock(&cfg80211_mutex);
}
/* Processes beacon hints -- this has nothing to do with country IEs */
@@ -1494,6 +1517,12 @@ static void reg_todo(struct work_struct *work)
reg_process_pending_beacon_hints();
}
+static void reg_delayed_todo(struct work_struct *work)
+{
+ reg_process_pending_hints();
+ reg_process_pending_beacon_hints();
+}
+
static DECLARE_WORK(reg_work, reg_todo);
static void queue_regulatory_request(struct regulatory_request *request)
@@ -1746,11 +1775,11 @@ static void restore_regulatory_settings(bool reset_user)
/* First restore to the basic regulatory settings */
cfg80211_regdomain = cfg80211_world_regdom;
+ regulatory_hint_core(cfg80211_regdomain->alpha2);
+
mutex_unlock(®_mutex);
mutex_unlock(&cfg80211_mutex);
- regulatory_hint_core(cfg80211_regdomain->alpha2);
-
/*
* This restores the ieee80211_regdom module parameter
* preference or the last user requested regulatory
@@ -2061,6 +2090,8 @@ int set_regdom(const struct ieee80211_regdomain *rd)
nl80211_send_reg_change_event(last_request);
+ last_request->processed = true;
+
mutex_unlock(®_mutex);
return r;
@@ -2105,10 +2136,15 @@ int __init regulatory_init(void)
user_alpha2[0] = '9';
user_alpha2[1] = '7';
+ mutex_lock(&cfg80211_mutex);
+ mutex_lock(®_mutex);
+
/* We always try to get an update for the static regdomain */
err = regulatory_hint_core(cfg80211_regdomain->alpha2);
if (err) {
if (err == -ENOMEM)
+ mutex_unlock(&cfg80211_mutex);
+ mutex_unlock(®_mutex);
return err;
/*
* N.B. kobject_uevent_env() can fail mainly for when we're out
@@ -2125,6 +2161,9 @@ int __init regulatory_init(void)
#endif
}
+ mutex_unlock(®_mutex);
+ mutex_unlock(&cfg80211_mutex);
+
/*
* Finally, if the user set the module parameter treat it
* as a user hint.
@@ -2141,6 +2180,7 @@ void /* __init_or_exit */ regulatory_exit(void)
struct reg_beacon *reg_beacon, *btmp;
cancel_work_sync(®_work);
+ cancel_delayed_work_sync(®_delayed_work);
mutex_lock(&cfg80211_mutex);
mutex_lock(®_mutex);
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
