| Message ID | 1458688354-29863-1-git-send-email-greearb@candelatech.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 2016-03-22 at 19:12 -0400, greearb@candelatech.com wrote: > From: Ben Greear <greearb@candelatech.com> > > This gives configurable control over whether to consider > roaming based on scan results. I find this useful when > doing explicit roaming tests, where I do not want scan > requests to cause the roam automatically. Manually triggered ones, or *all* scan requests even background ones? Dan > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > wpa_supplicant/config.c | 6 ++++++ > wpa_supplicant/config.h | 9 +++++++++ > wpa_supplicant/events.c | 6 ++++++ > 3 files changed, 21 insertions(+) > > diff --git a/wpa_supplicant/config.c b/wpa_supplicant/config.c > index 83703a0..298d910 100644 > --- a/wpa_supplicant/config.c > +++ b/wpa_supplicant/config.c > @@ -3644,6 +3644,9 @@ struct wpa_config * > wpa_config_alloc_empty(const char *ctrl_interface, > if (driver_param) > config->driver_param = os_strdup(driver_param); > > +#ifndef CONFIG_NO_ROAMING > + config->disable_ess_roaming = DEFAULT_DISABLE_ESS_ROAMING; > +#endif > config->concurrent_assoc_ok = DEFAULT_CONCURRENT_ASSOC_OK; > config->accept_external_scan_results = > DEFAULT_ACCEPT_EXTERNAL_SCAN_RESULTS; > > @@ -4335,6 +4338,9 @@ static const struct global_parse_data > global_fields[] = { > { INT(sched_scan_interval), 0 }, > { INT(tdls_external_control), 0}, > { STR(osu_dir), 0 }, > +#ifndef CONFIG_NO_ROAMING > + { INT(disable_ess_roaming), 0 }, > +#endif > { INT(concurrent_assoc_ok), 0 }, > { INT(accept_external_scan_results), 0 }, > { STR(wowlan_triggers), 0 }, > diff --git a/wpa_supplicant/config.h b/wpa_supplicant/config.h > index 81ed410..b04f4a5 100644 > --- a/wpa_supplicant/config.h > +++ b/wpa_supplicant/config.h > @@ -33,6 +33,7 @@ > #define DEFAULT_BSS_EXPIRATION_SCAN_COUNT 2 > #define DEFAULT_MIN_SCAN_GAP 0 > #define DEFAULT_MAX_ASSOC_PER_SCAN 25 > +#define DEFAULT_DISABLE_ESS_ROAMING 0 > #define DEFAULT_CONCURRENT_ASSOC_OK 0 > #define DEFAULT_ACCEPT_EXTERNAL_SCAN_RESULTS 0 > #define DEFAULT_MAX_NUM_STA 128 > @@ -857,6 +858,14 @@ struct wpa_config { > */ > unsigned int changed_parameters; > > +#ifndef CONFIG_NO_ROAMING > + /* If CONFIG_NO_ROAMING is not enabled, then scan results > will > + * be used to automatically roam. Allow disabling this > automated > + * roaming without having to re-build the binary. > + */ > + int disable_ess_roaming; > +#endif > + > /* By default, scans are no longer shared once one of the > stations starts > * to associate. This makes bringing up lots of vifs take a > long time. > * This override below lets us propagate scans even if a > station is > diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c > index 1adf57d..2d2c29e 100644 > --- a/wpa_supplicant/events.c > +++ b/wpa_supplicant/events.c > @@ -1391,6 +1391,12 @@ static int wpa_supplicant_need_to_roam(struct > wpa_supplicant *wpa_s, > return 1; /* current BSS not seen in the last scan > */ > > #ifndef CONFIG_NO_ROAMING > + if (wpa_s->conf->disable_ess_roaming) { > + wpa_dbg(wpa_s, MSG_DEBUG, > + "NOT considering within-ESS reassociation, > disable_ess_roaming is true"); > + return 0; > + } > + > wpa_dbg(wpa_s, MSG_DEBUG, "Considering within-ESS > reassociation"); > wpa_dbg(wpa_s, MSG_DEBUG, "Current BSS: " MACSTR > " level=%d snr=%d est_throughput=%u",
On 03/23/2016 06:41 AM, Dan Williams wrote: > On Tue, 2016-03-22 at 19:12 -0400, greearb@candelatech.com wrote: >> From: Ben Greear <greearb@candelatech.com> >> >> This gives configurable control over whether to consider >> roaming based on scan results. I find this useful when >> doing explicit roaming tests, where I do not want scan >> requests to cause the roam automatically. > > Manually triggered ones, or *all* scan requests even background ones? I believe my patch would be for all scan results. In my particular case, I really care only about manual scans and external scans, so if that makes the patch more useful for others, I can see if there is a way to distinguish one scan from another. Thanks, Ben
On Wed, 2016-03-23 at 07:24 -0700, Ben Greear wrote: > > On 03/23/2016 06:41 AM, Dan Williams wrote: > > > > On Tue, 2016-03-22 at 19:12 -0400, greearb@candelatech.com wrote: > > > > > > From: Ben Greear <greearb@candelatech.com> > > > > > > This gives configurable control over whether to consider > > > roaming based on scan results. I find this useful when > > > doing explicit roaming tests, where I do not want scan > > > requests to cause the roam automatically. > > Manually triggered ones, or *all* scan requests even background > > ones? > I believe my patch would be for all scan results. In my particular > case, I really care only about manual scans and external scans, > so if that makes the patch more useful for others, I can see if there > is > a way to distinguish one scan from another. Because there's already the flag to prevent roam on manual (eg, control interface initiated) scan requests, I was asking to clarify if you explicitly wanted to allow it to be disabled for all scans. The only downside I see here is that with this patch/option, the supplicant will never roam, and there's no official command to request roaming either (except the testing one, which is IIRC only supposed to be used for testing...). Dan
On 03/23/2016 08:18 AM, Dan Williams wrote: > On Wed, 2016-03-23 at 07:24 -0700, Ben Greear wrote: >> >> On 03/23/2016 06:41 AM, Dan Williams wrote: >>> >>> On Tue, 2016-03-22 at 19:12 -0400, greearb@candelatech.com wrote: >>>> >>>> From: Ben Greear <greearb@candelatech.com> >>>> >>>> This gives configurable control over whether to consider >>>> roaming based on scan results. I find this useful when >>>> doing explicit roaming tests, where I do not want scan >>>> requests to cause the roam automatically. >>> Manually triggered ones, or *all* scan requests even background >>> ones? >> I believe my patch would be for all scan results. In my particular >> case, I really care only about manual scans and external scans, >> so if that makes the patch more useful for others, I can see if there >> is >> a way to distinguish one scan from another. > > Because there's already the flag to prevent roam on manual (eg, control > interface initiated) scan requests, I was asking to clarify if you > explicitly wanted to allow it to be disabled for all scans. The only > downside I see here is that with this patch/option, the supplicant will > never roam, and there's no official command to request roaming either > (except the testing one, which is IIRC only supposed to be used for > testing...). Well, I am doing testing. But, what flag are you talking about? I can check the code to see if it will meet my needs. Either way, it may be useful (if rarely used) to be able to completely disable roaming with a config change instead of having to re-compile. The scan that was causing the roam for me was from "wpa_cli -i foo scan" command. Thanks, Ben
On Wed, 2016-03-23 at 08:24 -0700, Ben Greear wrote: > On 03/23/2016 08:18 AM, Dan Williams wrote: > > > > On Wed, 2016-03-23 at 07:24 -0700, Ben Greear wrote: > > > > > > > > > On 03/23/2016 06:41 AM, Dan Williams wrote: > > > > > > > > > > > > On Tue, 2016-03-22 at 19:12 -0400, greearb@candelatech.com > > > > wrote: > > > > > > > > > > > > > > > From: Ben Greear <greearb@candelatech.com> > > > > > > > > > > This gives configurable control over whether to consider > > > > > roaming based on scan results. I find this useful when > > > > > doing explicit roaming tests, where I do not want scan > > > > > requests to cause the roam automatically. > > > > Manually triggered ones, or *all* scan requests even background > > > > ones? > > > I believe my patch would be for all scan results. In my > > > particular > > > case, I really care only about manual scans and external scans, > > > so if that makes the patch more useful for others, I can see if > > > there > > > is > > > a way to distinguish one scan from another. > > Because there's already the flag to prevent roam on manual (eg, > > control > > interface initiated) scan requests, I was asking to clarify if you > > explicitly wanted to allow it to be disabled for all scans. The > > only > > downside I see here is that with this patch/option, the supplicant > > will > > never roam, and there's no official command to request roaming > > either > > (except the testing one, which is IIRC only supposed to be used for > > testing...). > Well, I am doing testing. > > But, what flag are you talking about? I can check the code to see if > it will meet my needs. > > Either way, it may be useful (if rarely used) to be able to > completely disable roaming > with a config change instead of having to re-compile. > > The scan that was causing the roam for me was from "wpa_cli -i foo > scan" command. "TYPE=ONLY" if you're using the socket control interface scan command, or the "AllowRoam" boolean in the D-Bus Scan method options dict. Dan
On Tue, Mar 22, 2016 at 07:12:34PM -0400, greearb@candelatech.com wrote: > This gives configurable control over whether to consider > roaming based on scan results. I find this useful when > doing explicit roaming tests, where I do not want scan > requests to cause the roam automatically. Please check if the "SCAN TYPE=ONLY" command that Dan mentions works for your use case. That's already used on Android to do exactly what you describe here as far as I can tell. If there is other use cases that needs this separate configuration parameter, I might accept it. That said, this would need to be rebased on top of the hostap.git master branch since almost none of this applies as-is. I would also drop the #ifndef CONFIG_NO_ROAMING/#endif lines to keep the source code cleaner. Saving couple of bytes in binary if CONFIG_NO_ROAMING is defined (which is something I would not recommend defining) is not sufficient justification for making the implementation more complex.
On 03/25/2016 03:03 AM, Jouni Malinen wrote: > On Tue, Mar 22, 2016 at 07:12:34PM -0400, greearb@candelatech.com wrote: >> This gives configurable control over whether to consider >> roaming based on scan results. I find this useful when >> doing explicit roaming tests, where I do not want scan >> requests to cause the roam automatically. > > Please check if the "SCAN TYPE=ONLY" command that Dan mentions works for > your use case. That's already used on Android to do exactly what you > describe here as far as I can tell. > > If there is other use cases that needs this separate configuration > parameter, I might accept it. That said, this would need to be rebased > on top of the hostap.git master branch since almost none of this applies > as-is. I would also drop the #ifndef CONFIG_NO_ROAMING/#endif lines to > keep the source code cleaner. Saving couple of bytes in binary if > CONFIG_NO_ROAMING is defined (which is something I would not recommend > defining) is not sufficient justification for making the implementation > more complex. First, I am also running a patch that allows supplicant to use external scan results (from iw, for instance), and I would want it to not roam when using those scan results as well (unless the station vdev is *configured* to allow roaming). Second, the reason I request a scan before roaming is that the bss table needs to be fresh before calling the roam CLI command. I tried reading the code, and from what I can tell, the TYPE=ONLY thing causes it to not update the bss entries? So, I think I will want this patch in my tree regardless. My test setup for this particular use-case has been reconfigured, so I do not have an easy way to test out the TYPE=ONLY at this time. If you would like the patch upstream, I'll be happy to rebase it on top of stock hostapd and remove the #ifdefs as you suggested. Thanks, Ben
On Fri, 2016-03-25 at 14:42 -0700, Ben Greear wrote: > On 03/25/2016 03:03 AM, Jouni Malinen wrote: > > > > On Tue, Mar 22, 2016 at 07:12:34PM -0400, greearb@candelatech.com > > wrote: > > > > > > This gives configurable control over whether to consider > > > roaming based on scan results. I find this useful when > > > doing explicit roaming tests, where I do not want scan > > > requests to cause the roam automatically. > > Please check if the "SCAN TYPE=ONLY" command that Dan mentions > > works for > > your use case. That's already used on Android to do exactly what > > you > > describe here as far as I can tell. > > > > If there is other use cases that needs this separate configuration > > parameter, I might accept it. That said, this would need to be > > rebased > > on top of the hostap.git master branch since almost none of this > > applies > > as-is. I would also drop the #ifndef CONFIG_NO_ROAMING/#endif lines > > to > > keep the source code cleaner. Saving couple of bytes in binary if > > CONFIG_NO_ROAMING is defined (which is something I would not > > recommend > > defining) is not sufficient justification for making the > > implementation > > more complex. > First, I am also running a patch that allows supplicant > to use external scan results (from iw, for instance), and I would > want > it to not roam when using those scan results as well (unless the > station vdev is *configured* to allow roaming). > > Second, the reason I request a scan before roaming is that the bss > table needs to be fresh before calling the roam CLI command. > > I tried reading the code, and from what I can tell, the TYPE=ONLY > thing causes it to not update the bss entries? When the scan is done the driver sends EVENT_SCAN_RESULTS which calls wpa_supplicant_event_scan_results() which calls wpa_supplicant_get_scan_results() which updates the BSS list. Only after that point does TYPE=ONLY have an effect, where it basically just cuts _wpa_supplicant_event_scan_results() short. Thus that function never gets to wpas_select_network_from_last_scan(). Dan > So, I think I will want this patch in my tree regardless. > > My test setup for this particular use-case has been reconfigured, > so I do not have an easy way to test out the TYPE=ONLY > at this time. > > If you would like the patch upstream, I'll be happy to rebase it > on top of stock hostapd and remove the #ifdefs as you suggested. > > Thanks, > Ben >
diff --git a/wpa_supplicant/config.c b/wpa_supplicant/config.c index 83703a0..298d910 100644 --- a/wpa_supplicant/config.c +++ b/wpa_supplicant/config.c @@ -3644,6 +3644,9 @@ struct wpa_config * wpa_config_alloc_empty(const char *ctrl_interface, if (driver_param) config->driver_param = os_strdup(driver_param); +#ifndef CONFIG_NO_ROAMING + config->disable_ess_roaming = DEFAULT_DISABLE_ESS_ROAMING; +#endif config->concurrent_assoc_ok = DEFAULT_CONCURRENT_ASSOC_OK; config->accept_external_scan_results = DEFAULT_ACCEPT_EXTERNAL_SCAN_RESULTS; @@ -4335,6 +4338,9 @@ static const struct global_parse_data global_fields[] = { { INT(sched_scan_interval), 0 }, { INT(tdls_external_control), 0}, { STR(osu_dir), 0 }, +#ifndef CONFIG_NO_ROAMING + { INT(disable_ess_roaming), 0 }, +#endif { INT(concurrent_assoc_ok), 0 }, { INT(accept_external_scan_results), 0 }, { STR(wowlan_triggers), 0 }, diff --git a/wpa_supplicant/config.h b/wpa_supplicant/config.h index 81ed410..b04f4a5 100644 --- a/wpa_supplicant/config.h +++ b/wpa_supplicant/config.h @@ -33,6 +33,7 @@ #define DEFAULT_BSS_EXPIRATION_SCAN_COUNT 2 #define DEFAULT_MIN_SCAN_GAP 0 #define DEFAULT_MAX_ASSOC_PER_SCAN 25 +#define DEFAULT_DISABLE_ESS_ROAMING 0 #define DEFAULT_CONCURRENT_ASSOC_OK 0 #define DEFAULT_ACCEPT_EXTERNAL_SCAN_RESULTS 0 #define DEFAULT_MAX_NUM_STA 128 @@ -857,6 +858,14 @@ struct wpa_config { */ unsigned int changed_parameters; +#ifndef CONFIG_NO_ROAMING + /* If CONFIG_NO_ROAMING is not enabled, then scan results will + * be used to automatically roam. Allow disabling this automated + * roaming without having to re-build the binary. + */ + int disable_ess_roaming; +#endif + /* By default, scans are no longer shared once one of the stations starts * to associate. This makes bringing up lots of vifs take a long time. * This override below lets us propagate scans even if a station is diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c index 1adf57d..2d2c29e 100644 --- a/wpa_supplicant/events.c +++ b/wpa_supplicant/events.c @@ -1391,6 +1391,12 @@ static int wpa_supplicant_need_to_roam(struct wpa_supplicant *wpa_s, return 1; /* current BSS not seen in the last scan */ #ifndef CONFIG_NO_ROAMING + if (wpa_s->conf->disable_ess_roaming) { + wpa_dbg(wpa_s, MSG_DEBUG, + "NOT considering within-ESS reassociation, disable_ess_roaming is true"); + return 0; + } + wpa_dbg(wpa_s, MSG_DEBUG, "Considering within-ESS reassociation"); wpa_dbg(wpa_s, MSG_DEBUG, "Current BSS: " MACSTR " level=%d snr=%d est_throughput=%u",