[RFC PATCH v2 0/4] mac80211: beacon filtering

[RFC PATCH v2 0/4] mac80211: beacon filtering

[Kalle Valo]

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(-)

[RFC PATCH v2 1/4] mac80211: decrease execution of the associated timer

[Kalle Valo]

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 <kalle.valo@nokia.com>
---

 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;

Re: [RFC PATCH v2 1/4] mac80211: decrease execution of the associated timer

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 576 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH v2 1/4] mac80211: decrease execution of the associated timer

[Kalle Valo]

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

Re: [RFC PATCH v2 1/4] mac80211: decrease execution of the associated timer

[Luis R. Rodriguez]

On Sat, Mar 14, 2009 at 11:46 AM, Kalle Valo <kalle.valo@nokia.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 1/4] mac80211: decrease execution of the associated timer

[Kalle Valo]

Kalle Valo <kalle.valo@nokia.com> writes:

> 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.

Now I remember what this test was about. In ieee80211_associated()
there are two tests: probe request check after an data idle period
(currently 60 secs) and beacon loss check for drivers not supporting
filtering (2 secs).

The former test has been originally (even before my beacon filter
patches) implemented so that the probe request is sent even though we
are receiving beacons. mod_timer() is called only for unicast traffic
so that the probe request test will trigger when we are still
receiving beacons.

I'll add a proper comment explaining all this.

Also currently ieee80211_associated() is called every two seconds, we
need to fix this in future to avoid waking up cpu needlessly. I don't
want to make too intrusive changes right now.

-- 
Kalle Valo

Re: [RFC PATCH v2 1/4] mac80211: decrease execution of the associated timer

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]

On Fri, 2009-03-20 at 11:22 +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.
> 
> Now I remember what this test was about. In ieee80211_associated()
> there are two tests: probe request check after an data idle period
> (currently 60 secs) and beacon loss check for drivers not supporting
> filtering (2 secs).
> 
> The former test has been originally (even before my beacon filter
> patches) implemented so that the probe request is sent even though we
> are receiving beacons. mod_timer() is called only for unicast traffic
> so that the probe request test will trigger when we are still
> receiving beacons.

Why would we not want to change that? I don't see a reason to send a
probe request when we're still receiving beacons. It was meant to be
used when beacons got lost _during_ a data idle period to verify that
the AP is still there, afaict.

> I'll add a proper comment explaining all this.
> 
> Also currently ieee80211_associated() is called every two seconds, we
> need to fix this in future to avoid waking up cpu needlessly. I don't
> want to make too intrusive changes right now.

I thought you already did that too :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH v2 1/4] mac80211: decrease execution of the associated timer

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2009-03-20 at 11:22 +0200, Kalle Valo wrote:
>
>> Now I remember what this test was about. In ieee80211_associated()
>> there are two tests: probe request check after an data idle period
>> (currently 60 secs) and beacon loss check for drivers not supporting
>> filtering (2 secs).
>> 
>> The former test has been originally (even before my beacon filter
>> patches) implemented so that the probe request is sent even though we
>> are receiving beacons. mod_timer() is called only for unicast traffic
>> so that the probe request test will trigger when we are still
>> receiving beacons.
>
> Why would we not want to change that? I don't see a reason to send a
> probe request when we're still receiving beacons. It was meant to be
> used when beacons got lost _during_ a data idle period to verify that
> the AP is still there, afaict.

The original (or current) implementation sends probe requests even
when beacons are received or there is unicast traffic going on. My
patches change this so that the probe request is only sent when there
is data idle period (ie. no unicast traffic).

Like we discussed earlier, I think the most optimal solution would be
to send nullfunc frames during data idle periods just to check that the
connection is working bothways.

What should AP do if it receives a nullfunc frame from a station which
is not associated? Send a deauth frame back to the station? It would
be nice to check the association state also during the data idle
period. Sometimes I have noticed that AP might deauthenticate the
station without station noticing it. It would be just perfect if
nullfunc frame would do the assocition also during data idle periods.

>> I'll add a proper comment explaining all this.
>> 
>> Also currently ieee80211_associated() is called every two seconds, we
>> need to fix this in future to avoid waking up cpu needlessly. I don't
>> want to make too intrusive changes right now.
>
> I thought you already did that too :)

Sorry, I did that only for the period when there is data transmission.
To get rid of unncessary wakeups altogether I need to do something
more advanced, like using two timers or something. I haven't thought
that yet.

I'll mention this in the commit logs.

-- 
Kalle Valo

Re: [RFC PATCH v2 1/4] mac80211: decrease execution of the associated timer

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]

On Fri, 2009-03-20 at 12:37 +0200, Kalle Valo wrote:

> The original (or current) implementation sends probe requests even
> when beacons are received or there is unicast traffic going on. My
> patches change this so that the probe request is only sent when there
> is data idle period (ie. no unicast traffic).

Ok.

> Like we discussed earlier, I think the most optimal solution would be
> to send nullfunc frames during data idle periods just to check that the
> connection is working bothways.
> 
> What should AP do if it receives a nullfunc frame from a station which
> is not associated? Send a deauth frame back to the station? It would
> be nice to check the association state also during the data idle
> period. Sometimes I have noticed that AP might deauthenticate the
> station without station noticing it. It would be just perfect if
> nullfunc frame would do the assocition also during data idle periods.

Yes, I think nullfunc, being data, counts as class 3, and as such the AP
will deauth the station.

> Sorry, I did that only for the period when there is data transmission.
> To get rid of unncessary wakeups altogether I need to do something
> more advanced, like using two timers or something. I haven't thought
> that yet.

Ok, guess I got lost in all the timers.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[RFC PATCH v2 2/4] mac80211: track beacons separately from the rx path activity

[Kalle Valo]

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 <kalle.valo@nokia.com>
---

 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))

Re: [RFC PATCH v2 2/4] mac80211: track beacons separately from the rx path activity

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH v2 2/4] mac80211: track beacons separately from the rx path activity

[Luis R. Rodriguez]

On Sat, Mar 14, 2009 at 10:14 AM, Kalle Valo <kalle.valo@nokia.com> 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 <kalle.valo@nokia.com>

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

Re: [RFC PATCH v2 2/4] mac80211: track beacons separately from the rx path activity

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

On Sat, 2009-03-14 at 12:45 -0700, Luis R. Rodriguez wrote:
> On Sat, Mar 14, 2009 at 10:14 AM, Kalle Valo <kalle.valo@nokia.com> 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 <kalle.valo@nokia.com>
> 
> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH v2 2/4] mac80211: track beacons separately from the rx path activity

[Luis R. Rodriguez]

On Sat, Mar 14, 2009 at 12:55 PM, Johannes Berg
<johannes@sipsolutions.net> 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 <kalle.valo@nokia.com> 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 <kalle.valo@nokia.com>
>>
>> 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

Re: [RFC PATCH v2 2/4] mac80211: track beacons separately from the rx path activity

[Kalle Valo]

"Luis R. Rodriguez" <lrodriguez@atheros.com> writes:

> On Sat, Mar 14, 2009 at 10:14 AM, Kalle Valo <kalle.valo@nokia.com> 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 <kalle.valo@nokia.com>
>
> 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

[RFC PATCH v2 3/4] mac80211: disable power save when scanning

[Kalle Valo]

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 <kalle.valo@nokia.com>
---

 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);

Re: [RFC PATCH v2 3/4] mac80211: disable power save when scanning

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH v2 3/4] mac80211: disable power save when scanning

[Kalle Valo]

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

[RFC PATCH v2 4/4] mac80211: add beacon filtering support

[Kalle Valo]

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 <kalle.valo@nokia.com>
---

 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,

Re: [RFC PATCH v2 4/4] mac80211: add beacon filtering support

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH v2 4/4] mac80211: add beacon filtering support

[Kalle Valo]

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

Re: [RFC PATCH v2 4/4] mac80211: add beacon filtering support

[Luis R. Rodriguez]

On Sat, Mar 14, 2009 at 10:14 AM, Kalle Valo <kalle.valo@nokia.com> 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 <kalle.valo@nokia.com>
> ---
>
> =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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 4/4] mac80211: add beacon filtering support

[Kalle Valo]

"Luis R. Rodriguez" <lrodriguez@atheros.com> 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

Re: [RFC PATCH v2 4/4] mac80211: add beacon filtering support

[Johannes Berg]

On Sun, 2009-03-15 at 09:22 +0200, Kalle Valo wrote:
> "Luis R. Rodriguez" <lrodriguez@atheros.com> 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

Re: [RFC PATCH v2 4/4] mac80211: add beacon filtering support

[Luis R. Rodriguez]

On Sun, Mar 15, 2009 at 10:59 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Sun, 2009-03-15 at 09:22 +0200, Kalle Valo wrote:
>> "Luis R. Rodriguez" <lrodriguez@atheros.com> 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

Re: [RFC PATCH v2 4/4] mac80211: add beacon filtering support

[Kalle Valo]

Luis R. Rodriguez wrote:
> On Sun, Mar 15, 2009 at 10:59 AM, Johannes Berg
> <johannes@sipsolutions.net> 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

Re: [RFC PATCH v2 4/4] mac80211: add beacon filtering support

[Jouni Malinen]

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

Re: [RFC PATCH v2 4/4] mac80211: add beacon filtering support

[Kalle Valo]

Jouni Malinen <j@w1.fi> 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