Patchwork driver_nl80211: set_country fix

login
register
mail settings
Submitter Igal Chernobelsky
Date June 23, 2013, 1:07 p.m.
Message ID <1371992866-18168-1-git-send-email-igalc@ti.com>
Download mbox | patch
Permalink /patch/253467/
State Rejected
Headers show

Comments

Igal Chernobelsky - June 23, 2013, 1:07 p.m.
Regulatory domain setting may take some time.
Getting reg domain info immediately after reg domain setting returns
invalid reg domain data. Loop is added to read reg domain info back and
to compare set country. If read country does not match, fail to 1 sec sleep
and retry get/compare again.

Signed-off-by: Igal Chernobelsky <igalc@ti.com>
---
 src/drivers/driver_nl80211.c |   53 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 52 insertions(+), 1 deletions(-)
Johannes Berg - June 24, 2013, 8:04 a.m.
On Sun, 2013-06-23 at 16:07 +0300, Igal Chernobelsky wrote:
> Regulatory domain setting may take some time.
> Getting reg domain info immediately after reg domain setting returns
> invalid reg domain data. Loop is added to read reg domain info back and
> to compare set country. If read country does not match, fail to 1 sec sleep
> and retry get/compare again.
> 
> Signed-off-by: Igal Chernobelsky <igalc@ti.com>
> ---
>  src/drivers/driver_nl80211.c |   53 +++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 52 insertions(+), 1 deletions(-)
> 
> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
> index f705a0c..cc5084b 100644
> --- a/src/drivers/driver_nl80211.c
> +++ b/src/drivers/driver_nl80211.c
> @@ -2689,6 +2689,37 @@ static void wpa_driver_nl80211_event_receive(int sock, void *eloop_ctx,
>  }
>  
> 
> +static int get_country_handler(struct nl_msg *msg, void *arg)
> +{
> +	char * alpha2 = (char *) arg;
> +	struct nlattr *tb_msg[NL80211_ATTR_MAX + 1];
> +	struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg));
> +
> +	nla_parse(tb_msg, NL80211_ATTR_MAX, genlmsg_attrdata(gnlh, 0),
> +		  genlmsg_attrlen(gnlh, 0), NULL);
> +	if (!tb_msg[NL80211_ATTR_REG_ALPHA2]) {
> +		wpa_printf(MSG_DEBUG, "nl80211: No country information "
> +			   "available");
> +		return NL_SKIP;
> +	}
> +	os_memcpy(alpha2, nla_data(tb_msg[NL80211_ATTR_REG_ALPHA2]), 3);
> +	return NL_SKIP;
> +}
> +
> +static int nl80211_get_country(struct wpa_driver_nl80211_data *drv,
> +				char *alpha2)
> +{
> +	struct nl_msg *msg;
> +
> +	msg = nlmsg_alloc();
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	nl80211_cmd(drv, msg, 0, NL80211_CMD_GET_REG);
> +	return send_and_recv_msgs(drv, msg, get_country_handler, alpha2);
> +}
> +
> +
>  /**
>   * wpa_driver_nl80211_set_country - ask nl80211 to set the regulatory domain
>   * @priv: driver_nl80211 private data
> @@ -2703,7 +2734,10 @@ static int wpa_driver_nl80211_set_country(void *priv, const char *alpha2_arg)
>  	struct i802_bss *bss = priv;
>  	struct wpa_driver_nl80211_data *drv = bss->drv;
>  	char alpha2[3];
> +	char alpha2_res[3];
>  	struct nl_msg *msg;
> +	int count;
> +	int ret;
>  
>  	msg = nlmsg_alloc();
>  	if (!msg)
> @@ -2718,7 +2752,24 @@ static int wpa_driver_nl80211_set_country(void *priv, const char *alpha2_arg)
>  	NLA_PUT_STRING(msg, NL80211_ATTR_REG_ALPHA2, alpha2);
>  	if (send_and_recv_msgs(drv, msg, NULL, NULL))
>  		return -EINVAL;
> -	return 0;
> +
> +	/* Validate coutry setting and retry up to 3 times if not match */
> +	for (count = 0; count < 3; count++) {
> +		os_memset(alpha2_res, 0, sizeof(alpha2));
> +		ret = nl80211_get_country(drv, alpha2_res);
> +		if (ret)
> +			return ret;
> +		if (os_strncmp(alpha2, alpha2_res, 3)) {
> +			wpa_printf(MSG_DEBUG, "wpa_driver_nl80211_set_country:"
> +					"retry country set after delay");
> +			os_sleep(1, 0);
> +		} else {
> +			/* contry set is completed */
> +			return 0;
> +		}
> +	}
> +	wpa_printf(MSG_ERROR, "wpa_driver_nl80211_set_country: failed");
> +	return -EINVAL;

I don't think this is really a failure -- if you request a country but
the kernel already has one it might create an intersection etc. Might
want to even abort the loop in that case.

johannes
Igal Chernobelsky - June 25, 2013, 7:43 a.m.
On 06/24/2013 11:04 AM, Johannes Berg wrote:
> On Sun, 2013-06-23 at 16:07 +0300, Igal Chernobelsky wrote:
>> Regulatory domain setting may take some time.
>> Getting reg domain info immediately after reg domain setting returns
>> invalid reg domain data. Loop is added to read reg domain info back and
>> to compare set country. If read country does not match, fail to 1 sec sleep
>> and retry get/compare again.
>>
>> Signed-off-by: Igal Chernobelsky<igalc@ti.com>
>> ---
>>   src/drivers/driver_nl80211.c |   53 +++++++++++++++++++++++++++++++++++++++++-
>>   1 files changed, 52 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
>> index f705a0c..cc5084b 100644
>> --- a/src/drivers/driver_nl80211.c
>> +++ b/src/drivers/driver_nl80211.c
>> @@ -2689,6 +2689,37 @@ static void wpa_driver_nl80211_event_receive(int sock, void *eloop_ctx,
>>   }
>>
>>
>> +static int get_country_handler(struct nl_msg *msg, void *arg)
>> +{
>> +	char * alpha2 = (char *) arg;
>> +	struct nlattr *tb_msg[NL80211_ATTR_MAX + 1];
>> +	struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg));
>> +
>> +	nla_parse(tb_msg, NL80211_ATTR_MAX, genlmsg_attrdata(gnlh, 0),
>> +		  genlmsg_attrlen(gnlh, 0), NULL);
>> +	if (!tb_msg[NL80211_ATTR_REG_ALPHA2]) {
>> +		wpa_printf(MSG_DEBUG, "nl80211: No country information "
>> +			   "available");
>> +		return NL_SKIP;
>> +	}
>> +	os_memcpy(alpha2, nla_data(tb_msg[NL80211_ATTR_REG_ALPHA2]), 3);
>> +	return NL_SKIP;
>> +}
>> +
>> +static int nl80211_get_country(struct wpa_driver_nl80211_data *drv,
>> +				char *alpha2)
>> +{
>> +	struct nl_msg *msg;
>> +
>> +	msg = nlmsg_alloc();
>> +	if (!msg)
>> +		return -ENOMEM;
>> +
>> +	nl80211_cmd(drv, msg, 0, NL80211_CMD_GET_REG);
>> +	return send_and_recv_msgs(drv, msg, get_country_handler, alpha2);
>> +}
>> +
>> +
>>   /**
>>    * wpa_driver_nl80211_set_country - ask nl80211 to set the regulatory domain
>>    * @priv: driver_nl80211 private data
>> @@ -2703,7 +2734,10 @@ static int wpa_driver_nl80211_set_country(void *priv, const char *alpha2_arg)
>>   	struct i802_bss *bss = priv;
>>   	struct wpa_driver_nl80211_data *drv = bss->drv;
>>   	char alpha2[3];
>> +	char alpha2_res[3];
>>   	struct nl_msg *msg;
>> +	int count;
>> +	int ret;
>>
>>   	msg = nlmsg_alloc();
>>   	if (!msg)
>> @@ -2718,7 +2752,24 @@ static int wpa_driver_nl80211_set_country(void *priv, const char *alpha2_arg)
>>   	NLA_PUT_STRING(msg, NL80211_ATTR_REG_ALPHA2, alpha2);
>>   	if (send_and_recv_msgs(drv, msg, NULL, NULL))
>>   		return -EINVAL;
>> -	return 0;
>> +
>> +	/* Validate coutry setting and retry up to 3 times if not match */
>> +	for (count = 0; count<  3; count++) {
>> +		os_memset(alpha2_res, 0, sizeof(alpha2));
>> +		ret = nl80211_get_country(drv, alpha2_res);
>> +		if (ret)
>> +			return ret;
>> +		if (os_strncmp(alpha2, alpha2_res, 3)) {
>> +			wpa_printf(MSG_DEBUG, "wpa_driver_nl80211_set_country:"
>> +					"retry country set after delay");
>> +			os_sleep(1, 0);
>> +		} else {
>> +			/* contry set is completed */
>> +			return 0;
>> +		}
>> +	}
>> +	wpa_printf(MSG_ERROR, "wpa_driver_nl80211_set_country: failed");
>> +	return -EINVAL;
> I don't think this is really a failure -- if you request a country but
> the kernel already has one it might create an intersection etc. Might
> want to even abort the loop in that case.
>
> johannes
>

Johannes, is it possible for you to elaborate about this use case?

The patch is intended to fix hostapd issue while starting on 5 GHz channel
(e.g. country_code=US,  channel=36). Hostapd initialization always fails
at the first start while reg domain is still being configured to US
(tested on TI Sitara am335x evm board).

In this use case hostapd sets US reg domain and then immediately reads back
hw supported modes and channels (get_hw_feature_data) which returns 
invalid data
at this phase (still for default reg domain and not US). So AP 
initialization fails due to
5GHz channels is not supported to start Beacons tx. Re-running hostapd 
the next time is OK
as reg domain has been already correctly set to US.

The patch propose synchronization mechanism for setting reg domain (set 
and read back)
before reading hw supported modes.
Jouni Malinen - June 25, 2013, 8:19 a.m.
On Sun, Jun 23, 2013 at 04:07:46PM +0300, Igal Chernobelsky wrote:
> Regulatory domain setting may take some time.
> Getting reg domain info immediately after reg domain setting returns
> invalid reg domain data. Loop is added to read reg domain info back and
> to compare set country. If read country does not match, fail to 1 sec sleep
> and retry get/compare again.

Please add the details from your email from today to the commit
message.. It was not immediately clear why this message would map with
the implementation change and especially so why this would be needed in
the first place.

I don't really like the one second sleep (blocking everything
wpa_supplicant/hostapd is doing) here, though. cfg80211 regulatory is
supposed to use events to indicate when regulatory changes. Couldn't
those be used for this, too? A blocking sleep could work fine for cases
where there is only a single interface, but it does not behave nicely if
there are concurrent operations, so I would rather make only the case
that absolutely needs this to block wait (or ideally, use a callback
from the nl80211 event handler in case the country code change needs to
wait for the kernel to complete processing).
Johannes Berg - June 25, 2013, 8:30 a.m.
On Tue, 2013-06-25 at 10:43 +0300, Igal Chernobelsky wrote:

[please trim your quotes]

> >> +	wpa_printf(MSG_ERROR, "wpa_driver_nl80211_set_country: failed");
> >> +	return -EINVAL;
> > I don't think this is really a failure -- if you request a country but
> > the kernel already has one it might create an intersection etc. Might
> > want to even abort the loop in that case.

> Johannes, is it possible for you to elaborate about this use case?

I think I pretty much said it all ... "if you request a country but the
kernel already has one it might create an intersection" ... then it will
never report the country back, but will report "01" or something like
that (don't remember.)

Thus you'll wait for nothing.

johannes
Igal Chernobelsky - June 25, 2013, 11:16 a.m.
On 06/25/2013 11:19 AM, Jouni Malinen wrote:
> On Sun, Jun 23, 2013 at 04:07:46PM +0300, Igal Chernobelsky wrote:
>> Regulatory domain setting may take some time.
>> Getting reg domain info immediately after reg domain setting returns
>> invalid reg domain data. Loop is added to read reg domain info back and
>> to compare set country. If read country does not match, fail to 1 sec sleep
>> and retry get/compare again.
> Please add the details from your email from today to the commit
> message.. It was not immediately clear why this message would map with
> the implementation change and especially so why this would be needed in
> the first place.
Ok, I will update.
> I don't really like the one second sleep (blocking everything
> wpa_supplicant/hostapd is doing) here, though. cfg80211 regulatory is
> supposed to use events to indicate when regulatory changes. Couldn't
> those be used for this, too? A blocking sleep could work fine for cases
> where there is only a single interface, but it does not behave nicely if
> there are concurrent operations, so I would rather make only the case
> that absolutely needs this to block wait (or ideally, use a callback
> from the nl80211 event handler in case the country code change needs to
> wait for the kernel to complete processing).
>
I agree with you blocking-sleep function is not appropriate solution for 
many common tasks
but for some specific cases it can be simple and robust solution,
like for system/interface initialization where it is not possible to 
continue
until system setup is completed.
Please note, the function fails to blocking/sleep only in the case when 
reg domain setting is still
not finished and this use case usually happens only once during system 
initialization.
So I would regard code simplicity and performing system initialization 
synchronously.

Patch

diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
index f705a0c..cc5084b 100644
--- a/src/drivers/driver_nl80211.c
+++ b/src/drivers/driver_nl80211.c
@@ -2689,6 +2689,37 @@  static void wpa_driver_nl80211_event_receive(int sock, void *eloop_ctx,
 }
 
 
+static int get_country_handler(struct nl_msg *msg, void *arg)
+{
+	char * alpha2 = (char *) arg;
+	struct nlattr *tb_msg[NL80211_ATTR_MAX + 1];
+	struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg));
+
+	nla_parse(tb_msg, NL80211_ATTR_MAX, genlmsg_attrdata(gnlh, 0),
+		  genlmsg_attrlen(gnlh, 0), NULL);
+	if (!tb_msg[NL80211_ATTR_REG_ALPHA2]) {
+		wpa_printf(MSG_DEBUG, "nl80211: No country information "
+			   "available");
+		return NL_SKIP;
+	}
+	os_memcpy(alpha2, nla_data(tb_msg[NL80211_ATTR_REG_ALPHA2]), 3);
+	return NL_SKIP;
+}
+
+static int nl80211_get_country(struct wpa_driver_nl80211_data *drv,
+				char *alpha2)
+{
+	struct nl_msg *msg;
+
+	msg = nlmsg_alloc();
+	if (!msg)
+		return -ENOMEM;
+
+	nl80211_cmd(drv, msg, 0, NL80211_CMD_GET_REG);
+	return send_and_recv_msgs(drv, msg, get_country_handler, alpha2);
+}
+
+
 /**
  * wpa_driver_nl80211_set_country - ask nl80211 to set the regulatory domain
  * @priv: driver_nl80211 private data
@@ -2703,7 +2734,10 @@  static int wpa_driver_nl80211_set_country(void *priv, const char *alpha2_arg)
 	struct i802_bss *bss = priv;
 	struct wpa_driver_nl80211_data *drv = bss->drv;
 	char alpha2[3];
+	char alpha2_res[3];
 	struct nl_msg *msg;
+	int count;
+	int ret;
 
 	msg = nlmsg_alloc();
 	if (!msg)
@@ -2718,7 +2752,24 @@  static int wpa_driver_nl80211_set_country(void *priv, const char *alpha2_arg)
 	NLA_PUT_STRING(msg, NL80211_ATTR_REG_ALPHA2, alpha2);
 	if (send_and_recv_msgs(drv, msg, NULL, NULL))
 		return -EINVAL;
-	return 0;
+
+	/* Validate coutry setting and retry up to 3 times if not match */
+	for (count = 0; count < 3; count++) {
+		os_memset(alpha2_res, 0, sizeof(alpha2));
+		ret = nl80211_get_country(drv, alpha2_res);
+		if (ret)
+			return ret;
+		if (os_strncmp(alpha2, alpha2_res, 3)) {
+			wpa_printf(MSG_DEBUG, "wpa_driver_nl80211_set_country:"
+					"retry country set after delay");
+			os_sleep(1, 0);
+		} else {
+			/* contry set is completed */
+			return 0;
+		}
+	}
+	wpa_printf(MSG_ERROR, "wpa_driver_nl80211_set_country: failed");
+	return -EINVAL;
 nla_put_failure:
 	nlmsg_free(msg);
 	return -EINVAL;