| Message ID | 1413428706-5792-1-git-send-email-sujith@msujith.org |
|---|---|
| State | Deferred |
| Headers | show |
On Thu, Oct 16, 2014 at 08:35:06AM +0530, Sujith Manoharan wrote: > The max remain-on-channel for cfg80211/mac80211 drivers is > obtained using the NL80211_ATTR_MAX_REMAIN_ON_CHANNEL_DURATION > attribute. This patch adds a config parameter to override > the driver-provided duration, which will help certain > use-cases. Wouldn't such use cases benefit from being able to set this dynamically at runtime? The design used here seems to limit this to a one-time configuration option when starting the interface. In addition, I'd assume it would be useful to do such limitation on off-channel operations mainly when there is concurrent operations in progress (one or more wpa_s instances connected as a station or operating as an AP). Or is the goal really to limit the operation duration even if there is no such concurrent tasks on the radio? > diff --git a/wpa_supplicant/config.c b/wpa_supplicant/config.c > @@ -3919,6 +3919,7 @@ static const struct global_parse_data global_fields[] = { > + { INT(max_remain_on_chan), 0 }, This is the parser for the new config variable, but there should be a matching change in config_file.c to write the config variable (if non-zero, i.e., not the default value). > diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c > @@ -3797,6 +3797,10 @@ static int wpa_supplicant_init_iface(struct wpa_supplicant *wpa_s, > + if (wpa_s->conf->max_remain_on_chan) > + wpa_s->max_remain_on_chan = wpa_s->conf->max_remain_on_chan; This is the only place where wpa_s->max_remain_on_chan gets updated. First of all, this must not allow the driver-advertised value to be increased (that would result in driver commands getting rejected). Secondly, this would be nice to be able to update at runtime based on current use case. In other words, it may be useful to keep wpa_s->max_remain_on_chan as-is and use wpa_s->conf->max_remain_on_chan to reduce that limit in the concurrent cases so that changes to wpa_s->conf->max_remain_on_chan will be taken into use on the next attempt. min() of those two (if wpa_s->conf->max_remain_on_chan != 0) would likely be the best approach for this.
Jouni Malinen wrote: > Wouldn't such use cases benefit from being able to set this dynamically > at runtime? The design used here seems to limit this to a one-time > configuration option when starting the interface. > > In addition, I'd assume it would be useful to do such limitation on > off-channel operations mainly when there is concurrent operations in > progress (one or more wpa_s instances connected as a station or > operating as an AP). Or is the goal really to limit the operation > duration even if there is no such concurrent tasks on the radio? Yes, even if there is only a single GO interface, for example, offchannel duration needs to be limited. > This is the parser for the new config variable, but there should be a > matching change in config_file.c to write the config variable (if > non-zero, i.e., not the default value). I'll fix this. > This is the only place where wpa_s->max_remain_on_chan gets updated. > First of all, this must not allow the driver-advertised value to be > increased (that would result in driver commands getting rejected). > Secondly, this would be nice to be able to update at runtime based on > current use case. In other words, it may be useful to keep > wpa_s->max_remain_on_chan as-is and use wpa_s->conf->max_remain_on_chan > to reduce that limit in the concurrent cases so that changes to > wpa_s->conf->max_remain_on_chan will be taken into use on the next > attempt. min() of those two (if wpa_s->conf->max_remain_on_chan != 0) > would likely be the best approach for this. I'll make these changes and send a v2. Sujith
On Sun, Oct 19, 2014 at 04:58:12PM +0530, Sujith Manoharan wrote: > Jouni Malinen wrote: > > In addition, I'd assume it would be useful to do such limitation on > > off-channel operations mainly when there is concurrent operations in > > progress (one or more wpa_s instances connected as a station or > > operating as an AP). Or is the goal really to limit the operation > > duration even if there is no such concurrent tasks on the radio? > > Yes, even if there is only a single GO interface, for example, offchannel > duration needs to be limited. I included that as a concurrent operation.. In other words, I'm comparing a case where no interface is operating as a station, AP, GO, etc. to a case where one or more does. I don't see why a smaller limit would be needed when there is no other operation in progress. Taken into account that the shorter limit can cause harm (e.g., P2P interop issues if you go below 500 ms), I'd take it into use only if there is real identified need for it (e.g., that single GO operating).
diff --git a/wpa_supplicant/config.c b/wpa_supplicant/config.c index d56e203..0dc917b 100644 --- a/wpa_supplicant/config.c +++ b/wpa_supplicant/config.c @@ -3919,6 +3919,7 @@ static const struct global_parse_data global_fields[] = { { INT(mac_addr), 0 }, { INT(rand_addr_lifetime), 0 }, { INT(preassoc_mac_addr), 0 }, + { INT(max_remain_on_chan), 0 }, }; #undef FUNC diff --git a/wpa_supplicant/config.h b/wpa_supplicant/config.h index f4c2f88..8285077 100644 --- a/wpa_supplicant/config.h +++ b/wpa_supplicant/config.h @@ -1088,6 +1088,11 @@ struct wpa_config { * 2 = like 1, but maintain OUI (with local admin bit set) */ int preassoc_mac_addr; + + /** + * max_remain_on_chan - Maximum remain-on-channel duration, in msecs. + */ + unsigned int max_remain_on_chan; }; diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c index e7aeeac..6e4190b 100644 --- a/wpa_supplicant/wpa_supplicant.c +++ b/wpa_supplicant/wpa_supplicant.c @@ -3797,6 +3797,10 @@ static int wpa_supplicant_init_iface(struct wpa_supplicant *wpa_s, wpa_s->num_multichan_concurrent = capa.num_multichan_concurrent; } + + if (wpa_s->conf->max_remain_on_chan) + wpa_s->max_remain_on_chan = wpa_s->conf->max_remain_on_chan; + if (wpa_s->max_remain_on_chan == 0) wpa_s->max_remain_on_chan = 1000;