[PATCH v2] cfg80211: reg: track crda request

[PATCH v2] cfg80211: reg: track crda request

[Janusz Dziedzic]

Track CRDA request and handle timeout when no answer
from CRDA. This could happen when crda is not available
yet (not mounted fs), eg. during OS startup and cfg80211
built-in to the kernel or when CRDA is not available
at all in user mode.

Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
---
V2: added counter of crda calls without an answer. After
    get maximum tries print warning.

 include/net/regulatory.h |    2 ++
 net/wireless/reg.c       |   53 +++++++++++++++++++++++++++++++++++++---------
 2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/include/net/regulatory.h b/include/net/regulatory.h
index 75fc1f5..30c34c4 100644
--- a/include/net/regulatory.h
+++ b/include/net/regulatory.h
@@ -72,6 +72,7 @@ enum environment_cap {
  * @country_ie_env: lets us know if the AP is telling us we are outdoor,
  *	indoor, or if it doesn't matter
  * @list: used to insert into the reg_requests_list linked list
+ * @crda_call_count: count of crda calls without an answer
  */
 struct regulatory_request {
 	struct rcu_head rcu_head;
@@ -84,6 +85,7 @@ struct regulatory_request {
 	bool processed;
 	enum environment_cap country_ie_env;
 	struct list_head list;
+	unsigned char crda_call_count;
 };
 
 /**
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index f59aaac..11804e8 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -479,6 +479,7 @@ static void reg_regdb_size_check(void)
 	WARN_ONCE(!reg_regdb_size, "db.txt is empty, you should update it...");
 }
 #else
+#define REG_CRDA_CALL_COUNT_MAX	5
 static inline void reg_regdb_size_check(void) {}
 static inline void reg_regdb_query(const char *alpha2) {}
 #endif /* CONFIG_CFG80211_INTERNAL_REGDB */
@@ -512,6 +513,10 @@ reg_call_crda(struct regulatory_request *request)
 {
 	if (call_crda(request->alpha2))
 		return REG_REQ_IGNORE;
+
+	/* Setup timeout to check regdb configuration */
+	queue_delayed_work(system_power_efficient_wq,
+			   &reg_timeout, msecs_to_jiffies(3142));
 	return REG_REQ_OK;
 }
 
@@ -1535,8 +1540,7 @@ static void reg_set_request_processed(void)
 		need_more_processing = true;
 	spin_unlock(&reg_requests_lock);
 
-	if (lr->initiator == NL80211_REGDOM_SET_BY_USER)
-		cancel_delayed_work(&reg_timeout);
+	cancel_delayed_work(&reg_timeout);
 
 	if (need_more_processing)
 		schedule_work(&reg_work);
@@ -1803,6 +1807,9 @@ static void reg_process_hint(struct regulatory_request *reg_request)
 	struct wiphy *wiphy = NULL;
 	enum reg_request_treatment treatment;
 
+	REG_DBG_PRINT("Process regulatory hint called by %s\n",
+		      reg_initiator_name(reg_request->initiator));
+
 	if (reg_request->wiphy_idx != WIPHY_IDX_INVALID)
 		wiphy = wiphy_idx_to_wiphy(reg_request->wiphy_idx);
 
@@ -1811,12 +1818,7 @@ static void reg_process_hint(struct regulatory_request *reg_request)
 		reg_process_hint_core(reg_request);
 		return;
 	case NL80211_REGDOM_SET_BY_USER:
-		treatment = reg_process_hint_user(reg_request);
-		if (treatment == REG_REQ_IGNORE ||
-		    treatment == REG_REQ_ALREADY_SET)
-			return;
-		queue_delayed_work(system_power_efficient_wq,
-				   &reg_timeout, msecs_to_jiffies(3142));
+		reg_process_hint_user(reg_request);
 		return;
 	case NL80211_REGDOM_SET_BY_DRIVER:
 		if (!wiphy)
@@ -2608,9 +2610,40 @@ void wiphy_regulatory_deregister(struct wiphy *wiphy)
 
 static void reg_timeout_work(struct work_struct *work)
 {
-	REG_DBG_PRINT("Timeout while waiting for CRDA to reply, restoring regulatory settings\n");
+	struct regulatory_request *lr;
+
 	rtnl_lock();
-	restore_regulatory_settings(true);
+
+	lr = get_last_request();
+	REG_DBG_PRINT("Timeout while waiting for CRDA to reply %s request\n",
+		      reg_initiator_name(lr->initiator));
+
+	switch (lr->initiator) {
+	case NL80211_REGDOM_SET_BY_CORE:
+	case NL80211_REGDOM_SET_BY_DRIVER:
+#ifdef CONFIG_CFG80211_INTERNAL_REGDB
+		pr_warn("invalid db.txt file, will use limited/restricted regulatory settings\n");
+		break;
+#else
+		if (lr->crda_call_count < REG_CRDA_CALL_COUNT_MAX) {
+			/* Call CRDA again for last request */
+			lr->crda_call_count++;
+			reg_process_hint(lr);
+		} else
+			pr_warn("CRDA not available, will use limited/restricted regulatory settings\n");
+
+		break;
+#endif /* CONFIG_CFG80211_INTERNAL_REGDB */
+	case NL80211_REGDOM_SET_BY_USER:
+		restore_regulatory_settings(true);
+		break;
+	case NL80211_REGDOM_SET_BY_COUNTRY_IE:
+		restore_regulatory_settings(false);
+		break;
+	default:
+		WARN_ON(1);
+		break;
+	}
 	rtnl_unlock();
 }
 
-- 
1.7.9.5

Re: [PATCH v2] cfg80211: reg: track crda request

[Luis R. Rodriguez]

On Fri, May 9, 2014 at 1:04 AM, Janusz Dziedzic
<janusz.dziedzic@tieto.com> wrote:
> @@ -1803,6 +1807,9 @@ static void reg_process_hint(struct regulatory_request *reg_request)
>         struct wiphy *wiphy = NULL;
>         enum reg_request_treatment treatment;
>
> +       REG_DBG_PRINT("Process regulatory hint called by %s\n",

"Processing"

> +                     reg_initiator_name(reg_request->initiator));
> +
>         if (reg_request->wiphy_idx != WIPHY_IDX_INVALID)
>                 wiphy = wiphy_idx_to_wiphy(reg_request->wiphy_idx);
>
> @@ -1811,12 +1818,7 @@ static void reg_process_hint(struct regulatory_request *reg_request)
>                 reg_process_hint_core(reg_request);
>                 return;
>         case NL80211_REGDOM_SET_BY_USER:
> -               treatment = reg_process_hint_user(reg_request);
> -               if (treatment == REG_REQ_IGNORE ||
> -                   treatment == REG_REQ_ALREADY_SET)
> -                       return;
> -               queue_delayed_work(system_power_efficient_wq,
> -                                  &reg_timeout, msecs_to_jiffies(3142));
> +               reg_process_hint_user(reg_request);

You do something more than your commit log says it does, in this case
its ignoring REG_REQ_IGNORE and REG_REQ_ALREADY_SET, explain why that
is being done, that is very important part of this change.

For example you want to explain that we typically never allowed
retries on the same regulatory hint, but this makes sense in the
corner case of udev not picking sending hints to CRDA as CRDA may be
on a partition not mounted yet. It would also be good for you to do
homework on whether or not udev with systemd guarantees rules using a
binary on a path will get all its queued up stuff only sent to it once
the partition it could be on is available. This is the right long term
solution to this and we need to be mindful of it. If you find that
systemd+udev will ensure proper synchronization then you can then say
that this timeout is only then important for legacy systems, if its
not we should look to see if we can add udev synchronization for
depending binaries used on rules.

>                 return;
>         case NL80211_REGDOM_SET_BY_DRIVER:
>                 if (!wiphy)
> @@ -2608,9 +2610,40 @@ void wiphy_regulatory_deregister(struct wiphy *wiphy)
>
>  static void reg_timeout_work(struct work_struct *work)
>  {
> -       REG_DBG_PRINT("Timeout while waiting for CRDA to reply, restoring regulatory settings\n");
> +       struct regulatory_request *lr;
> +
>         rtnl_lock();
> -       restore_regulatory_settings(true);
> +
> +       lr = get_last_request();
> +       REG_DBG_PRINT("Timeout while waiting for CRDA to reply %s request\n",
> +                     reg_initiator_name(lr->initiator));
> +
> +       switch (lr->initiator) {
> +       case NL80211_REGDOM_SET_BY_CORE:
> +       case NL80211_REGDOM_SET_BY_DRIVER:
> +#ifdef CONFIG_CFG80211_INTERNAL_REGDB

Can you use config_enabled(CONFIG_CFG80211_INTERNAL_REGDB) instead ?

> +               pr_warn("invalid db.txt file, will use limited/restricted regulatory settings\n");
> +               break;
> +#else

There is a different between CRDA not being available (we'd know if
CRDA was present once if at least for one call it was successful, but
note we don't yet distinguish the source of a regulatory domain
successful call, whether its from the internal db or not, and a change
for this is welcomed, although note that upon  restore regulatory
settings call we could be coming back from a suspend/resume remount),
a specific alpha2 not being available, and an alpha2 not being
available on the internal regdb, the above and below does not
distinguish any of this and it seems it should. If it proves to be
adding quite a bit of code, don't worry -- splitting this up into
separate calls for each case might make this code more manageable, at
least that's what happened with the processing of regulatory hints,
leave it up to you as you see the code as modify it to handle the
different cases I just outlined.

> +               if (lr->crda_call_count < REG_CRDA_CALL_COUNT_MAX) {
> +                       /* Call CRDA again for last request */
> +                       lr->crda_call_count++;
> +                       reg_process_hint(lr);

A comment specifying the things I mentioned about udev could be useful
here, but it'd be a bit nicer if this was instead just properly
documented on the crda_call_count variable enum documentation, then
you can go to town on the documentation there.

> +               } else
> +                       pr_warn("CRDA not available, will use limited/restricted regulatory settings\n");
> +
> +               break;
> +#endif /* CONFIG_CFG80211_INTERNAL_REGDB */
> +       case NL80211_REGDOM_SET_BY_USER:
> +               restore_regulatory_settings(true);
> +               break;
> +       case NL80211_REGDOM_SET_BY_COUNTRY_IE:
> +               restore_regulatory_settings(false);
> +               break;
> +       default:
> +               WARN_ON(1);
> +               break;

I'd prefer to leave this one out and instead let the compiler deal
with compilation warning about the switch not handling new cases,
which it does today because of the enum.

  Luis

Re: [PATCH v2] cfg80211: reg: track crda request

[Janusz Dziedzic]

On 20 May 2014 09:27, Luis R. Rodriguez <mcgrof@do-not-panic.com> wrote:
> On Fri, May 9, 2014 at 1:04 AM, Janusz Dziedzic
> <janusz.dziedzic@tieto.com> wrote:
>> @@ -1803,6 +1807,9 @@ static void reg_process_hint(struct regulatory_request *reg_request)
>>         struct wiphy *wiphy = NULL;
>>         enum reg_request_treatment treatment;
>>
>> +       REG_DBG_PRINT("Process regulatory hint called by %s\n",
>
> "Processing"
>
>> +                     reg_initiator_name(reg_request->initiator));
>> +
>>         if (reg_request->wiphy_idx != WIPHY_IDX_INVALID)
>>                 wiphy = wiphy_idx_to_wiphy(reg_request->wiphy_idx);
>>
>> @@ -1811,12 +1818,7 @@ static void reg_process_hint(struct regulatory_request *reg_request)
>>                 reg_process_hint_core(reg_request);
>>                 return;
>>         case NL80211_REGDOM_SET_BY_USER:
>> -               treatment = reg_process_hint_user(reg_request);
>> -               if (treatment == REG_REQ_IGNORE ||
>> -                   treatment == REG_REQ_ALREADY_SET)
>> -                       return;
>> -               queue_delayed_work(system_power_efficient_wq,
>> -                                  &reg_timeout, msecs_to_jiffies(3142));
>> +               reg_process_hint_user(reg_request);
>
> You do something more than your commit log says it does, in this case
> its ignoring REG_REQ_IGNORE and REG_REQ_ALREADY_SET, explain why that
> is being done, that is very important part of this change.
>
While timer is moved to reg_call_crda() code this check is not
required here while the same is done
in reg_process_hint_user(). So, SET_BY_USER will be handle exactly the
same like before.
So, could you describe this more ...?

> For example you want to explain that we typically never allowed
> retries on the same regulatory hint, but this makes sense in the
> corner case of udev not picking sending hints to CRDA as CRDA may be
> on a partition not mounted yet. It would also be good for you to do
> homework on whether or not udev with systemd guarantees rules using a
> binary on a path will get all its queued up stuff only sent to it once
> the partition it could be on is available. This is the right long term
> solution to this and we need to be mindful of it. If you find that
> systemd+udev will ensure proper synchronization then you can then say
> that this timeout is only then important for legacy systems, if its
> not we should look to see if we can add udev synchronization for
> depending binaries used on rules.
>
I don't care much about user mode code now (in the future we could
have some other staff on the top).
My intention of this patch was to print warning/message in dmesg,
while we don't have udev/crda/regdb/internal_regdb, we didn't have any
indication what was wrong - we could just guessing. With my patch you
will have some hint what could be wrong and I don't care much if that
is udev/systemd problem (I am not sure they are required at all, and
depends on system configuration you are using).

>>                 return;
>>         case NL80211_REGDOM_SET_BY_DRIVER:
>>                 if (!wiphy)
>> @@ -2608,9 +2610,40 @@ void wiphy_regulatory_deregister(struct wiphy *wiphy)
>>
>>  static void reg_timeout_work(struct work_struct *work)
>>  {
>> -       REG_DBG_PRINT("Timeout while waiting for CRDA to reply, restoring regulatory settings\n");
>> +       struct regulatory_request *lr;
>> +
>>         rtnl_lock();
>> -       restore_regulatory_settings(true);
>> +
>> +       lr = get_last_request();
>> +       REG_DBG_PRINT("Timeout while waiting for CRDA to reply %s request\n",
>> +                     reg_initiator_name(lr->initiator));
>> +
>> +       switch (lr->initiator) {
>> +       case NL80211_REGDOM_SET_BY_CORE:
>> +       case NL80211_REGDOM_SET_BY_DRIVER:
>> +#ifdef CONFIG_CFG80211_INTERNAL_REGDB
>
> Can you use config_enabled(CONFIG_CFG80211_INTERNAL_REGDB) instead ?
>
Sure.

>> +               pr_warn("invalid db.txt file, will use limited/restricted regulatory settings\n");
>> +               break;
>> +#else
>
> There is a different between CRDA not being available (we'd know if
> CRDA was present once if at least for one call it was successful, but
> note we don't yet distinguish the source of a regulatory domain
> successful call, whether its from the internal db or not, and a change
> for this is welcomed, although note that upon  restore regulatory
> settings call we could be coming back from a suspend/resume remount),
> a specific alpha2 not being available, and an alpha2 not being
> available on the internal regdb, the above and below does not
> distinguish any of this and it seems it should. If it proves to be
> adding quite a bit of code, don't worry -- splitting this up into
> separate calls for each case might make this code more manageable, at
> least that's what happened with the processing of regulatory hints,
> leave it up to you as you see the code as modify it to handle the
> different cases I just outlined.
>
>> +               if (lr->crda_call_count < REG_CRDA_CALL_COUNT_MAX) {
>> +                       /* Call CRDA again for last request */
>> +                       lr->crda_call_count++;
>> +                       reg_process_hint(lr);
>
> A comment specifying the things I mentioned about udev could be useful
> here, but it'd be a bit nicer if this was instead just properly
> documented on the crda_call_count variable enum documentation, then
> you can go to town on the documentation there.
>
Sure

>> +               } else
>> +                       pr_warn("CRDA not available, will use limited/restricted regulatory settings\n");
>> +
>> +               break;
>> +#endif /* CONFIG_CFG80211_INTERNAL_REGDB */
>> +       case NL80211_REGDOM_SET_BY_USER:
>> +               restore_regulatory_settings(true);
>> +               break;
>> +       case NL80211_REGDOM_SET_BY_COUNTRY_IE:
>> +               restore_regulatory_settings(false);
>> +               break;
>> +       default:
>> +               WARN_ON(1);
>> +               break;
>
> I'd prefer to leave this one out and instead let the compiler deal
> with compilation warning about the switch not handling new cases,
> which it does today because of the enum.
>
Sure

>   Luis