[RFC PATCH v2 0/2] mac80211: ps-poll implementation

[RFC PATCH v2 0/2] mac80211: ps-poll implementation

[Kalle Valo]

Here's my suggestion for how to implement ps-poll in mac80211. Also
this fixes power save again for p54/stlc45xx, but most probably breaks
ath9k. I would like to hear opinions from others, especially from
Atheros. How does ath9k cope with the ps-poll method? Also I highly
recommend to get hardware waking up for multicast frames, it would be
a lot easier and reliable that way.

Open question is that should power save be disabled whenever mac80211
is ps-polling the frames. For example, p54/stlc45xx does not require to
disable power save in that case, it just stays awake long enough to
receive the data frame from the AP. So I did not disable power save
mode in this case, but I would like to hear comments what other
hardware needs.

v2:

o RX_CONTINUE after sending ps-poll frame in ieee80211_rx_h_check_more_data()
o remove debug printk

---

Kalle Valo (2):
      mac80211: use ps-poll to request frames in power save mode
      mac80211: remove multicast check from check_tim()


 net/mac80211/ieee80211_i.h |    3 +++
 net/mac80211/mlme.c        |   51 +++++++++++++++++++++++++++++++++++---------
 net/mac80211/rx.c          |   34 +++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 10 deletions(-)

[RFC PATCH v2 1/2] mac80211: remove multicast check from check_tim()

[Kalle Valo]

Currently mac80211 checks for the multicast tim bit from beacons and
disables power save if the bit is set. This was added to support
ath9k. But this is a bit controversial because the AP will send
multicast frames immediately after the beacon and the time constraints
are really high. Relying mac80211 to be fast enough here might not be
reliable in all situations.

Also with current implementation mac80211 disables power save whenever
multicast bit is set but it's never enabled again. So if dynamic power
save is disabled (iwconfig wlan0 power timeout 0), the power save mode
is enabled only until first multicast/broadcast frame is received, which
isn't a long time in a normal network.

The current implementation is not usable on p54/stlc45xx and the
easiest way to fix this is to remove the multicast tim bit check
altogether. Handling multicast tim bit in host is rare, most of the
designs do this in firmware/hardware, so it's better not to have it in
mac80211. It's a lot better to do this in firmware/hardware, or if
that's not possible it could be done in the driver.

Also renamed the function to ieee80211_check_tim() to follow the style
of the file.

Signed-off-by: Kalle Valo <kalle.valo@iki.fi>
---

 net/mac80211/mlme.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 51f021f..c7e61e2 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -575,7 +575,7 @@ static void ieee80211_sta_wmm_params(struct ieee80211_local *local,
 	}
 }
 
-static bool check_tim(struct ieee802_11_elems *elems, u16 aid, bool *is_mc)
+static bool ieee80211_check_tim(struct ieee802_11_elems *elems, u16 aid)
 {
 	u8 mask;
 	u8 index, indexn1, indexn2;
@@ -585,9 +585,6 @@ static bool check_tim(struct ieee802_11_elems *elems, u16 aid, bool *is_mc)
 	index = aid / 8;
 	mask  = 1 << (aid & 7);
 
-	if (tim->bitmap_ctrl & 0x01)
-		*is_mc = true;
-
 	indexn1 = tim->bitmap_ctrl & 0xfe;
 	indexn2 = elems->tim_len + indexn1 - 4;
 
@@ -1788,7 +1785,7 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
 	struct ieee802_11_elems elems;
 	struct ieee80211_local *local = sdata->local;
 	u32 changed = 0;
-	bool erp_valid, directed_tim, is_mc = false;
+	bool erp_valid, directed_tim;
 	u8 erp_value = 0;
 
 	/* Process beacon from the current BSS */
@@ -1816,9 +1813,9 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
 
 	if (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK &&
 	    local->hw.conf.flags & IEEE80211_CONF_PS) {
-		directed_tim = check_tim(&elems, ifsta->aid, &is_mc);
+		directed_tim = ieee80211_check_tim(&elems, ifsta->aid);
 
-		if (directed_tim || is_mc) {
+		if (directed_tim) {
 			local->hw.conf.flags &= ~IEEE80211_CONF_PS;
 			ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
 			ieee80211_send_nullfunc(local, sdata, 0);

[RFC PATCH v2 2/2] mac80211: use ps-poll to request frames in power save mode

[Kalle Valo]

When a directed tim bit is set, mac80211 currently disables power save
ands sends a null frame to the AP. But if dynamic power save is
disabled, mac80211 will not enable power save again. Fix this by
adding ps-poll functionality to mac80211. When a directed tim bit is
set, mac80211 sends a ps-poll frame to the AP and checks for the more
data bit in the returned data frames. Power save mode is not disabled
during ps-polling, the assumption is that firmware still receives
responses to the ps-poll frame.

Using ps-poll is slower than waking up with null frame, but I
recommend using dynamic power save in cases where throughput is
important. User space can enable it with 'iwconfig wlan0 power timeout
<value>'.

Signed-off-by: Kalle Valo <kalle.valo@iki.fi>
---

 net/mac80211/ieee80211_i.h |    3 +++
 net/mac80211/mlme.c        |   40 +++++++++++++++++++++++++++++++++++++---
 net/mac80211/rx.c          |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c9ffadb..76814e9 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -710,6 +710,7 @@ struct ieee80211_local {
 	unsigned int wmm_acm; /* bit field of ACM bits (BIT(802.1D tag)) */
 
 	bool powersave;
+	bool pspolling;
 	struct work_struct dynamic_ps_enable_work;
 	struct work_struct dynamic_ps_disable_work;
 	struct timer_list dynamic_ps_timer;
@@ -902,6 +903,8 @@ u64 ieee80211_sta_get_rates(struct ieee80211_local *local,
 			    enum ieee80211_band band);
 void ieee80211_send_probe_req(struct ieee80211_sub_if_data *sdata, u8 *dst,
 			      u8 *ssid, size_t ssid_len);
+void ieee80211_send_pspoll(struct ieee80211_local *local,
+			   struct ieee80211_sub_if_data *sdata);
 
 /* scan/BSS handling */
 int ieee80211_request_scan(struct ieee80211_sub_if_data *sdata,
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index c7e61e2..c8778f3 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -475,6 +475,41 @@ static void ieee80211_send_deauth_disassoc(struct ieee80211_sub_if_data *sdata,
 	ieee80211_tx_skb(sdata, skb, ifsta->flags & IEEE80211_STA_MFP_ENABLED);
 }
 
+void ieee80211_send_pspoll(struct ieee80211_local *local,
+			   struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_if_sta *ifsta = &sdata->u.sta;
+	struct ieee80211_pspoll *pspoll;
+	struct sk_buff *skb;
+	u16 fc;
+
+	skb = dev_alloc_skb(local->hw.extra_tx_headroom + sizeof(*pspoll));
+	if (!skb) {
+		printk(KERN_DEBUG "%s: failed to allocate buffer for "
+		       "pspoll frame\n", sdata->dev->name);
+		return;
+	}
+	skb_reserve(skb, local->hw.extra_tx_headroom);
+
+	pspoll = (struct ieee80211_pspoll *) skb_put(skb, sizeof(*pspoll));
+	memset(pspoll, 0, sizeof(*pspoll));
+	fc = IEEE80211_FTYPE_CTL | IEEE80211_STYPE_PSPOLL;
+	pspoll->frame_control = cpu_to_le16(fc);
+	pspoll->aid = cpu_to_le16(ifsta->aid);
+
+	/* aid in PS-Poll has its two MSBs each set to 1 */
+	pspoll->aid |= cpu_to_le16(1 << 15 | 1 << 14);
+
+	memcpy(pspoll->bssid, ifsta->bssid, ETH_ALEN);
+	memcpy(pspoll->ta, sdata->dev->dev_addr, ETH_ALEN);
+
+	printk(KERN_DEBUG "sending ps-poll");
+
+	ieee80211_tx_skb(sdata, skb, 0);
+
+	return;
+}
+
 /* MLME */
 static void ieee80211_sta_def_wmm_params(struct ieee80211_sub_if_data *sdata,
 					 struct ieee80211_bss *bss)
@@ -1816,9 +1851,8 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
 		directed_tim = ieee80211_check_tim(&elems, ifsta->aid);
 
 		if (directed_tim) {
-			local->hw.conf.flags &= ~IEEE80211_CONF_PS;
-			ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
-			ieee80211_send_nullfunc(local, sdata, 0);
+			local->pspolling = true;
+			ieee80211_send_pspoll(local, sdata);
 		}
 	}
 
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 9fd9d21..59b0a50 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -740,6 +740,39 @@ ieee80211_rx_h_decrypt(struct ieee80211_rx_data *rx)
 	return result;
 }
 
+static ieee80211_rx_result debug_noinline
+ieee80211_rx_h_check_more_data(struct ieee80211_rx_data *rx)
+{
+	struct ieee80211_local *local;
+	struct ieee80211_hdr *hdr;
+	struct sk_buff *skb;
+
+	local = rx->local;
+	skb = rx->skb;
+	hdr = (struct ieee80211_hdr *) skb->data;
+
+	if (!local->pspolling)
+		return RX_CONTINUE;
+
+	if (!ieee80211_has_fromds(hdr->frame_control))
+		/* this is not from AP */
+		return RX_CONTINUE;
+
+	if (!ieee80211_is_data(hdr->frame_control))
+		return RX_CONTINUE;
+
+	if (!ieee80211_has_moredata(hdr->frame_control)) {
+		/* AP has no more frames buffered for us */
+		local->pspolling = false;
+		return RX_CONTINUE;
+	}
+
+	/* more data bit is set, let's request a new frame from the AP */
+	ieee80211_send_pspoll(local, rx->sdata);
+
+	return RX_CONTINUE;
+}
+
 static void ap_sta_ps_start(struct sta_info *sta)
 {
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
@@ -2006,6 +2039,7 @@ static void ieee80211_invoke_rx_handlers(struct ieee80211_sub_if_data *sdata,
 	CALL_RXH(ieee80211_rx_h_passive_scan)
 	CALL_RXH(ieee80211_rx_h_check)
 	CALL_RXH(ieee80211_rx_h_decrypt)
+	CALL_RXH(ieee80211_rx_h_check_more_data)
 	CALL_RXH(ieee80211_rx_h_sta_process)
 	CALL_RXH(ieee80211_rx_h_defragment)
 	CALL_RXH(ieee80211_rx_h_ps_poll)

[RFC PATCH v2 1/2] mac80211: remove multicast check from check_tim()

[Kalle Valo]

Currently mac80211 checks for the multicast tim bit from beacons,
disables power save and sends a null frame if the bit is set. This was
added to support ath9k. But this is a bit controversial because the AP will
send multicast frames immediately after the beacon and the time constraints
are really high. Relying mac80211 to be fast enough here might not be
reliable in all situations. And there's no need to send a null frame, AP
will send the frames immediately after the dtim beacon no matter what.

Also if dynamic power save is disabled (iwconfig wlan0 power timeout 0)
currently mac80211 disables power save whenever the multicast bit is set
but it's never enabled again after receiving the first multicast/broadcast
frame.

The current implementation is not usable on p54/stlc45xx and the
easiest way to fix this is to remove the multicast tim bit check
altogether. Handling multicast tim bit in host is rare, most of the
designs do this in firmware/hardware, so it's better not to have it in
mac80211. It's a lot better to do this in firmware/hardware, or if
that's not possible it could be done in the driver.

Also renamed the function to ieee80211_check_tim() to follow the style
of the file.

Signed-off-by: Kalle Valo <kalle.valo@nokia.com>
---

 net/mac80211/mlme.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 51f021f..c7e61e2 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -575,7 +575,7 @@ static void ieee80211_sta_wmm_params(struct ieee80211_local *local,
 	}
 }
 
-static bool check_tim(struct ieee802_11_elems *elems, u16 aid, bool *is_mc)
+static bool ieee80211_check_tim(struct ieee802_11_elems *elems, u16 aid)
 {
 	u8 mask;
 	u8 index, indexn1, indexn2;
@@ -585,9 +585,6 @@ static bool check_tim(struct ieee802_11_elems *elems, u16 aid, bool *is_mc)
 	index = aid / 8;
 	mask  = 1 << (aid & 7);
 
-	if (tim->bitmap_ctrl & 0x01)
-		*is_mc = true;
-
 	indexn1 = tim->bitmap_ctrl & 0xfe;
 	indexn2 = elems->tim_len + indexn1 - 4;
 
@@ -1788,7 +1785,7 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
 	struct ieee802_11_elems elems;
 	struct ieee80211_local *local = sdata->local;
 	u32 changed = 0;
-	bool erp_valid, directed_tim, is_mc = false;
+	bool erp_valid, directed_tim;
 	u8 erp_value = 0;
 
 	/* Process beacon from the current BSS */
@@ -1816,9 +1813,9 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
 
 	if (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK &&
 	    local->hw.conf.flags & IEEE80211_CONF_PS) {
-		directed_tim = check_tim(&elems, ifsta->aid, &is_mc);
+		directed_tim = ieee80211_check_tim(&elems, ifsta->aid);
 
-		if (directed_tim || is_mc) {
+		if (directed_tim) {
 			local->hw.conf.flags &= ~IEEE80211_CONF_PS;
 			ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
 			ieee80211_send_nullfunc(local, sdata, 0);

Re: [RFC PATCH v2 1/2] mac80211: remove multicast check from check_tim()

[Vivek Natarajan]

On Thu, Jan 22, 2009 at 5:15 PM, Kalle Valo <kalle.valo@nokia.com> wrote:
> Currently mac80211 checks for the multicast tim bit from beacons,
> disables power save and sends a null frame if the bit is set. This was
> added to support ath9k. But this is a bit controversial because the AP will
> send multicast frames immediately after the beacon and the time constraints
> are really high. Relying mac80211 to be fast enough here might not be
> reliable in all situations.

I agree that mac80211 is not fast enough to disable power save when multicast
bit is set. But a quick testing with multicast traffic with power save
enabled shows
me that the traffic is passing without much failures(97% passes
through and this is
not a small number considering my high traffic/noisy channel
conditions). So, the reason
 is not strong enough or the mac80211 is not too slow to respond to a
mc traffic in
practical conditions and hence the check need not be removed.

> And there's no need to send a null frame, AP
> will send the frames immediately after the dtim beacon no matter what.

You are right. In that case, if mc bit is set, we can change the code
to just disable ps
and not to send a null frame.
But this pops up another issue! How to enable ps after waking up for mc traffic?
For this we have got to have the dynamic_ps_timer called from the rx path also.
This enables ps after the specified timeout. I don't see any good reason for not
to have this timer in Rx path also. Thoughts?

Thanks
Vivek.

Re: [RFC PATCH v2 1/2] mac80211: remove multicast check from check_tim()

[Kalle Valo]

Vivek Natarajan <vivek.natraj@gmail.com> writes:

> On Thu, Jan 22, 2009 at 5:15 PM, Kalle Valo <kalle.valo@nokia.com>
> wrote:
>> Currently mac80211 checks for the multicast tim bit from beacons,
>> disables power save and sends a null frame if the bit is set. This
>> was added to support ath9k. But this is a bit controversial because
>> the AP will send multicast frames immediately after the beacon and
>> the time constraints are really high. Relying mac80211 to be fast
>> enough here might not be reliable in all situations.
>
> I agree that mac80211 is not fast enough to disable power save when
> multicast bit is set. But a quick testing with multicast traffic with
> power save enabled shows me that the traffic is passing without much
> failures(97% passes through and this is not a small number considering
> my high traffic/noisy channel conditions). 

I'm still not convinced. Even if it works for your setup, somebody
else might have problems.

> So, the reason is not strong enough or the mac80211 is not too slow
> to respond to a mc traffic in practical conditions and hence the
> check need not be removed.

Well, we have to do something because stlc45xx/p54spi doesn't need
this. Either we need to do add a yet another hw flag or move the
functionality to ath9k. And I don't see the benefit of adding a new hw
flag just for one driver. If there are more hardware designs requiring
this, then adding support to mac80211 might make sense.

There's also a third option: do this in hardware. Are you really sure
that your hardware doesn't support waking up for broadcast/multicast
frames? It would only need to check two bits, multicast tim bit from
beacons and moredata bits from received broadcast/multicast frames.

>> And there's no need to send a null frame, AP will send the frames
>> immediately after the dtim beacon no matter what.
>
> You are right. In that case, if mc bit is set, we can change the code
> to just disable ps and not to send a null frame.

Like I said earlier, I don't like this approach. 

> But this pops up another issue! How to enable ps after waking up for
> mc traffic?

Just go back to sleep after receiving a broadcast/multicast frame with
moredata bit not set.

> For this we have got to have the dynamic_ps_timer called from the rx
> path also. This enables ps after the specified timeout. I don't see
> any good reason for not to have this timer in Rx path also.

Please take into account that dynamic_ps_timeout can be zero, you
cannot rely on dynamic_ps_timer.

Having dynamic timeout in rx path also is possible, but that's a
separate issue from multicast/broadcast problem. I haven't considered
it myself, though.

-- 
Kalle Valo