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.
Before we could stuck when processing last request
without ability to process current and any further
regulatory requests.
Reported-by: Sander Eikelenboom <[email protected]>
Tested-by: Sander Eikelenboom <[email protected]>
Tested-by: Colleen Twitty <[email protected]>
Signed-off-by: Janusz Dziedzic <[email protected]>
---
net/wireless/reg.c | 45 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 220c4a2..a2d2719 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -512,6 +512,10 @@ reg_call_crda(struct regulatory_request *request)
{
if (call_crda(request->alpha2))
return REG_REQ_IGNORE;
+
+ /* Setup timeout to check if CRDA is alive */
+ queue_delayed_work(system_power_efficient_wq,
+ ®_timeout, msecs_to_jiffies(3142));
return REG_REQ_OK;
}
@@ -1542,8 +1546,7 @@ static void reg_set_request_processed(void)
need_more_processing = true;
spin_unlock(®_requests_lock);
- if (lr->initiator == NL80211_REGDOM_SET_BY_USER)
- cancel_delayed_work(®_timeout);
+ cancel_delayed_work(®_timeout);
if (need_more_processing)
schedule_work(®_work);
@@ -1810,6 +1813,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);
@@ -1818,12 +1824,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,
- ®_timeout, msecs_to_jiffies(3142));
+ reg_process_hint_user(reg_request);
return;
case NL80211_REGDOM_SET_BY_DRIVER:
if (!wiphy)
@@ -1864,7 +1865,8 @@ static void reg_process_pending_hints(void)
/* When last_request->processed becomes true this will be rescheduled */
if (lr && !lr->processed) {
- REG_DBG_PRINT("Pending regulatory request, waiting for it to be processed...\n");
+ REG_DBG_PRINT("Pending %s regulatory request, waiting for it to be processed...\n",
+ reg_initiator_name(lr->initiator));
return;
}
@@ -2615,9 +2617,30 @@ 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, restoring regulatory settings\n",
+ reg_initiator_name(lr->initiator));
+
+ switch (lr->initiator) {
+ case NL80211_REGDOM_SET_BY_CORE:
+ case NL80211_REGDOM_SET_BY_DRIVER:
+ /* Call CRDA again for last request */
+ reg_process_hint(lr);
+ break;
+ 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
On Tue, Apr 22, 2014 at 10:24 AM, Luis R. Rodriguez
<[email protected]> wrote:
> I'll work on this and send out something
> shortly.
Actually just posted my review of the patch. If we address this I
think it can go in but I'd like it split up as much as possible -- as
is right now we don't set a limit for the recurring timeout. That
should be addressed.
Luis
On Tue, Apr 22, 2014 at 8:22 AM, Johannes Berg
<[email protected]> wrote:
> On Wed, 2014-04-16 at 12:53 +0200, Janusz Dziedzic wrote:
>> 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.
>> Before we could stuck when processing last request
>> without ability to process current and any further
>> regulatory requests.
>
> Hmm, this doesn't apply now on top of the patches Luis sent. Was this
> the patch that was meant alternatively?
Sorry I failed to pick this up amongst the threads, this one is a
replacement for the my real original 2nd and 3rd patch (2nd in
yesterday's series, but I hadn't redone the 3rd patch from the
original series) but I rather we treat things linearly and see if we
can split this up a bit, as I see this is an evolution of the work
from my original 2nd patch. I'll work on this and send out something
shortly.
Luis
On Thu, Apr 24, 2014 at 12:24 PM, Janusz Dziedzic
<[email protected]> wrote:
> On 22 April 2014 19:33, Luis R. Rodriguez <[email protected]> wrote:
>> As is, we don't ever bail out and could end up triggering CRDA to be
>> called every ~3 seconds with this patch. This is why I had added the
>> opportunistic triggers, which are technically still in place today but
>> there are only a few entries for those to kick off: beacon hints on 5
>> GHz for cards that enable beacon hints. I don't think its a good idea
>> we have a a recurring timer trigger for this. I think we need to
>> consider a time at which we should give up on CRDA being present and
>> instead rely on userspace to let us know its ready (ideally userspace
>> would do something with inotify on the path) so we can kick the queue
>> again.
>>
>
> I think about such counter/limitation before.
> But next I remove such counter.
> In case we don't have CRDA nor using INTERNAL_REGD - this mean we
> don't have correct regulatory configuration.
Just a clarification -- by don't have correct regulatory configuration
you mean 'limited' regulatory configuration, or restricted -- the
world regulatory domain.
> Why then should we stop warning about this?
WARN_ONCE() should solve this and get users / system integrators who
don't install either to do so, having a message pop up on the log ever
3 seconds however is doomed to cause a rant by anyone. An example
reasonable complaint might come from system integrators who are
comfortable with the restricted world regulatory domain, or by some
system integrators who are OK with the custom regulatory domains which
some drivers have that the API supports. Filling up their hard drive's
is not nice, specially if we can avoid it sensibly.
I'm learning that with systemd's socket activation support [0] we
should have a guarantee that any messages sent by the kernel even
early can can eventually be eaten up by udev, but what is unclear to
me is if udev will queue up any pending requests for rules to be
triggered for a binary which is on a filesystem not yet mounted. In
other words -- I think we should strive to solve this on userspace as
adding support for this in the kernel seems silly if we can do it in
userspace to solve all similar type of udev event rule dependency on
binaries.
Since I am not exactly sure if without systemd udev will get all
kernel events, even during early boot, I do think having something
sort of timeout for now maybe with WARN_ONCE() is reasonable but do
not think something recurring would be good.
[0] http://0pointer.de/blog/projects/socket-activation.html
> In case we will have counter eg. 10 x 3 seconds, we can't be sure if that is:
> - not mounted fs (fsck other ...) case
Hadn't considered at what point a forced long fsck might last but
again this is an issue other udev rules would run into as well.
> - there isn't any CRDA in the system (quite possible when using openwrt) case
Well we can distinguish this by the type of call that set the
regulatory domain, if the db.txt was compiled into the kernel cfg80211
manages setting that, so we can distinguish there if needed, right now
set_regdom() doesn't tell us if an internal db was used or not, but
that can easily change. Also if we need to change code to behave
differently for the timeout consideration, or maybe disregard it,
config_enabled(CONFIG_CFG80211_INTERNAL_REGDB) can be used.
> - when have a lot of messages in dmesg you will not know there is not
> CRDA in the system
Since you broke out the different cases -- I think this timeout seems
to make more sense now only if CONFIG_CFG80211_INTERNAL_REGDB was not
enabled, at least using WARN_ONCE() as that can be important for any
system integrator, but CONFIG_CFG80211_INTERNAL_REGDB was enabled a
simple info message stating CRDA was not found seems reasonable.
> In case of recurring timer when you will check dmes you will be sure
> there isn't any CRDA binary - and there will be clear message about
> this.
> Because of that I didn't add this counter.
I think we can do better, let me know what you think of the above.
Luis
On Wed, Apr 16, 2014 at 3:53 AM, Janusz Dziedzic
<[email protected]> wrote:
>
> 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, restoring regulatory settings\n",
> + reg_initiator_name(lr->initiator));
> +
> + switch (lr->initiator) {
> + case NL80211_REGDOM_SET_BY_CORE:
> + case NL80211_REGDOM_SET_BY_DRIVER:
> + /* Call CRDA again for last request */
> + reg_process_hint(lr);
> + break;
> + 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();
> }
Janusz,
As is, we don't ever bail out and could end up triggering CRDA to be
called every ~3 seconds with this patch. This is why I had added the
opportunistic triggers, which are technically still in place today but
there are only a few entries for those to kick off: beacon hints on 5
GHz for cards that enable beacon hints. I don't think its a good idea
we have a a recurring timer trigger for this. I think we need to
consider a time at which we should give up on CRDA being present and
instead rely on userspace to let us know its ready (ideally userspace
would do something with inotify on the path) so we can kick the queue
again.
Thoughts?
Luis
On Wed, 2014-04-16 at 12:53 +0200, Janusz Dziedzic wrote:
> 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.
> Before we could stuck when processing last request
> without ability to process current and any further
> regulatory requests.
Hmm, this doesn't apply now on top of the patches Luis sent. Was this
the patch that was meant alternatively?
johannes
On 22 April 2014 19:33, Luis R. Rodriguez <[email protected]> wrote:
> On Wed, Apr 16, 2014 at 3:53 AM, Janusz Dziedzic
> <[email protected]> wrote:
>>
>> 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, restoring regulatory settings\n",
>> + reg_initiator_name(lr->initiator));
>> +
>> + switch (lr->initiator) {
>> + case NL80211_REGDOM_SET_BY_CORE:
>> + case NL80211_REGDOM_SET_BY_DRIVER:
>> + /* Call CRDA again for last request */
>> + reg_process_hint(lr);
>> + break;
>> + 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();
>> }
>
> Janusz,
>
> As is, we don't ever bail out and could end up triggering CRDA to be
> called every ~3 seconds with this patch. This is why I had added the
> opportunistic triggers, which are technically still in place today but
> there are only a few entries for those to kick off: beacon hints on 5
> GHz for cards that enable beacon hints. I don't think its a good idea
> we have a a recurring timer trigger for this. I think we need to
> consider a time at which we should give up on CRDA being present and
> instead rely on userspace to let us know its ready (ideally userspace
> would do something with inotify on the path) so we can kick the queue
> again.
>
I think about such counter/limitation before.
But next I remove such counter.
In case we don't have CRDA nor using INTERNAL_REGD - this mean we
don't have correct regulatory configuration.
Why then should we stop warning about this?
In case we will have counter eg. 10 x 3 seconds, we can't be sure if that is:
- not mounted fs (fsck other ...) case
- there isn't any CRDA in the system (quite possible when using openwrt) case
- when have a lot of messages in dmesg you will not know there is not
CRDA in the system
In case of recurring timer when you will check dmes you will be sure
there isn't any CRDA binary - and there will be clear message about
this.
Because of that I didn't add this counter.
BR
Janusz