From: Emmanuel Grumbach <[email protected]>
This patch makes mac80211 able to send action frames when toggling
(SM PS) Spacial Multiplexing Power Save mode.
Examples:
1.
A driver in order to lower power consumption may choose to close some of RX chains
in PS mode and needs to announce to AP that a RTS frame is need in order to wake all
RX chains (SM_PS_DYNAMIC), on other hand SM PS can be disabled in CAM mode or if HW is able
to wake all chains without RTS frame and thus reducing traffic overhead.
2.
iwlwifi can call this function when it detects that only
one antenna can effectively receive then SM PS mode is set to STATIC and
AP is asked not to send MIMO rates packets at all.
Signed-off-by: Emmanuel Grumbach <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
include/linux/ieee80211.h | 14 +++++++-
include/net/mac80211.h | 10 ++++++
net/mac80211/ht.c | 79 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 102 insertions(+), 1 deletions(-)
diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 14126bc..d0bd612 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -623,6 +623,10 @@ struct ieee80211_mgmt {
} __attribute__((packed)) addba_resp;
struct{
u8 action_code;
+ u8 ps_ctrl;
+ } __attribute__((packed)) sm_ps;
+ struct{
+ u8 action_code;
__le16 params;
__le16 reason_code;
} __attribute__((packed)) delba;
@@ -759,12 +763,14 @@ struct ieee80211_ht_addt_info {
#define IEEE80211_MIN_AMPDU_BUF 0x8
#define IEEE80211_MAX_AMPDU_BUF 0x40
-
/* Spatial Multiplexing Power Save Modes */
#define WLAN_HT_CAP_SM_PS_STATIC 0
#define WLAN_HT_CAP_SM_PS_DYNAMIC 1
#define WLAN_HT_CAP_SM_PS_INVALID 2
#define WLAN_HT_CAP_SM_PS_DISABLED 3
+/* Spatial Multiplexing control bits */
+#define IEEE80211_SM_PS_ENABLE 0x1
+#define IEEE80211_SM_PS_DYNAMIC 0x2
/* Authentication algorithms */
#define WLAN_AUTH_OPEN 0
@@ -964,6 +970,7 @@ enum ieee80211_category {
WLAN_CATEGORY_QOS = 1,
WLAN_CATEGORY_DLS = 2,
WLAN_CATEGORY_BACK = 3,
+ WLAN_CATEGORY_HT = 7,
WLAN_CATEGORY_WMM = 17,
};
@@ -976,6 +983,11 @@ enum ieee80211_spectrum_mgmt_actioncode {
WLAN_ACTION_SPCT_CHL_SWITCH = 4,
};
+/* HT action code */
+enum ieee80211_ht_actioncode {
+ WLAN_ACTION_SM_PS = 1,
+};
+
/* BACK action code */
enum ieee80211_back_actioncode {
WLAN_ACTION_ADDBA_REQ = 0,
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index f5f5b1f..f15e495 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1868,4 +1868,14 @@ rate_lowest_index(struct ieee80211_supported_band *sband,
int ieee80211_rate_control_register(struct rate_control_ops *ops);
void ieee80211_rate_control_unregister(struct rate_control_ops *ops);
+/**
+ * ieee80211_send_sm_ps_update - send SM Power Save Action frame
+ * @hw: pointer as obtained from ieee80211_alloc_hw().
+ * @new_mode: new SM Power Save mode WLAN_HT_CAP_SM_PS_*
+ *
+ * This function must be called by low level driver to inform AP about change
+ * in SM Power Save state.
+ */
+void ieee80211_sm_ps_update(struct ieee80211_hw *hw, u8 new_mode);
+
#endif /* MAC80211_H */
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index dc7d9a3..e468137 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -990,3 +990,82 @@ void ieee80211_process_delba(struct ieee80211_sub_if_data *sdata,
WLAN_BACK_RECIPIENT);
}
}
+
+static void ieee80211_send_sm_ps(struct ieee80211_sub_if_data *sdata, u8 mode)
+{
+ struct ieee80211_local *local = sdata->local;
+ struct ieee80211_mgmt *mgmt;
+ struct sk_buff *skb;
+ DECLARE_MAC_BUF(mac);
+
+ struct ieee80211_if_sta *ifsta = &sdata->u.sta;
+ struct net_device *dev = sdata->dev;
+
+ /* Implemented for STA only */
+ if (sdata->vif.type != NL80211_IFTYPE_STATION)
+ return;
+
+ skb = dev_alloc_skb(sizeof(*mgmt) +
+ local->hw.extra_tx_headroom);
+
+ if (!skb) {
+ printk(KERN_ERR "%s: failed to allocate buffer "
+ "for SM_PS frame\n", dev->name);
+ return;
+ }
+ skb_reserve(skb, local->hw.extra_tx_headroom);
+ mgmt = (struct ieee80211_mgmt *) skb_put(skb, 24);
+ memset(mgmt, 0, 24);
+ memcpy(mgmt->da, ifsta->bssid, ETH_ALEN);
+ memcpy(mgmt->sa, dev->dev_addr, ETH_ALEN);
+ memcpy(mgmt->bssid, ifsta->bssid, ETH_ALEN);
+
+ mgmt->frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT |
+ IEEE80211_STYPE_ACTION);
+
+ skb_put(skb, 1 + sizeof(mgmt->u.action.u.sm_ps));
+
+ mgmt->u.action.category = WLAN_CATEGORY_HT;
+ mgmt->u.action.u.sm_ps.action_code = WLAN_ACTION_SM_PS;
+
+ switch (mode) {
+
+ case WLAN_HT_CAP_SM_PS_DISABLED:
+ mgmt->u.action.u.sm_ps.ps_ctrl &=
+ ~(IEEE80211_SM_PS_ENABLE |
+ IEEE80211_SM_PS_DYNAMIC);
+ break;
+ case WLAN_HT_CAP_SM_PS_DYNAMIC:
+ mgmt->u.action.u.sm_ps.ps_ctrl |=
+ IEEE80211_SM_PS_ENABLE |
+ IEEE80211_SM_PS_DYNAMIC;
+ break;
+ case WLAN_HT_CAP_SM_PS_STATIC:
+ mgmt->u.action.u.sm_ps.ps_ctrl |=
+ IEEE80211_SM_PS_ENABLE;
+ mgmt->u.action.u.sm_ps.ps_ctrl &=
+ ~IEEE80211_SM_PS_DYNAMIC;
+ break;
+ default:
+ printk(KERN_DEBUG "%s: invalid power save mode\n",
+ dev->name);
+ WARN_ON(1);
+ }
+
+ if (ifsta->flags & IEEE80211_STA_ASSOCIATED)
+ ieee80211_tx_skb(sdata, skb, 0);
+}
+
+void ieee80211_sm_ps_update(struct ieee80211_hw *hw, u8 new_mode)
+{
+ struct ieee80211_sub_if_data *sdata;
+ struct ieee80211_local *local = hw_to_local(hw);
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+ ieee80211_send_sm_ps(sdata, new_mode);
+ }
+ rcu_read_unlock();
+}
+EXPORT_SYMBOL(ieee80211_sm_ps_update);
+
--
1.5.4.3
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
From: Emmanuel Grumbach <[email protected]>
Upon change in power save state the driver may want to change also the SM_PS
state. PS and SM_PS are generally independent but it's logical to tight them together
to narrow the interface.
In PS mode driver if switches off RX chains, it also might need to change
to DYNAMIC SM_PS state.
When going back to CAM mode, the driver may need to go to DISABLE SM_PS state.
This patch allows the driver to configure which SM_SP state to switch to
upon change in power state.
Signed-off-by: Emmanuel Grumbach <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
include/net/mac80211.h | 8 ++++++++
net/mac80211/ht.c | 5 +++++
net/mac80211/main.c | 5 +++++
net/mac80211/mlme.c | 3 +++
net/mac80211/wext.c | 32 ++++++++++++++++++++++----------
5 files changed, 43 insertions(+), 10 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index f15e495..0edcd18 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -459,6 +459,8 @@ enum ieee80211_conf_flags {
* @max_antenna_gain: maximum antenna gain (in dBi)
* @antenna_sel_tx: transmit antenna selection, 0: default/diversity,
* 1/2: antenna 0/1
+ * @sm_ps_mode: spatial multiplexing power save mode
+ * takes values of: WLAN_HT_CAP_SM_PS_YYYY
* @antenna_sel_rx: receive antenna selection, like @antenna_sel_tx
* @ht_conf: describes current self configuration of 802.11n HT capabilies
* @ht_bss_conf: describes current BSS configuration of 802.11n HT parameters
@@ -474,6 +476,8 @@ struct ieee80211_conf {
int max_antenna_gain;
u8 antenna_sel_tx;
u8 antenna_sel_rx;
+ u8 sm_ps_mode;
+
struct ieee80211_channel *channel;
@@ -828,6 +832,8 @@ enum ieee80211_hw_flags {
* within &struct ieee80211_vif.
* @sta_data_size: size (in bytes) of the drv_priv data area
* within &struct ieee80211_sta.
+ * @sm_ps_psp_mode: what sm_ps mode should be use while in PSP mode
+ * @sm_ps_cam_mode: what sm_ps mode should be use while in CAM mode
*/
struct ieee80211_hw {
struct ieee80211_conf conf;
@@ -843,6 +849,8 @@ struct ieee80211_hw {
u16 queues;
u16 ampdu_queues;
u16 max_listen_interval;
+ u8 sm_ps_psp_mode;
+ u8 sm_ps_cam_mode;
s8 max_signal;
};
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index e468137..c0f7454 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -1061,11 +1061,16 @@ void ieee80211_sm_ps_update(struct ieee80211_hw *hw, u8 new_mode)
struct ieee80211_sub_if_data *sdata;
struct ieee80211_local *local = hw_to_local(hw);
+ if (local->hw.conf.sm_ps_mode == new_mode)
+ return;
+
rcu_read_lock();
list_for_each_entry_rcu(sdata, &local->interfaces, list) {
ieee80211_send_sm_ps(sdata, new_mode);
}
rcu_read_unlock();
+
+ local->hw.conf.sm_ps_mode = new_mode;
}
EXPORT_SYMBOL(ieee80211_sm_ps_update);
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index d608c44..ba5834b 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -760,6 +760,11 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
local->short_retry_limit = 7;
local->long_retry_limit = 4;
local->hw.conf.radio_enabled = 1;
+
+ local->hw.sm_ps_psp_mode = WLAN_HT_CAP_SM_PS_DISABLED;
+ local->hw.sm_ps_cam_mode = WLAN_HT_CAP_SM_PS_DISABLED;
+ local->hw.conf.sm_ps_mode = WLAN_HT_CAP_SM_PS_DISABLED;
+
INIT_LIST_HEAD(&local->interfaces);
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 4c9dc81..1ead57b 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -416,6 +416,9 @@ static void ieee80211_send_assoc(struct ieee80211_sub_if_data *sdata,
break;
}
+ cap &= ~IEEE80211_HT_CAP_SM_PS;
+ cap |= local->hw.conf.sm_ps_mode << 2;
+
tmp = cpu_to_le16(cap);
pos = skb_put(skb, sizeof(struct ieee80211_ht_cap)+2);
*pos++ = WLAN_EID_HT_CAPABILITY;
diff --git a/net/mac80211/wext.c b/net/mac80211/wext.c
index 7e0d53a..ba907f4 100644
--- a/net/mac80211/wext.c
+++ b/net/mac80211/wext.c
@@ -977,21 +977,33 @@ static int ieee80211_ioctl_siwpower(struct net_device *dev,
{
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
struct ieee80211_conf *conf = &local->hw.conf;
+ struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+ u8 sm_ps_mode;
+
+ if (sdata->vif.type == NL80211_IFTYPE_AP)
+ return -EOPNOTSUPP;
if (wrq->disabled) {
conf->flags &= ~IEEE80211_CONF_PS;
- return ieee80211_hw_config(local);
+ sm_ps_mode = local->hw.sm_ps_cam_mode;
+ } else {
+ 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;
+ sm_ps_mode = local->hw.sm_ps_psp_mode;
+ break;
+ default: /* Otherwise we don't support it */
+ return -EINVAL;
+ }
}
- 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;
- break;
- default: /* Otherwise we don't support it */
- return -EINVAL;
- }
+ if (sdata->bss_conf.assoc_ht)
+ /* we need to update the AP about the change.
+ * If we are in HT, then we are in STA mode
+ * conf->sm_ps_mode is updated insde the function*/
+ ieee80211_sm_ps_update(&local->hw, sm_ps_mode);
return ieee80211_hw_config(local);
}
--
1.5.4.3
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
On Thu, 2008-10-02 at 14:03 +0300, Tomas Winkler wrote:
> >> +static void ieee80211_send_sm_ps(struct ieee80211_sub_if_data *sdata, u8 mode)
> >
> > Can we use an enum for 'mode'?
>
> We use spec numbers here why to invent more numbers.
Well we can also just put the spec numbers into an enum and use that
enum here. enum doesn't mean that we don't fix the numbers :)
> >> + /* Implemented for STA only */
> >> + if (sdata->vif.type != NL80211_IFTYPE_STATION)
> >> + return;
> >
> > What about mesh, ibss, wds, ...? I see this doesn't make much sense for
> > an AP, but...?
>
> This is explicitly defined in spec for STA mode.
Ok. Maybe change the comment then so that it's more obvious.
> > It seems that for the second use case you're citing you've now done
> > something that might lead to bouncing back and forth because there is no
> > central instance deciding on the powersave mode. You seem to be trying
> > to avoid this by introducing the two new variables sm_ps_psp_mode and
> > sm_ps_cam_mode, but this seems very strange. Is the driver supposed to
> > modify them at runtime?
>
> No it should be set on registration only
>
> What sort of policy does the driver impose on
> > them? Why is this policy driver-specific?
>
> Depends on radio ability rather then policy. The only policy that is
> set here is that we coupling SM PS and PS
>
> Why doesn't the driver just
> > inform mac80211 of the antenna status and mac80211 then decides based on
> > the antenna status and the requested powersave mode which SM PS mode
> > should be activated, and then notifies the driver of that?
>
> Correct that's the solution, just keep in mind that driver change rx
> chain configuration upon SM PS request as well.
Yeah but you were saying it's about detecting whether MIMO is effective
or not. So I think what makes more sense is to have the driver _only_
push that information to mac80211, and then wait for mac80211 to make a
decision about the SM PS mode, and not reconfigure its chains
immediately. Therefore, it wouldn't change chain configuration when it
detects MIMO doesn't work, it would only do that when mac80211 requests
it based on the information.
> > I'd much rather see you implement this in a different way:
> > * a HW flag that determines whether the driver can wake up with or
> > without an RTS frame (?)
>
> Not enough there are 3 states you may request in SM_PS
Yes, but this is just a hardware capability which indirectly influences
the PS mode, no? So not having the wakeup-without-rts capability may
mean that SM_PS_XYZ cannot be used or whatever.
> > - possibly interface modes (maybe no need for MIMO when in mesh etc.)
>
> Out of scope, HT is not enabled at all in these modes.
currently.
I'll look at more details after lunch.
johannes
On Thu, Oct 2, 2008 at 11:37 AM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2008-09-30 at 23:07 +0300, Tomas Winkler wrote:
>
> Some code comments first, I'll cover the design below.
>
>> +static void ieee80211_send_sm_ps(struct ieee80211_sub_if_data *sdata, u8 mode)
>
> Can we use an enum for 'mode'?
We use spec numbers here why to invent more numbers.
>> + /* Implemented for STA only */
>> + if (sdata->vif.type != NL80211_IFTYPE_STATION)
>> + return;
>
> What about mesh, ibss, wds, ...? I see this doesn't make much sense for
> an AP, but...?
This is explicitly defined in spec for STA mode.
>
>> + switch (mode) {
>> +
>
> spurious blank line
>
>> + if (ifsta->flags & IEEE80211_STA_ASSOCIATED)
>> + ieee80211_tx_skb(sdata, skb, 0);
>
> Umm. Why don't you check this before the allocation so that it doesn't
> leak? Also, this probably should return the out of memory status so that
> the driver can decide to not switch SM PS mode in that case? Although
> then the system is probably pretty much dead anyway...
>
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
>> + ieee80211_send_sm_ps(sdata, new_mode);
>> + }
>
> no need for braces there
Will fix.
>
> So, let's get to the design, it's really confusing. Especially with
> patch 2 in there as well, this gets _really_ confusing. On the one hand,
> this is being called by mac80211 when the user changes the power save
> mode and in that case the driver is notified of the powersave mode, but
> on the other hand this is also exported to drivers?
It should not be exported to driver, I think I was clear in the last
mail that I didn't like it myself
and I'm looking for popper way to do that.
> It seems that for the second use case you're citing you've now done
> something that might lead to bouncing back and forth because there is no
> central instance deciding on the powersave mode. You seem to be trying
> to avoid this by introducing the two new variables sm_ps_psp_mode and
> sm_ps_cam_mode, but this seems very strange. Is the driver supposed to
> modify them at runtime?
No it should be set on registration only
What sort of policy does the driver impose on
> them? Why is this policy driver-specific?
Depends on radio ability rather then policy. The only policy that is
set here is that we coupling SM PS and PS
Why doesn't the driver just
> inform mac80211 of the antenna status and mac80211 then decides based on
> the antenna status and the requested powersave mode which SM PS mode
> should be activated, and then notifies the driver of that?
Correct that's the solution, just keep in mind that driver change rx
chain configuration upon SM PS request as well.
> I don't like this patch. You're putting some policy into the driver that
> seems should not be different between drivers. Also, these backward
> calls from the driver to mac80211 that just update the AP's status make
> it rather complex to get the API right, with multiple variables that
> need updating.
I don't like it either, the development strategy is first make it work
and then beautify and optimize
> I'd much rather see you implement this in a different way:
> * a HW flag that determines whether the driver can wake up with or
> without an RTS frame (?)
Not enough there are 3 states you may request in SM_PS
> * some way to tell mac80211 that MIMO isn't currently effective
Yes that we shell implement.
> * mac80211 determines the SM PS mode based on
> - the two inputs from the driver above
> - user input
Correct
> - association status (no need for all chains when not associated)
SM_PS mode has meaning only in association state, before association
it will only make sense in what state we enter association and we may
announce our SM_PS state already in association.
> - possibly interface modes (maybe no need for MIMO when in mesh etc.)
Out of scope, HT is not enabled at all in these modes.
> * mac80211 notifies the driver about the SM PS mode it should switch to
> via the hw config callback
That's the current design
> I'm worried especially about the third bullet point, you've implemented
> the fourth but the driver is allowed to override this and mac80211 can't
> even make a proper decision because it doesn't have the inputs. I don't
> think having the first two inputs to mac80211 is "opening too much
> guts", it is, after all, something the policy engine needs, and that
> policy engine shouldn't be split up between the driver and mac80211.
I didn't submit any patch for that yet, believe me if I thought it's
closed issue you will have it in your mailbox already :)
Thanks for your valuable input
Tomas
On Tue, 2008-09-30 at 23:07 +0300, Tomas Winkler wrote:
Some code comments first, I'll cover the design below.
> +static void ieee80211_send_sm_ps(struct ieee80211_sub_if_data *sdata, u8 mode)
Can we use an enum for 'mode'?
> + /* Implemented for STA only */
> + if (sdata->vif.type != NL80211_IFTYPE_STATION)
> + return;
What about mesh, ibss, wds, ...? I see this doesn't make much sense for
an AP, but...?
> + switch (mode) {
> +
spurious blank line
> + if (ifsta->flags & IEEE80211_STA_ASSOCIATED)
> + ieee80211_tx_skb(sdata, skb, 0);
Umm. Why don't you check this before the allocation so that it doesn't
leak? Also, this probably should return the out of memory status so that
the driver can decide to not switch SM PS mode in that case? Although
then the system is probably pretty much dead anyway...
> + rcu_read_lock();
> + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> + ieee80211_send_sm_ps(sdata, new_mode);
> + }
no need for braces there
So, let's get to the design, it's really confusing. Especially with
patch 2 in there as well, this gets _really_ confusing. On the one hand,
this is being called by mac80211 when the user changes the power save
mode and in that case the driver is notified of the powersave mode, but
on the other hand this is also exported to drivers?
It seems that for the second use case you're citing you've now done
something that might lead to bouncing back and forth because there is no
central instance deciding on the powersave mode. You seem to be trying
to avoid this by introducing the two new variables sm_ps_psp_mode and
sm_ps_cam_mode, but this seems very strange. Is the driver supposed to
modify them at runtime? What sort of policy does the driver impose on
them? Why is this policy driver-specific? Why doesn't the driver just
inform mac80211 of the antenna status and mac80211 then decides based on
the antenna status and the requested powersave mode which SM PS mode
should be activated, and then notifies the driver of that?
I don't like this patch. You're putting some policy into the driver that
seems should not be different between drivers. Also, these backward
calls from the driver to mac80211 that just update the AP's status make
it rather complex to get the API right, with multiple variables that
need updating.
I'd much rather see you implement this in a different way:
* a HW flag that determines whether the driver can wake up with or
without an RTS frame (?)
* some way to tell mac80211 that MIMO isn't currently effective
* mac80211 determines the SM PS mode based on
- the two inputs from the driver above
- user input
- association status (no need for all chains when not associated)
- possibly interface modes (maybe no need for MIMO when in mesh etc.)
* mac80211 notifies the driver about the SM PS mode it should switch to
via the hw config callback
I'm worried especially about the third bullet point, you've implemented
the fourth but the driver is allowed to override this and mac80211 can't
even make a proper decision because it doesn't have the inputs. I don't
think having the first two inputs to mac80211 is "opening too much
guts", it is, after all, something the policy engine needs, and that
policy engine shouldn't be split up between the driver and mac80211.
johannes