The patch basically enhances the scanning state machine by two further
states (SCAN_SET_OPER_CHANNEL & SCAN_OPERATION). In state SCAN_SET_OPER_CHANNEL
the driver is advised to switch back to the operating channel while
SCAN_OPERATION tells the access point about being back from power saving and
restarts the tx queue. Just before SCAN_SET_CHANNEL sets the next channel to
scan it notifies the access point about going to power save state and stops
the tx queue.
It does not add any periodic scan behavior, it only reduces the impact of a
scan onto the traffic (multiple short interruptions instead of one long)
which results in a smoother user experience.
Signed-off-by: Helmut Schaa <[email protected]>
---
Could somebody please have a look at the TODO comments (I have no idea how
to wait until all null-func frames are ACKed)? Thanks.
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 3912fba..21c623d 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -646,11 +647,11 @@ struct ieee80211_local {
/* Scanning and BSS list */
- bool sw_scanning, hw_scanning;
+ bool sw_scanning, hw_scanning, bg_scanning;
int scan_channel_idx;
enum ieee80211_band scan_band;
- enum { SCAN_SET_CHANNEL, SCAN_SEND_PROBE } scan_state;
+ enum { SCAN_SET_CHANNEL, SCAN_SEND_PROBE, SCAN_SET_OPER_CHANNEL, SCAN_OPERATION } scan_state;
unsigned long last_scan_completed;
struct delayed_work scan_work;
struct ieee80211_sub_if_data *scan_sdata;
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 92d898b..49b5c29 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -414,7 +414,7 @@ ieee80211_rx_h_passive_scan(struct ieee80211_rx_data *rx)
return RX_QUEUED;
}
- if (unlikely(rx->flags & IEEE80211_RX_IN_SCAN)) {
+ if (unlikely((rx->flags & IEEE80211_RX_IN_SCAN) && !local->bg_scanning)) {
/* scanning finished during invoking of handlers */
I802_DEBUG_INC(local->rx_handlers_drop_passive_scan);
return RX_DROP_UNUSABLE;
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 8e6685e..9120e6b 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -29,6 +29,7 @@
#define IEEE80211_PROBE_DELAY (HZ / 33)
#define IEEE80211_CHANNEL_TIME (HZ / 33)
#define IEEE80211_PASSIVE_CHANNEL_TIME (HZ / 5)
+#define IEEE80211_BG_SCAN_INTERRUPT (HZ / 4)
void ieee80211_rx_bss_list_init(struct ieee80211_local *local)
{
@@ -455,6 +456,7 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
}
local->sw_scanning = false;
+ local->bg_scanning = false;
if (ieee80211_hw_config(local))
printk(KERN_DEBUG "%s: failed to restore operational "
"channel after scan\n", wiphy_name(local->hw.wiphy));
@@ -510,6 +512,36 @@ void ieee80211_scan_work(struct work_struct *work)
switch (local->scan_state) {
case SCAN_SET_CHANNEL:
+ if (local->bg_scanning) {
+ /*
+ * background scan is in progress, notify all associated
+ * access points about us leaving the channel and
+ * update the filter flags
+ */
+ local->sw_scanning = 1;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+ if (sdata->vif.type == NL80211_IFTYPE_STATION &&
+ (sdata->u.sta.flags & IEEE80211_STA_ASSOCIATED)) {
+ ieee80211_send_nullfunc(local, sdata, 1);
+ /* TODO: wait until all nullfunc frames are ACKed */
+ msleep(1);
+ netif_tx_stop_all_queues(sdata->dev);
+ }
+ }
+ rcu_read_unlock();
+
+ netif_tx_lock_bh(local->mdev);
+ local->filter_flags |= FIF_BCN_PRBRESP_PROMISC;
+ local->ops->configure_filter(local_to_hw(local),
+ FIF_BCN_PRBRESP_PROMISC,
+ &local->filter_flags,
+ local->mdev->mc_count,
+ local->mdev->mc_list);
+ netif_tx_unlock_bh(local->mdev);
+ }
+
/*
* Get current scan band. scan_band may be IEEE80211_NUM_BANDS
* after we successfully scanned the last channel of the last
@@ -574,7 +606,10 @@ void ieee80211_scan_work(struct work_struct *work)
break;
case SCAN_SEND_PROBE:
next_delay = IEEE80211_PASSIVE_CHANNEL_TIME;
- local->scan_state = SCAN_SET_CHANNEL;
+ if (!local->bg_scanning)
+ local->scan_state = SCAN_SET_CHANNEL;
+ else
+ local->scan_state = SCAN_SET_OPER_CHANNEL;
if (local->scan_channel->flags & IEEE80211_CHAN_PASSIVE_SCAN)
break;
@@ -582,6 +617,49 @@ void ieee80211_scan_work(struct work_struct *work)
local->scan_ssid_len);
next_delay = IEEE80211_CHANNEL_TIME;
break;
+ case SCAN_SET_OPER_CHANNEL:
+ local->scan_state = SCAN_OPERATION;
+ /* interrupt the current scan */
+ local->sw_scanning = 0;
+
+ /* switch back to the operating channel */
+ if (ieee80211_hw_config(local))
+ printk(KERN_DEBUG "%s: failed to restore operational "
+ "channel after scan\n", sdata->dev->name);
+
+ /* reconfigure filter flags*/
+ netif_tx_lock_bh(local->mdev);
+ local->filter_flags &= ~FIF_BCN_PRBRESP_PROMISC;
+ local->ops->configure_filter(local_to_hw(local),
+ FIF_BCN_PRBRESP_PROMISC,
+ &local->filter_flags,
+ local->mdev->mc_count,
+ local->mdev->mc_list);
+
+ netif_tx_unlock_bh(local->mdev);
+
+ /* wait for the channel switch */
+ next_delay = usecs_to_jiffies(local->hw.channel_change_time);
+ break;
+
+ case SCAN_OPERATION:
+ rcu_read_lock();
+ list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+ /* Tell AP we're back */
+ if (sdata->vif.type == NL80211_IFTYPE_STATION &&
+ sdata->u.sta.flags & IEEE80211_STA_ASSOCIATED) {
+ ieee80211_send_nullfunc(local, sdata, 0);
+ /* TODO: wait until all nullfunc frames are ACKed */
+ msleep(1);
+ netif_tx_wake_all_queues(sdata->dev);
+ }
+ }
+ rcu_read_unlock();
+
+ next_delay = IEEE80211_BG_SCAN_INTERRUPT;
+ local->scan_state = SCAN_SET_CHANNEL;
+
+ break;
}
queue_delayed_work(local->hw.workqueue, &local->scan_work,
@@ -615,7 +693,7 @@ int ieee80211_start_scan(struct ieee80211_sub_if_data *scan_sdata,
* ResultCode: SUCCESS, INVALID_PARAMETERS
*/
- if (local->sw_scanning || local->hw_scanning) {
+ if (local->sw_scanning || local->hw_scanning || local->bg_scanning) {
if (local->scan_sdata == scan_sdata)
return 0;
return -EBUSY;
@@ -636,18 +714,28 @@ int ieee80211_start_scan(struct ieee80211_sub_if_data *scan_sdata,
local->sw_scanning = true;
+ /*
+ * if at least one station interface is associated start a background scan
+ * instead of a common software scan
+ */
rcu_read_lock();
list_for_each_entry_rcu(sdata, &local->interfaces, list) {
if (sdata->vif.type == NL80211_IFTYPE_STATION) {
if (sdata->u.sta.flags & IEEE80211_STA_ASSOCIATED) {
- netif_tx_stop_all_queues(sdata->dev);
- ieee80211_send_nullfunc(local, sdata, 1);
+ /*
+ * no need to stop station interaces here, that will be done in
+ * the scan handler
+ */
+ local->bg_scanning = true;
}
} else
netif_tx_stop_all_queues(sdata->dev);
}
rcu_read_unlock();
+ if (!local->bg_scanning)
+ local->sw_scanning = true;
+
if (ssid) {
local->scan_ssid_len = ssid_len;
memcpy(local->scan_ssid, ssid, ssid_len);
@@ -658,14 +746,16 @@ int ieee80211_start_scan(struct ieee80211_sub_if_data *scan_sdata,
local->scan_band = IEEE80211_BAND_2GHZ;
local->scan_sdata = scan_sdata;
- netif_addr_lock_bh(local->mdev);
- local->filter_flags |= FIF_BCN_PRBRESP_PROMISC;
- local->ops->configure_filter(local_to_hw(local),
- FIF_BCN_PRBRESP_PROMISC,
- &local->filter_flags,
- local->mdev->mc_count,
- local->mdev->mc_list);
- netif_addr_unlock_bh(local->mdev);
+ if (!local->bg_scanning) {
+ netif_addr_lock_bh(local->mdev);
+ local->filter_flags |= FIF_BCN_PRBRESP_PROMISC;
+ local->ops->configure_filter(local_to_hw(local),
+ FIF_BCN_PRBRESP_PROMISC,
+ &local->filter_flags,
+ local->mdev->mc_count,
+ local->mdev->mc_list);
+ netif_addr_unlock_bh(local->mdev);
+ }
/* TODO: start scan as soon as all nullfunc frames are ACKed */
queue_delayed_work(local->hw.workqueue, &local->scan_work,
diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
index 7e0d53a..fd7783a 100644
--- a/net/mac80211/wext.c
+++ b/net/mac80211/wext.c
@@ -566,7 +566,7 @@ static int ieee80211_ioctl_giwscan(struct net_device *dev,
sdata = IEEE80211_DEV_TO_SUB_IF(dev);
- if (local->sw_scanning || local->hw_scanning)
+ if (local->sw_scanning || local->hw_scanning || local->bg_scanning)
return -EAGAIN;
res = ieee80211_scan_results(local, info, extra, data->length);
On Wed, 2008-09-24 at 17:55 +0200, Helmut Schaa wrote:
> > What I meant to say is that it'll give problems with drivers that don't
> > do status reporting properly, and what are you going to do when one of
> > them fails anyway? retry it? how long? assume the connection was lost if
> > it isn't acked? I see little point in it to start with.
>
> The main reason why I'd like to know when the frame was acked is that it might
> happen (and it did happen in my tests already) that the frame notifying the
> AP about entering power save state wasn't send before switching to another
> channel. Hence the AP won't buffer any frames for us.
We should make these frames able to "skip the queue" so to speak, that
would be smarter either way.
> > > > > + netif_tx_wake_all_queues(sdata->dev);
> > > >
> > > > This is worsening a problem we already have -- you can enable queues
> > > > that the driver asked to be disabled. Until we fix that, I don't think
> > > > we should tempt our luck even more.
> > >
> > > I see! That's really problematic.
> > > Do you have already an idea on how to fix it?
> >
> > Not really; introduce bits somewhere to keep track of who wants to
> > enable/disable queues I guess.
>
> A first trivial solution would be to just store which queues are active
> when the scan is started and restarting only these queues after the scan
> completed.
Actually, well, you have to deal with drivers like adm8211 that
stop/start the queues for each packet...
johannes
Am Mittwoch, 24. September 2008 16:46:18 schrieb Johannes Berg:
> On Wed, 2008-09-24 at 16:36 +0200, Helmut Schaa wrote:
> > Could somebody please have a look at the TODO comments (I have no idea
> > how to wait until all null-func frames are ACKed)? Thanks.
>
> It's not really possible.
:(
> > + if (local->bg_scanning) {
> > + /*
> > + * background scan is in progress, notify all associated
> > + * access points about us leaving the channel and
> > + * update the filter flags
> > + */
> > + local->sw_scanning = 1;
>
> use true/false please
Sure, my fault.
> > + netif_tx_wake_all_queues(sdata->dev);
>
> This is worsening a problem we already have -- you can enable queues
> that the driver asked to be disabled. Until we fix that, I don't think
> we should tempt our luck even more.
I see! That's really problematic.
Do you have already an idea on how to fix it?
> > - if (local->sw_scanning || local->hw_scanning)
> > + if (local->sw_scanning || local->hw_scanning || local->bg_scanning)
>
> I don't really like that. I think these three bools should become an
> enum now.
Agreed.
> And why is sw_scanning false if bg_scanning is true anyway?
During a background scan the sw_scanning flag is set when a scan phase is
active and unset when an operation phase is active. Therefore I did not need
to adapt all checks for sw_scanning.
Thanks for your comments,
Helmut
Am Mittwoch, 24. September 2008 17:33:41 schrieb Johannes Berg:
> On Wed, 2008-09-24 at 17:19 +0200, Helmut Schaa wrote:
> > Am Mittwoch, 24. September 2008 16:46:18 schrieb Johannes Berg:
> > > On Wed, 2008-09-24 at 16:36 +0200, Helmut Schaa wrote:
> > > > Could somebody please have a look at the TODO comments (I have no
> > > > idea how to wait until all null-func frames are ACKed)? Thanks.
> > >
> > > It's not really possible.
> > >
> > :(
>
> What I meant to say is that it'll give problems with drivers that don't
> do status reporting properly, and what are you going to do when one of
> them fails anyway? retry it? how long? assume the connection was lost if
> it isn't acked? I see little point in it to start with.
The main reason why I'd like to know when the frame was acked is that it might
happen (and it did happen in my tests already) that the frame notifying the
AP about entering power save state wasn't send before switching to another
channel. Hence the AP won't buffer any frames for us.
> > > > + netif_tx_wake_all_queues(sdata->dev);
> > >
> > > This is worsening a problem we already have -- you can enable queues
> > > that the driver asked to be disabled. Until we fix that, I don't think
> > > we should tempt our luck even more.
> >
> > I see! That's really problematic.
> > Do you have already an idea on how to fix it?
>
> Not really; introduce bits somewhere to keep track of who wants to
> enable/disable queues I guess.
A first trivial solution would be to just store which queues are active
when the scan is started and restarting only these queues after the scan
completed.
> > > And why is sw_scanning false if bg_scanning is true anyway?
> >
> > During a background scan the sw_scanning flag is set when a scan phase is
> > active and unset when an operation phase is active. Therefore I did not
> > need to adapt all checks for sw_scanning.
>
> Oh, hmm, ok, that might make the enum problematic. Or do something like
>
> SCAN_OFF,
> SCAN_BG,
> SCAN_SW,
> SCAN_HW
>
> then you can check for scan >= SCAN_SW
Maybe. Thinking about it.
Helmut
On Wed, 2008-09-24 at 17:19 +0200, Helmut Schaa wrote:
> Am Mittwoch, 24. September 2008 16:46:18 schrieb Johannes Berg:
> > On Wed, 2008-09-24 at 16:36 +0200, Helmut Schaa wrote:
> > > Could somebody please have a look at the TODO comments (I have no idea
> > > how to wait until all null-func frames are ACKed)? Thanks.
> >
> > It's not really possible.
>
> :(
What I meant to say is that it'll give problems with drivers that don't
do status reporting properly, and what are you going to do when one of
them fails anyway? retry it? how long? assume the connection was lost if
it isn't acked? I see little point in it to start with.
> > > + netif_tx_wake_all_queues(sdata->dev);
> >
> > This is worsening a problem we already have -- you can enable queues
> > that the driver asked to be disabled. Until we fix that, I don't think
> > we should tempt our luck even more.
>
> I see! That's really problematic.
> Do you have already an idea on how to fix it?
Not really; introduce bits somewhere to keep track of who wants to
enable/disable queues I guess.
> > And why is sw_scanning false if bg_scanning is true anyway?
>
> During a background scan the sw_scanning flag is set when a scan phase is
> active and unset when an operation phase is active. Therefore I did not need
> to adapt all checks for sw_scanning.
Oh, hmm, ok, that might make the enum problematic. Or do something like
SCAN_OFF,
SCAN_BG,
SCAN_SW,
SCAN_HW
then you can check for scan >= SCAN_SW
johannes
Am Mittwoch, 24. September 2008 17:59:01 schrieb Johannes Berg:
> On Wed, 2008-09-24 at 17:55 +0200, Helmut Schaa wrote:
>
> > > What I meant to say is that it'll give problems with drivers that don't
> > > do status reporting properly, and what are you going to do when one of
> > > them fails anyway? retry it? how long? assume the connection was lost if
> > > it isn't acked? I see little point in it to start with.
> >
> > The main reason why I'd like to know when the frame was acked is that it might
> > happen (and it did happen in my tests already) that the frame notifying the
> > AP about entering power save state wasn't send before switching to another
> > channel. Hence the AP won't buffer any frames for us.
>
> We should make these frames able to "skip the queue" so to speak, that
> would be smarter either way.
Agreed. That would at least enhance the probability that the frame is sent
out fast enough.
> > > > > > + netif_tx_wake_all_queues(sdata->dev);
> > > > >
> > > > > This is worsening a problem we already have -- you can enable queues
> > > > > that the driver asked to be disabled. Until we fix that, I don't think
> > > > > we should tempt our luck even more.
> > > >
> > > > I see! That's really problematic.
> > > > Do you have already an idea on how to fix it?
> > >
> > > Not really; introduce bits somewhere to keep track of who wants to
> > > enable/disable queues I guess.
> >
> > A first trivial solution would be to just store which queues are active
> > when the scan is started and restarting only these queues after the scan
> > completed.
>
> Actually, well, you have to deal with drivers like adm8211 that
> stop/start the queues for each packet...
Oops. I did not know about drivers behaving like that
=> have to find a better way to deal with starting/stopping queues.
Thanks,
Helmut
On Wed, 2008-09-24 at 16:36 +0200, Helmut Schaa wrote:
> Could somebody please have a look at the TODO comments (I have no idea how
> to wait until all null-func frames are ACKed)? Thanks.
It's not really possible.
> + if (local->bg_scanning) {
> + /*
> + * background scan is in progress, notify all associated
> + * access points about us leaving the channel and
> + * update the filter flags
> + */
> + local->sw_scanning = 1;
use true/false please
> + rcu_read_lock();
> + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> + if (sdata->vif.type == NL80211_IFTYPE_STATION &&
> + (sdata->u.sta.flags & IEEE80211_STA_ASSOCIATED)) {
> + ieee80211_send_nullfunc(local, sdata, 1);
> + /* TODO: wait until all nullfunc frames are ACKed */
> + msleep(1);
I don't think you can msleep here?
> + break;
> +
> + case SCAN_OPERATION:
> + rcu_read_lock();
> + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> + /* Tell AP we're back */
> + if (sdata->vif.type == NL80211_IFTYPE_STATION &&
> + sdata->u.sta.flags & IEEE80211_STA_ASSOCIATED) {
> + ieee80211_send_nullfunc(local, sdata, 0);
> + /* TODO: wait until all nullfunc frames are ACKed */
> + msleep(1);
> + netif_tx_wake_all_queues(sdata->dev);
This is worsening a problem we already have -- you can enable queues
that the driver asked to be disabled. Until we fix that, I don't think
we should tempt our luck even more.
> - if (local->sw_scanning || local->hw_scanning)
> + if (local->sw_scanning || local->hw_scanning || local->bg_scanning)
I don't really like that. I think these three bools should become an
enum now. And why is sw_scanning false if bg_scanning is true anyway?
johannes
>>
>> SCAN_OFF,
>> SCAN_BG,
>> SCAN_SW,
>> SCAN_HW
>>
>> then you can check for scan >= SCAN_SW
>
> Maybe. Thinking about it.
SCAN_BG is under soft scan so why SCAN_HW is in the same level?
Conceptually this is wrong
Tomas