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

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

[Kalle Valo]

Here's my suggestion for how to implement ps-poll in mac80211. Also
this fixes the case when dynamic_ps_timeout is zero.

I decided to use ps-polling when dynamic_ps_timeout is zero. If
userspace sets timeout to zero, it means that it's ready to sacrifice
throughput over power consumption. But in case there throughput is
more, userspace can set timeout to a non-zero value and null frame
wakeup is used instead, throughput is higher but power consumption
also increases.

Most probably this patchset breaks ath9k multicast handling. I
recommend ath9k to try do multicast tim bit handling in hardware, I
would really assume that to be possible. But if that's really
impossible, then it can be done in the driver. But having support for
waking up multicast tim bits in mac80211 is unreliable and complicated
the implementation, I just recommend dropping it.

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.

v3:

o remove another leftover debug printk

v2:

o RX_CONTINUE after sending ps-poll frame in ieee80211_rx_h_check_more_data()
o remove leftover debug printk
o use ps-poll only when dynamic_ps_timeout is zero, otherwise null frame
  wakeup is used

---

Kalle Valo (2):
      mac80211: use ps-poll when dynamic power save mode is disabled
      mac80211: remove multicast check from check_tim()


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

[PATCH v3 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 9d51e27..1bf72e6 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -611,7 +611,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;
@@ -621,9 +621,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;
 
@@ -1815,7 +1812,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 */
@@ -1843,9 +1840,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);

[PATCH v3 2/2] mac80211: use ps-poll when dynamic power save mode is disabled

[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 ever gain. 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.

Using ps-poll is slower than waking up with null frame, but it's saves more
power in cases where the traffic is low. Userspace can control if either
ps-poll or null wakeup method is used by enabling and disabling dynamic
power save.

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

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

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index eaf3603..1f5d162 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -728,6 +728,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;
@@ -921,6 +922,8 @@ u32 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 1bf72e6..0399afc 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -511,6 +511,39 @@ 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 | IEEE80211_FCTL_PM;
+	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);
+
+	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)
@@ -1843,9 +1876,24 @@ 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);
+			if (local->hw.conf.dynamic_ps_timeout > 0) {
+				local->hw.conf.flags &= ~IEEE80211_CONF_PS;
+				ieee80211_hw_config(local,
+						    IEEE80211_CONF_CHANGE_PS);
+				ieee80211_send_nullfunc(local, sdata, 0);
+			} else {
+				local->pspolling = true;
+
+				/*
+				 * Here is assumed that the driver will be
+				 * able to send ps-poll frame and receive a
+				 * response even though power save mode is
+				 * enabled, but some drivers might require
+				 * to disable power save here. This needs
+				 * to be investigated.
+				 */
+				ieee80211_send_pspoll(local, sdata);
+			}
 		}
 	}
 
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 19ffc8e..72a6d7b 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;
@@ -1996,6 +2029,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)

Re: [PATCH v3 2/2] mac80211: use ps-poll when dynamic power save mode is disabled

[Vivek Natarajan]

On Wed, Jan 28, 2009 at 10:33 PM, Kalle Valo <kalle.valo@nokia.com> wrote:

> +                                * Here is assumed that the driver will be
> +                                * able to send ps-poll frame and receive a
> +                                * response even though power save mode is
> +                                * enabled, but some drivers might require
> +                                * to disable power save here. This needs
> +                                * to be investigated.

ath9k too needs power save to be disabled to send or receive any frame. Same as
broadcom. I hope it is the same for many others too. I'm curious about
the design of
stlc45xx/p54 since it is capable of sending/receiving even though power save is
enabled. I wonder what blocks of the hw are off when ps is enabled
since it is capable
of doing the basic operation of Tx/Rx which is the only thing expected
from a driver.
I seriously doubt if NIC is saving any power at all or the hw design
is very novel and
interesting.

Thanks,
Vivek.

Re: [PATCH v3 2/2] mac80211: use ps-poll when dynamic power save mode is disabled

[Kalle Valo]

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

> On Wed, Jan 28, 2009 at 10:33 PM, Kalle Valo <kalle.valo@nokia.com>
> wrote:
>
>> +                                * Here is assumed that the driver
>> + will be * able to send ps-poll frame and receive a * response even
>> + though power save mode is * enabled, but some drivers might
>> + require * to disable power save here. This needs * to be
>> + investigated.
>
> ath9k too needs power save to be disabled to send or receive any
> frame. Same as broadcom. I hope it is the same for many others too.
> I'm curious about the design of stlc45xx/p54 since it is capable of
> sending/receiving even though power save is enabled. I wonder what
> blocks of the hw are off when ps is enabled since it is capable of
> doing the basic operation of Tx/Rx which is the only thing expected
> from a driver. I seriously doubt if NIC is saving any power at all or
> the hw design is very novel and interesting.

Trust me, stlc4560 is very capable of saving power. For example, I
have managed to get over 7 days of association time with nokia n810
and the proprietary umac solution (display off, no traffic, openwrt AP
at home, occasionally pinging from the network to check that the
device is still connected).

I'm not familiar with the internals of stlc4560, but I can tell you
what I know from while developing a driver for the chipset.

The firmware has a special command to enable power save (LM_OID_PSM).
When the driver sends the command followed by a null frame, the
firmware will go to a power save mode. In this power save mode the
firmware will wake up automatically for beacons (interval is
configurable) and listens for multicast frames after DTIM beacons. It
also seems to stay awake long enough to receive the data frame
following a transmitted ps-poll, but I'm not sure how that logic works
yet.

In addition to the power save command, the firmware interface has a
separate interrupt for waking up firmware (SPI_TARGET_INT_WAKEUP) and
allowing it to sleep again (SPI_TARGET_INT_SLEEP). Whenever driver
wants to upload a frame for transmission, retrieve a received frame
from the buffer or send any command, it must first wakeup the firmware.
And after the driver knows that there isn't anything to do, it must
allow the firmware to sleep again.

When the firmware is sleeping and radio is turned off, the power
consumption is next to minimal. I don't know the exact figures,
though.

My guess is that the firmware awake command is independent from the
radio. For example, if the driver has enabled power save command and
later issued a firmware wakeup command but not followed with a sleep
command, the radio will be turned off between the beacons while the
firmware is still awake. But this is just a guess, I haven't tested
this.

This type of distinction between firmware wakeup and power save isn't
special. I have seen other hardware designs having similar methods.

After reading comments from you and Johannes I'm getting the feeling
that atheros and broadcom have somehow combined the power save and
firmware wakup commands into one. So maybe we should separate this is
in the mac80211 as well? I don't know, I need to think more about
this.

It's getting late for me and I'm sure I missed something, but
hopefully you now got better understanding. And please do ask if
there's anything unclear.

-- 
Kalle Valo