[PATCH 0/4] cfg80211: fix bug with multiple cards and a CRDA delay

[PATCH 0/4] cfg80211: fix bug with multiple cards and a CRDA delay

[Luis R. Rodriguez]

This series addresses the bug pointed out by Mark Mentovai <mark@moxienet.com>
when there is a delay on CRDA and you use multiple cards with the same regulatory
domain. After some testing I realized this also introduced an issue when using
multiple cards with different regulatory settings. The issue is present only
when there is a delay in CRDA so technically this is not a regression so I am
not marking these as stable fixes. It also does not seem to be a regulatory bug
as when the issue is present you'd end up with an intersected regulatory domain
(same alpha2s) or get the regulatory domain rejected for the second card if
the alpha2 was different.

Thanks to Mark for his testing, good ideas on alternative ways to resolve this
and thorough reports.

Luis R. Rodriguez (4):
  cfg80211: put core regulatory request into queue
  cfg80211: move reg_work and reg_todo above
  cfg80211: move mutex locking to reg_process_pending_hints()
  cfg80211: Fix regulatory bug with multiple cards and delays

 include/net/regulatory.h |    7 ++++
 net/wireless/reg.c       |   80 +++++++++++++++++++++++++++++++--------------
 2 files changed, 62 insertions(+), 25 deletions(-)

-- 
1.7.3.2.90.gd4c43

[PATCH 1/4] cfg80211: put core regulatory request into queue

[Luis R. Rodriguez]

This will simplify the synchronization for pending requests.
Without this we have a race between the core and when we
restore regulatory settings, although this is unlikely
its best to just avoid that race altogether.

Cc: Mark Mentovai <mark@moxienet.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/reg.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 3be18d9..9830db6 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1530,12 +1530,7 @@ static int regulatory_hint_core(const char *alpha2)
 	request->alpha2[1] = alpha2[1];
 	request->initiator = NL80211_REGDOM_SET_BY_CORE;
 
-	/*
-	 * This ensures last_request is populated once modules
-	 * come swinging in and calling regulatory hints and
-	 * wiphy_apply_custom_regulatory().
-	 */
-	reg_process_hint(request);
+	queue_regulatory_request(request);
 
 	return 0;
 }
-- 
1.7.3.2.90.gd4c43

[PATCH 2/4] cfg80211: move reg_work and reg_todo above

[Luis R. Rodriguez]

These will be used earlier in the next few patches.

Cc: Mark Mentovai <mark@moxienet.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/reg.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 9830db6..3fa2474 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -96,6 +96,9 @@ struct reg_beacon {
 	struct ieee80211_channel chan;
 };
 
+static void reg_todo(struct work_struct *work);
+static DECLARE_WORK(reg_work, reg_todo);
+
 /* We keep a static world regulatory domain in case of the absence of CRDA */
 static const struct ieee80211_regdomain world_regdom = {
 	.n_reg_rules = 5,
@@ -1494,8 +1497,6 @@ static void reg_todo(struct work_struct *work)
 	reg_process_pending_beacon_hints();
 }
 
-static DECLARE_WORK(reg_work, reg_todo);
-
 static void queue_regulatory_request(struct regulatory_request *request)
 {
 	if (isalpha(request->alpha2[0]))
-- 
1.7.3.2.90.gd4c43

[PATCH 3/4] cfg80211: move mutex locking to reg_process_pending_hints()

[Luis R. Rodriguez]

This will be required in the next patch and it makes the
next patch easier to review.

Cc: Mark Mentovai <mark@moxienet.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/reg.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 3fa2474..b522c46 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1412,16 +1412,13 @@ static void reg_process_hint(struct regulatory_request *reg_request)
 
 	BUG_ON(!reg_request->alpha2);
 
-	mutex_lock(&cfg80211_mutex);
-	mutex_lock(&reg_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);
@@ -1429,16 +1426,16 @@ 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(&reg_mutex);
-	mutex_unlock(&cfg80211_mutex);
 }
 
 /* Processes regulatory hints, this is all the NL80211_REGDOM_SET_BY_* */
 static void reg_process_pending_hints(void)
-	{
+{
 	struct regulatory_request *reg_request;
 
+	mutex_lock(&cfg80211_mutex);
+	mutex_lock(&reg_mutex);
+
 	spin_lock(&reg_requests_lock);
 	while (!list_empty(&reg_requests_list)) {
 		reg_request = list_first_entry(&reg_requests_list,
@@ -1451,6 +1448,9 @@ static void reg_process_pending_hints(void)
 		spin_lock(&reg_requests_lock);
 	}
 	spin_unlock(&reg_requests_lock);
+
+	mutex_unlock(&reg_mutex);
+	mutex_unlock(&cfg80211_mutex);
 }
 
 /* Processes beacon hints -- this has nothing to do with country IEs */
-- 
1.7.3.2.90.gd4c43

Re: [PATCH 0/4] cfg80211: fix bug with multiple cards and a CRDA delay

[Bruno Randolf]

On Thu November 18 2010 14:46:05 Luis R. Rodriguez wrote:
> This series addresses the bug pointed out by Mark Mentovai
> <mark@moxienet.com> when there is a delay on CRDA and you use multiple
> cards with the same regulatory domain. After some testing I realized this
> also introduced an issue when using multiple cards with different
> regulatory settings. The issue is present only when there is a delay in
> CRDA so technically this is not a regression so I am not marking these as
> stable fixes. It also does not seem to be a regulatory bug as when the
> issue is present you'd end up with an intersected regulatory domain (same
> alpha2s) or get the regulatory domain rejected for the second card if the
> alpha2 was different.
> 
> Thanks to Mark for his testing, good ideas on alternative ways to resolve
> this and thorough reports.
> 
> Luis R. Rodriguez (4):
>   cfg80211: put core regulatory request into queue
>   cfg80211: move reg_work and reg_todo above
>   cfg80211: move mutex locking to reg_process_pending_hints()
>   cfg80211: Fix regulatory bug with multiple cards and delays
> 
>  include/net/regulatory.h |    7 ++++
>  net/wireless/reg.c       |   80
> +++++++++++++++++++++++++++++++-------------- 2 files changed, 62
> insertions(+), 25 deletions(-)

Yep - this fixes also my case (slow board under load + two or more ath5k 
cards). Thanks, Luis!

bruno

Re: [PATCH 0/4] cfg80211: fix bug with multiple cards and a CRDA delay

[Luis R. Rodriguez]

On Wed, Nov 17, 2010 at 10:36 PM, Bruno Randolf <br1@einfach.org> wrote:
> On Thu November 18 2010 14:46:05 Luis R. Rodriguez wrote:
>> This series addresses the bug pointed out by Mark Mentovai
>> <mark@moxienet.com> when there is a delay on CRDA and you use multiple
>> cards with the same regulatory domain. After some testing I realized this
>> also introduced an issue when using multiple cards with different
>> regulatory settings. The issue is present only when there is a delay in
>> CRDA so technically this is not a regression so I am not marking these as
>> stable fixes. It also does not seem to be a regulatory bug as when the
>> issue is present you'd end up with an intersected regulatory domain (same
>> alpha2s) or get the regulatory domain rejected for the second card if the
>> alpha2 was different.
>>
>> Thanks to Mark for his testing, good ideas on alternative ways to resolve
>> this and thorough reports.
>>
>> Luis R. Rodriguez (4):
>>   cfg80211: put core regulatory request into queue
>>   cfg80211: move reg_work and reg_todo above
>>   cfg80211: move mutex locking to reg_process_pending_hints()
>>   cfg80211: Fix regulatory bug with multiple cards and delays
>>
>>  include/net/regulatory.h |    7 ++++
>>  net/wireless/reg.c       |   80
>> +++++++++++++++++++++++++++++++-------------- 2 files changed, 62
>> insertions(+), 25 deletions(-)
>
> Yep - this fixes also my case (slow board under load + two or more ath5k
> cards). Thanks, Luis!

Thanks for testing and sorry it took so long to look into this, but
I've just been piled with tons of stuff.

  Luis

Re: [PATCH 0/4] cfg80211: fix bug with multiple cards and a CRDA delay

[Mark Mentovai]

Luis R. Rodriguez wrote:
> This series addresses the bug pointed out by Mark Mentovai <mark@moxienet.com>
> when there is a delay on CRDA and you use multiple cards with the same regulatory
> domain. After some testing I realized this also introduced an issue when using
> multiple cards with different regulatory settings. The issue is present only
> when there is a delay in CRDA so technically this is not a regression so I am
> not marking these as stable fixes. It also does not seem to be a regulatory bug
> as when the issue is present you'd end up with an intersected regulatory domain
> (same alpha2s) or get the regulatory domain rejected for the second card if
> the alpha2 was different.
>
> Thanks to Mark for his testing, good ideas on alternative ways to resolve this
> and thorough reports.

Excellent! I’ve read through this patch set and approve. I’ve tested
it out. It definitely does the trick.

The one thing I spotted was that you didn’t revise the @processed
comment in regulatory.h from patch 4.

I don’t know which of Signed-off-by, Acked-by, or Tested-by is
appropriate here, but as far as I’m concerned, this is a winner.

Re: [PATCH 0/4] cfg80211: fix bug with multiple cards and a CRDA delay

[Luis R. Rodriguez]

On Thu, Nov 18, 2010 at 9:41 AM, Mark Mentovai <mark@moxienet.com> wrote:
> Luis R. Rodriguez wrote:
>> This series addresses the bug pointed out by Mark Mentovai <mark@moxienet.com>
>> when there is a delay on CRDA and you use multiple cards with the same regulatory
>> domain. After some testing I realized this also introduced an issue when using
>> multiple cards with different regulatory settings. The issue is present only
>> when there is a delay in CRDA so technically this is not a regression so I am
>> not marking these as stable fixes. It also does not seem to be a regulatory bug
>> as when the issue is present you'd end up with an intersected regulatory domain
>> (same alpha2s) or get the regulatory domain rejected for the second card if
>> the alpha2 was different.
>>
>> Thanks to Mark for his testing, good ideas on alternative ways to resolve this
>> and thorough reports.
>
> Excellent! I’ve read through this patch set and approve. I’ve tested
> it out. It definitely does the trick.
>
> The one thing I spotted was that you didn’t revise the @processed
> comment in regulatory.h from patch 4.

I did but did not see any difference in your comments.

> I don’t know which of Signed-off-by, Acked-by, or Tested-by is
> appropriate here, but as far as I’m concerned, this is a winner.

Tesed-by: foo would be good.

  Luis

Re: [PATCH 0/4] cfg80211: fix bug with multiple cards and a CRDA delay

[Mark Mentovai]

Luis R. Rodriguez wrote:
> I did but did not see any difference in your comments.

“whether or not this requests has already” — request, not requests.
“currently regulatory domain set” — should be “currently set regulatory domain”.
“When a the last request” — no a, just the.

> Tesed-by: foo would be good.

Sure!

Tested-by: Mark Mentovai <mark@moxienet.com>

Re: [PATCH 0/4] cfg80211: fix bug with multiple cards and a CRDA delay

[Luis R. Rodriguez]

On Thu, Nov 18, 2010 at 10:52 AM, Mark Mentovai <mark@moxienet.com> wrote:
> Luis R. Rodriguez wrote:
>> I did but did not see any difference in your comments.
>
> “whether or not this requests has already” — request, not requests.
> “currently regulatory domain set” — should be “currently set regulatory domain”.
> “When a the last request” — no a, just the.

Don't get it, please send a patch on top of this :)

>> Tesed-by: foo would be good.
>
> Sure!
>
> Tested-by: Mark Mentovai <mark@moxienet.com>

Thanks,

  Luis

Re: [PATCH 0/4] cfg80211: fix bug with multiple cards and a CRDA delay

[Bruno Randolf]

On Fri November 19 2010 03:52:39 Mark Mentovai wrote:
> Luis R. Rodriguez wrote:
> > I did but did not see any difference in your comments.
> 
> “whether or not this requests has already” — request, not requests.
> “currently regulatory domain set” — should be “currently set regulatory
> domain”. “When a the last request” — no a, just the.
> 
> > Tesed-by: foo would be good.
> 
> Sure!
> 
> Tested-by: Mark Mentovai <mark@moxienet.com>

Tested-by: Bruno Randolf <br1@einfach.org>