Message ID | 1305057374-9875-1-git-send-email-greearb@candelatech.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
On 05/10/2011 12:56 PM, greearb@candelatech.com wrote: > From: Ben Greear<greearb@candelatech.com> > > Enable this by passing a -1 for a scan frequency. > > When enabled, the system will only scan the current active > channel if at least one VIF is actively using it. If no > VIFS are active or this flag is disabled, then default > behaviour is used. > > This helps when using multiple STA interfaces that otherwise might > constantly be trying to scan all channels. A more intrusive version of this patch was previously NAK'd, but since the scan and off-channel optimizations went in, I thought this might have a chance. I'll not send again if NAK'd this time. Thanks, Ben
See inline On Tue, May 10, 2011 at 12:56 PM, <greearb@candelatech.com> wrote: > > + > + /* > + printk("start_sw_scan, can_scan_one: %i n_channels: %i\n", > + local->scan_req->can_scan_one, local->scan_req->n_channels); > + WARN_ON(!local->scan_req->can_scan_one); > + */ Remove the debug print? > + > + /* If one sta is associated, we don't want another to start > + * scanning on all channels, as that will interfere with the > + * one already associated. > + */ Comment style is wrong. > > + /* Special hack: channel -1 means 'scan only active > + * channel if any VIFs on this device are associated > + * on the channel. > + */ Same. > > + /* If at least one VIF on this hardware is already associated, then > + * only scan on the active channel. > + */ Same. Dan -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/10/2011 12:59 PM, Daniel Halperin wrote: > See inline > > On Tue, May 10, 2011 at 12:56 PM,<greearb@candelatech.com> wrote: >> >> + >> + /* >> + printk("start_sw_scan, can_scan_one: %i n_channels: %i\n", >> + local->scan_req->can_scan_one, local->scan_req->n_channels); >> + WARN_ON(!local->scan_req->can_scan_one); >> + */ > > Remove the debug print? I'll clean all of the comments if that is the only complaint. Johannes didn't like it for more substantial reasons last time... Thanks, Ben
On Tue, 2011-05-10 at 12:56 -0700, greearb@candelatech.com wrote: > From: Ben Greear <greearb@candelatech.com> > > Enable this by passing a -1 for a scan frequency. > > When enabled, the system will only scan the current active > channel if at least one VIF is actively using it. If no > VIFS are active or this flag is disabled, then default > behaviour is used. > > This helps when using multiple STA interfaces that otherwise might > constantly be trying to scan all channels. I still don't see what stops you from doing this in userspace, I don't remember a compelling reason for implementing this in the kernel. Clearly, you need to modify userspace anyhow. Also, what if, in the future, we have multiplel STA interfaces on different channels? Which one(s) should it pick then? The -1 trick will also break all non-mac80211 drivers, or maybe simply not work? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/12/2011 01:26 AM, Johannes Berg wrote: > On Tue, 2011-05-10 at 12:56 -0700, greearb@candelatech.com wrote: >> From: Ben Greear<greearb@candelatech.com> >> >> Enable this by passing a -1 for a scan frequency. >> >> When enabled, the system will only scan the current active >> channel if at least one VIF is actively using it. If no >> VIFS are active or this flag is disabled, then default >> behaviour is used. >> >> This helps when using multiple STA interfaces that otherwise might >> constantly be trying to scan all channels. > > I still don't see what stops you from doing this in userspace, I don't > remember a compelling reason for implementing this in the kernel. The reasons are relatively small, but enough to make the patch useful to me. First, it works with non-userspace scan logic (sme). Second, if user is running more than one supplicant, it might be difficult to know how many interfaces are associated. Or, if a single supplicant process isn't managing all wireless interfaces (maybe a few VAPs plus some STA interfaces using in-kernel scanning (sme))? > Clearly, you need to modify userspace anyhow. Also, what if, in the > future, we have multiplel STA interfaces on different channels? Which I'm not sure that having STA on different channels would ever actually work. The attempt to support that kind of thing in ath9k certainly didn't work too well. If it ever was implemented, then the -1 could mean choose all channels upon which something is associated. I do have a patch to wpa_supplicant that lets the user enable this feature if they want. I currently probe the support for this feature by attempting a scan on channel -1 and detecting failure. This way, I can have my code work on un-patched kernels as well as kernels with this patch installed. > one(s) should it pick then? The -1 trick will also break all > non-mac80211 drivers, or maybe simply not work? If passing a bogus value from user-space breaks the driver, then the driver is busted. I would assume it would simply not work. My patch to wpa_supplicant disables the feature by default, so someone has to actively turn it on to pass in -1. Thanks, Ben > > johannes
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index c32e683..c0be322 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -780,6 +780,8 @@ struct cfg80211_ssid { * @wiphy: the wiphy this was for * @dev: the interface * @aborted: (internal) scan request was notified as aborted + * @can_scan_one: If true, only scan active channel if at least one + * vif is already associated. */ struct cfg80211_scan_request { struct cfg80211_ssid *ssids; @@ -792,6 +794,7 @@ struct cfg80211_scan_request { struct wiphy *wiphy; struct net_device *dev; bool aborted; + bool can_scan_one; /* keep last */ struct ieee80211_channel *channels[0]; diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c index 489b6ad..dec0a12 100644 --- a/net/mac80211/scan.c +++ b/net/mac80211/scan.c @@ -353,6 +353,63 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local) * nullfunc frames and probe requests will be dropped in * ieee80211_tx_h_check_assoc(). */ + int associated_station_vifs = 0; + int running_station_vifs = 0; /* not necessarily associated */ + int running_other_vifs = 0; /* AP, etc */ + struct ieee80211_sub_if_data *sdata; + + /* + printk("start_sw_scan, can_scan_one: %i n_channels: %i\n", + local->scan_req->can_scan_one, local->scan_req->n_channels); + WARN_ON(!local->scan_req->can_scan_one); + */ + if (local->scan_req->can_scan_one && local->scan_req->n_channels >= 1) { + struct sta_info *sta; + mutex_lock(&local->iflist_mtx); + list_for_each_entry(sdata, &local->interfaces, list) { + if (!ieee80211_sdata_running(sdata)) + continue; + + if (sdata->vif.type == NL80211_IFTYPE_STATION) + running_station_vifs++; + else + running_other_vifs++; + } + mutex_unlock(&local->iflist_mtx); + + rcu_read_lock(); + list_for_each_entry_rcu(sta, &local->sta_list, list) { + if (!ieee80211_sdata_running(sta->sdata)) + continue; + if (sta->sdata->vif.type != NL80211_IFTYPE_STATION) + continue; + if (test_sta_flags(sta, WLAN_STA_ASSOC)) + associated_station_vifs++; + } + rcu_read_unlock(); + + /* If one sta is associated, we don't want another to start + * scanning on all channels, as that will interfere with the + * one already associated. + */ + if ((running_other_vifs > 0) || + (associated_station_vifs > 1)) { + local->scan_req->channels[0] = local->hw.conf.channel; + local->scan_req->n_channels = 1; + printk(KERN_ERR "start_sw_scan: running-other-vifs: %i " + "running-station-vifs: %i, associated-stations: %i" + " scanning current channel: %u MHz\n", + running_other_vifs, running_station_vifs, + associated_station_vifs, + local->hw.conf.channel->center_freq); + } else + printk(KERN_ERR "start_sw_scan: running-other-vifs: %i " + "running-station-vifs: %i, associated-stations: %i" + " scanning all channels.\n", + running_other_vifs, running_station_vifs, + associated_station_vifs); + } + drv_sw_scan_start(local); local->leave_oper_channel_time = 0; diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 0a199a1..4e854b7 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -3176,6 +3176,9 @@ static int validate_scan_freqs(struct nlattr *freqs) int n_channels = 0, tmp1, tmp2; nla_for_each_nested(attr1, freqs, tmp1) { + if (nla_get_u32(attr1) == 0xFFFFFFFF) + continue; /* skip can-scan-one flag */ + n_channels++; /* * Some hardware has a limited channel list for @@ -3207,6 +3210,7 @@ static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info) int err, tmp, n_ssids = 0, n_channels, i; enum ieee80211_band band; size_t ie_len; + bool do_all_chan = true; if (!is_valid_ie_attr(info->attrs[NL80211_ATTR_IE])) return -EINVAL; @@ -3223,8 +3227,9 @@ static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info) n_channels = validate_scan_freqs( info->attrs[NL80211_ATTR_SCAN_FREQUENCIES]); if (!n_channels) - return -EINVAL; + goto auto_channels; } else { +auto_channels: n_channels = 0; for (band = 0; band < IEEE80211_NUM_BANDS; band++) @@ -3270,6 +3275,16 @@ static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info) nla_for_each_nested(attr, info->attrs[NL80211_ATTR_SCAN_FREQUENCIES], tmp) { struct ieee80211_channel *chan; + /* Special hack: channel -1 means 'scan only active + * channel if any VIFs on this device are associated + * on the channel. + */ + if (nla_get_u32(attr) == 0xFFFFFFFF) { + request->can_scan_one = true; + continue; + } + + do_all_chan = false; chan = ieee80211_get_channel(wiphy, nla_get_u32(attr)); if (!chan) { @@ -3284,7 +3299,9 @@ static int nl80211_trigger_scan(struct sk_buff *skb, struct genl_info *info) request->channels[i] = chan; i++; } - } else { + } + + if (do_all_chan) { /* all channels */ for (band = 0; band < IEEE80211_NUM_BANDS; band++) { int j; diff --git a/net/wireless/sme.c b/net/wireless/sme.c index e17b0be..3e0e638 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c @@ -105,6 +105,11 @@ static int cfg80211_conn_scan(struct wireless_dev *wdev) if (!request) return -ENOMEM; + /* If at least one VIF on this hardware is already associated, then + * only scan on the active channel. + */ + request->can_scan_one = true; + if (wdev->conn->params.channel) request->channels[0] = wdev->conn->params.channel; else {