Re: [RFC] mac80211: Enhancements to dynamic power save.

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Johannes Berg]

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

On Mon, 2008-12-01 at 17:17 -0800, Vivek Natarajan wrote:
> This patch is based on Kalle's initial RFC patches on dynamic power save.
> Since ath9k/ath5k,stlc45xx and b43 need the driver to send the null
> frame, it is appropriate to do it from mac80211. 
> This patch enables mac80211 to send a null frame and also to
> check for tim in the beacon if power save is enabled.

Nice! Looks pretty much good to me, but could use some documentation
(Kalle will hopefully add some too). For instance, a casual observer
might wonder how the hardware can be saving power when the host software
has to parse beacons -- obviously things will only fall into place once
we support beacon miss offload.

> +static bool check_tim(struct ieee802_11_elems *elems, u16 aid, bool *is_mc)
> +{
> +	u8 mask;
> +	u8 index, indexn1, indexn2;
> +	struct ieee80211_tim_ie *tim = (struct ieee80211_tim_ie *) elems->tim;
> +
> +	aid &= 0x3fff;
> +	index = aid / 8;
> +	mask  = 1 << (aid & 7);
> +
> +	indexn1 = tim->bitmap_ctrl & 0xfe;
> +	indexn2 = elems->tim_len + indexn1 - 4;
> +
> +	if (index < indexn1 || index > indexn2)
> +		return false;
> +
> +	index -= indexn1;
> +
> +	if (tim->bitmap_ctrl & 0x01)
> +		*is_mc = true;

Shouldn't you update *is_mc before the above return false? And also set
it to false in the other case, I think.

> +	return (bool)(tim->virtual_map[index] & mask);

I think
	return !!(... & mask);
would be more appropriate.

> @@ -1734,6 +1759,15 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
>  	ieee80211_sta_wmm_params(local, ifsta, elems.wmm_param,
>  				 elems.wmm_param_len);
>  
> +	directed_tim = check_tim(&elems, ifsta->aid, &is_mc);
> +
> +	if (directed_tim || is_mc) {
> +		if (local->hw.conf.flags && IEEE80211_CONF_PS) {
> +			local->hw.conf.flags &= ~IEEE80211_CONF_PS;
> +			ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
> +			ieee80211_send_nullfunc(local, sdata, 0);
> +		}
> +	}

How will we get back into PS mode after 
 
> @@ -2616,13 +2650,15 @@ void ieee80211_mlme_notify_scan_completed(struct ieee80211_local *local)
>  
>  void ieee80211_ps_enable_work(struct work_struct *work)
>  {
> -	struct ieee80211_local *local = container_of(work,
> +	struct ieee80211_local *local =	container_of(work,
>  						     struct ieee80211_local,
>  						     ps_enable_work);

Huh, what's the change here?

> diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
> index 9e74e9f..254fb4e 100644
> --- a/net/mac80211/wext.c
> +++ b/net/mac80211/wext.c
> @@ -985,12 +985,18 @@ set:
>  	local->powersave = ps;
>  
>  	if (ifsta->flags && IEEE80211_STA_ASSOCIATED) {
> -		if (local->powersave)
> +		if (local->powersave) {
> +			ieee80211_send_nullfunc(local, sdata, 1);
>  			conf->flags |= IEEE80211_CONF_PS;
> -		else
> +			ret = ieee80211_hw_config(local,
> +						  IEEE80211_CONF_CHANGE_PS);
> +			}
> +		else {

The indentation here looks a bit odd, please put the closing brace
before the else

johannes

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

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Johannes Berg]

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

On Mon, 2008-12-01 at 17:17 -0800, Vivek Natarajan wrote:
> This patch is based on Kalle's initial RFC patches on dynamic power save.
> Since ath9k/ath5k,stlc45xx and b43 need the driver to send the null
> frame, it is appropriate to do it from mac80211. 
> This patch enables mac80211 to send a null frame and also to
> check for tim in the beacon if power save is enabled.

One thing I forgot, do we need a special iwlwifi flag to turn this off?
Or can iwlwifi be programmed to let the host handle this (*), which
might be beneficial for interoperability and uniformity purposes?

johannes

(*) it would need to implement a beacon filter that passes up only
modified beacons (where modified doesn't take into account the TIM IE
except for the own AID/bcast bits), and turns off the null function
stuff in firmware

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

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Vivek Natarajan]

 Johannes Berg <johannes@sipsolutions.net> wrote:

>> +     if (index < indexn1 || index > indexn2)
>> +             return false;
>> +
>> +     index -= indexn1;
>> +
>> +     if (tim->bitmap_ctrl & 0x01)
>> +             *is_mc = true;
>
> Shouldn't you update *is_mc before the above return false? And also set
> it to false in the other case, I think.

Yes, you are right.Thanks.

>> +     return (bool)(tim->virtual_map[index] & mask);
>
> I think
>        return !!(... & mask);
> would be more appropriate.

Thanks. I will modify it.

>> @@ -1734,6 +1759,15 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
>>       ieee80211_sta_wmm_params(local, ifsta, elems.wmm_param,
>>                                elems.wmm_param_len);
>>
>> +     directed_tim = check_tim(&elems, ifsta->aid, &is_mc);
>> +
>> +     if (directed_tim || is_mc) {
>> +             if (local->hw.conf.flags && IEEE80211_CONF_PS) {
>> +                     local->hw.conf.flags &= ~IEEE80211_CONF_PS;
>> +                     ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
>> +                     ieee80211_send_nullfunc(local, sdata, 0);
>> +             }
>> +     }
>
> How will we get back into PS mode after

Once we send a null frame with pm bit off and CONF_PS changed,
the tx_idle timer kicks in and we once again move to sleep state
after the timeout. That piece of code is already there in Kalle's patch.

> One thing I forgot, do we need a special iwlwifi flag to turn this off?

Kalle added a flag 'IEEE80211_HW_NO_DYNAMIC_PS' in v3 RFC.
May be, it can be used for iwlwifi to use its own power save and
null frames as it does now instead of using from mac80211.
We can check this flag and then change CONF_PS wherever it is used.

Thanks,
Vivek.

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Kalle Valo]

Vivek Natarajan <vnatarajan@atheros.com> writes:

> This patch is based on Kalle's initial RFC patches on dynamic power save.
> Since ath9k/ath5k,stlc45xx and b43 need the driver to send the null
> frame, it is appropriate to do it from mac80211. 

Yes, I agree. Actually the way I implemented this in stlc45xx was to
use PS-Poll. In future, it would be nice to have it supported as well,
but we can add it later.

> +	directed_tim = check_tim(&elems, ifsta->aid, &is_mc);
> +
> +	if (directed_tim || is_mc) {
> +		if (local->hw.conf.flags && IEEE80211_CONF_PS) {
> +			local->hw.conf.flags &= ~IEEE80211_CONF_PS;
> +			ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
> +			ieee80211_send_nullfunc(local, sdata, 0);
> +		}
> +	}

Maybe I have been working too much with slow embedded hardware, but can
ath5k really wake up for the beacon, send the beacon to the host,
mac80211 read the beacon and then wakeup the hardware to listen for
multicast frames following the beacon? The multicast frames are sent
immeaditely after the dtim beacon, so this should happen really quick.
But most probably I'm just missing something here.

In stlc45xx the firmware listens automatically for the multicast
beacons following dtim beacons and mac80211 doesn't have to do
anything for them.

> -		ret = ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
> +			ret = ieee80211_hw_config(local,
> +						  IEEE80211_CONF_CHANGE_PS);
> +			ieee80211_send_nullfunc(local, sdata, 0);

Maybe we should have a seperate function for enabling and disabling
power save mode. So that we don't forget sending of the null frame.

-- 
Kalle Valo

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Mon, 2008-12-01 at 17:17 -0800, Vivek Natarajan wrote:
>> This patch is based on Kalle's initial RFC patches on dynamic power save.
>> Since ath9k/ath5k,stlc45xx and b43 need the driver to send the null
>> frame, it is appropriate to do it from mac80211. 
>> This patch enables mac80211 to send a null frame and also to
>> check for tim in the beacon if power save is enabled.
>
> Nice! Looks pretty much good to me, but could use some documentation
> (Kalle will hopefully add some too).

I'm adding some documentation in my next round of patches, but we
definitely need more documentation.

> For instance, a casual observer might wonder how the hardware can be
> saving power when the host software has to parse beacons --

Even without beacon filtering and if there is no traffic, we can turn
off the radios for the time between beacons. For example, with beacon
interval 100 ms and dtim 1 the radios might be turned on for only 10
ms per dtim period. So 90% of the time the radios would be turned off
and we would save power. Of course this is very much hardware
specific, but just to give an idea.

> obviously things will only fall into place once we support beacon
> miss offload.

Beacon filtering improves cpu power consumption.

-- 
Kalle Valo

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Kalle Valo]

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

>> One thing I forgot, do we need a special iwlwifi flag to turn this off?

Or a flag to turn it on? Either of them are fine, I guess.

> Kalle added a flag 'IEEE80211_HW_NO_DYNAMIC_PS' in v3 RFC.
> May be, it can be used for iwlwifi to use its own power save and
> null frames as it does now instead of using from mac80211.
> We can check this flag and then change CONF_PS wherever it is used.

I think we should have a separate hw flag for null frames.

-- 
Kalle Valo

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Mon, 2008-12-01 at 17:17 -0800, Vivek Natarajan wrote:
>> This patch is based on Kalle's initial RFC patches on dynamic power save.
>> Since ath9k/ath5k,stlc45xx and b43 need the driver to send the null
>> frame, it is appropriate to do it from mac80211. 
>> This patch enables mac80211 to send a null frame and also to
>> check for tim in the beacon if power save is enabled.
>
> One thing I forgot, do we need a special iwlwifi flag to turn this off?
> Or can iwlwifi be programmed to let the host handle this (*), which
> might be beneficial for interoperability and uniformity purposes?

Having the null frame handling in firmware is faster and we might get
some power savings from that. But is the saving observable, that's an
another question :)

-- 
Kalle Valo

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Tomas Winkler]

On Mon, Dec 1, 2008 at 6:17 PM, Kalle Valo <kalle.valo@nokia.com> wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
>
>> On Mon, 2008-12-01 at 17:17 -0800, Vivek Natarajan wrote:
>>> This patch is based on Kalle's initial RFC patches on dynamic power save.
>>> Since ath9k/ath5k,stlc45xx and b43 need the driver to send the null
>>> frame, it is appropriate to do it from mac80211.
>>> This patch enables mac80211 to send a null frame and also to
>>> check for tim in the beacon if power save is enabled.
>>
>> One thing I forgot, do we need a special iwlwifi flag to turn this off?
>> Or can iwlwifi be programmed to let the host handle this (*), which
>> might be beneficial for interoperability and uniformity purposes?
>
> Having the null frame handling in firmware is faster and we might get
> some power savings from that. But is the saving observable, that's an
> another question :)

This patch is not much relevant in actual saving , since this is the
waking up part, you are powering the whole nic up anyway.But sending 2
null packets instead of one doesn't really help power save either.

If you want numbers  you can measure power consumption on power lines
of the chip, simple loop and power probe with scope will do the the
trick.

The firmware handling of power save is more relevant in going to sleep
phase and sustaining where decision about sending the null frame is
faster then from host. The NIC is powered down in stages up to state
where only slow clock is ticking so this is rather time sensitive

Hope this helps

Tomas

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Luis R. Rodriguez]

On Mon, Dec 01, 2008 at 03:53:51AM -0800, Johannes Berg wrote:
> On Mon, 2008-12-01 at 17:17 -0800, Vivek Natarajan wrote:
> > This patch is based on Kalle's initial RFC patches on dynamic power save.
> > Since ath9k/ath5k,stlc45xx and b43 need the driver to send the null
> > frame, it is appropriate to do it from mac80211. 
> > This patch enables mac80211 to send a null frame and also to
> > check for tim in the beacon if power save is enabled.
> 
> Nice! Looks pretty much good to me, but could use some documentation
> (Kalle will hopefully add some too). For instance, a casual observer
> might wonder how the hardware can be saving power when the host software
> has to parse beacons -- obviously things will only fall into place once
> we support beacon miss offload.

BTW does broadcom hw support beacon miss stuff? We need to consider
carefully how we add beacon miss support because if we want to rely on
it *and* also disable say the beacon filter we will be missing out on
DFS channel switch announcements and HT channel badwidth changes from
the AP.

  Luis

[RFC] mac80211: Enhancements to dynamic power save.

[Vivek Natarajan]

This patch is based on Kalle's initial RFC patches on dynamic power save.
Since ath9k/ath5k,stlc45xx and b43 need the driver to send the null
frame, it is appropriate to do it from mac80211. 
This patch enables mac80211 to send a null frame and also to
check for tim in the beacon if power save is enabled.

Signed-off-by: Vivek Natarajan <vnatarajan@atheros.com>
---
 net/mac80211/ieee80211_i.h |    3 +++
 net/mac80211/mlme.c        |   40 ++++++++++++++++++++++++++++++++++++++--
 net/mac80211/scan.c        |    2 +-
 net/mac80211/wext.c        |   14 ++++++++++----
 4 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index ac4779a..d9e3169 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -979,6 +979,9 @@ u64 ieee80211_mandatory_rates(struct ieee80211_local *local,
 void ieee80211_ps_enable_work(struct work_struct *work);
 void ieee80211_ps_disable_work(struct work_struct *work);
 void ieee80211_dynamic_ps_timer(unsigned long data);
+void ieee80211_send_nullfunc(struct ieee80211_local *local,
+			     struct ieee80211_sub_if_data *sdata,
+			     int powersave);
 
 #ifdef CONFIG_MAC80211_NOINLINE
 #define debug_noinline noinline
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 437a180..dbc1577 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -568,6 +568,30 @@ static void ieee80211_sta_wmm_params(struct ieee80211_local *local,
 	}
 }
 
+static bool check_tim(struct ieee802_11_elems *elems, u16 aid, bool *is_mc)
+{
+	u8 mask;
+	u8 index, indexn1, indexn2;
+	struct ieee80211_tim_ie *tim = (struct ieee80211_tim_ie *) elems->tim;
+
+	aid &= 0x3fff;
+	index = aid / 8;
+	mask  = 1 << (aid & 7);
+
+	indexn1 = tim->bitmap_ctrl & 0xfe;
+	indexn2 = elems->tim_len + indexn1 - 4;
+
+	if (index < indexn1 || index > indexn2)
+		return false;
+
+	index -= indexn1;
+
+	if (tim->bitmap_ctrl & 0x01)
+		*is_mc = true;
+
+	return (bool)(tim->virtual_map[index] & mask);
+}
+
 static u32 ieee80211_handle_bss_capability(struct ieee80211_sub_if_data *sdata,
 					   u16 capab, bool erp_valid, u8 erp)
 {
@@ -752,6 +776,7 @@ static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
 				  msecs_to_jiffies(local->dynamic_ps_timeout));
 		else {
 			conf->flags |= IEEE80211_CONF_PS;
+			ieee80211_send_nullfunc(local, sdata, 1);
 			ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
 	}
 	}
@@ -1711,7 +1736,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;
+	bool erp_valid, directed_tim, is_mc = false;
 	u8 erp_value = 0;
 
 	/* Process beacon from the current BSS */
@@ -1734,6 +1759,15 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
 	ieee80211_sta_wmm_params(local, ifsta, elems.wmm_param,
 				 elems.wmm_param_len);
 
+	directed_tim = check_tim(&elems, ifsta->aid, &is_mc);
+
+	if (directed_tim || is_mc) {
+		if (local->hw.conf.flags && IEEE80211_CONF_PS) {
+			local->hw.conf.flags &= ~IEEE80211_CONF_PS;
+			ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+			ieee80211_send_nullfunc(local, sdata, 0);
+		}
+	}
 
 	if (elems.erp_info && elems.erp_info_len >= 1) {
 		erp_valid = true;
@@ -2616,13 +2650,15 @@ void ieee80211_mlme_notify_scan_completed(struct ieee80211_local *local)
 
 void ieee80211_ps_enable_work(struct work_struct *work)
 {
-	struct ieee80211_local *local = container_of(work,
+	struct ieee80211_local *local =	container_of(work,
 						     struct ieee80211_local,
 						     ps_enable_work);
+	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
 
 	if (local->hw.conf.flags && IEEE80211_CONF_PS)
 		return;
 
+	ieee80211_send_nullfunc(local, sdata, 1);
 	local->hw.conf.flags |= IEEE80211_CONF_PS;
 	ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
 }
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index f5c7c33..a2caeed 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -395,7 +395,7 @@ ieee80211_scan_rx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
 	return RX_QUEUED;
 }
 
-static void ieee80211_send_nullfunc(struct ieee80211_local *local,
+void ieee80211_send_nullfunc(struct ieee80211_local *local,
 				    struct ieee80211_sub_if_data *sdata,
 				    int powersave)
 {
diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
index 9e74e9f..254fb4e 100644
--- a/net/mac80211/wext.c
+++ b/net/mac80211/wext.c
@@ -985,12 +985,18 @@ set:
 	local->powersave = ps;
 
 	if (ifsta->flags && IEEE80211_STA_ASSOCIATED) {
-		if (local->powersave)
+		if (local->powersave) {
+			ieee80211_send_nullfunc(local, sdata, 1);
 			conf->flags |= IEEE80211_CONF_PS;
-		else
+			ret = ieee80211_hw_config(local,
+						  IEEE80211_CONF_CHANGE_PS);
+			}
+		else {
 			conf->flags &= ~IEEE80211_CONF_PS;
-
-		ret = ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+			ret = ieee80211_hw_config(local,
+						  IEEE80211_CONF_CHANGE_PS);
+			ieee80211_send_nullfunc(local, sdata, 0);
+		}
 	}
 	return ret;
 }
-- 
1.6.0.1

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Vivek Natarajan]

Tomas Winkler <tomasw@gmail.com> wrote:
> On Mon, Dec 1, 2008 at 6:17 PM, Kalle Valo <kalle.valo@nokia.com> wrote:
>> Johannes Berg <johannes@sipsolutions.net> writes:
>>
>>> On Mon, 2008-12-01 at 17:17 -0800, Vivek Natarajan wrote:
>>>> This patch is based on Kalle's initial RFC patches on dynamic power save.
>>>> Since ath9k/ath5k,stlc45xx and b43 need the driver to send the null
>>>> frame, it is appropriate to do it from mac80211.
>>>> This patch enables mac80211 to send a null frame and also to
>>>> check for tim in the beacon if power save is enabled.
>>>
>>> One thing I forgot, do we need a special iwlwifi flag to turn this off?
>>> Or can iwlwifi be programmed to let the host handle this (*), which
>>> might be beneficial for interoperability and uniformity purposes?
>>
>> Having the null frame handling in firmware is faster and we might get
>> some power savings from that. But is the saving observable, that's an
>> another question :)
>
> This patch is not much relevant in actual saving , since this is the
> waking up part, you are powering the whole nic up anyway.

 I agree but with PM bit on-off, this is what one can do to reduce some power
 consumption although not significant. If more significant savings is needed,
 we may need to implement PS-POLL or the APSD mechanism. But this
 is a good start to power save in mac80211.

> The firmware handling of power save is more relevant in going to sleep
> phase and sustaining where decision about sending the null frame is
> faster then from host. The NIC is powered down in stages up to state
> where only slow clock is ticking so this is rather time sensitive

AFAIK, only iwlwifi has this firmware support and we need mac80211's
support in this for ath9k/ath5k,stlc45xx and b43. I believe a powerful
2GHz host processor can manage these timings as a Windows driver
for these vendor cards seems to support power save without firmware support.

--
Vivek.

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Tomas Winkler]

On Tue, Dec 2, 2008 at 2:58 PM, Vivek Natarajan <vivek.natraj@gmail.com> wrote:
> Tomas Winkler <tomasw@gmail.com> wrote:
>> On Mon, Dec 1, 2008 at 6:17 PM, Kalle Valo <kalle.valo@nokia.com> wrote:
>>> Johannes Berg <johannes@sipsolutions.net> writes:
>>>
>>>> On Mon, 2008-12-01 at 17:17 -0800, Vivek Natarajan wrote:
>>>>> This patch is based on Kalle's initial RFC patches on dynamic power save.
>>>>> Since ath9k/ath5k,stlc45xx and b43 need the driver to send the null
>>>>> frame, it is appropriate to do it from mac80211.
>>>>> This patch enables mac80211 to send a null frame and also to
>>>>> check for tim in the beacon if power save is enabled.
>>>>
>>>> One thing I forgot, do we need a special iwlwifi flag to turn this off?
>>>> Or can iwlwifi be programmed to let the host handle this (*), which
>>>> might be beneficial for interoperability and uniformity purposes?
>>>
>>> Having the null frame handling in firmware is faster and we might get
>>> some power savings from that. But is the saving observable, that's an
>>> another question :)
>>
>> This patch is not much relevant in actual saving , since this is the
>> waking up part, you are powering the whole nic up anyway.
>
>  I agree but with PM bit on-off, this is what one can do to reduce some power
>  consumption although not significant. If more significant savings is needed,
>  we may need to implement PS-POLL or the APSD mechanism. But this
>  is a good start to power save in mac80211.

I was more referring to Kalle comment whether more power is saved when
host or firmware sends the null packet,
then to actual implementation of PM.
PM be done through either toggling PM bit or PS-POLL.  I'm not sure
which one is better. In PS-POLL the station has more timing control
but there is additional overhead since for each packet PS-POLL frame
has to be issued.
Simplified explanation to APSD would be this as APSD  is fine grained
PM per AC category,  if I'm not confusing with something else :)

>
>> The firmware handling of power save is more relevant in going to sleep
>> phase and sustaining where decision about sending the null frame is
>> faster then from host. The NIC is powered down in stages up to state
>> where only slow clock is ticking so this is rather time sensitive
>
> AFAIK, only iwlwifi has this firmware support and we need mac80211's
> support in this for ath9k/ath5k,stlc45xx and b43. I believe a powerful
> 2GHz host processor can manage these timings as a Windows driver
> for these vendor cards seems to support power save without firmware support.

I think I think what you meant is that only iwlwifi issues null
packets from the firmware but I believe that all other NICs supports
PM in HW/firmware a.k.a shutting down some parts of the HW.
So to be good also to all NICs null packet shell not be sent for PM
for iwlwifi driver.  In good case it's just redundant in worst it can
screw up the PM state machine. I suggest to add flag that indicates
whether null packet is required or not.


Thanks
Tomas

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Johannes Berg]

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

On Mon, 2008-12-22 at 20:44 -0800, Vivek Natarajan wrote:
> This patch enables mac80211 to send a null frame and also to
> check for tim in the beacon if power save is enabled.
> 
> Signed-off-by: Vivek Natarajan <vnatarajan@atheros.com>

Looks good to me, to be sure, this is all dependent on !NO_STACK_PS,
right?

johannes

> ---
>  net/mac80211/ieee80211_i.h |    3 +++
>  net/mac80211/mlme.c        |   41 ++++++++++++++++++++++++++++++++++++++++-
>  net/mac80211/scan.c        |    2 +-
>  net/mac80211/wext.c        |   13 +++++++++----
>  4 files changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index f3eec98..2038498 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -986,6 +986,9 @@ u64 ieee80211_mandatory_rates(struct ieee80211_local *local,
>  void ieee80211_dynamic_ps_enable_work(struct work_struct *work);
>  void ieee80211_dynamic_ps_disable_work(struct work_struct *work);
>  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_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 b23e62b..ca22718 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -568,6 +568,30 @@ static void ieee80211_sta_wmm_params(struct ieee80211_local *local,
>  	}
>  }
>  
> +static bool check_tim(struct ieee802_11_elems *elems, u16 aid, bool *is_mc)
> +{
> +	u8 mask;
> +	u8 index, indexn1, indexn2;
> +	struct ieee80211_tim_ie *tim = (struct ieee80211_tim_ie *) elems->tim;
> +
> +	aid &= 0x3fff;
> +	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;
> +
> +	if (index < indexn1 || index > indexn2)
> +		return false;
> +
> +	index -= indexn1;
> +
> +	return !!(tim->virtual_map[index] & mask);
> +}
> +
>  static u32 ieee80211_handle_bss_capability(struct ieee80211_sub_if_data *sdata,
>  					   u16 capab, bool erp_valid, u8 erp)
>  {
> @@ -752,6 +776,7 @@ static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
>  			mod_timer(&local->dynamic_ps_timer, jiffies +
>  				  msecs_to_jiffies(local->dynamic_ps_timeout));
>  		else {
> +			ieee80211_send_nullfunc(local, sdata, 1);
>  			conf->flags |= IEEE80211_CONF_PS;
>  			ieee80211_hw_config(local,
>  					    IEEE80211_CONF_CHANGE_PS);
> @@ -1729,7 +1754,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;
> +	bool erp_valid, directed_tim, is_mc = false;
>  	u8 erp_value = 0;
>  
>  	/* Process beacon from the current BSS */
> @@ -1752,6 +1777,18 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
>  	ieee80211_sta_wmm_params(local, ifsta, elems.wmm_param,
>  				 elems.wmm_param_len);
>  
> +	if (!(local->hw.flags & IEEE80211_HW_NO_STACK_DYNAMIC_PS)) {
> +		directed_tim = check_tim(&elems, ifsta->aid, &is_mc);
> +
> +		if (directed_tim || is_mc) {
> +			if (local->hw.conf.flags && IEEE80211_CONF_PS) {
> +				local->hw.conf.flags &= ~IEEE80211_CONF_PS;
> +				ieee80211_hw_config(local,
> +						IEEE80211_CONF_CHANGE_PS);
> +				ieee80211_send_nullfunc(local, sdata, 0);
> +			}
> +		}
> +	}
>  
>  	if (elems.erp_info && elems.erp_info_len >= 1) {
>  		erp_valid = true;
> @@ -2651,10 +2688,12 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
>  	struct ieee80211_local *local =
>  		container_of(work, struct ieee80211_local,
>  			     dynamic_ps_enable_work);
> +	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
>  
>  	if (local->hw.conf.flags & IEEE80211_CONF_PS)
>  		return;
>  
> +	ieee80211_send_nullfunc(local, sdata, 1);
>  	local->hw.conf.flags |= IEEE80211_CONF_PS;
>  
>  	ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
> index f5c7c33..a2caeed 100644
> --- a/net/mac80211/scan.c
> +++ b/net/mac80211/scan.c
> @@ -395,7 +395,7 @@ ieee80211_scan_rx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
>  	return RX_QUEUED;
>  }
>  
> -static void ieee80211_send_nullfunc(struct ieee80211_local *local,
> +void ieee80211_send_nullfunc(struct ieee80211_local *local,
>  				    struct ieee80211_sub_if_data *sdata,
>  				    int powersave)
>  {
> diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
> index 673c5d7..6470614 100644
> --- a/net/mac80211/wext.c
> +++ b/net/mac80211/wext.c
> @@ -871,12 +871,17 @@ set:
>  			mod_timer(&local->dynamic_ps_timer, jiffies +
>  				  msecs_to_jiffies(local->dynamic_ps_timeout));
>  		else {
> -			if (local->powersave)
> +			if (local->powersave) {
> +				ieee80211_send_nullfunc(local, sdata, 1);
>  				conf->flags |= IEEE80211_CONF_PS;
> -			else
> +				ret = ieee80211_hw_config(local,
> +						IEEE80211_CONF_CHANGE_PS);
> +			} else {
>  				conf->flags &= ~IEEE80211_CONF_PS;
> -			ret = ieee80211_hw_config(local,
> -					IEEE80211_CONF_CHANGE_PS);
> +				ret = ieee80211_hw_config(local,
> +						IEEE80211_CONF_CHANGE_PS);
> +				ieee80211_send_nullfunc(local, sdata, 0);
> +			}
>  		}
>  	}
>  

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

[RFC] mac80211: Enhancements to dynamic power save.

[Vivek Natarajan]

This patch enables mac80211 to send a null frame and also to
check for tim in the beacon if power save is enabled.

Signed-off-by: Vivek Natarajan <vnatarajan@atheros.com>
---
 net/mac80211/ieee80211_i.h |    3 +++
 net/mac80211/mlme.c        |   41 ++++++++++++++++++++++++++++++++++++++++-
 net/mac80211/scan.c        |    2 +-
 net/mac80211/wext.c        |   13 +++++++++----
 4 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index f3eec98..2038498 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -986,6 +986,9 @@ u64 ieee80211_mandatory_rates(struct ieee80211_local *local,
 void ieee80211_dynamic_ps_enable_work(struct work_struct *work);
 void ieee80211_dynamic_ps_disable_work(struct work_struct *work);
 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_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 b23e62b..ca22718 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -568,6 +568,30 @@ static void ieee80211_sta_wmm_params(struct ieee80211_local *local,
 	}
 }
 
+static bool check_tim(struct ieee802_11_elems *elems, u16 aid, bool *is_mc)
+{
+	u8 mask;
+	u8 index, indexn1, indexn2;
+	struct ieee80211_tim_ie *tim = (struct ieee80211_tim_ie *) elems->tim;
+
+	aid &= 0x3fff;
+	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;
+
+	if (index < indexn1 || index > indexn2)
+		return false;
+
+	index -= indexn1;
+
+	return !!(tim->virtual_map[index] & mask);
+}
+
 static u32 ieee80211_handle_bss_capability(struct ieee80211_sub_if_data *sdata,
 					   u16 capab, bool erp_valid, u8 erp)
 {
@@ -752,6 +776,7 @@ static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
 			mod_timer(&local->dynamic_ps_timer, jiffies +
 				  msecs_to_jiffies(local->dynamic_ps_timeout));
 		else {
+			ieee80211_send_nullfunc(local, sdata, 1);
 			conf->flags |= IEEE80211_CONF_PS;
 			ieee80211_hw_config(local,
 					    IEEE80211_CONF_CHANGE_PS);
@@ -1729,7 +1754,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;
+	bool erp_valid, directed_tim, is_mc = false;
 	u8 erp_value = 0;
 
 	/* Process beacon from the current BSS */
@@ -1752,6 +1777,18 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
 	ieee80211_sta_wmm_params(local, ifsta, elems.wmm_param,
 				 elems.wmm_param_len);
 
+	if (!(local->hw.flags & IEEE80211_HW_NO_STACK_DYNAMIC_PS)) {
+		directed_tim = check_tim(&elems, ifsta->aid, &is_mc);
+
+		if (directed_tim || is_mc) {
+			if (local->hw.conf.flags && IEEE80211_CONF_PS) {
+				local->hw.conf.flags &= ~IEEE80211_CONF_PS;
+				ieee80211_hw_config(local,
+						IEEE80211_CONF_CHANGE_PS);
+				ieee80211_send_nullfunc(local, sdata, 0);
+			}
+		}
+	}
 
 	if (elems.erp_info && elems.erp_info_len >= 1) {
 		erp_valid = true;
@@ -2651,10 +2688,12 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
 	struct ieee80211_local *local =
 		container_of(work, struct ieee80211_local,
 			     dynamic_ps_enable_work);
+	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
 
 	if (local->hw.conf.flags & IEEE80211_CONF_PS)
 		return;
 
+	ieee80211_send_nullfunc(local, sdata, 1);
 	local->hw.conf.flags |= IEEE80211_CONF_PS;
 
 	ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index f5c7c33..a2caeed 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -395,7 +395,7 @@ ieee80211_scan_rx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
 	return RX_QUEUED;
 }
 
-static void ieee80211_send_nullfunc(struct ieee80211_local *local,
+void ieee80211_send_nullfunc(struct ieee80211_local *local,
 				    struct ieee80211_sub_if_data *sdata,
 				    int powersave)
 {
diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
index 673c5d7..6470614 100644
--- a/net/mac80211/wext.c
+++ b/net/mac80211/wext.c
@@ -871,12 +871,17 @@ set:
 			mod_timer(&local->dynamic_ps_timer, jiffies +
 				  msecs_to_jiffies(local->dynamic_ps_timeout));
 		else {
-			if (local->powersave)
+			if (local->powersave) {
+				ieee80211_send_nullfunc(local, sdata, 1);
 				conf->flags |= IEEE80211_CONF_PS;
-			else
+				ret = ieee80211_hw_config(local,
+						IEEE80211_CONF_CHANGE_PS);
+			} else {
 				conf->flags &= ~IEEE80211_CONF_PS;
-			ret = ieee80211_hw_config(local,
-					IEEE80211_CONF_CHANGE_PS);
+				ret = ieee80211_hw_config(local,
+						IEEE80211_CONF_CHANGE_PS);
+				ieee80211_send_nullfunc(local, sdata, 0);
+			}
 		}
 	}
 
-- 
1.6.0.1

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Vivek Natarajan]

On Mon, Dec 22, 2008 at 10:42 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2008-12-22 at 20:44 -0800, Vivek Natarajan wrote:
>> This patch enables mac80211 to send a null frame and also to
>> check for tim in the beacon if power save is enabled.
>>
>> Signed-off-by: Vivek Natarajan <vnatarajan@atheros.com>
>
> Looks good to me, to be sure, this is all dependent on !NO_STACK_PS,
> right?

Yes, it all depends on that flag. I will send these RFCs as patches.

Thanks,
Vivek.

Re: [RFC] mac80211: Enhancements to dynamic power save.

[Kalle Valo]

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

> This patch enables mac80211 to send a null frame and also to
> check for tim in the beacon if power save is enabled.

I would like to see a separate hw flag for disabling this feature. Not
all drivers need this, for example iwlwifi.

> diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
> index 673c5d7..6470614 100644
> --- a/net/mac80211/wext.c
> +++ b/net/mac80211/wext.c
> @@ -871,12 +871,17 @@ set:
>  			mod_timer(&local->dynamic_ps_timer, jiffies +
>  				  msecs_to_jiffies(local->dynamic_ps_timeout));
>  		else {
> -			if (local->powersave)
> +			if (local->powersave) {
> +				ieee80211_send_nullfunc(local, sdata, 1);
>  				conf->flags |= IEEE80211_CONF_PS;
> -			else
> +				ret = ieee80211_hw_config(local,
> +						IEEE80211_CONF_CHANGE_PS);
> +			} else {
>  				conf->flags &= ~IEEE80211_CONF_PS;
> -			ret = ieee80211_hw_config(local,
> -					IEEE80211_CONF_CHANGE_PS);
> +				ret = ieee80211_hw_config(local,
> +						IEEE80211_CONF_CHANGE_PS);
> +				ieee80211_send_nullfunc(local, sdata, 0);

Maybe add a helper function for checking the new flag I proposed and
sending the null frame?

-- 
Kalle Valo