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

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        |   69 +++++++++++++++++++++++++++++++++++++-------
 net/mac80211/rx.c          |   34 ++++++++++++++++++++++
 3 files changed, 95 insertions(+), 11 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,
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

[RFC PATCH v2 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        |   56 ++++++++++++++++++++++++++++++++++++++++++--
 net/mac80211/rx.c          |   34 +++++++++++++++++++++++++++
 3 files changed, 90 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..26b3ed4 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 | 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);
+
+	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,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 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)

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

[Johannes Berg]

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

On Thu, 2009-01-22 at 13:45 +0200, Kalle Valo wrote:

> +	printk(KERN_DEBUG "sending ps-poll");

remove one, add another? Or forgot quilt refresh? :)

If we receive a frame, but don't send any, we'll still stay awake.
Should receiving update the dynamic timer as well, to avoid that case?
Here, I mean:

> @@ -1816,9 +1851,24 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
...
> +                       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);

johannes

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

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

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2009-01-22 at 13:45 +0200, Kalle Valo wrote:
>
>> +	printk(KERN_DEBUG "sending ps-poll");
>
> remove one, add another? Or forgot quilt refresh? :)

I have no idea what happened :) I'll fix that.

> If we receive a frame, but don't send any, we'll still stay awake.
> Should receiving update the dynamic timer as well, to avoid that case?
> Here, I mean:
>
>> @@ -1816,9 +1851,24 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
> ...
>> +                       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);

You mean case where directed_time is set, dynamic_ps_timeout is zero
and station doesn't transmit anything? That's not a problem because in
that case power save is not disabled at all, ps-poll is sent while
power save mode is enabled. So there won't any problem.

Like I said last week, I need to revisit this case later to get proper
support for other hw designs, eg. broadcom.

-- 
Kalle Valo

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

[Johannes Berg]

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

On Wed, 2009-01-28 at 18:49 +0200, Kalle Valo wrote:

> > If we receive a frame, but don't send any, we'll still stay awake.
> > Should receiving update the dynamic timer as well, to avoid that case?
> > Here, I mean:
> >
> >> @@ -1816,9 +1851,24 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
> > ...
> >> +                       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);
> 
> You mean case where directed_time is set, dynamic_ps_timeout is zero
> and station doesn't transmit anything? That's not a problem because in
> that case power save is not disabled at all, ps-poll is sent while
> power save mode is enabled. So there won't any problem.

No, I mean in the timeout > 0 case, what vivek mentioned too. We never
go back to sleep unless we send a packet.

johannes

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

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

[Kalle Valo]

Johannes Berg wrote:
> On Wed, 2009-01-28 at 18:49 +0200, Kalle Valo wrote:
> 
>>> If we receive a frame, but don't send any, we'll still stay awake.
>>> Should receiving update the dynamic timer as well, to avoid that case?
>>> Here, I mean:
>>>
>>>> @@ -1816,9 +1851,24 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
>>> ...
>>>> +                       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);
>> You mean case where directed_time is set, dynamic_ps_timeout is zero
>> and station doesn't transmit anything? That's not a problem because in
>> that case power save is not disabled at all, ps-poll is sent while
>> power save mode is enabled. So there won't any problem.
> 
> No, I mean in the timeout > 0 case, what vivek mentioned too. We never
> go back to sleep unless we send a packet.

First of all, this patchset doesn't change the logic for timeout > 0 
case. But that case was working already earlier, here's my analysis:

1. tim bit is set
2. if timeout > 0 disable power save and send null frame
3. null frame enables dynamic_ps_timer in ieee80211_master_start_xmit()
4. after dynamic_ps_timer triggers, power save is enabled again

I understood that this was the reason why Vivek moved the running of 
dynamic_ps_timer from subif to the master interface. Or did I miss 
something?

Kalle

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

[Johannes Berg]

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

On Thu, 2009-01-29 at 20:15 +0200, Kalle Valo wrote:

> > No, I mean in the timeout > 0 case, what vivek mentioned too. We never
> > go back to sleep unless we send a packet.
> 
> First of all, this patchset doesn't change the logic for timeout > 0 
> case. But that case was working already earlier, here's my analysis:
> 
> 1. tim bit is set
> 2. if timeout > 0 disable power save and send null frame
> 3. null frame enables dynamic_ps_timer in ieee80211_master_start_xmit()
> 4. after dynamic_ps_timer triggers, power save is enabled again
> 
> I understood that this was the reason why Vivek moved the running of 
> dynamic_ps_timer from subif to the master interface. Or did I miss 
> something?

Hah, no, you're right, sorry, I got confused.

johannes

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

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

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2009-01-29 at 20:15 +0200, Kalle Valo wrote:
>
>> > No, I mean in the timeout > 0 case, what vivek mentioned too. We never
>> > go back to sleep unless we send a packet.
>> 
>> First of all, this patchset doesn't change the logic for timeout > 0 
>> case. But that case was working already earlier, here's my analysis:
>> 
>> 1. tim bit is set
>> 2. if timeout > 0 disable power save and send null frame
>> 3. null frame enables dynamic_ps_timer in ieee80211_master_start_xmit()
>> 4. after dynamic_ps_timer triggers, power save is enabled again
>> 
>> I understood that this was the reason why Vivek moved the running of 
>> dynamic_ps_timer from subif to the master interface. Or did I miss 
>> something?
>
> Hah, no, you're right, sorry, I got confused.

It confused me also. I would prefer to have the dynamic_ps_timer stuff
in subif and have an explicit timer for the null frames, just to avoid
confusion. I'll try to see if I can come up with a patch later on.

-- 
Kalle Valo

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

[Tomas Winkler]

On Thu, Jan 22, 2009 at 1:45 PM, Kalle Valo <kalle.valo@nokia.com> wrote:
> 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.

Why not?

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.

Still have to see the implementation but this sounds strange.

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

Do you have numbers how much power you save?. With PS-Poll you must
stay awak longer not saying that issueing TX (PS-poll) also consume
power.Ususally TXing has higher power consumption then RX.Instead of
tx(null PM=0) rx tx(ack)...rx tx(ack) tx(null PM=1) you have now
tx(ps-poll) rx tx (ack) tx(ps-poll) rx tx(ack).  So if the AP holds
more then 3 packetes PS poll will consume more power in this very
simplified computation.

Do you measure power consumption on of the NIC or the whole system?

Thanks
Tomas


>
> Signed-off-by: Kalle Valo <kalle.valo@nokia.com>
> ---
>
>  net/mac80211/ieee80211_i.h |    3 ++
>  net/mac80211/mlme.c        |   56 ++++++++++++++++++++++++++++++++++++++++++--
>  net/mac80211/rx.c          |   34 +++++++++++++++++++++++++++
>  3 files changed, 90 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..26b3ed4 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 | 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);
> +
> +       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,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 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)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" 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 2/2] mac80211: use ps-poll when dynamic power save mode is disabled

[Kalle Valo]

Tomas Winkler <tomasw@gmail.com> writes:

> On Thu, Jan 22, 2009 at 1:45 PM, Kalle Valo <kalle.valo@nokia.com> wrote:
>> 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.
>
> Why not?

Because of a bug.

>> 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.
>
> Still have to see the implementation but this sounds strange.

Please be more specific, I don't understand what part you think is
strange. This is specified in the spec, see chapter 11.2.

>> 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.
>
> Do you have numbers how much power you save?

Unfortunately not yet.

> With PS-Poll you must stay awak longer not saying that issueing TX
> (PS-poll) also consume power.Ususally TXing has higher power
> consumption then RX. Instead of tx(null PM=0) rx tx(ack)...rx
> tx(ack) tx(null PM=1) you have now tx(ps-poll) rx tx (ack)
> tx(ps-poll) rx tx(ack). So if the AP holds more then 3 packetes PS
> poll will consume more power in this very simplified computation.

Let's take the null frame approach:

1. notice tim bit set
2. send null frame (pm=0)
3. receive data frames
4. wait for all buffered data frames
5. send null frame (pm=1)

How do you know when the station has received all buffered data frames
from the AP (item 4 above)? By waiting a certain period after the last
received data frame? If that timeout is too short, we will get packet
loss, which is not good. If the timeout is too long, we waste too much
power.

With ps-poll we don't have to set any timeouts, the station can go to
sleep immeadiately after receiving a data frame with moredata bit not
set. So I see a clear advantage in certain cases where the data
troughput is very low and maximum power savings is desired. For
example when the device is idle on the table, display turned off and
only irc/jabber session is running in the background.

And remember that userspace can control this with the power timeout
setting. My recommendation would be to use this only when the device
is idle and display is turned off.

> Do you measure power consumption on of the NIC or the whole system?

Usually I measure just the whole system. I can measure the nic, but
that's very time consuming.

-- 
Kalle Valo

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

[Tomas Winkler]

On Wed, Jan 28, 2009 at 10:39 AM, Kalle Valo <kalle.valo@iki.fi> wrote:
> Tomas Winkler <tomasw@gmail.com> writes:
>
>> On Thu, Jan 22, 2009 at 1:45 PM, Kalle Valo <kalle.valo@nokia.com> wrote:
>>> 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.
>>
>> Why not?
>
> Because of a bug.

So wouldn't be better to fix the bug.

>
>>> 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.
>>
>> Still have to see the implementation but this sounds strange.
>
> Please be more specific, I don't understand what part you think is
> strange. This is specified in the spec, see chapter 11.2.
>
Strange you've decided implementing another feature instead of fixing
a bug. Not that I'm against existence of PS-poll implementation in
mac80211 it just should not be in order to fix a bug.


>>> 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.
>>
>> Do you have numbers how much power you save?
>
> Unfortunately not yet.
>
>> With PS-Poll you must stay awak longer not saying that issueing TX
>> (PS-poll) also consume power.Ususally TXing has higher power
>> consumption then RX. Instead of tx(null PM=0) rx tx(ack)...rx
>> tx(ack) tx(null PM=1) you have now tx(ps-poll) rx tx (ack)
>> tx(ps-poll) rx tx(ack). So if the AP holds more then 3 packetes PS
>> poll will consume more power in this very simplified computation.
>
> Let's take the null frame approach:
>
> 1. notice tim bit set
> 2. send null frame (pm=0)
> 3. receive data frames
> 4. wait for all buffered data frames
> 5. send null frame (pm=1)
>
> How do you know when the station has received all buffered data frames
> from the AP (item 4 above)? By waiting a certain period after the last
> received data frame? If that timeout is too short, we will get packet
> loss, which is not good. If the timeout is too long, we waste too much
> power.

You can manage the idle timeout according the power save
aggressiveness you want to achieve. There is no free lunch
as result of power save you get some packet loss and jitter. Packet
loss and jitter are natural to wifi also in normal operation. Last
packet loss is not really an issue it's guarded by 802.11
retransmission mechanism and by higher protocol.


> With ps-poll we don't have to set any timeouts, the station can go to
> sleep immeadiately after receiving a data frame with moredata bit not
> set. So I see a clear advantage in certain cases where the data
> troughput is very low and maximum power savings is desired. For
> example when the device is idle on the table, display turned off and
> only irc/jabber session is running in the background.

You certainly cannot predict the RX activity especially when you have
irc running in background.

>
> And remember that userspace can control this with the power timeout
> setting. My recommendation would be to use this only when the device
> is idle and display is turned off.

I guess that environment is more dynamic then user space can react
upon and I thing that null packet method is more
flexible then PS-poll but I also doesn't  have numbers to support my view

Now maybe I'm a bit biased towards null frame approach because what we
are doing in iwlwifi with and I know there was a lot of data collected
in different environments to tune the numbers deep in HW and firmware
but still this is was only on laptops usage in corporate or
coffee/airport environments.
There might be a case that for handhelds and different HW PS-poll
might be preferable.  My point is that you cannot find universally
said what is correct and there shell be configuration possibility from
HW, user space, environment perspective.

Frankly I don't like much mapping power save aggressiveness to timeout
number because there so much more parameters that governs it that is
says nothing.

I would suggest to expose to user space non-unit index of power save
aggressiveness which can be mapped to different methods such as
PS-Poll, dynamic power save or iwlwifi firmware implementation by
other more reach configurable interface


>
>> Do you measure power consumption on of the NIC or the whole system?
>
> Usually I measure just the whole system. I can measure the nic, but
> that's very time consuming.

This can be important in tuning since you cannot distinguish between
wasting of power by some wakening up timer or thread in the driver or
by the fact that NIC itself doesn't enter power save state.

Thanks
Tomas

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

[Kalle Valo]

Tomas Winkler <tomasw@gmail.com> writes:

> On Wed, Jan 28, 2009 at 10:39 AM, Kalle Valo <kalle.valo@iki.fi> wrote:
>> Tomas Winkler <tomasw@gmail.com> writes:
>>
>>> On Thu, Jan 22, 2009 at 1:45 PM, Kalle Valo <kalle.valo@nokia.com> wrote:
>>>> 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.
>>>
>>> Why not?
>>
>> Because of a bug.
>
> So wouldn't be better to fix the bug.

I was planning to implement ps-poll feature anyway and concluded that
it's good idea to do it now that I can fix the bug at the same time.

>>>> 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.
>>>
>>> Still have to see the implementation but this sounds strange.
>>
>> Please be more specific, I don't understand what part you think is
>> strange. This is specified in the spec, see chapter 11.2.
>>
> Strange you've decided implementing another feature instead of fixing
> a bug. Not that I'm against existence of PS-poll implementation in
> mac80211 it just should not be in order to fix a bug.

It was just convenient for, nothing else. If this is very important, I
can add a new bug which fixes the bug first.

>> Let's take the null frame approach:
>>
>> 1. notice tim bit set
>> 2. send null frame (pm=0)
>> 3. receive data frames
>> 4. wait for all buffered data frames
>> 5. send null frame (pm=1)
>>
>> How do you know when the station has received all buffered data frames
>> from the AP (item 4 above)? By waiting a certain period after the last
>> received data frame? If that timeout is too short, we will get packet
>> loss, which is not good. If the timeout is too long, we waste too much
>> power.
>
> You can manage the idle timeout according the power save
> aggressiveness you want to achieve. There is no free lunch
> as result of power save you get some packet loss and jitter. Packet
> loss and jitter are natural to wifi also in normal operation.

Exactly what I was thinking. We seem to agree quite a lot :)

> Last packet loss is not really an issue it's guarded by 802.11
> retransmission mechanism and by higher protocol.

Yes, but counting on upper level retransmission has a bad vibe. And
upper level retransmission means more 802.11 frames, which means
increased power consumption. My assumption is that we will need
ps-poll feature in mac80211 in certain cases. It just needs to be
dynamically enabled, most probably userspace is the best place to make
this decision.

>> With ps-poll we don't have to set any timeouts, the station can go to
>> sleep immeadiately after receiving a data frame with moredata bit not
>> set. So I see a clear advantage in certain cases where the data
>> troughput is very low and maximum power savings is desired. For
>> example when the device is idle on the table, display turned off and
>> only irc/jabber session is running in the background.
>
> You certainly cannot predict the RX activity especially when you have
> irc running in background.

Of course not, nobody can. We just have to make good guesses.

>> And remember that userspace can control this with the power timeout
>> setting. My recommendation would be to use this only when the device
>> is idle and display is turned off.
>
> I guess that environment is more dynamic then user space can react
> upon and I thing that null packet method is more
> flexible then PS-poll but I also doesn't  have numbers to support my view
>
> Now maybe I'm a bit biased towards null frame approach because what we
> are doing in iwlwifi with and I know there was a lot of data collected
> in different environments to tune the numbers deep in HW and firmware
> but still this is was only on laptops usage in corporate or
> coffee/airport environments.
> There might be a case that for handhelds and different HW PS-poll
> might be preferable.  My point is that you cannot find universally
> said what is correct and there shell be configuration possibility from
> HW, user space, environment perspective.

If we find that the null frame wakeup is a better solution we can
remove ps-poll feature from mac80211, that's easy.

> Frankly I don't like much mapping power save aggressiveness to timeout
> number because there so much more parameters that governs it that is
> says nothing.

We have to start somewhere, this is definitely not the final solution.
We should add proper power save configuration to nl80211, but I think
we don't yet have enough understanding to do that. Better to use the
existing interfaces for now and try to get at least minimal power save
support to the drivers.

> I would suggest to expose to user space non-unit index of power save
> aggressiveness which can be mapped to different methods such as
> PS-Poll, dynamic power save or iwlwifi firmware implementation by
> other more reach configurable interface

That's something which we need to think about.

>>> Do you measure power consumption on of the NIC or the whole system?
>>
>> Usually I measure just the whole system. I can measure the nic, but
>> that's very time consuming.
>
> This can be important in tuning since you cannot distinguish between
> wasting of power by some wakening up timer or thread in the driver or
> by the fact that NIC itself doesn't enter power save state.

In N800/N810 the radio consumes so much power compared to the cpu that
I can spot the differences. But you are right, of course. It is good
to measure the consumption of the wi-fi chip itself also.

-- 
Kalle Valo

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

[Johannes Berg]

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

Just for the record, and I was still planning to reply to your other
mail, not trying to hijack this thread at all.

> I would suggest to expose to user space non-unit index of power save
> aggressiveness which can be mapped to different methods such as
> PS-Poll, dynamic power save or iwlwifi firmware implementation by
> other more reach configurable interface

I **completely** disagree on this. The non-unit index is a **horrible**
thing because it says **nothing** of interest to the user. The only
approach the user can then take is try which value will give him the
desired behaviour, and it will differ across all hardware combinations.
Therefore, it's completely _useless_.

What we really need to do is tie into the pm_qos framework and make
latency guarantees.

johannes

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

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

[Tomas Winkler]

On Wed, Jan 28, 2009 at 3:00 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> Just for the record, and I was still planning to reply to your other
> mail, not trying to hijack this thread at all.
>
>> I would suggest to expose to user space non-unit index of power save
>> aggressiveness which can be mapped to different methods such as
>> PS-Poll, dynamic power save or iwlwifi firmware implementation by
>> other more reach configurable interface
>
> I **completely** disagree on this. The non-unit index is a **horrible**
> thing because it says **nothing** of interest to the user. The only
> approach the user can then take is try which value will give him the
> desired behaviour, and it will differ across all hardware combinations.
> Therefore, it's completely _useless_.

What I suggest is to have rich configuration for setting different
parameters so this is more flexible with different HW but in run time
only play with single number to create uniform interface for user
space.
I'm not sure that you want to user jungle with all the numbers, we run
empirical tests for very long time how do you want to user space take
the decision.

> What we really need to do is tie into the pm_qos framework and make
> latency guarantees.

Even pm_qos doesn't talk directly in language of all  PM paramters you
need to configure so this is just equivalent to my proposition and can
be staged after the legacy PM is implemented. Also PM doesn't map only
to traffic requirements.

Tomas

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

[Johannes Berg]

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

On Wed, 2009-01-28 at 15:26 +0200, Tomas Winkler wrote:

> > I **completely** disagree on this. The non-unit index is a **horrible**
> > thing because it says **nothing** of interest to the user. The only
> > approach the user can then take is try which value will give him the
> > desired behaviour, and it will differ across all hardware combinations.
> > Therefore, it's completely _useless_.
> 
> What I suggest is to have rich configuration for setting different
> parameters so this is more flexible with different HW but in run time
> only play with single number to create uniform interface for user
> space.

Which is completely pointless, as I'm saying, because the user has no
idea what to base the decision on. This might be nice for a developer to
play with this, but for a user it's completely useless because they
cannot base the decision on any information.

Basically, you're saying: Let us vary the blackbox parameters for you,
and we'll give you a 0-5 scale on which to select. Except that the user
has no idea what 0-5 mean in terms of what he's trying to do.

That just means we'll end up with howtos like this:
"If you have Intel hardware, and want to use VOIP, then you need to do
iwconfig wlan0 power index 2, because otherwise the latency is
unacceptable (if you have bad hearing 3 might be acceptable); if you're
just running ICQ then 5 is fine."

Can you see what I'm trying to say?

> I'm not sure that you want to user jungle with all the numbers, we run
> empirical tests for very long time how do you want to user space take
> the decision.

No, we definitely do NOT want userspace to juggle any of the numbers.
But that's completely orthogonal. We want userspace to tell us what it
needs, and adjust all the possible parameters based on that. We
definitely do not want userspace to give us an arbitrary number it
obtained by rolling a dice.

> > What we really need to do is tie into the pm_qos framework and make
> > latency guarantees.
> 
> Even pm_qos doesn't talk directly in language of all  PM paramters you
> need to configure so this is just equivalent to my proposition and can
> be staged after the legacy PM is implemented. Also PM doesn't map only
> to traffic requirements.

No, you're thinking too technical if you say this is equivalent to your
proposition. Yes, it is similar in that userspace gives us one or two
values and we select a number of parameters based on these values. But
it is _completely_ different in that userspace actually has a good idea
what the parameters it can tell us _do_. Unlike an arbitrary power index
it can only select by rolling a dice and hoping it'll work.

johannes

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

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

[Johannes Berg]

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

On Thu, 2009-01-22 at 13:45 +0200, Kalle Valo wrote:
> Here's my suggestion for how to implement ps-poll in mac80211. Also
> this fixes the case when dynamic_ps_timeout is zero.

Looks good to me.

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

Not a bad choice.

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

I agree, we should push this as close to the device as possible.
mac80211 has to defer to a workqueue which adds latency, while if this
really needs to be done in software the driver can do it much faster
because it doesn't necessarily have any generic workqueue requirement
and can possibly even wake the hardware in irq context.

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

I just checked, Broadcom's hardware/firmware definitely requires doing
this in software.

johannes

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

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

[Kalle Valo]

"Johannes Berg" <johannes@sipsolutions.net> writes:

>> 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.
>
> I just checked, Broadcom's hardware/firmware definitely requires doing
> this in software.

Ok. The problem is that stlc45xx/p54spi requires us to send a null
frame every time we enable power save and that's causing problems, at
least that's my current theory. So I need to solve this somehow, I
need to think this more.

But can we still apply these patches? Because b43 doesn't support
power save at all, and ath9k with dynamic power save disabled was
already broken before my patches, I consider these patches as a step
forward. And I would like to go forward one small step at a time,
maintaining huge patches myself until all issues are fixed is quite
difficult.

-- 
Kalle Valo

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

[Johannes Berg]

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

On Fri, 2009-01-23 at 11:31 +0200, Kalle Valo wrote:

> Ok. The problem is that stlc45xx/p54spi requires us to send a null
> frame every time we enable power save and that's causing problems, at
> least that's my current theory. So I need to solve this somehow, I
> need to think this more.

True.

> But can we still apply these patches? Because b43 doesn't support
> power save at all, and ath9k with dynamic power save disabled was
> already broken before my patches, I consider these patches as a step
> forward. And I would like to go forward one small step at a time,
> maintaining huge patches myself until all issues are fixed is quite
> difficult.

Yes, agree, can you repost without RFC and the printk removed (and
possibly rebased)?

johannes

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

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

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2009-01-23 at 11:31 +0200, Kalle Valo wrote:
>
>> Ok. The problem is that stlc45xx/p54spi requires us to send a null
>> frame every time we enable power save and that's causing problems, at
>> least that's my current theory. So I need to solve this somehow, I
>> need to think this more.
>
> True.
>
>> But can we still apply these patches? Because b43 doesn't support
>> power save at all, and ath9k with dynamic power save disabled was
>> already broken before my patches, I consider these patches as a step
>> forward. And I would like to go forward one small step at a time,
>> maintaining huge patches myself until all issues are fixed is quite
>> difficult.
>
> Yes, agree, can you repost without RFC and the printk removed (and
> possibly rebased)?

Sure thing. I'm rebasing it right now.

-- 
Kalle Valo