2008-12-18 21:36:32

by Kalle Valo

[permalink] [raw]
Subject: [PATCH v6 0/3] mac80211 dynamic power save

Here is v5 of my dynamic powersave patches. I have tested them with
stlc45xx on Nokia N810.

I think (and hope) that the patches are close to inclusion now. Please
review.

changes in v6:

o use IEEE80211_CONF_CHANGE_PS in ieee80211_set_disassoc()

changes in v5:

o check STA interface earlier in siwpower()
o moved queue track patch before dynamic power save patch
o add new functions ieee80211_wake_queues_by_reason() and
ieee80211_stop_queues_by_reason() for optimised locking
o dropped "move dynamic_ps_timeout to hw.conf" for now, will resubmit
it later when I have example driver implementation
o renamed the hw flag to IEEE80211_HW_NO_STACK_DYNAMIC_PS and inverted
it's functionality, now drivers need to explicitly disable the feature
o folded hw flag patch to the main dynamic power save patch

changes in v4:
o add check for station interface to siwpower()
o call hw_config() also for dynamic timeout changes
o add dynamic_ prefix to workqueues
o test for IEEE80211_HW_NO_DYNAMIC_PS in siwpower()
o wrote documentation for IEEE80211_HW_NO_DYNAMIC_PS
o wrote documentation for conf->dynamic_ps_timeout

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

Power save related features which I'll implement later:
o disable power save when software scanning
o move dynamic_ps_timeout to hw.conf so that drivers can use it in
case they need to
o beacon period (wakeup period) to the drivers, can be set from user
space but otherwise is bss's DTIM value

---

Kalle Valo (3):
mac80211: implement dynamic power save
mac80211: track master queue status
mac80211: enable IEEE80211_CONF_PS only when associated


include/net/mac80211.h | 6 +++
net/mac80211/ieee80211_i.h | 23 +++++++++++-
net/mac80211/main.c | 9 +++++
net/mac80211/mlme.c | 60 ++++++++++++++++++++++++++++++-
net/mac80211/tx.c | 13 +++++++
net/mac80211/util.c | 86 +++++++++++++++++++++++++++++++++++++++++---
net/mac80211/wext.c | 46 +++++++++++++++++++-----
7 files changed, 226 insertions(+), 17 deletions(-)



2008-12-19 09:21:52

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: [PATCH v6 3/3] mac80211: implement dynamic power save

HI,

>> I need your thoughts on having null frame being sent from mac80211(
>> the RFC I submitted earlier). Do you feel a state machine is
>> necessary to go to power save? ( like awake, sleep, pending_sleep if
>> the AP hasn't acked the null frame with a timer or a work queue to
>> send null frame periodically in the pending state).
>
> I haven't thinked much about this yet, but your proposal of having a
> state machine most probably is the best option. It's just that earlier
> Johannes mentioned that it's not currently possible to know if null
> frame has been acked:
>
> http://www.spinics.net/lists/linux-wireless/msg21212.html
>
> We would have to solve that before implementing the state machine.
>
>> Or altogether you want that to be part of the driver and not in
>> mac80211?
>
> I think this needs to be in mac80211 because there are three drivers
> which need it (p54/stlc45xx, ath5/9k and b43).

You can add the rt2x00 family to that list as well, I have already a patch
which implements power saving in rt2x00 but it will need this patch to get
it working correctly.

Ivo

2008-12-18 21:35:43

by Kalle Valo

[permalink] [raw]
Subject: [PATCH v6 3/3] mac80211: implement dynamic power save

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

Power save now only works with devices which handle power save in firmware.
It's also disabled by default and the heuristics when and how to enable is
considered as a policy decision and will be left for the userspace to handle.
In case the firmware has support for this, drivers can disable this feature
with IEEE80211_HW_NO_STACK_DYNAMIC_PS.

Big thanks to Johannes Berg for the help with the design and code.

Signed-off-by: Kalle Valo <[email protected]>
---

include/net/mac80211.h | 6 +++++
net/mac80211/ieee80211_i.h | 10 +++++++++
net/mac80211/main.c | 7 ++++++
net/mac80211/mlme.c | 49 ++++++++++++++++++++++++++++++++++++++++++--
net/mac80211/tx.c | 13 ++++++++++++
net/mac80211/wext.c | 30 ++++++++++++++++++---------
6 files changed, 103 insertions(+), 12 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 9428d3e..b3bd00a 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -854,6 +854,11 @@ enum ieee80211_tkip_key_type {
*
* @IEEE80211_HW_AMPDU_AGGREGATION:
* Hardware supports 11n A-MPDU aggregation.
+ *
+ * @IEEE80211_HW_NO_STACK_DYNAMIC_PS:
+ * Hardware which has dynamic power save support, meaning
+ * that power save is enabled in idle periods, and don't need support
+ * from stack.
*/
enum ieee80211_hw_flags {
IEEE80211_HW_RX_INCLUDES_FCS = 1<<1,
@@ -866,6 +871,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_STACK_DYNAMIC_PS = 1<<11,
};

/**
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index a74d673..f3eec98 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -540,6 +540,7 @@ enum {

enum queue_stop_reason {
IEEE80211_QUEUE_STOP_REASON_DRIVER,
+ IEEE80211_QUEUE_STOP_REASON_PS,
};

/* maximum number of hardware queues we support. */
@@ -693,7 +694,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 dynamic_ps_enable_work;
+ struct work_struct dynamic_ps_disable_work;
+ struct timer_list dynamic_ps_timer;

#ifdef CONFIG_MAC80211_DEBUGFS
struct local_debugfsdentries {
@@ -977,6 +983,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_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_wake_queues_by_reason(struct ieee80211_hw *hw,
enum queue_stop_reason reason);
void ieee80211_stop_queues_by_reason(struct ieee80211_hw *hw,
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 7c6e90a..a4f413e 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -728,6 +728,13 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,

INIT_DELAYED_WORK(&local->scan_work, ieee80211_scan_work);

+ INIT_WORK(&local->dynamic_ps_enable_work,
+ ieee80211_dynamic_ps_enable_work);
+ INIT_WORK(&local->dynamic_ps_disable_work,
+ ieee80211_dynamic_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 2c5f2ac..5173b0e 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);
@@ -872,6 +878,9 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
local->oper_channel_type = NL80211_CHAN_NO_HT;
config_changed |= IEEE80211_CONF_CHANGE_HT;

+ del_timer_sync(&local->dynamic_ps_timer);
+ cancel_work_sync(&local->dynamic_ps_enable_work);
+
if (local->hw.conf.flags & IEEE80211_CONF_PS) {
local->hw.conf.flags &= ~IEEE80211_CONF_PS;
config_changed |= IEEE80211_CONF_CHANGE_PS;
@@ -2620,3 +2629,39 @@ void ieee80211_mlme_notify_scan_completed(struct ieee80211_local *local)
ieee80211_restart_sta_timer(sdata);
rcu_read_unlock();
}
+
+void ieee80211_dynamic_ps_disable_work(struct work_struct *work)
+{
+ struct ieee80211_local *local =
+ container_of(work, struct ieee80211_local,
+ dynamic_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);
+ }
+
+ ieee80211_wake_queues_by_reason(&local->hw,
+ IEEE80211_QUEUE_STOP_REASON_PS);
+}
+
+void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
+{
+ struct ieee80211_local *local =
+ container_of(work, struct ieee80211_local,
+ dynamic_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->dynamic_ps_enable_work);
+}
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 22702e7..27474cf 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1475,6 +1475,19 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
goto fail;
}

+ if (!(local->hw.flags & IEEE80211_HW_NO_STACK_DYNAMIC_PS) &&
+ local->dynamic_ps_timeout > 0) {
+ if (local->hw.conf.flags & IEEE80211_CONF_PS) {
+ ieee80211_stop_queues_by_reason(&local->hw,
+ IEEE80211_QUEUE_STOP_REASON_PS);
+ queue_work(local->hw.workqueue,
+ &local->dynamic_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 f6640d0..7162d58 100644
--- a/net/mac80211/wext.c
+++ b/net/mac80211/wext.c
@@ -833,7 +833,7 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
struct ieee80211_conf *conf = &local->hw.conf;
- int ret = 0;
+ int ret = 0, timeout = 0;
bool ps;

if (sdata->vif.type != NL80211_IFTYPE_STATION)
@@ -841,6 +841,7 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,

if (wrq->disabled) {
ps = false;
+ timeout = 0;
goto set;
}

@@ -850,22 +851,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 (ps == local->powersave)
- return ret;
+ if (wrq->flags & IW_POWER_TIMEOUT)
+ timeout = wrq->value / 1000;

set:
+ if (ps == local->powersave && timeout == local->dynamic_ps_timeout)
+ return ret;
+
local->powersave = ps;
+ local->dynamic_ps_timeout = timeout;

if (sdata->u.sta.flags & IEEE80211_STA_ASSOCIATED) {
- if (local->powersave)
- conf->flags |= IEEE80211_CONF_PS;
- else
- conf->flags &= ~IEEE80211_CONF_PS;
-
+ if (!(local->hw.flags & IEEE80211_HW_NO_STACK_DYNAMIC_PS) &&
+ 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);
}



2008-12-19 06:21:25

by Vivek Natarajan

[permalink] [raw]
Subject: Re: [PATCH v6 3/3] mac80211: implement dynamic power save

On Fri, Dec 19, 2008 at 11:47 AM, Vivek Natarajan
<[email protected]> wrote:
> On Fri, Dec 19, 2008 at 3:05 AM, Kalle Valo <[email protected]> wrote:
>> @@ -872,6 +878,9 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>> local->oper_channel_type = NL80211_CHAN_NO_HT;
>> config_changed |= IEEE80211_CONF_CHANGE_HT;
>>
>> + del_timer_sync(&local->dynamic_ps_timer);
>> + cancel_work_sync(&local->dynamic_ps_enable_work);
>
> In addition, shouldn't we have this 'cancel_work' in ieee80211_stop
> or where ever in the stop/detach path since the ps timer may be
> triggered even after the interface is brought down.

Sorry, ieee80211_set_disassoc is called from ieee80211_stop too...


Vivek.

2008-12-18 21:36:54

by Kalle Valo

[permalink] [raw]
Subject: [PATCH v6 2/3] mac80211: track master queue status

This is a preparation for the dynamic power save support. In future there are
two paths to stop the master queues and we need to track this properly to
avoid starting queues incorrectly. Implement this by adding a status
array for each queue.

The original idea and design is from Johannes Berg, I just did
the implementation based on his notes. All the bugs are mine, of course.

Signed-off-by: Kalle Valo <[email protected]>
---

net/mac80211/ieee80211_i.h | 12 ++++++
net/mac80211/main.c | 2 +
net/mac80211/util.c | 86 +++++++++++++++++++++++++++++++++++++++++---
3 files changed, 93 insertions(+), 7 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 18b9160..a74d673 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -538,6 +538,10 @@ enum {
IEEE80211_ADDBA_MSG = 4,
};

+enum queue_stop_reason {
+ IEEE80211_QUEUE_STOP_REASON_DRIVER,
+};
+
/* maximum number of hardware queues we support. */
#define QD_MAX_QUEUES (IEEE80211_MAX_AMPDU_QUEUES + IEEE80211_MAX_QUEUES)

@@ -554,7 +558,8 @@ struct ieee80211_local {
const struct ieee80211_ops *ops;

unsigned long queue_pool[BITS_TO_LONGS(QD_MAX_QUEUES)];
-
+ unsigned long queue_stop_reasons[IEEE80211_MAX_QUEUES];
+ spinlock_t queue_stop_reason_lock;
struct net_device *mdev; /* wmaster# - "master" 802.11 device */
int open_count;
int monitors, cooked_mntrs;
@@ -972,6 +977,11 @@ 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_wake_queues_by_reason(struct ieee80211_hw *hw,
+ enum queue_stop_reason reason);
+void ieee80211_stop_queues_by_reason(struct ieee80211_hw *hw,
+ enum queue_stop_reason reason);
+
#ifdef CONFIG_MAC80211_NOINLINE
#define debug_noinline noinline
#else
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index a0371ca..7c6e90a 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -724,6 +724,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,

spin_lock_init(&local->key_lock);

+ spin_lock_init(&local->queue_stop_reason_lock);
+
INIT_DELAYED_WORK(&local->scan_work, ieee80211_scan_work);

sta_info_init(local);
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 71a8391..fb89e1d 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -330,10 +330,20 @@ __le16 ieee80211_ctstoself_duration(struct ieee80211_hw *hw,
}
EXPORT_SYMBOL(ieee80211_ctstoself_duration);

-void ieee80211_wake_queue(struct ieee80211_hw *hw, int queue)
+static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue,
+ enum queue_stop_reason reason)
{
struct ieee80211_local *local = hw_to_local(hw);

+ /* we don't need to track ampdu queues */
+ if (queue < ieee80211_num_regular_queues(hw)) {
+ __clear_bit(reason, &local->queue_stop_reasons[queue]);
+
+ if (local->queue_stop_reasons[queue] != 0)
+ /* someone still has this queue stopped */
+ return;
+ }
+
if (test_bit(queue, local->queues_pending)) {
set_bit(queue, local->queues_pending_run);
tasklet_schedule(&local->tx_pending_tasklet);
@@ -341,22 +351,74 @@ void ieee80211_wake_queue(struct ieee80211_hw *hw, int queue)
netif_wake_subqueue(local->mdev, queue);
}
}
+
+void ieee80211_wake_queue_by_reason(struct ieee80211_hw *hw, int queue,
+ enum queue_stop_reason reason)
+{
+ struct ieee80211_local *local = hw_to_local(hw);
+ unsigned long flags;
+
+ spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
+ __ieee80211_wake_queue(hw, queue, reason);
+ spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
+}
+
+void ieee80211_wake_queue(struct ieee80211_hw *hw, int queue)
+{
+ ieee80211_wake_queue_by_reason(hw, queue,
+ IEEE80211_QUEUE_STOP_REASON_DRIVER);
+}
EXPORT_SYMBOL(ieee80211_wake_queue);

-void ieee80211_stop_queue(struct ieee80211_hw *hw, int queue)
+static void __ieee80211_stop_queue(struct ieee80211_hw *hw, int queue,
+ enum queue_stop_reason reason)
{
struct ieee80211_local *local = hw_to_local(hw);

+ /* we don't need to track ampdu queues */
+ if (queue < ieee80211_num_regular_queues(hw))
+ __set_bit(reason, &local->queue_stop_reasons[queue]);
+
netif_stop_subqueue(local->mdev, queue);
}
+
+void ieee80211_stop_queue_by_reason(struct ieee80211_hw *hw, int queue,
+ enum queue_stop_reason reason)
+{
+ struct ieee80211_local *local = hw_to_local(hw);
+ unsigned long flags;
+
+ spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
+ __ieee80211_stop_queue(hw, queue, reason);
+ spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
+}
+
+void ieee80211_stop_queue(struct ieee80211_hw *hw, int queue)
+{
+ ieee80211_stop_queue_by_reason(hw, queue,
+ IEEE80211_QUEUE_STOP_REASON_DRIVER);
+}
EXPORT_SYMBOL(ieee80211_stop_queue);

-void ieee80211_stop_queues(struct ieee80211_hw *hw)
+void ieee80211_stop_queues_by_reason(struct ieee80211_hw *hw,
+ enum queue_stop_reason reason)
{
+ struct ieee80211_local *local = hw_to_local(hw);
+ unsigned long flags;
int i;

+ spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
+
for (i = 0; i < ieee80211_num_queues(hw); i++)
- ieee80211_stop_queue(hw, i);
+ __ieee80211_stop_queue(hw, i, reason);
+
+ spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
+}
+
+void ieee80211_stop_queues(struct ieee80211_hw *hw)
+{
+ ieee80211_stop_queues_by_reason(hw,
+ IEEE80211_QUEUE_STOP_REASON_DRIVER);
}
EXPORT_SYMBOL(ieee80211_stop_queues);

@@ -367,12 +429,24 @@ int ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue)
}
EXPORT_SYMBOL(ieee80211_queue_stopped);

-void ieee80211_wake_queues(struct ieee80211_hw *hw)
+void ieee80211_wake_queues_by_reason(struct ieee80211_hw *hw,
+ enum queue_stop_reason reason)
{
+ struct ieee80211_local *local = hw_to_local(hw);
+ unsigned long flags;
int i;

+ spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
+
for (i = 0; i < hw->queues + hw->ampdu_queues; i++)
- ieee80211_wake_queue(hw, i);
+ __ieee80211_wake_queue(hw, i, reason);
+
+ spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
+}
+
+void ieee80211_wake_queues(struct ieee80211_hw *hw)
+{
+ ieee80211_wake_queues_by_reason(hw, IEEE80211_QUEUE_STOP_REASON_DRIVER);
}
EXPORT_SYMBOL(ieee80211_wake_queues);



2008-12-18 21:36:52

by Kalle Valo

[permalink] [raw]
Subject: [PATCH v6 1/3] mac80211: enable IEEE80211_CONF_PS only when associated

Also disable power save when disassociated. It makes no sense to have
power save enabled while disassociated.

iwlwifi seems to have this check in the driver, but it's better to do this
in mac80211 instead.

Signed-off-by: Kalle Valo <[email protected]>
---

net/mac80211/ieee80211_i.h | 1 +
net/mac80211/mlme.c | 15 +++++++++++++--
net/mac80211/wext.c | 32 ++++++++++++++++++++++++++------
3 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index a7dabae..18b9160 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 9a06905..2c5f2ac 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();

@@ -865,8 +870,14 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,

local->hw.conf.ht.enabled = false;
local->oper_channel_type = NL80211_CHAN_NO_HT;
- 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_CHANGE_PS;
+ }

+ ieee80211_hw_config(local, config_changed);
ieee80211_bss_info_change_notify(sdata, changed);

rcu_read_lock();
diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
index 1542804..f6640d0 100644
--- a/net/mac80211/wext.c
+++ b/net/mac80211/wext.c
@@ -830,25 +830,46 @@ 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_conf *conf = &local->hw.conf;
+ int ret = 0;
+ bool ps;
+
+ if (sdata->vif.type != NL80211_IFTYPE_STATION)
+ return -EINVAL;

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 (sdata->u.sta.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,
@@ -857,9 +878,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;
}


2008-12-19 07:37:32

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v6 3/3] mac80211: implement dynamic power save

Vivek Natarajan <[email protected]> writes:

> On Fri, Dec 19, 2008 at 11:47 AM, Vivek Natarajan
> <[email protected]> wrote:
>> On Fri, Dec 19, 2008 at 3:05 AM, Kalle Valo <[email protected]> wrote:
>>> @@ -872,6 +878,9 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>>> local->oper_channel_type = NL80211_CHAN_NO_HT;
>>> config_changed |= IEEE80211_CONF_CHANGE_HT;
>>>
>>> + del_timer_sync(&local->dynamic_ps_timer);
>>> + cancel_work_sync(&local->dynamic_ps_enable_work);
>>
>> In addition, shouldn't we have this 'cancel_work' in ieee80211_stop
>> or where ever in the stop/detach path since the ps timer may be
>> triggered even after the interface is brought down.
>
> Sorry, ieee80211_set_disassoc is called from ieee80211_stop too...

Yeah, I got lucky. John fixed this a while ago.

--
Kalle Valo

2008-12-28 12:55:00

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v6 0/3] mac80211 dynamic power save

Johannes Berg <[email protected]> writes:

> On Thu, 2008-12-18 at 23:35 +0200, Kalle Valo wrote:
>> Here is v5 of my dynamic powersave patches. I have tested them with
>> stlc45xx on Nokia N810.
>
>> Power save related features which I'll implement later:
>> o disable power save when software scanning
>> o move dynamic_ps_timeout to hw.conf so that drivers can use it in
>> case they need to
>> o beacon period (wakeup period) to the drivers, can be set from user
>> space but otherwise is bss's DTIM value
>
> Now that these are in, and more things for user interface are on the
> way, we should probably think about implementing the policy in
> userspace.

Good idea. Also we should consider the interface to the user space,
what parameters we want to provide to user space etc.

> Should that be part of NM?

I think it should be in NetworkManager, and also Intel's Connection
Manager might want to control power save parameters. And in Nokia we
want to control power save parameters from wlancond, our own Wireless
Extension dbus wrapper.

> It could take into account things like the system power state
> (AC/battery) for the decision, at least by default?

Actually I'm not that enthuastic about AC/battery mode detection, I
would like to save power even in AC mode. In my opinion we should have
more dynamic logic based on current state, for example based on amount
of transfered traffic, QoS, user activity (eg. is the display blanked)
and what not. But I guess AC/battery detection is a good start.

As for the user space interface, here's some ideas I have been
thinking:

Wakeup period (iwconfig wlan0 power period 2), sleep timeout (iwconfig
wlan0 power timeout 500m) and enable/disable of power save (iwconfig
wlan0 power on) are enough for now. If needed, we can add more
parameters later.

Wakeup period should be the DTIM value of the BSS by default and set
by mac80211, but user space can change the value whenever it wants.
Power save should be disabled by default in mac80211 to avoid all the
interoperability problems (there's enough of those, especially with
old APs), but it would be nice to have an easy way for the user to
enable power save from NetworkManager.

--
Kalle Valo

2008-12-23 21:55:40

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v6 0/3] mac80211 dynamic power save

On Thu, 2008-12-18 at 23:35 +0200, Kalle Valo wrote:
> Here is v5 of my dynamic powersave patches. I have tested them with
> stlc45xx on Nokia N810.

> Power save related features which I'll implement later:
> o disable power save when software scanning
> o move dynamic_ps_timeout to hw.conf so that drivers can use it in
> case they need to
> o beacon period (wakeup period) to the drivers, can be set from user
> space but otherwise is bss's DTIM value

Now that these are in, and more things for user interface are on the
way, we should probably think about implementing the policy in
userspace. Should that be part of NM? It could take into account things
like the system power state (AC/battery) for the decision, at least by
default?

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-12-19 07:45:33

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v6 3/3] mac80211: implement dynamic power save

Vivek Natarajan <[email protected]> writes:

> I need your thoughts on having null frame being sent from mac80211(
> the RFC I submitted earlier). Do you feel a state machine is
> necessary to go to power save? ( like awake, sleep, pending_sleep if
> the AP hasn't acked the null frame with a timer or a work queue to
> send null frame periodically in the pending state).

I haven't thinked much about this yet, but your proposal of having a
state machine most probably is the best option. It's just that earlier
Johannes mentioned that it's not currently possible to know if null
frame has been acked:

http://www.spinics.net/lists/linux-wireless/msg21212.html

We would have to solve that before implementing the state machine.

> Or altogether you want that to be part of the driver and not in
> mac80211?

I think this needs to be in mac80211 because there are three drivers
which need it (p54/stlc45xx, ath5/9k and b43).

--
Kalle Valo

2008-12-19 06:18:02

by Vivek Natarajan

[permalink] [raw]
Subject: Re: [PATCH v6 3/3] mac80211: implement dynamic power save

On Fri, Dec 19, 2008 at 3:05 AM, Kalle Valo <[email protected]> wrote:
> @@ -872,6 +878,9 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
> local->oper_channel_type = NL80211_CHAN_NO_HT;
> config_changed |= IEEE80211_CONF_CHANGE_HT;
>
> + del_timer_sync(&local->dynamic_ps_timer);
> + cancel_work_sync(&local->dynamic_ps_enable_work);

In addition, shouldn't we have this 'cancel_work' in ieee80211_stop
or where ever in the stop/detach path since the ps timer may be
triggered even after the interface is brought down.

I need your thoughts on having null frame being sent from mac80211(
the RFC I submitted earlier).
Do you feel a state machine is necessary to go to power save? ( like
awake, sleep, pending_sleep if the AP hasn't acked the null frame with
a timer or a work queue to send null frame periodically in the pending
state). Or altogether you want that to be part of the driver and not
in mac80211?

Thanks,
Vivek.