Here's my proposal how to implement beacon filtering in mac80211. The
basic idea is simple, the driver enables filtering whenever mac80211
enables power save and the driver calls ieee80211_beacon_loss()
whenever hardware informs about beacon loss.
I have tested this with stlc45xx and based on simple tests this seems
to work. For reference I earlier sent stlc45xx patch to this list.
v2:
o rebase to 2.6.29-rc7-wl
o rename ieee80211_rx_trigger() to ieee80211_sta_rx_notify()
o move check for sta mode in ieee80211_sta_rx_notify() to rx.c
o move unicast check from rx.c to ieee80211_sta_rx_notify()
o add comment for setting last_beacon in ieee80211_rx_mgmt_assoc_resp()
o rename IEEE80211_HW_BEACON_FILTERING to IEEE80211_HW_BEACON_FILTER
o send probe request from ieee80211_beacon_loss_work()
o API documentation
o enable beacon filtering only when power save is enabled
o disable power save (and hence beacon filtering) while scanning to make
it possible to have reliable scan results even when beacon filtering
is enabled
TODO:
o test with different hardware (without beacon filtering) to
avoid regressions
---
Kalle Valo (4):
mac80211: add beacon filtering support
mac80211: disable power save when scanning
mac80211: track beacons separately from the rx path activity
mac80211: decrease execution of the associated timer
include/net/mac80211.h | 30 +++++++++--
net/mac80211/ieee80211_i.h | 5 ++
net/mac80211/iface.c | 3 +
net/mac80211/mlme.c | 124 +++++++++++++++++++++++++++++++++-----------
net/mac80211/rx.c | 9 +++
net/mac80211/scan.c | 54 ++++++++++++++++++-
6 files changed, 185 insertions(+), 40 deletions(-)
Johannes Berg wrote:
> On Sat, 2009-03-14 at 19:14 +0200, Kalle Valo wrote:
>
>> +void ieee80211_sta_rx_notify(struct ieee80211_sub_if_data *sdata,
>> + struct ieee80211_hdr *hdr)
>> +{
>> + /* timer triggers only when there is no unicast traffic */
>> + if (!is_multicast_ether_addr(hdr->addr1))
>> + mod_timer(&sdata->u.mgd.timer,
>> + jiffies + IEEE80211_MONITORING_INTERVAL);
>> +}
>
> Do we really need the multicast check? The frame will be coming from the
> AP in managed mode in both cases, so why regard the data path as idle
> when we're receiving multicast traffic?
It seems that I had a good vacation because I can't remember anymore why
I added the check :D Too bad that even the comment I wrote was next to
useless.
I'll remove the multicast check because I don't see the point for it.
And if in case I finally recall the reason I'll definitely add a better
comment.
Thanks for all the review!
Kalle
Add IEEE80211_HW_BEACON_FILTERING flag so that driver inform that it supports
beacon filtering. Drivers need to call the new function
ieee80211_beacon_loss() to notify about beacon loss.
Signed-off-by: Kalle Valo <[email protected]>
---
include/net/mac80211.h | 18 ++++++++++++++++++
net/mac80211/ieee80211_i.h | 2 ++
net/mac80211/iface.c | 3 +++
net/mac80211/mlme.c | 39 ++++++++++++++++++++++++++++++++++++++-
4 files changed, 61 insertions(+), 1 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 9ef131d..1262b83 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -888,6 +888,13 @@ enum ieee80211_tkip_key_type {
*
* @IEEE80211_HW_MFP_CAPABLE:
* Hardware supports management frame protection (MFP, IEEE 802.11w).
+ *
+ * @IEEE80211_HW_BEACON_FILTER: Hardware can drop similar beacon frames to
+ * avoid waking up cpu. Enabling this flag disables the beacon check
+ * in mac80211 when in power save mode and the hardware must have an
+ * event to notify that beacons are lost. Use ieee80211_beacon_loss()
+ * to notify the event to the stack. The filtering must be enabled
+ * only when IEEE80211_CONF_PS is set.
*/
enum ieee80211_hw_flags {
IEEE80211_HW_RX_INCLUDES_FCS = 1<<1,
@@ -903,6 +910,7 @@ enum ieee80211_hw_flags {
IEEE80211_HW_PS_NULLFUNC_STACK = 1<<11,
IEEE80211_HW_SUPPORTS_DYNAMIC_PS = 1<<12,
IEEE80211_HW_MFP_CAPABLE = 1<<13,
+ IEEE80211_HW_BEACON_FILTER = 1<<14,
};
/**
@@ -1981,6 +1989,16 @@ void ieee80211_stop_tx_ba_cb_irqsafe(struct ieee80211_hw *hw, const u8 *ra,
struct ieee80211_sta *ieee80211_find_sta(struct ieee80211_hw *hw,
const u8 *addr);
+/**
+ * ieee80211_beacon_loss - inform that hardware does not receive beacons
+ *
+ * @hw: pointer as obtained from ieee80211_alloc_hw()
+ *
+ * When beacon filtering is enabled with IEEE80211_HW_BEACON_FILTERING and
+ * IEEE80211_CONF_PS is set, the driver needs to inform whenever the
+ * hardware is not receiving beacons with this function.
+ */
+void ieee80211_beacon_loss(struct ieee80211_hw *hw);
/* Rate control API */
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 5d3d0bf..488c112 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -272,6 +272,7 @@ struct ieee80211_if_managed {
struct timer_list chswitch_timer;
struct work_struct work;
struct work_struct chswitch_work;
+ struct work_struct beacon_loss_work;
u8 bssid[ETH_ALEN], prev_bssid[ETH_ALEN];
@@ -1083,6 +1084,7 @@ void ieee80211_send_nullfunc(struct ieee80211_local *local,
int powersave);
void ieee80211_sta_rx_notify(struct ieee80211_sub_if_data *sdata,
struct ieee80211_hdr *hdr);
+void ieee80211_beacon_loss_work(struct work_struct *work);
void ieee80211_wake_queues_by_reason(struct ieee80211_hw *hw,
enum queue_stop_reason reason);
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 2acc416..e0fa20f 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -470,6 +470,9 @@ static int ieee80211_stop(struct net_device *dev)
*/
cancel_work_sync(&sdata->u.mgd.work);
cancel_work_sync(&sdata->u.mgd.chswitch_work);
+
+ cancel_work_sync(&sdata->u.mgd.beacon_loss_work);
+
/*
* When we get here, the interface is marked down.
* Call synchronize_rcu() to wait for the RX path
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index c235d39..053d75b 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -920,6 +920,41 @@ void ieee80211_sta_rx_notify(struct ieee80211_sub_if_data *sdata,
jiffies + IEEE80211_MONITORING_INTERVAL);
}
+void ieee80211_beacon_loss_work(struct work_struct *work)
+{
+ struct ieee80211_sub_if_data *sdata =
+ container_of(work, struct ieee80211_sub_if_data,
+ u.mgd.beacon_loss_work);
+ struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
+
+ printk(KERN_DEBUG "%s: driver reports beacon loss from AP %pM "
+ "- sending probe request\n", sdata->dev->name,
+ sdata->u.mgd.bssid);
+
+ ifmgd->flags |= IEEE80211_STA_PROBEREQ_POLL;
+ ieee80211_send_probe_req(sdata, ifmgd->bssid, ifmgd->ssid,
+ ifmgd->ssid_len, NULL, 0);
+
+ mod_timer(&ifmgd->timer, jiffies + IEEE80211_MONITORING_INTERVAL);
+}
+
+void ieee80211_beacon_loss(struct ieee80211_hw *hw)
+{
+ struct ieee80211_local *local = hw_to_local(hw);
+ struct ieee80211_sub_if_data *sdata;
+
+ rcu_read_lock();
+ list_for_each_entry(sdata, &local->interfaces, list) {
+ if (sdata->vif.type != NL80211_IFTYPE_STATION)
+ continue;
+
+ queue_work(local->hw.workqueue,
+ &sdata->u.mgd.beacon_loss_work);
+ }
+ rcu_read_unlock();
+}
+EXPORT_SYMBOL(ieee80211_beacon_loss);
+
static void ieee80211_associated(struct ieee80211_sub_if_data *sdata)
{
struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
@@ -954,7 +989,8 @@ static void ieee80211_associated(struct ieee80211_sub_if_data *sdata)
goto unlock;
}
- if (time_after(jiffies,
+ if (!(local->hw.flags & IEEE80211_HW_BEACON_FILTER) &&
+ time_after(jiffies,
ifmgd->last_beacon + IEEE80211_MONITORING_INTERVAL)) {
printk(KERN_DEBUG "%s: beacon loss from AP %pM "
"- sending probe request\n",
@@ -1839,6 +1875,7 @@ void ieee80211_sta_setup_sdata(struct ieee80211_sub_if_data *sdata)
ifmgd = &sdata->u.mgd;
INIT_WORK(&ifmgd->work, ieee80211_sta_work);
INIT_WORK(&ifmgd->chswitch_work, ieee80211_chswitch_work);
+ INIT_WORK(&ifmgd->beacon_loss_work, ieee80211_beacon_loss_work);
setup_timer(&ifmgd->timer, ieee80211_sta_timer,
(unsigned long) sdata);
setup_timer(&ifmgd->chswitch_timer, ieee80211_chswitch_timer,
"Luis R. Rodriguez" <[email protected]> writes:
> On Sat, Mar 14, 2009 at 10:14 AM, Kalle Valo <[email protected]> wrote:
>> Separate beacon and rx path tracking in preparation for the beacon filtering
>> support. At the same time change ieee80211_associated() to look a bit simpler.
>>
>> Probe requests are now sent only after IEEE80211_PROBE_IDLE_TIME, which
>> is now set to 60 seconds.
>>
>> Signed-off-by: Kalle Valo <[email protected]>
>
> Can this be split into 3 separate patches, to make the one with the
> actual functional changes easier to review. One is the making of
> ieee80211_associated() look easier, the other the rename, and lastly
> the actual functional changes. When reviewing the commit log it would
> then be easier to see what was done.
Is it really worth the trouble? The patch is relatively small so
spotting the changes shouldn't be that difficult.
--
Kalle Valo
"Luis R. Rodriguez" <[email protected]> writes:
> Last we reviewed this it seemed we were set on only allowing this for
> hardware which supports beacon filtering with support for checksumming
> of the beacons. Reason checksumming is important is for considerations
> of DFS with the channel switch announcements, HT with with channel width
> changes. In fact I'm curious why checksumming would be required for 2.4 GHz
> single band devices. Kalle, do you happen to know?
In my opinion also 2.4 GHz band needs checksum support. At least ERP
protection changes come to my mind, but maybe there are also others.
> Considering that power saving features are important I think its
> worth to revisit this position in detail.
>
> An alternative to this above position is that if the devices do not
> support checksumming of the beacons to let the driver figure when
> this should be enabled dynamically. For example if devices do not
> support checksumming but are single 2.4 GHz band non-HT capable
> devices we should let drivers enable this feature.
>
> Other devices which do not support checksumming should only enable this only
> when associating to an AP on non-DFS channels and when not using HT.
I'm not fully convinced about this.
> In practice this would mean allowing both ath5k and ath9k to take advantage
> of this feature.
Do you mean that they have beacon filter support but do not do any
checksumming in hardware?
Other option is that the hardware which does not support checksumming
would just pass beacons periodically to the stack, for example every 5
seconds. Even though beacon filter is enabled in mac80211 it does not
prevent hardware from sending beacons. This is not the most optimal
solution, but there's not much choices if the hardware doesn't support
checksumming.
> Technically we could move the conditional logic check when this
> should be enabled to mac80211 as well. When we reviewed this it was
> seen as unnecessary complexity, I actually don't see it as too
> complex and think the the advantage is worth to consider.
>
> Thoughts?
I think this is future stuff, I would like to get the basic (and the
simplest) implementation to the tree first and then we can start
improving and extending it.
--
Kalle Valo
On Sat, 2009-03-14 at 19:14 +0200, Kalle Valo wrote:
> +/**
> + * ieee80211_beacon_loss - inform that hardware does not receive beacons
> + *
> + * @hw: pointer as obtained from ieee80211_alloc_hw()
> + *
> + * When beacon filtering is enabled with IEEE80211_HW_BEACON_FILTERING and
> + * IEEE80211_CONF_PS is set, the driver needs to inform whenever the
> + * hardware is not receiving beacons with this function.
> + */
> +void ieee80211_beacon_loss(struct ieee80211_hw *hw);
> +void ieee80211_beacon_loss(struct ieee80211_hw *hw)
> +{
> + struct ieee80211_local *local = hw_to_local(hw);
> + struct ieee80211_sub_if_data *sdata;
> +
> + rcu_read_lock();
> + list_for_each_entry(sdata, &local->interfaces, list) {
> + if (sdata->vif.type != NL80211_IFTYPE_STATION)
> + continue;
> +
> + queue_work(local->hw.workqueue,
> + &sdata->u.mgd.beacon_loss_work);
> + }
> + rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(ieee80211_beacon_loss);
Shouldn't the driver just pass the relevant vif pointer?
johannes
On Sat, Mar 14, 2009 at 10:14 AM, Kalle Valo <[email protected]> wro=
te:
> Add IEEE80211_HW_BEACON_FILTERING flag so that driver inform that it =
supports
> beacon filtering. Drivers need to call the new function
> ieee80211_beacon_loss() to notify about beacon loss.
>
> Signed-off-by: Kalle Valo <[email protected]>
> ---
>
> =C2=A0include/net/mac80211.h =C2=A0 =C2=A0 | =C2=A0 18 ++++++++++++++=
++++
> =C2=A0net/mac80211/ieee80211_i.h | =C2=A0 =C2=A02 ++
> =C2=A0net/mac80211/iface.c =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A03 +++
> =C2=A0net/mac80211/mlme.c =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 39 ++++=
++++++++++++++++++++++++++++++++++-
> =C2=A04 files changed, 61 insertions(+), 1 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 9ef131d..1262b83 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -888,6 +888,13 @@ enum ieee80211_tkip_key_type {
> =C2=A0*
> =C2=A0* @IEEE80211_HW_MFP_CAPABLE:
> =C2=A0* =C2=A0 =C2=A0 Hardware supports management frame protection (=
MFP, IEEE 802.11w).
> + *
> + * @IEEE80211_HW_BEACON_FILTER: Hardware can drop similar beacon fra=
mes to
> + * =C2=A0 =C2=A0 avoid waking up cpu. Enabling this flag disables th=
e beacon check
> + * =C2=A0 =C2=A0 in mac80211 when in power save mode and the hardwar=
e must have an
> + * =C2=A0 =C2=A0 event to notify that beacons are lost. Use ieee8021=
1_beacon_loss()
> + * =C2=A0 =C2=A0 to notify the event to the stack. The filtering mus=
t be enabled
> + * =C2=A0 =C2=A0 only when IEEE80211_CONF_PS is set.
> =C2=A0*/
Lets add to the documentation a little detail explaining how beacon fil=
tering
is expected to work, and elaborate on what we expect from the driver, i=
f
anything all.
I'd like to review this just once more.
Last we reviewed this it seemed we were set on only allowing this for
hardware which supports beacon filtering with support for checksumming
of the beacons. Reason checksumming is important is for considerations
of DFS with the channel switch announcements, HT with with channel widt=
h
changes. In fact I'm curious why checksumming would be required for 2.4=
GHz
single band devices. Kalle, do you happen to know?
Considering that power saving features are important I think its worth
to revisit
this position in detail.
An alternative to this above position is that if the devices do not
support checksumming
of the beacons to let the driver figure when this should be enabled
dynamically. For example
if devices do not support checksumming but are single 2.4 GHz band
non-HT capable
devices we should let drivers enable this feature.
Other devices which do not support checksumming should only enable this=
only
when associating to an AP on non-DFS channels and when not using HT.
In practice this would mean allowing both ath5k and ath9k to take advan=
tage
of this feature.
Technically we could move the conditional logic check when this should
be enabled
to mac80211 as well. When we reviewed this it was seen as unnecessary
complexity,
I actually don't see it as too complex and think the the advantage is
worth to consider.
Thoughts?
Luis
On Sat, Mar 14, 2009 at 11:46 AM, Kalle Valo <[email protected]> wro=
te:
> Johannes Berg wrote:
>>
>> On Sat, 2009-03-14 at 19:14 +0200, Kalle Valo wrote:
>>
>>> +void ieee80211_sta_rx_notify(struct ieee80211_sub_if_data *sdata,
>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct ieee80211_hdr *hdr)
>>> +{
>>> + =C2=A0 =C2=A0 =C2=A0 /* timer triggers only when there is no unic=
ast traffic */
>>> + =C2=A0 =C2=A0 =C2=A0 if (!is_multicast_ether_addr(hdr->addr1))
>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mod_timer(&sdata=
->u.mgd.timer,
>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 jiffies + IEEE80211_MONITORING_INTERVAL);
>>> +}
>>
>> Do we really need the multicast check? The frame will be coming from=
the
>> AP in managed mode in both cases, so why regard the data path as idl=
e
>> when we're receiving multicast traffic?
>
> It seems that I had a good vacation because I can't remember anymore =
why I
> added the check :D Too bad that even the comment I wrote was next to
> useless.
>
> I'll remove the multicast check because I don't see the point for it.=
And if
> in case I finally recall the reason I'll definitely add a better comm=
ent.
>
> Thanks for all the review!
If that's the case better just remove a whole routine just for that.
Luis
On Sat, 2009-03-14 at 19:14 +0200, Kalle Valo wrote:
> +void ieee80211_sta_rx_notify(struct ieee80211_sub_if_data *sdata,
> + struct ieee80211_hdr *hdr)
> +{
> + /* timer triggers only when there is no unicast traffic */
> + if (!is_multicast_ether_addr(hdr->addr1))
> + mod_timer(&sdata->u.mgd.timer,
> + jiffies + IEEE80211_MONITORING_INTERVAL);
> +}
Do we really need the multicast check? The frame will be coming from the
AP in managed mode in both cases, so why regard the data path as idle
when we're receiving multicast traffic?
johannes
ext Johannes Berg wrote:
> On Sat, 2009-03-14 at 19:14 +0200, Kalle Valo wrote:
>
>> + if (!ps || (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK))
>> + /*
>> + * with IEEE80211_HW_PS_NULLFUNC_STACK and power save
>> + * enabled the firmware sent a null frame when power save
>> + * was disabled, so we need to send a new null frame
>> + */
>> + ieee80211_send_nullfunc(local, sdata, 1);
>> +}
>
> Now I'm confused a little. I thought with PS_NULLFUNC_STACK the firmware
> _didn't_ send such frames.
That's a bug, good catch! It should be:
!(local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK)
I'll fix that in v3.
Kalle
On Sun, Mar 15, 2009 at 09:22:38AM +0200, Kalle Valo wrote:
> In my opinion also 2.4 GHz band needs checksum support. At least ERP
> protection changes come to my mind, but maybe there are also others.
Johannes mentioned Quiet IE as one such example. WMM can also change its
parameters dynamically. Both of these need to be detected from Beacon
frames. In addition, Channel Switch Announcement can be used to move the
BSS to another channel, even on 2.4 GHz band.
> Other option is that the hardware which does not support checksumming
> would just pass beacons periodically to the stack, for example every 5
> seconds. Even though beacon filter is enabled in mac80211 it does not
> prevent hardware from sending beacons. This is not the most optimal
> solution, but there's not much choices if the hardware doesn't support
> checksumming.
While it would be possible to do that, it does not really solve many of
the use cases where Beacon contents can change and the STA is required
to take some action. For example, Channel Switch Announcement IEs would
likely be missed if a Beacon frames is passed through that infrequently.
I would assume the driver could enable this even if the hardware does
not use checksumming to detect Beacon changes, but this may severely
limit functionality in number of cases, so I'm not sure I would
recommend this to be done in general.
--
Jouni Malinen PGP id EFC895FA
Currently the timer is triggering every two seconds
(IEEE80211_MONITORING_INTERVAL). Decrease the timer to only trigger when
nothing is received to avoid waking up CPU.
Now there's a functional change that probe requests are sent only when the
data path is idle, earlier they were sent also while there was activity
on the data path.
This is also preparation for beacon filtering support. Thanks to Johannes
Berg for the idea.
Signed-off-by: Kalle Valo <[email protected]>
---
net/mac80211/ieee80211_i.h | 2 ++
net/mac80211/mlme.c | 8 ++++++++
net/mac80211/rx.c | 3 +++
3 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index ecbc8e0..74c4e13 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1080,6 +1080,8 @@ void ieee80211_dynamic_ps_timer(unsigned long data);
void ieee80211_send_nullfunc(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata,
int powersave);
+void ieee80211_sta_rx_notify(struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_hdr *hdr);
void ieee80211_wake_queues_by_reason(struct ieee80211_hw *hw,
enum queue_stop_reason reason);
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 391445c..397a2e7 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -911,6 +911,14 @@ static void ieee80211_associate(struct ieee80211_sub_if_data *sdata)
mod_timer(&ifmgd->timer, jiffies + IEEE80211_ASSOC_TIMEOUT);
}
+void ieee80211_sta_rx_notify(struct ieee80211_sub_if_data *sdata,
+ struct ieee80211_hdr *hdr)
+{
+ /* timer triggers only when there is no unicast traffic */
+ if (!is_multicast_ether_addr(hdr->addr1))
+ mod_timer(&sdata->u.mgd.timer,
+ jiffies + IEEE80211_MONITORING_INTERVAL);
+}
static void ieee80211_associated(struct ieee80211_sub_if_data *sdata)
{
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 66f7ecf..25982c0 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -855,6 +855,9 @@ ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
if (!(rx->flags & IEEE80211_RX_RA_MATCH))
return RX_CONTINUE;
+ if (rx->sdata->vif.type == NL80211_IFTYPE_STATION)
+ ieee80211_sta_rx_notify(rx->sdata, hdr);
+
sta->rx_fragments++;
sta->rx_bytes += rx->skb->len;
sta->last_signal = rx->status->signal;
Jouni Malinen <[email protected]> writes:
>> Other option is that the hardware which does not support checksumming
>> would just pass beacons periodically to the stack, for example every 5
>> seconds. Even though beacon filter is enabled in mac80211 it does not
>> prevent hardware from sending beacons. This is not the most optimal
>> solution, but there's not much choices if the hardware doesn't support
>> checksumming.
>
> While it would be possible to do that, it does not really solve many of
> the use cases where Beacon contents can change and the STA is required
> to take some action. For example, Channel Switch Announcement IEs would
> likely be missed if a Beacon frames is passed through that infrequently.
> I would assume the driver could enable this even if the hardware does
> not use checksumming to detect Beacon changes, but this may severely
> limit functionality in number of cases, so I'm not sure I would
> recommend this to be done in general.
Yes, definitely not recommended. I would classify it to "the ugly
hack" category.
--
Kalle Valo
On Sat, Mar 14, 2009 at 10:14 AM, Kalle Valo <[email protected]> wrote:
> Separate beacon and rx path tracking in preparation for the beacon filtering
> support. At the same time change ieee80211_associated() to look a bit simpler.
>
> Probe requests are now sent only after IEEE80211_PROBE_IDLE_TIME, which
> is now set to 60 seconds.
>
> Signed-off-by: Kalle Valo <[email protected]>
Can this be split into 3 separate patches, to make the one with the
actual functional changes easier to review. One is the making of
ieee80211_associated() look easier, the other the rename, and lastly
the actual functional changes. When reviewing the commit log it would
then be easier to see what was done.
Luis
When software scanning we need to disable power save so that all possible
probe responses and beacons are received. For hardware scanning assume that
hardware will take care of that and document that assumption.
Signed-off-by: Kalle Valo <[email protected]>
---
include/net/mac80211.h | 12 ++++++-----
net/mac80211/scan.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 59 insertions(+), 7 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 12a52ef..9ef131d 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1318,11 +1318,13 @@ enum ieee80211_ampdu_mlme_action {
*
* @hw_scan: Ask the hardware to service the scan request, no need to start
* the scan state machine in stack. The scan must honour the channel
- * configuration done by the regulatory agent in the wiphy's registered
- * bands. When the scan finishes, ieee80211_scan_completed() must be
- * called; note that it also must be called when the scan cannot finish
- * because the hardware is turned off! Anything else is a bug!
- * Returns a negative error code which will be seen in userspace.
+ * configuration done by the regulatory agent in the wiphy's
+ * registered bands. The hardware (or the driver) needs to make sure
+ * that power save is disabled. When the scan finishes,
+ * ieee80211_scan_completed() must be called; note that it also must
+ * be called when the scan cannot finish because the hardware is
+ * turned off! Anything else is a bug! Returns a negative error code
+ * which will be seen in userspace.
*
* @sw_scan_start: Notifier function that is called just before a software scan
* is started. Can be NULL, if the driver doesn't need this notification.
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 0e81e16..dec7d5a 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -202,6 +202,56 @@ ieee80211_scan_rx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
return RX_QUEUED;
}
+/*
+ * inform AP that we will go to sleep so that it will buffer the frames
+ * while we scan
+ */
+static void ieee80211_scan_ps_enable(struct ieee80211_sub_if_data *sdata)
+{
+ struct ieee80211_local *local = sdata->local;
+ bool ps = false;
+
+ /* FIXME: what to do when local->pspolling is true? */
+
+ del_timer_sync(&local->dynamic_ps_timer);
+ cancel_work_sync(&local->dynamic_ps_enable_work);
+
+ if (local->hw.conf.flags & IEEE80211_CONF_PS) {
+ ps = true;
+ local->hw.conf.flags &= ~IEEE80211_CONF_PS;
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+ }
+
+ if (!ps || (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK))
+ /*
+ * with IEEE80211_HW_PS_NULLFUNC_STACK and power save
+ * enabled the firmware sent a null frame when power save
+ * was disabled, so we need to send a new null frame
+ */
+ ieee80211_send_nullfunc(local, sdata, 1);
+}
+
+/* inform AP that we are awake again, unless power save is enabled */
+static void ieee80211_scan_ps_disable(struct ieee80211_sub_if_data *sdata)
+{
+ struct ieee80211_local *local = sdata->local;
+
+ if (!local->powersave)
+ ieee80211_send_nullfunc(local, sdata, 0);
+ else {
+ /*
+ * In IEEE80211_HW_PS_NULLFUNC_STACK case firmware will
+ * send a null frame with powersave bit set again even
+ * though the AP already knows that we are sleeping. This
+ * could be avoided by sending a null frame with power save
+ * bit disabled before enabling the power save but it
+ * doesn't gain anything.
+ */
+ local->hw.conf.flags |= IEEE80211_CONF_PS;
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+ }
+}
+
void ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted)
{
struct ieee80211_local *local = hw_to_local(hw);
@@ -256,7 +306,7 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted)
/* Tell AP we're back */
if (sdata->vif.type == NL80211_IFTYPE_STATION) {
if (sdata->u.mgd.flags & IEEE80211_STA_ASSOCIATED) {
- ieee80211_send_nullfunc(local, sdata, 0);
+ ieee80211_scan_ps_disable(sdata);
netif_tx_wake_all_queues(sdata->dev);
}
} else
@@ -416,7 +466,7 @@ int ieee80211_start_scan(struct ieee80211_sub_if_data *scan_sdata,
if (sdata->vif.type == NL80211_IFTYPE_STATION) {
if (sdata->u.mgd.flags & IEEE80211_STA_ASSOCIATED) {
netif_tx_stop_all_queues(sdata->dev);
- ieee80211_send_nullfunc(local, sdata, 1);
+ ieee80211_scan_ps_enable(sdata);
}
} else
netif_tx_stop_all_queues(sdata->dev);
On Sat, 2009-03-14 at 19:14 +0200, Kalle Valo wrote:
> + if (!ps || (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK))
> + /*
> + * with IEEE80211_HW_PS_NULLFUNC_STACK and power save
> + * enabled the firmware sent a null frame when power save
> + * was disabled, so we need to send a new null frame
> + */
> + ieee80211_send_nullfunc(local, sdata, 1);
> +}
Now I'm confused a little. I thought with PS_NULLFUNC_STACK the firmware
_didn't_ send such frames.
johannes
Separate beacon and rx path tracking in preparation for the beacon filtering
support. At the same time change ieee80211_associated() to look a bit simpler.
Probe requests are now sent only after IEEE80211_PROBE_IDLE_TIME, which
is now set to 60 seconds.
Signed-off-by: Kalle Valo <[email protected]>
---
net/mac80211/ieee80211_i.h | 1 +
net/mac80211/mlme.c | 79 ++++++++++++++++++++++++++------------------
net/mac80211/rx.c | 6 +++
3 files changed, 53 insertions(+), 33 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 74c4e13..5d3d0bf 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -305,6 +305,7 @@ struct ieee80211_if_managed {
unsigned long request;
unsigned long last_probe;
+ unsigned long last_beacon;
unsigned int flags;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 397a2e7..c235d39 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -30,7 +30,7 @@
#define IEEE80211_ASSOC_TIMEOUT (HZ / 5)
#define IEEE80211_ASSOC_MAX_TRIES 3
#define IEEE80211_MONITORING_INTERVAL (2 * HZ)
-#define IEEE80211_PROBE_INTERVAL (60 * HZ)
+#define IEEE80211_PROBE_IDLE_TIME (60 * HZ)
#define IEEE80211_RETRY_AUTH_INTERVAL (1 * HZ)
/* utils */
@@ -925,7 +925,7 @@ static void ieee80211_associated(struct ieee80211_sub_if_data *sdata)
struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
struct ieee80211_local *local = sdata->local;
struct sta_info *sta;
- int disassoc;
+ bool disassoc = false;
/* TODO: start monitoring current AP signal quality and number of
* missed beacons. Scan other channels every now and then and search
@@ -940,36 +940,39 @@ static void ieee80211_associated(struct ieee80211_sub_if_data *sdata)
if (!sta) {
printk(KERN_DEBUG "%s: No STA entry for own AP %pM\n",
sdata->dev->name, ifmgd->bssid);
- disassoc = 1;
- } else {
- disassoc = 0;
- if (time_after(jiffies,
- sta->last_rx + IEEE80211_MONITORING_INTERVAL)) {
- if (ifmgd->flags & IEEE80211_STA_PROBEREQ_POLL) {
- printk(KERN_DEBUG "%s: No ProbeResp from "
- "current AP %pM - assume out of "
- "range\n",
- sdata->dev->name, ifmgd->bssid);
- disassoc = 1;
- } else
- ieee80211_send_probe_req(sdata, ifmgd->bssid,
- ifmgd->ssid,
- ifmgd->ssid_len,
- NULL, 0);
- ifmgd->flags ^= IEEE80211_STA_PROBEREQ_POLL;
- } else {
- ifmgd->flags &= ~IEEE80211_STA_PROBEREQ_POLL;
- if (time_after(jiffies, ifmgd->last_probe +
- IEEE80211_PROBE_INTERVAL)) {
- ifmgd->last_probe = jiffies;
- ieee80211_send_probe_req(sdata, ifmgd->bssid,
- ifmgd->ssid,
- ifmgd->ssid_len,
- NULL, 0);
- }
- }
+ disassoc = true;
+ goto unlock;
+ }
+
+ if ((ifmgd->flags & IEEE80211_STA_PROBEREQ_POLL) &&
+ time_after(jiffies, sta->last_rx + IEEE80211_MONITORING_INTERVAL)) {
+ printk(KERN_DEBUG "%s: no probe response from AP %pM "
+ "- disassociating\n",
+ sdata->dev->name, ifmgd->bssid);
+ disassoc = true;
+ ifmgd->flags &= ~IEEE80211_STA_PROBEREQ_POLL;
+ goto unlock;
+ }
+
+ if (time_after(jiffies,
+ ifmgd->last_beacon + IEEE80211_MONITORING_INTERVAL)) {
+ printk(KERN_DEBUG "%s: beacon loss from AP %pM "
+ "- sending probe request\n",
+ sdata->dev->name, ifmgd->bssid);
+ ifmgd->flags |= IEEE80211_STA_PROBEREQ_POLL;
+ ieee80211_send_probe_req(sdata, ifmgd->bssid, ifmgd->ssid,
+ ifmgd->ssid_len, NULL, 0);
+ goto unlock;
+
+ }
+
+ if (time_after(jiffies, sta->last_rx + IEEE80211_PROBE_IDLE_TIME)) {
+ ifmgd->flags |= IEEE80211_STA_PROBEREQ_POLL;
+ ieee80211_send_probe_req(sdata, ifmgd->bssid, ifmgd->ssid,
+ ifmgd->ssid_len, NULL, 0);
}
+ unlock:
rcu_read_unlock();
if (disassoc)
@@ -977,7 +980,7 @@ static void ieee80211_associated(struct ieee80211_sub_if_data *sdata)
WLAN_REASON_PREV_AUTH_NOT_VALID);
else
mod_timer(&ifmgd->timer, jiffies +
- IEEE80211_MONITORING_INTERVAL);
+ IEEE80211_MONITORING_INTERVAL);
}
@@ -1358,6 +1361,12 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
bss_conf->assoc_capability = capab_info;
ieee80211_set_associated(sdata, changed);
+ /*
+ * initialise the time of last beacon to be the association time,
+ * otherwise beacon loss check will trigger immediately
+ */
+ ifmgd->last_beacon = jiffies;
+
ieee80211_associated(sdata);
}
@@ -1405,9 +1414,12 @@ static void ieee80211_rx_mgmt_probe_resp(struct ieee80211_sub_if_data *sdata,
size_t len,
struct ieee80211_rx_status *rx_status)
{
+ struct ieee80211_if_managed *ifmgd;
size_t baselen;
struct ieee802_11_elems elems;
+ ifmgd = &sdata->u.mgd;
+
if (memcmp(mgmt->da, sdata->dev->dev_addr, ETH_ALEN))
return; /* ignore ProbeResp to foreign address */
@@ -1422,11 +1434,14 @@ static void ieee80211_rx_mgmt_probe_resp(struct ieee80211_sub_if_data *sdata,
/* direct probe may be part of the association flow */
if (test_and_clear_bit(IEEE80211_STA_REQ_DIRECT_PROBE,
- &sdata->u.mgd.request)) {
+ &ifmgd->request)) {
printk(KERN_DEBUG "%s direct probe responded\n",
sdata->dev->name);
ieee80211_authenticate(sdata);
}
+
+ if (ifmgd->flags & IEEE80211_STA_PROBEREQ_POLL)
+ ifmgd->flags &= ~IEEE80211_STA_PROBEREQ_POLL;
}
static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 25982c0..4990e76 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -849,7 +849,11 @@ ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
* Mesh beacons will update last_rx when if they are found to
* match the current local configuration when processed.
*/
- sta->last_rx = jiffies;
+ if (rx->sdata->vif.type == NL80211_IFTYPE_STATION &&
+ ieee80211_is_beacon(hdr->frame_control)) {
+ rx->sdata->u.mgd.last_beacon = jiffies;
+ } else
+ sta->last_rx = jiffies;
}
if (!(rx->flags & IEEE80211_RX_RA_MATCH))
Johannes Berg wrote:
> On Sat, 2009-03-14 at 19:14 +0200, Kalle Valo wrote:
>
>> +void ieee80211_beacon_loss(struct ieee80211_hw *hw)
>> +{
>> + struct ieee80211_local *local = hw_to_local(hw);
>> + struct ieee80211_sub_if_data *sdata;
>> +
>> + rcu_read_lock();
>> + list_for_each_entry(sdata, &local->interfaces, list) {
>> + if (sdata->vif.type != NL80211_IFTYPE_STATION)
>> + continue;
>> +
>> + queue_work(local->hw.workqueue,
>> + &sdata->u.mgd.beacon_loss_work);
>> + }
>> + rcu_read_unlock();
>> +}
>> +EXPORT_SYMBOL(ieee80211_beacon_loss);
>
> Shouldn't the driver just pass the relevant vif pointer?
Nice, that way the implementation would be a lot simpler and consistent
with the rest of the driver interface. Yes, I'll change it to use vif
pointer.
Kalle
Luis R. Rodriguez wrote:
> On Sun, Mar 15, 2009 at 10:59 AM, Johannes Berg
> <[email protected]> wrote:
>> On Sun, 2009-03-15 at 09:22 +0200, Kalle Valo wrote:
>>
>>> I think this is future stuff, I would like to get the basic (and the
>>> simplest) implementation to the tree first and then we can start
>>> improving and extending it.
>> Agreed, anything that is more complex than this basic stuff which
>> requires proper beacon change watching by the firmware is too much for
>> the first cut here -- we're having enough trouble as-is, for example
>> with the BSS struct stuff.
>
> Understood, can we just add some text to the doc then elaborating what
> is expected of the hardware for now.
Sure thing. I already have a todo item to write a better description.
Kalle
On Sun, 2009-03-15 at 09:22 +0200, Kalle Valo wrote:
> "Luis R. Rodriguez" <[email protected]> writes:
>
> > Last we reviewed this it seemed we were set on only allowing this for
> > hardware which supports beacon filtering with support for checksumming
> > of the beacons. Reason checksumming is important is for considerations
> > of DFS with the channel switch announcements, HT with with channel width
> > changes. In fact I'm curious why checksumming would be required for 2.4 GHz
> > single band devices. Kalle, do you happen to know?
>
> In my opinion also 2.4 GHz band needs checksum support. At least ERP
> protection changes come to my mind, but maybe there are also others.
Also, the virtual hardware support Jouni is working on might use quiet
elements, and these would have to be detected by the stations. We really
want beacon change detection somewhere.
> I think this is future stuff, I would like to get the basic (and the
> simplest) implementation to the tree first and then we can start
> improving and extending it.
Agreed, anything that is more complex than this basic stuff which
requires proper beacon change watching by the firmware is too much for
the first cut here -- we're having enough trouble as-is, for example
with the BSS struct stuff.
johannes
On Sat, 2009-03-14 at 12:45 -0700, Luis R. Rodriguez wrote:
> On Sat, Mar 14, 2009 at 10:14 AM, Kalle Valo <[email protected]> wrote:
> > Separate beacon and rx path tracking in preparation for the beacon filtering
> > support. At the same time change ieee80211_associated() to look a bit simpler.
> >
> > Probe requests are now sent only after IEEE80211_PROBE_IDLE_TIME, which
> > is now set to 60 seconds.
> >
> > Signed-off-by: Kalle Valo <[email protected]>
>
> Can this be split into 3 separate patches, to make the one with the
> actual functional changes easier to review. One is the making of
> ieee80211_associated() look easier, the other the rename, and lastly
> the actual functional changes. When reviewing the commit log it would
> then be easier to see what was done.
Disagree, the rename is part of the functional change, it's not just a
plain rename.
johannes
On Sat, 2009-03-14 at 19:14 +0200, Kalle Valo wrote:
> Separate beacon and rx path tracking in preparation for the beacon filtering
> support. At the same time change ieee80211_associated() to look a bit simpler.
>
> Probe requests are now sent only after IEEE80211_PROBE_IDLE_TIME, which
> is now set to 60 seconds.
Looks good.
> else
> mod_timer(&ifmgd->timer, jiffies +
> - IEEE80211_MONITORING_INTERVAL);
> + IEEE80211_MONITORING_INTERVAL);
That seems to be indented a little oddly though :)
johannes
On Sat, Mar 14, 2009 at 12:55 PM, Johannes Berg
<[email protected]> wrote:
> On Sat, 2009-03-14 at 12:45 -0700, Luis R. Rodriguez wrote:
>> On Sat, Mar 14, 2009 at 10:14 AM, Kalle Valo <[email protected]> wrote:
>> > Separate beacon and rx path tracking in preparation for the beacon filtering
>> > support. At the same time change ieee80211_associated() to look a bit simpler.
>> >
>> > Probe requests are now sent only after IEEE80211_PROBE_IDLE_TIME, which
>> > is now set to 60 seconds.
>> >
>> > Signed-off-by: Kalle Valo <[email protected]>
>>
>> Can this be split into 3 separate patches, to make the one with the
>> actual functional changes easier to review. One is the making of
>> ieee80211_associated() look easier, the other the rename, and lastly
>> the actual functional changes. When reviewing the commit log it would
>> then be easier to see what was done.
>
> Disagree, the rename is part of the functional change, it's not just a
> plain rename.
Alright, then 2.
Luis
On Sun, Mar 15, 2009 at 10:59 AM, Johannes Berg
<[email protected]> wrote:
> On Sun, 2009-03-15 at 09:22 +0200, Kalle Valo wrote:
>> "Luis R. Rodriguez" <[email protected]> writes:
>>
>> > Last we reviewed this it seemed we were set on only allowing this for
>> > hardware which supports beacon filtering with support for checksumming
>> > of the beacons. Reason checksumming is important is for considerations
>> > of DFS with the channel switch announcements, HT with with channel width
>> > changes. In fact I'm curious why checksumming would be required for 2.4 GHz
>> > single band devices. Kalle, do you happen to know?
>>
>> In my opinion also 2.4 GHz band needs checksum support. At least ERP
>> protection changes come to my mind, but maybe there are also others.
>
> Also, the virtual hardware support Jouni is working on might use quiet
> elements, and these would have to be detected by the stations. We really
> want beacon change detection somewhere.
>
>> I think this is future stuff, I would like to get the basic (and the
>> simplest) implementation to the tree first and then we can start
>> improving and extending it.
>
> Agreed, anything that is more complex than this basic stuff which
> requires proper beacon change watching by the firmware is too much for
> the first cut here -- we're having enough trouble as-is, for example
> with the BSS struct stuff.
Understood, can we just add some text to the doc then elaborating what
is expected of the hardware for now.
Luis