[RFC] mac80211: add PS flag to bss_conf

[RFC] mac80211: add PS flag to bss_conf

[Eliad Peller]

Currently, ps mode is indicated per device (rather than
per interface), which doesn't make a lot of sense.

Moreover, there are subtle bugs caused by the inability
to indicate ps change along with other changes
(e.g. when the AP deauth us, we'd like to indicate
CHANGED_PS | CHANGED_ASSOC, as changing PS before
notifying about disassociation will result in null-packets
being sent (if IEEE80211_HW_SUPPORTS_DYNAMIC_PS) while
the sta is already disconnected.)

Keep the current per-device notifications, and add
additional per-vif notifications. In order to keep it
simple, these notifications are NOT called due to
(temporary) dynamic_ps/offchannel operations.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 include/net/mac80211.h |    5 +++++
 net/mac80211/mlme.c    |   22 ++++++++++++++++++++++
 net/mac80211/util.c    |    3 ++-
 3 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 3a700fd..1fcb109 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -171,6 +171,7 @@ struct ieee80211_low_level_stats {
  * @BSS_CHANGED_IDLE: Idle changed for this BSS/interface.
  * @BSS_CHANGED_SSID: SSID changed for this BSS (AP mode)
  * @BSS_CHANGED_AP_PROBE_RESP: Probe Response changed for this BSS (AP mode)
+ * @BSS_CHANGED_PS: PS changed for this BSS (STA mode)
  */
 enum ieee80211_bss_change {
 	BSS_CHANGED_ASSOC		= 1<<0,
@@ -190,6 +191,7 @@ enum ieee80211_bss_change {
 	BSS_CHANGED_IDLE		= 1<<14,
 	BSS_CHANGED_SSID		= 1<<15,
 	BSS_CHANGED_AP_PROBE_RESP	= 1<<16,
+	BSS_CHANGED_PS			= 1<<17,
 
 	/* when adding here, make sure to change ieee80211_reconfig */
 };
@@ -266,6 +268,8 @@ enum ieee80211_rssi_event {
  * @idle: This interface is idle. There's also a global idle flag in the
  *	hardware config which may be more appropriate depending on what
  *	your driver/device needs to do.
+ * @ps: power-save mode (STA only). This flag is NOT affected by
+ *	offchannel/dynamic_ps operations.
  * @ssid: The SSID of the current vif. Only valid in AP-mode.
  * @ssid_len: Length of SSID given in @ssid.
  * @hidden_ssid: The SSID of the current vif is hidden. Only valid in AP-mode.
@@ -296,6 +300,7 @@ struct ieee80211_bss_conf {
 	bool arp_filter_enabled;
 	bool qos;
 	bool idle;
+	bool ps;
 	u8 ssid[IEEE80211_MAX_SSID_LEN];
 	size_t ssid_len;
 	bool hidden_ssid;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index bf3da5a..b8452d5 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1004,6 +1004,26 @@ void ieee80211_recalc_ps(struct ieee80211_local *local, s32 latency)
 		local->ps_sdata = NULL;
 	}
 
+	list_for_each_entry(sdata, &local->interfaces, list) {
+		if (!ieee80211_sdata_running(sdata))
+			continue;
+		if (sdata->vif.type != NL80211_IFTYPE_STATION)
+			continue;
+
+		/* already in ps */
+		if (sdata == local->ps_sdata &&
+		    sdata->vif.bss_conf.ps)
+			continue;
+
+		/* already active */
+		if (sdata != local->ps_sdata &&
+		    !sdata->vif.bss_conf.ps)
+			continue;
+
+		sdata->vif.bss_conf.ps = local->ps_sdata == sdata;
+		ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_PS);
+	}
+
 	ieee80211_change_ps(local);
 }
 
@@ -1368,6 +1388,8 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 	if (local->hw.conf.flags & IEEE80211_CONF_PS) {
 		local->hw.conf.flags &= ~IEEE80211_CONF_PS;
 		ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+		sdata->vif.bss_conf.ps = false;
+		changed |= BSS_CHANGED_PS;
 	}
 	local->ps_sdata = NULL;
 
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 39b82fe..037d148 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1359,7 +1359,8 @@ int ieee80211_reconfig(struct ieee80211_local *local)
 		switch (sdata->vif.type) {
 		case NL80211_IFTYPE_STATION:
 			changed |= BSS_CHANGED_ASSOC |
-				   BSS_CHANGED_ARP_FILTER;
+				   BSS_CHANGED_ARP_FILTER |
+				   BSS_CHANGED_PS;
 			mutex_lock(&sdata->u.mgd.mtx);
 			ieee80211_bss_info_change_notify(sdata, changed);
 			mutex_unlock(&sdata->u.mgd.mtx);
-- 
1.7.6.401.g6a319

Re: [RFC] mac80211: add PS flag to bss_conf

[Johannes Berg]

On Thu, 2012-07-19 at 16:11 +0300, Eliad Peller wrote:
> Currently, ps mode is indicated per device (rather than
> per interface), which doesn't make a lot of sense.
> 
> Moreover, there are subtle bugs caused by the inability
> to indicate ps change along with other changes
> (e.g. when the AP deauth us, we'd like to indicate
> CHANGED_PS | CHANGED_ASSOC, as changing PS before
> notifying about disassociation will result in null-packets
> being sent (if IEEE80211_HW_SUPPORTS_DYNAMIC_PS) while
> the sta is already disconnected.)
> 
> Keep the current per-device notifications, and add
> additional per-vif notifications. In order to keep it
> simple, these notifications are NOT called due to
> (temporary) dynamic_ps/offchannel operations.

So I think if we do this, it would make sense to not do it in recalc_ps,
but like you did in disassoc:

> @@ -1368,6 +1388,8 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>  	if (local->hw.conf.flags & IEEE80211_CONF_PS) {
>  		local->hw.conf.flags &= ~IEEE80211_CONF_PS;
>  		ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
> +		sdata->vif.bss_conf.ps = false;
> +		changed |= BSS_CHANGED_PS;
>  	}
>  	local->ps_sdata = NULL;

set it directly in the relevant code.


However, I'm not sure that we really should toggle it with association?
For the original CONF_PS semantics where mac80211 actually woke up for
dynamic PS etc. this made sense, but for smarter devices I think the PS
flag could be solely mirroring the combination of
 a) broken AP => no PS
 b) user enable/disable

even when not associated?

johannes

Re: [RFC] mac80211: add PS flag to bss_conf

[Eliad Peller]

On Tue, Jul 24, 2012 at 5:34 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2012-07-19 at 16:11 +0300, Eliad Peller wrote:
>> Currently, ps mode is indicated per device (rather than
>> per interface), which doesn't make a lot of sense.
>>
>> Moreover, there are subtle bugs caused by the inability
>> to indicate ps change along with other changes
>> (e.g. when the AP deauth us, we'd like to indicate
>> CHANGED_PS | CHANGED_ASSOC, as changing PS before
>> notifying about disassociation will result in null-packets
>> being sent (if IEEE80211_HW_SUPPORTS_DYNAMIC_PS) while
>> the sta is already disconnected.)
>>
>> Keep the current per-device notifications, and add
>> additional per-vif notifications. In order to keep it
>> simple, these notifications are NOT called due to
>> (temporary) dynamic_ps/offchannel operations.
>
> So I think if we do this, it would make sense to not do it in recalc_ps,
> but like you did in disassoc:
>
>> @@ -1368,6 +1388,8 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>>       if (local->hw.conf.flags & IEEE80211_CONF_PS) {
>>               local->hw.conf.flags &= ~IEEE80211_CONF_PS;
>>               ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
>> +             sdata->vif.bss_conf.ps = false;
>> +             changed |= BSS_CHANGED_PS;
>>       }
>>       local->ps_sdata = NULL;
>
> set it directly in the relevant code.
>
>
> However, I'm not sure that we really should toggle it with association?
> For the original CONF_PS semantics where mac80211 actually woke up for
> dynamic PS etc. this made sense, but for smarter devices I think the PS
> flag could be solely mirroring the combination of
>  a) broken AP => no PS
>  b) user enable/disable
>
ok, i think i got your point.

> even when not associated?

we actually check for authorization, so i think it does make some
sense to leave this check.

thanks for the review.
i'll apply your comments and send it as a patch.

Eliad.