On Tue, May 25, 2010 at 12:02 PM, Luis R. Rodriguez <mcgrof@xxxxxxxxx> wrote:
> On Mon, May 24, 2010 at 11:50 PM, Juuso Oikarinen
> <juuso.oikarinen@xxxxxxxxx> wrote:
>> The userspace crda utility can fail to respond to kernel requests in (at least)
>> two scenarios: it is not runnable for any reason, or it is invoked with a
>> country code not in its database.
>>
>> When the userspace crda utility fails to respond to kernel requests (i.e. it
>> does not use NL80211_CMD_SET_REG to provide the kernel regulatory information
>> for the requested country) the kernel crda subsystem will stall. It will
>> refuse to process any further regulatory hints. This is easiest demonstrated
>> by using for instance the "iw" tool:
>>
>> iw reg set EU
>> iw reg set US
>>
>> "EU" is not a country code present in the database, so user space crda will
>> not respond. Attempting to define US after that will be silently ignored
>> (internally, an -EAGAIN is the result, as the "EU" request is still
>> "being processed".)
>>
>> To fix this issue, this patch implements timeout protection for the userspace
>> crda invocation. If there is no response for five seconds, the crda code will
>> force itself to the world regulatory domain for maximum safety.
>>
>> Signed-off-by: Juuso Oikarinen <juuso.oikarinen@xxxxxxxxx>
>
> This is a great idea, I really like it and appreciate you taking the
> time to work on this, however to do this it requires a little more
> work and will point out the notes below. So NACK for now but with some
> fixes I think this would be great.
>
>> ---
>> net/wireless/reg.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 44 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> index 8f0d97d..6c945f0 100644
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -385,6 +385,40 @@ static inline void reg_regdb_query(const char *alpha2) {}
>> #endif /* CONFIG_CFG80211_INTERNAL_REGDB */
>>
>> /*
>> + * This gets invoked if crda in userspace is not responding (it's not getting
>> + * executed or the country code in the hint is not in the database.
>> + */
>> +
>> +static void call_crda_timeout_work(struct work_struct *work)
>> +{
>> + if (!last_request)
>> + return;
>> +
>> + printk(KERN_INFO "cfg80211: crda request timed out, reverting to 00\n");
>> +
>> + mutex_lock(&cfg80211_mutex);
>> +
>> + /*
>> + * As we are not getting data for the current country, force us back
>> + * to the world regdomain.
>> + */
>> + last_request->alpha2[0] = '0';
>> + last_request->alpha2[1] = '0';
>> + set_regdom(cfg80211_world_regdom);
>> + mutex_unlock(&cfg80211_mutex);
>> +}
>
>
> Actually you may want to consider using restore_regulatory_settings()
> instead as that would:
>
> * set the world regdom
> * then set the first user specified regulatory domain if they had one picked
>
> This would mean we would go into a loop here though if the user had
> specified 'EU' though so this routine first needs to be fixed to
> annotate a regulatory domain as invalid. Note that a regulatory cannot
> be invalid if CRDA just times out -- CRDA could time out if it was not
> present. So this is a bit tricky and not sure how to resolve it. John
> recently added support for building the regulatory database as part of
> the kernel but at the same time support letting CRDA still update the
> same info if it is present. Then for those setup it could be possible
> 'DE' exists on the core kernel regulatory database but if CRDA is not
> installed then CRDA will time out.
>
> Maybe what we can do is if both the internal kernel regdb *and* CRDA
> times out then mark the user specified request as invalid and restore
> to using the world regdom. This would allow the user to later install
> a CRDA with some newer regdb in userspace, for example. This allows
> for upgrading the db itself from userspace without making kernel
> changes. It allows for countries to be created.
>
>> +
>> +static DECLARE_WORK(crda_uevent_timeout_work, call_crda_timeout_work);
>> +
>> +#define CRDA_UEVENT_TIMEOUT 5000
>> +static void crda_uevent_timeout(unsigned long data)
>> +{
>> + schedule_work(&crda_uevent_timeout_work);
>> +}
>> +
>> +static DEFINE_TIMER(crda_uevent_timer, crda_uevent_timeout, 0, 0);
>
> I would prefer we use a completion here, seems a little cleaner. Check
> out ath9k/wmi.c for cmd_wait and its use with init_completion() and
> complete().
Actually it makes no sense to linger a thread here waiting for a
completion, your approach is better, never mind this comment.
Luis
--
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
