| Message ID | 1371992866-18168-1-git-send-email-igalc@ti.com |
|---|---|
| State | Rejected |
| Headers | show |
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
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.
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).
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
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.
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;
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(-)