Here is v3 of my dynamic powersave patches. I have tested them only
with stlc45xx on Nokia N810. I'm also working with iwl3945 and iwlagn
patches based on these patches, I'll try to send them during this
weekend.
These are RFC patches, not yet to be applied. Hopefully in next round
they are ready.
Patch 3 contains a new flag IEEE80211_HW_NO_DYNAMIC_PS. So all drivers
which firmware doesn't support dynamic power save should use this flag
to enable the feature in mac80211. This is a separate patch just for
easier review. If it's ok, I'll fold it to patch 2.
Patch 4 moves the dynamic_ps_timeout to ieee80211_conf struct. That
way the drivers which have dynamic power save support in firmware can
use the same timeout provided by user space. If this ok I'll fold this
to patch 2.
changes in v3:
o add a hw flag to notify that driver doesn't support dynamic power save and
which will enable mac80211 implementation
o delete dynamic_ps_timer and cancel ps_enable_work during
association, hopefully it's race free now
o provide timeout value to the drivers, for example iwlwifi should use
it (example patch under works)
o fix a bad bug in testing IEEE80211_STA_ASSOCIATED flag with && operator
TODO:
o test with iwlwifi (only in-tree driver using IEEE80211_CONF_PS)
o documentation for dynamic_ps_timeout and IEEE80211_HW_NO_DYNAMIC_PS
o prefix ieee80211_ps_[enable|disable]_work with _dynamic
Power save related features which I'll implement later:
o disable power save when software scanning
o beacon period (wakeup period) to the drivers, can be set from user
space but otherwise is bss's DTIM value
I'll add missing features to the wiki tomorrow.
Also disable power save when disassociated.
Signed-off-by: Kalle Valo <[email protected]>
---
net/mac80211/ieee80211_i.h | 1 +
net/mac80211/mlme.c | 15 +++++++++++++--
net/mac80211/wext.c | 30 ++++++++++++++++++++++++------
3 files changed, 38 insertions(+), 8 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 155a204..3f25955 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -688,6 +688,7 @@ struct ieee80211_local {
*/
int wifi_wme_noack_test;
unsigned int wmm_acm; /* bit field of ACM bits (BIT(802.1D tag)) */
+ bool powersave;
#ifdef CONFIG_MAC80211_DEBUGFS
struct local_debugfsdentries {
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 88b0b4d..e7ade66 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -746,6 +746,11 @@ static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
bss_info_changed |= BSS_CHANGED_BASIC_RATES;
ieee80211_bss_info_change_notify(sdata, bss_info_changed);
+ if (local->powersave) {
+ local->hw.conf.flags |= IEEE80211_CONF_PS;
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+ }
+
netif_tx_start_all_queues(sdata->dev);
netif_carrier_on(sdata->dev);
@@ -818,7 +823,7 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
{
struct ieee80211_local *local = sdata->local;
struct sta_info *sta;
- u32 changed = 0;
+ u32 changed = 0, config_changed = 0;
rcu_read_lock();
@@ -868,8 +873,14 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
sta_info_destroy(sta);
local->hw.conf.ht.enabled = false;
- ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_HT);
+ config_changed |= IEEE80211_CONF_CHANGE_HT;
+
+ if (local->hw.conf.flags & IEEE80211_CONF_PS) {
+ local->hw.conf.flags &= ~IEEE80211_CONF_PS;
+ config_changed |= IEEE80211_CONF_PS;
+ }
+ ieee80211_hw_config(local, config_changed);
ieee80211_bss_info_change_notify(sdata, changed);
}
diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
index b3ce28d..8329de2 100644
--- a/net/mac80211/wext.c
+++ b/net/mac80211/wext.c
@@ -953,25 +953,44 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,
struct iw_param *wrq,
char *extra)
{
+ struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
+ struct ieee80211_if_sta *ifsta = &sdata->u.sta;
struct ieee80211_conf *conf = &local->hw.conf;
+ int ret = 0;
+ bool ps;
if (wrq->disabled) {
- conf->flags &= ~IEEE80211_CONF_PS;
- return ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+ ps = false;
+ goto set;
}
switch (wrq->flags & IW_POWER_MODE) {
case IW_POWER_ON: /* If not specified */
case IW_POWER_MODE: /* If set all mask */
case IW_POWER_ALL_R: /* If explicitely state all */
- conf->flags |= IEEE80211_CONF_PS;
+ ps = true;
break;
default: /* Otherwise we don't support it */
return -EINVAL;
}
- return ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+ if (ps == local->powersave)
+ return ret;
+
+set:
+ local->powersave = ps;
+
+ if (ifsta->flags && IEEE80211_STA_ASSOCIATED) {
+ if (local->powersave)
+ conf->flags |= IEEE80211_CONF_PS;
+ else
+ conf->flags &= ~IEEE80211_CONF_PS;
+
+ ret = ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+ }
+
+ return ret;
}
static int ieee80211_ioctl_giwpower(struct net_device *dev,
@@ -980,9 +999,8 @@ static int ieee80211_ioctl_giwpower(struct net_device *dev,
char *extra)
{
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
- struct ieee80211_conf *conf = &local->hw.conf;
- wrqu->power.disabled = !(conf->flags & IEEE80211_CONF_PS);
+ wrqu->power.disabled = !local->powersave;
return 0;
}
--
1.5.6.5
On Fri, 2008-11-28 at 21:49 +0200, Kalle Valo wrote:
> Signed-off-by: Kalle Valo <[email protected]>
> ---
> include/net/mac80211.h | 1 +
> net/mac80211/tx.c | 3 ++-
> 2 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 6a1d4ea..53a0dc5 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -858,6 +858,7 @@ enum ieee80211_hw_flags {
> IEEE80211_HW_NOISE_DBM = 1<<8,
> IEEE80211_HW_SPECTRUM_MGMT = 1<<9,
> IEEE80211_HW_AMPDU_AGGREGATION = 1<<10,
> + IEEE80211_HW_NO_DYNAMIC_PS = 1<<11,
Add documentation please.
johannes
Signed-off-by: Kalle Valo <[email protected]>
---
include/net/mac80211.h | 1 +
net/mac80211/tx.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 6a1d4ea..53a0dc5 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -858,6 +858,7 @@ enum ieee80211_hw_flags {
IEEE80211_HW_NOISE_DBM = 1<<8,
IEEE80211_HW_SPECTRUM_MGMT = 1<<9,
IEEE80211_HW_AMPDU_AGGREGATION = 1<<10,
+ IEEE80211_HW_NO_DYNAMIC_PS = 1<<11,
};
/**
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 25d4a86..1d7ef18 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1476,7 +1476,8 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
goto fail;
}
- if (local->dynamic_ps_timeout > 0) {
+ if (local->hw.flags & IEEE80211_HW_NO_DYNAMIC_PS &&
+ local->dynamic_ps_timeout > 0) {
if (local->hw.conf.flags & IEEE80211_CONF_PS) {
netif_tx_stop_all_queues(local->mdev);
queue_work(local->hw.workqueue,
--
1.5.6.5
Johannes Berg <[email protected]> writes:
> On Fri, 2008-11-28 at 21:49 +0200, Kalle Valo wrote:
>> Signed-off-by: Kalle Valo <[email protected]>
>> ---
>> include/net/mac80211.h | 1 +
>> net/mac80211/tx.c | 3 ++-
>> 2 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>> index 6a1d4ea..53a0dc5 100644
>> --- a/include/net/mac80211.h
>> +++ b/include/net/mac80211.h
>> @@ -858,6 +858,7 @@ enum ieee80211_hw_flags {
>> IEEE80211_HW_NOISE_DBM = 1<<8,
>> IEEE80211_HW_SPECTRUM_MGMT = 1<<9,
>> IEEE80211_HW_AMPDU_AGGREGATION = 1<<10,
>> + IEEE80211_HW_NO_DYNAMIC_PS = 1<<11,
>
> Add documentation please.
Will do. Actually it was on my TODO list already.
--
Kalle Valo
Signed-off-by: Kalle Valo <[email protected]>
---
include/net/mac80211.h | 1 +
net/mac80211/ieee80211_i.h | 1 -
net/mac80211/mlme.c | 4 ++--
net/mac80211/tx.c | 4 ++--
net/mac80211/wext.c | 8 ++++----
5 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 53a0dc5..aae8991 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -560,6 +560,7 @@ struct ieee80211_conf {
u16 listen_interval;
bool radio_enabled;
+ int dynamic_ps_timeout;
u8 long_frame_max_tx_count, short_frame_max_tx_count;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index ab0e2df..1317c7a 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -690,7 +690,6 @@ struct ieee80211_local {
unsigned int wmm_acm; /* bit field of ACM bits (BIT(802.1D tag)) */
bool powersave;
- int dynamic_ps_timeout;
struct work_struct ps_enable_work;
struct work_struct ps_disable_work;
struct timer_list dynamic_ps_timer;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 1d3c72b..52b2239 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -747,9 +747,9 @@ static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
ieee80211_bss_info_change_notify(sdata, bss_info_changed);
if (local->powersave) {
- if (local->dynamic_ps_timeout > 0)
+ if (local->hw.conf.dynamic_ps_timeout > 0)
mod_timer(&local->dynamic_ps_timer, jiffies +
- msecs_to_jiffies(local->dynamic_ps_timeout));
+ msecs_to_jiffies(local->hw.conf.dynamic_ps_timeout));
else {
conf->flags |= IEEE80211_CONF_PS;
ieee80211_hw_config(local,
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1d7ef18..12256bd 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1477,7 +1477,7 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
}
if (local->hw.flags & IEEE80211_HW_NO_DYNAMIC_PS &&
- local->dynamic_ps_timeout > 0) {
+ local->hw.conf.dynamic_ps_timeout > 0) {
if (local->hw.conf.flags & IEEE80211_CONF_PS) {
netif_tx_stop_all_queues(local->mdev);
queue_work(local->hw.workqueue,
@@ -1485,7 +1485,7 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
}
mod_timer(&local->dynamic_ps_timer, jiffies +
- msecs_to_jiffies(local->dynamic_ps_timeout));
+ msecs_to_jiffies(local->hw.conf.dynamic_ps_timeout));
}
nh_pos = skb_network_header(skb) - skb->data;
diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
index b3ee0dc..95455d0 100644
--- a/net/mac80211/wext.c
+++ b/net/mac80211/wext.c
@@ -962,7 +962,7 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,
if (wrq->disabled) {
ps = false;
- local->dynamic_ps_timeout = 0;
+ local->hw.conf.dynamic_ps_timeout = 0;
goto set;
}
@@ -977,7 +977,7 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,
}
if (wrq->flags & IW_POWER_TIMEOUT)
- local->dynamic_ps_timeout = wrq->value / 1000;
+ local->hw.conf.dynamic_ps_timeout = wrq->value / 1000;
if (ps == local->powersave)
return ret;
@@ -986,9 +986,9 @@ set:
local->powersave = ps;
if (ifsta->flags & IEEE80211_STA_ASSOCIATED) {
- if (local->dynamic_ps_timeout > 0)
+ if (conf->dynamic_ps_timeout > 0)
mod_timer(&local->dynamic_ps_timer, jiffies +
- msecs_to_jiffies(local->dynamic_ps_timeout));
+ msecs_to_jiffies(conf->dynamic_ps_timeout));
else {
if (local->powersave)
conf->flags |= IEEE80211_CONF_PS;
--
1.5.6.5
Johannes Berg <[email protected]> writes:
>> +set:
>> + local->powersave = ps;
>> +
>> + if (ifsta->flags && IEEE80211_STA_ASSOCIATED) {
>
> This really needs to check you're actually in STA mode.
Thanks, I'll fix it.
I'm going with the assumption that we don't support power save in IBSS
mode. I think we talked about this a long time ago.
--
Kalle Valo
"Johannes Berg" <[email protected]> writes:
> On Fri, 2008-11-28 at 21:49 +0200, Kalle Valo wrote:
>> --- a/include/net/mac80211.h
>> +++ b/include/net/mac80211.h
>> @@ -560,6 +560,7 @@ struct ieee80211_conf {
>>
>> u16 listen_interval;
>> bool radio_enabled;
>> + int dynamic_ps_timeout;
>
> Same here, add documentation please;
I will.
> also, I think you need to call the hw_config call when this changes
> so drivers can upload the new value to the firmware or whatever?
Doh, I missed that. Yes, I think we need to do that.
> Which drivers actually will end up using it and how?
Currently I'm only aware of iwl3945 and iwlagn. I have some example
patches for them, but they aren't ready yet.
--
Kalle Valo
On Fri, 2008-11-28 at 21:49 +0200, Kalle Valo wrote:
> @@ -953,25 +953,44 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,
> struct iw_param *wrq,
> char *extra)
> {
> + struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
> + struct ieee80211_if_sta *ifsta = &sdata->u.sta;
> struct ieee80211_conf *conf = &local->hw.conf;
> + int ret = 0;
> + bool ps;
>
> if (wrq->disabled) {
> - conf->flags &= ~IEEE80211_CONF_PS;
> - return ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
> + ps = false;
> + goto set;
> }
>
> switch (wrq->flags & IW_POWER_MODE) {
> case IW_POWER_ON: /* If not specified */
> case IW_POWER_MODE: /* If set all mask */
> case IW_POWER_ALL_R: /* If explicitely state all */
> - conf->flags |= IEEE80211_CONF_PS;
> + ps = true;
> break;
> default: /* Otherwise we don't support it */
> return -EINVAL;
> }
>
> - return ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
> + if (ps == local->powersave)
> + return ret;
> +
> +set:
> + local->powersave = ps;
> +
> + if (ifsta->flags && IEEE80211_STA_ASSOCIATED) {
This really needs to check you're actually in STA mode.
johannes
Johannes Berg <[email protected]> writes:
> On Fri, 2008-11-28 at 21:49 +0200, Kalle Valo wrote:
>
>> + /* always wake queues to avoid having them stopped forever */
>> + netif_tx_wake_all_queues(local->mdev);
>
> I think we really need to think about keeping track of _who_ wanted the
> queues stopped now...
Makes sense.
> We have a number of users of stopping queues now:
>
> * driver, obviously
> * scanning code
> * PS code
> * ..?
>
> So when the driver wants a queue stopped, but the PS code wakes them up,
> the driver will end up having to drop a packet, which isn't really good.
> We really need to keep track, for each queue, who wants it
> stopped/started and then only start the real queue if all the code
> consents.
Frankly I'm not that familiar with mac80211 queues, but it's a good
time to learn now :) I'll look into this and will make a proposal.
--
Kalle Valo
On Fri, 2008-11-28 at 21:49 +0200, Kalle Valo wrote:
> Signed-off-by: Kalle Valo <[email protected]>
> ---
> include/net/mac80211.h | 1 +
> net/mac80211/ieee80211_i.h | 1 -
> net/mac80211/mlme.c | 4 ++--
> net/mac80211/tx.c | 4 ++--
> net/mac80211/wext.c | 8 ++++----
> 5 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 53a0dc5..aae8991 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -560,6 +560,7 @@ struct ieee80211_conf {
>
> u16 listen_interval;
> bool radio_enabled;
> + int dynamic_ps_timeout;
Same here, add documentation please; also, I think you need to call the
hw_config call when this changes so drivers can upload the new value to
the firmware or whatever? Which drivers actually will end up using it
and how?
johannes
This patch implements dynamic power save for mac80211. Basically it
means enabling power save mode after an idle period. Implementing it
dynamically gives a good compromise of low power consumption and low
latency. Some hardware have support for this in firmware, but some
require the host to do it.
The dynamic power save is implemented by adding an timeout to
ieee80211_subif_start_xmit(). The timeout can be enabled from user space
with Wireless Extensions. For example, the command below enables the
dynamic power save and sets the time timeout to 500 ms:
iwconfig wlan0 power timeout 500m
Thanks to Johannes Berg for the help with the design.
Signed-off-by: Kalle Valo <[email protected]>
---
net/mac80211/ieee80211_i.h | 9 ++++++++
net/mac80211/main.c | 5 ++++
net/mac80211/mlme.c | 47 ++++++++++++++++++++++++++++++++++++++++++-
net/mac80211/tx.c | 11 ++++++++++
net/mac80211/wext.c | 27 ++++++++++++++++--------
5 files changed, 88 insertions(+), 11 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 3f25955..ab0e2df 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -688,7 +688,12 @@ struct ieee80211_local {
*/
int wifi_wme_noack_test;
unsigned int wmm_acm; /* bit field of ACM bits (BIT(802.1D tag)) */
+
bool powersave;
+ int dynamic_ps_timeout;
+ struct work_struct ps_enable_work;
+ struct work_struct ps_disable_work;
+ struct timer_list dynamic_ps_timer;
#ifdef CONFIG_MAC80211_DEBUGFS
struct local_debugfsdentries {
@@ -973,6 +978,10 @@ int ieee80211_set_freq(struct ieee80211_sub_if_data *sdata, int freq);
u64 ieee80211_mandatory_rates(struct ieee80211_local *local,
enum ieee80211_band band);
+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);
+
#ifdef CONFIG_MAC80211_NOINLINE
#define debug_noinline noinline
#else
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index cec9b6d..b35bd74 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -701,6 +701,11 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
INIT_DELAYED_WORK(&local->scan_work, ieee80211_scan_work);
+ INIT_WORK(&local->ps_enable_work, ieee80211_ps_enable_work);
+ INIT_WORK(&local->ps_disable_work, ieee80211_ps_disable_work);
+ setup_timer(&local->dynamic_ps_timer,
+ ieee80211_dynamic_ps_timer, (unsigned long) local);
+
sta_info_init(local);
tasklet_init(&local->tx_pending_tasklet, ieee80211_tx_pending,
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index e7ade66..1d3c72b 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -747,8 +747,14 @@ static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
ieee80211_bss_info_change_notify(sdata, bss_info_changed);
if (local->powersave) {
- local->hw.conf.flags |= IEEE80211_CONF_PS;
- ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+ if (local->dynamic_ps_timeout > 0)
+ mod_timer(&local->dynamic_ps_timer, jiffies +
+ msecs_to_jiffies(local->dynamic_ps_timeout));
+ else {
+ conf->flags |= IEEE80211_CONF_PS;
+ ieee80211_hw_config(local,
+ IEEE80211_CONF_CHANGE_PS);
+ }
}
netif_tx_start_all_queues(sdata->dev);
@@ -875,6 +881,9 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
local->hw.conf.ht.enabled = false;
config_changed |= IEEE80211_CONF_CHANGE_HT;
+ del_timer_sync(&local->dynamic_ps_timer);
+ cancel_work_sync(&local->ps_enable_work);
+
if (local->hw.conf.flags & IEEE80211_CONF_PS) {
local->hw.conf.flags &= ~IEEE80211_CONF_PS;
config_changed |= IEEE80211_CONF_PS;
@@ -2608,3 +2617,37 @@ void ieee80211_mlme_notify_scan_completed(struct ieee80211_local *local)
ieee80211_restart_sta_timer(sdata);
rcu_read_unlock();
}
+
+void ieee80211_ps_disable_work(struct work_struct *work)
+{
+ struct ieee80211_local *local =
+ container_of(work, struct ieee80211_local, ps_disable_work);
+
+ if (local->hw.conf.flags & IEEE80211_CONF_PS) {
+ local->hw.conf.flags &= ~IEEE80211_CONF_PS;
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+ }
+
+ /* always wake queues to avoid having them stopped forever */
+ netif_tx_wake_all_queues(local->mdev);
+}
+
+void ieee80211_ps_enable_work(struct work_struct *work)
+{
+ struct ieee80211_local *local =
+ container_of(work, struct ieee80211_local, ps_enable_work);
+
+ if (local->hw.conf.flags & IEEE80211_CONF_PS)
+ return;
+
+ local->hw.conf.flags |= IEEE80211_CONF_PS;
+
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+}
+
+void ieee80211_dynamic_ps_timer(unsigned long data)
+{
+ struct ieee80211_local *local = (void *) data;
+
+ queue_work(local->hw.workqueue, &local->ps_enable_work);
+}
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 0855cac..25d4a86 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1476,6 +1476,17 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
goto fail;
}
+ if (local->dynamic_ps_timeout > 0) {
+ if (local->hw.conf.flags & IEEE80211_CONF_PS) {
+ netif_tx_stop_all_queues(local->mdev);
+ queue_work(local->hw.workqueue,
+ &local->ps_disable_work);
+ }
+
+ mod_timer(&local->dynamic_ps_timer, jiffies +
+ msecs_to_jiffies(local->dynamic_ps_timeout));
+ }
+
nh_pos = skb_network_header(skb) - skb->data;
h_pos = skb_transport_header(skb) - skb->data;
diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
index 8329de2..b3ee0dc 100644
--- a/net/mac80211/wext.c
+++ b/net/mac80211/wext.c
@@ -962,6 +962,7 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,
if (wrq->disabled) {
ps = false;
+ local->dynamic_ps_timeout = 0;
goto set;
}
@@ -971,23 +972,31 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,
case IW_POWER_ALL_R: /* If explicitely state all */
ps = true;
break;
- default: /* Otherwise we don't support it */
- return -EINVAL;
+ default: /* Otherwise we ignore */
+ break;
}
+ if (wrq->flags & IW_POWER_TIMEOUT)
+ local->dynamic_ps_timeout = wrq->value / 1000;
+
if (ps == local->powersave)
return ret;
set:
local->powersave = ps;
- if (ifsta->flags && IEEE80211_STA_ASSOCIATED) {
- if (local->powersave)
- conf->flags |= IEEE80211_CONF_PS;
- else
- conf->flags &= ~IEEE80211_CONF_PS;
-
- ret = ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+ if (ifsta->flags & IEEE80211_STA_ASSOCIATED) {
+ if (local->dynamic_ps_timeout > 0)
+ mod_timer(&local->dynamic_ps_timer, jiffies +
+ msecs_to_jiffies(local->dynamic_ps_timeout));
+ else {
+ if (local->powersave)
+ conf->flags |= IEEE80211_CONF_PS;
+ else
+ conf->flags &= ~IEEE80211_CONF_PS;
+ ret = ieee80211_hw_config(local,
+ IEEE80211_CONF_CHANGE_PS);
+ }
}
return ret;
--
1.5.6.5
On Fri, 2008-11-28 at 21:49 +0200, Kalle Valo wrote:
> + /* always wake queues to avoid having them stopped forever */
> + netif_tx_wake_all_queues(local->mdev);
I think we really need to think about keeping track of _who_ wanted the
queues stopped now... We have a number of users of stopping queues now:
* driver, obviously
* scanning code
* PS code
* ..?
So when the driver wants a queue stopped, but the PS code wakes them up,
the driver will end up having to drop a packet, which isn't really good.
We really need to keep track, for each queue, who wants it
stopped/started and then only start the real queue if all the code
consents.
johannes
Luis R. Rodriguez <[email protected]> writes:
> On Fri, Nov 28, 2008 at 11:49 AM, Kalle Valo <[email protected]> wrote:
>
>> diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
>> index b3ee0dc..95455d0 100644
>> --- a/net/mac80211/wext.c
>> +++ b/net/mac80211/wext.c
>> @@ -962,7 +962,7 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,
>>
>> if (wrq->disabled) {
>> ps = false;
>> - local->dynamic_ps_timeout = 0;
>> + local->hw.conf.dynamic_ps_timeout = 0;
>> goto set;
>> }
>>
>> @@ -977,7 +977,7 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,
>> }
>>
>> if (wrq->flags & IW_POWER_TIMEOUT)
>> - local->dynamic_ps_timeout = wrq->value / 1000;
>> + local->hw.conf.dynamic_ps_timeout = wrq->value / 1000;
>>
>> if (ps == local->powersave)
>> return ret;
>> @@ -986,9 +986,9 @@ set:
>> local->powersave = ps;
>>
>> if (ifsta->flags & IEEE80211_STA_ASSOCIATED) {
>> - if (local->dynamic_ps_timeout > 0)
>> + if (conf->dynamic_ps_timeout > 0)
>> mod_timer(&local->dynamic_ps_timer, jiffies +
>> - msecs_to_jiffies(local->dynamic_ps_timeout));
>> + msecs_to_jiffies(conf->dynamic_ps_timeout));
>> else {
>> if (local->powersave)
>> conf->flags |= IEEE80211_CONF_PS;
>
> Would it be too much trouble to add this via cfg80211/nl80211 instead
> or at least add both? The more we can push in that direction the
> better.
I definitely would like to see nl80211 support for power save. For
example, I think currently there's no way for the user space to know
dtim period of the bss. (If I'm wrong here, please let me know). We
definitely should fix that in nl80211 to make it easier for the user
space decide on correct power save settings.
Let's start to work on nl80211 after client power save features in
mac80211 are in better shape. At that time we also have a better
understanding of what's needed for the user space interface. Now
things are a bit sketchy still.
--
Kalle Valo
On Mon, 2008-12-01 at 11:10 -0800, Luis R. Rodriguez wrote:
> Might be worth noting that when dynamic power save is enabled for
> mac80211 that this will only work right now for devices which handle
> power save in firmware as power save is not yet implemented in
> mac80211.
Power save mode inherently is a feature that needs to be implemented on
the chip due to timing requirements, what else are you looking for in
mac80211? mac80211 tells the chip "it's ok to enter PS mode now" via the
hw conf callback, that's pretty much all it can do.
johannes
On Fri, Nov 28, 2008 at 11:49 AM, Kalle Valo <[email protected]> wrote:
> This patch implements dynamic power save for mac80211. Basically it
> means enabling power save mode after an idle period. Implementing it
> dynamically gives a good compromise of low power consumption and low
> latency. Some hardware have support for this in firmware, but some
> require the host to do it.
>
> The dynamic power save is implemented by adding an timeout to
> ieee80211_subif_start_xmit(). The timeout can be enabled from user space
> with Wireless Extensions. For example, the command below enables the
> dynamic power save and sets the time timeout to 500 ms:
>
> iwconfig wlan0 power timeout 500m
>
> Thanks to Johannes Berg for the help with the design.
Might be worth noting that when dynamic power save is enabled for
mac80211 that this will only work right now for devices which handle
power save in firmware as power save is not yet implemented in
mac80211.
Also that by default this is left disabled and the heuristics of how
and exactly how to enable this will be left to userspace.
Luis
On Mon, Dec 01, 2008 at 02:40:41PM -0800, Johannes Berg wrote:
> On Mon, 2008-12-01 at 11:10 -0800, Luis R. Rodriguez wrote:
>
> > Might be worth noting that when dynamic power save is enabled for
> > mac80211 that this will only work right now for devices which handle
> > power save in firmware as power save is not yet implemented in
> > mac80211.
>
> Power save mode inherently is a feature that needs to be implemented on
> the chip due to timing requirements, what else are you looking for in
> mac80211? mac80211 tells the chip "it's ok to enter PS mode now" via the
> hw conf callback, that's pretty much all it can do.
Well the stuff vivek is adding.
Luis
"ext Luis R. Rodriguez" <[email protected]> writes:
> On Fri, Nov 28, 2008 at 11:49 AM, Kalle Valo <[email protected]> wrote:
>> This patch implements dynamic power save for mac80211. Basically it
>> means enabling power save mode after an idle period. Implementing it
>> dynamically gives a good compromise of low power consumption and low
>> latency. Some hardware have support for this in firmware, but some
>> require the host to do it.
>>
>> The dynamic power save is implemented by adding an timeout to
>> ieee80211_subif_start_xmit(). The timeout can be enabled from user space
>> with Wireless Extensions. For example, the command below enables the
>> dynamic power save and sets the time timeout to 500 ms:
>>
>> iwconfig wlan0 power timeout 500m
>>
>> Thanks to Johannes Berg for the help with the design.
>
> Might be worth noting that when dynamic power save is enabled for
> mac80211 that this will only work right now for devices which handle
> power save in firmware as power save is not yet implemented in
> mac80211.
>
> Also that by default this is left disabled and the heuristics of how
> and exactly how to enable this will be left to userspace.
I will mention both of these in v4.
--
Kalle Valo
On Fri, Nov 28, 2008 at 11:49 AM, Kalle Valo <[email protected]> wrote:
> Signed-off-by: Kalle Valo <[email protected]>
> ---
> include/net/mac80211.h | 1 +
> net/mac80211/ieee80211_i.h | 1 -
> net/mac80211/mlme.c | 4 ++--
> net/mac80211/tx.c | 4 ++--
> net/mac80211/wext.c | 8 ++++----
> 5 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 53a0dc5..aae8991 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -560,6 +560,7 @@ struct ieee80211_conf {
>
> u16 listen_interval;
> bool radio_enabled;
> + int dynamic_ps_timeout;
>
> u8 long_frame_max_tx_count, short_frame_max_tx_count;
>
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index ab0e2df..1317c7a 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -690,7 +690,6 @@ struct ieee80211_local {
> unsigned int wmm_acm; /* bit field of ACM bits (BIT(802.1D tag)) */
>
> bool powersave;
> - int dynamic_ps_timeout;
> struct work_struct ps_enable_work;
> struct work_struct ps_disable_work;
> struct timer_list dynamic_ps_timer;
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index 1d3c72b..52b2239 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -747,9 +747,9 @@ static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
> ieee80211_bss_info_change_notify(sdata, bss_info_changed);
>
> if (local->powersave) {
> - if (local->dynamic_ps_timeout > 0)
> + if (local->hw.conf.dynamic_ps_timeout > 0)
> mod_timer(&local->dynamic_ps_timer, jiffies +
> - msecs_to_jiffies(local->dynamic_ps_timeout));
> + msecs_to_jiffies(local->hw.conf.dynamic_ps_timeout));
> else {
> conf->flags |= IEEE80211_CONF_PS;
> ieee80211_hw_config(local,
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 1d7ef18..12256bd 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1477,7 +1477,7 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
> }
>
> if (local->hw.flags & IEEE80211_HW_NO_DYNAMIC_PS &&
> - local->dynamic_ps_timeout > 0) {
> + local->hw.conf.dynamic_ps_timeout > 0) {
> if (local->hw.conf.flags & IEEE80211_CONF_PS) {
> netif_tx_stop_all_queues(local->mdev);
> queue_work(local->hw.workqueue,
> @@ -1485,7 +1485,7 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
> }
>
> mod_timer(&local->dynamic_ps_timer, jiffies +
> - msecs_to_jiffies(local->dynamic_ps_timeout));
> + msecs_to_jiffies(local->hw.conf.dynamic_ps_timeout));
> }
>
> nh_pos = skb_network_header(skb) - skb->data;
> diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
> index b3ee0dc..95455d0 100644
> --- a/net/mac80211/wext.c
> +++ b/net/mac80211/wext.c
> @@ -962,7 +962,7 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,
>
> if (wrq->disabled) {
> ps = false;
> - local->dynamic_ps_timeout = 0;
> + local->hw.conf.dynamic_ps_timeout = 0;
> goto set;
> }
>
> @@ -977,7 +977,7 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,
> }
>
> if (wrq->flags & IW_POWER_TIMEOUT)
> - local->dynamic_ps_timeout = wrq->value / 1000;
> + local->hw.conf.dynamic_ps_timeout = wrq->value / 1000;
>
> if (ps == local->powersave)
> return ret;
> @@ -986,9 +986,9 @@ set:
> local->powersave = ps;
>
> if (ifsta->flags & IEEE80211_STA_ASSOCIATED) {
> - if (local->dynamic_ps_timeout > 0)
> + if (conf->dynamic_ps_timeout > 0)
> mod_timer(&local->dynamic_ps_timer, jiffies +
> - msecs_to_jiffies(local->dynamic_ps_timeout));
> + msecs_to_jiffies(conf->dynamic_ps_timeout));
> else {
> if (local->powersave)
> conf->flags |= IEEE80211_CONF_PS;
Would it be too much trouble to add this via cfg80211/nl80211 instead
or at least add both? The more we can push in that direction the
better.
Luis
On Mon, Dec 01, 2008 at 11:41:07AM -0800, Johannes Berg wrote:
> On Mon, 2008-12-01 at 11:11 -0800, Luis R. Rodriguez wrote:
>
> > Would it be too much trouble to add this via cfg80211/nl80211 instead
> > or at least add both? The more we can push in that direction the
> > better.
>
> That would be good, but I don't think it's really a requirement right
> now since wext has the API we need already.
Sure, just wishful thinking.
Luis
On Mon, 2008-12-01 at 11:11 -0800, Luis R. Rodriguez wrote:
> Would it be too much trouble to add this via cfg80211/nl80211 instead
> or at least add both? The more we can push in that direction the
> better.
That would be good, but I don't think it's really a requirement right
now since wext has the API we need already.
johannes