Even if the null data frame is not acked by the AP, mac80211
goes into power save. This might lead to loss of frames
from the AP.
Prevent this by restarting dynamic_ps_timer when ack is not
received for null data frames.
Signed-off-by: Vivek Natarajan <[email protected]>
---
include/net/mac80211.h | 1 +
net/mac80211/ieee80211_i.h | 1 +
net/mac80211/mlme.c | 21 +++++++++++++++------
net/mac80211/status.c | 16 ++++++++++++++--
4 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 74ccf30..92a3caf 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -978,6 +978,7 @@ enum ieee80211_hw_flags {
IEEE80211_HW_SUPPORTS_STATIC_SMPS = 1<<15,
IEEE80211_HW_SUPPORTS_DYNAMIC_SMPS = 1<<16,
IEEE80211_HW_SUPPORTS_UAPSD = 1<<17,
+ IEEE80211_HW_TX_STATUS = 1<<18,
};
/**
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 3067fbd..f50a17a 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -316,6 +316,7 @@ enum ieee80211_sta_flags {
IEEE80211_STA_CSA_RECEIVED = BIT(5),
IEEE80211_STA_MFP_ENABLED = BIT(6),
IEEE80211_STA_UAPSD_ENABLED = BIT(7),
+ IEEE80211_STA_NULLFUNC_ACKED = BIT(8),
};
struct ieee80211_if_managed {
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 86c6ad1..c33956b 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -439,7 +439,8 @@ static void ieee80211_enable_ps(struct ieee80211_local *local,
if (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK)
ieee80211_send_nullfunc(local, sdata, 1);
conf->flags |= IEEE80211_CONF_PS;
- ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+ if (!(local->hw.flags & IEEE80211_HW_TX_STATUS))
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
}
@@ -545,19 +546,23 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
container_of(work, struct ieee80211_local,
dynamic_ps_enable_work);
struct ieee80211_sub_if_data *sdata = local->ps_sdata;
+ struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
/* can only happen when PS was just disabled anyway */
if (!sdata)
return;
- if (local->hw.conf.flags & IEEE80211_CONF_PS)
- return;
-
- if (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK)
+ if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
+ (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)))
ieee80211_send_nullfunc(local, sdata, 1);
local->hw.conf.flags |= IEEE80211_CONF_PS;
- ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+
+ if (!(local->hw.flags & IEEE80211_HW_TX_STATUS) ||
+ (ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) {
+ ifmgd->flags &= ~IEEE80211_STA_NULLFUNC_ACKED;
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+ }
}
void ieee80211_dynamic_ps_timer(unsigned long data)
@@ -567,6 +572,9 @@ void ieee80211_dynamic_ps_timer(unsigned long data)
if (local->quiescing || local->suspended)
return;
+ if (local->hw.conf.flags & IEEE80211_CONF_PS)
+ return;
+
ieee80211_queue_work(&local->hw, &local->dynamic_ps_enable_work);
}
@@ -1904,6 +1912,7 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
return -ENOMEM;
ifmgd->flags &= ~IEEE80211_STA_DISABLE_11N;
+ ifmgd->flags &= ~IEEE80211_STA_NULLFUNC_ACKED;
for (i = 0; i < req->crypto.n_ciphers_pairwise; i++)
if (req->crypto.ciphers_pairwise[i] == WLAN_CIPHER_SUITE_WEP40 ||
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index e57ad6b..35fa2df 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -188,6 +188,7 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
rcu_read_lock();
sband = local->hw.wiphy->bands[info->band];
+ fc = hdr->frame_control;
for_each_sta_info(local, hdr->addr1, sta, tmp) {
/* skip wrong virtual interface */
@@ -205,8 +206,6 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
return;
}
- fc = hdr->frame_control;
-
if ((info->flags & IEEE80211_TX_STAT_AMPDU_NO_BACK) &&
(ieee80211_is_data_qos(fc))) {
u16 tid, ssn;
@@ -275,6 +274,19 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
local->dot11FailedCount++;
}
+ if (ieee80211_is_nullfunc(fc) && ieee80211_has_pm(fc) &&
+ (local->hw.flags & IEEE80211_HW_TX_STATUS) &&
+ (local->hw.conf.flags & IEEE80211_CONF_PS)) {
+ if (info->flags & IEEE80211_TX_STAT_ACK) {
+ local->ps_sdata->u.mgd.flags |=
+ IEEE80211_STA_NULLFUNC_ACKED;
+ ieee80211_queue_work(&local->hw,
+ &local->dynamic_ps_enable_work);
+ } else
+ mod_timer(&local->dynamic_ps_timer, jiffies +
+ msecs_to_jiffies(10));
+ }
+
/* this was a transmitted frame, but now we want to reuse it */
skb_orphan(skb);
--
1.6.6.1
On Fri, Feb 5, 2010 at 7:32 PM, Johannes Berg <[email protected]> wrote:
> On Fri, 2010-02-05 at 19:29 +0530, Vivek Natarajan wrote:
>> On Fri, Feb 5, 2010 at 7:11 PM, Johannes Berg <[email protected]> wrote:
>> >
>> >> @@ -275,6 +274,19 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
>> >> ? ? ? ? ? ? ? ? ? ? ? local->dot11FailedCount++;
>> >> ? ? ? }
>> >>
>> >> + ? ? if (ieee80211_is_nullfunc(fc) && ieee80211_has_pm(fc) &&
>> >> + ? ? ? ? ? ? (local->hw.flags & IEEE80211_HW_TX_STATUS) &&
>> >> + ? ? ? ? ? ? (local->hw.conf.flags & IEEE80211_CONF_PS)) {
>> >> + ? ? ? ? ? ? if (info->flags & IEEE80211_TX_STAT_ACK) {
>> >> + ? ? ? ? ? ? ? ? ? ? local->ps_sdata->u.mgd.flags |=
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? IEEE80211_STA_NULLFUNC_ACKED;
>> >> + ? ? ? ? ? ? ? ? ? ? ieee80211_queue_work(&local->hw,
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&local->dynamic_ps_enable_work);
>> >> + ? ? ? ? ? ? } else
>> >> + ? ? ? ? ? ? ? ? ? ? mod_timer(&local->dynamic_ps_timer, jiffies +
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? msecs_to_jiffies(10));
>> >> + ? ? }
>> >> +
>> >> ? ? ? /* this was a transmitted frame, but now we want to reuse it */
>> >> ? ? ? skb_orphan(skb);
>> > And the mod_timer case is completely useless there anyway, at least the
>> > way you've done this now.
>> Ah, right! Thanks for pointing it out.
>
> I htink you should only set CONF_PS after the frame is ACKed, otherwise
> internal stuff might be confused too?
I thought about that. But we may not be able to differentiate if the
null frame is
for power save or for fake_sleep during scannig.
Vivek.
On Fri, Feb 5, 2010 at 7:11 PM, Johannes Berg <[email protected]> wrote:
>
>> @@ -275,6 +274,19 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
>> ? ? ? ? ? ? ? ? ? ? ? local->dot11FailedCount++;
>> ? ? ? }
>>
>> + ? ? if (ieee80211_is_nullfunc(fc) && ieee80211_has_pm(fc) &&
>> + ? ? ? ? ? ? (local->hw.flags & IEEE80211_HW_TX_STATUS) &&
>> + ? ? ? ? ? ? (local->hw.conf.flags & IEEE80211_CONF_PS)) {
>> + ? ? ? ? ? ? if (info->flags & IEEE80211_TX_STAT_ACK) {
>> + ? ? ? ? ? ? ? ? ? ? local->ps_sdata->u.mgd.flags |=
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? IEEE80211_STA_NULLFUNC_ACKED;
>> + ? ? ? ? ? ? ? ? ? ? ieee80211_queue_work(&local->hw,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&local->dynamic_ps_enable_work);
>> + ? ? ? ? ? ? } else
>> + ? ? ? ? ? ? ? ? ? ? mod_timer(&local->dynamic_ps_timer, jiffies +
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? msecs_to_jiffies(10));
>> + ? ? }
>> +
>> ? ? ? /* this was a transmitted frame, but now we want to reuse it */
>> ? ? ? skb_orphan(skb);
> And the mod_timer case is completely useless there anyway, at least the
> way you've done this now.
Ah, right! Thanks for pointing it out.
Vivek.
On Fri, 2010-02-05 at 19:29 +0530, Vivek Natarajan wrote:
> On Fri, Feb 5, 2010 at 7:11 PM, Johannes Berg <[email protected]> wrote:
> >
> >> @@ -275,6 +274,19 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
> >> local->dot11FailedCount++;
> >> }
> >>
> >> + if (ieee80211_is_nullfunc(fc) && ieee80211_has_pm(fc) &&
> >> + (local->hw.flags & IEEE80211_HW_TX_STATUS) &&
> >> + (local->hw.conf.flags & IEEE80211_CONF_PS)) {
> >> + if (info->flags & IEEE80211_TX_STAT_ACK) {
> >> + local->ps_sdata->u.mgd.flags |=
> >> + IEEE80211_STA_NULLFUNC_ACKED;
> >> + ieee80211_queue_work(&local->hw,
> >> + &local->dynamic_ps_enable_work);
> >> + } else
> >> + mod_timer(&local->dynamic_ps_timer, jiffies +
> >> + msecs_to_jiffies(10));
> >> + }
> >> +
> >> /* this was a transmitted frame, but now we want to reuse it */
> >> skb_orphan(skb);
> > And the mod_timer case is completely useless there anyway, at least the
> > way you've done this now.
> Ah, right! Thanks for pointing it out.
I htink you should only set CONF_PS after the frame is ACKed, otherwise
internal stuff might be confused too?
johannes
The current mac80211 implementation enables power save if there
is no Tx traffic for a specific timeout. Hence, PS is triggered
even if there is a continuous Rx only traffic(like UDP) going on.
This makes the drivers to wait on the tim bit in the next beacon
to awake which leads to redundant sleep-wake cycles.
Fix this by restarting the dynamic ps timer on receiving every
data packet.
Signed-off-by: Vivek Natarajan <[email protected]>
---
net/mac80211/rx.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 5709307..657ed66 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1719,6 +1719,7 @@ static ieee80211_rx_result debug_noinline
ieee80211_rx_h_data(struct ieee80211_rx_data *rx)
{
struct ieee80211_sub_if_data *sdata = rx->sdata;
+ struct ieee80211_local *local = rx->local;
struct net_device *dev = sdata->dev;
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)rx->skb->data;
__le16 fc = hdr->frame_control;
@@ -1750,6 +1751,13 @@ ieee80211_rx_h_data(struct ieee80211_rx_data *rx)
dev->stats.rx_packets++;
dev->stats.rx_bytes += rx->skb->len;
+ if (ieee80211_is_data(hdr->frame_control) &&
+ !is_multicast_ether_addr(hdr->addr1) &&
+ local->hw.conf.dynamic_ps_timeout > 0 && local->ps_sdata) {
+ mod_timer(&local->dynamic_ps_timer, jiffies +
+ msecs_to_jiffies(local->hw.conf.dynamic_ps_timeout));
+ }
+
ieee80211_deliver_skb(rx);
return RX_QUEUED;
--
1.6.6.1
On Fri, Feb 5, 2010 at 4:27 AM, Vivek Natarajan <[email protected]> wrote:
> The current mac80211 implementation enables power save if there
> is no Tx traffic for a specific timeout. Hence, PS is triggered
> even if there is a continuous Rx only traffic(like UDP) going on.
> This makes the drivers to wait on the tim bit in the next beacon
> to awake which leads to redundant sleep-wake cycles.
> Fix this by restarting the dynamic ps timer on receiving every
> data packet.
>
> Signed-off-by: Vivek Natarajan <[email protected]>
Seems small enough and worth it for stable to me.
Luis
> @@ -275,6 +274,19 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
> local->dot11FailedCount++;
> }
>
> + if (ieee80211_is_nullfunc(fc) && ieee80211_has_pm(fc) &&
> + (local->hw.flags & IEEE80211_HW_TX_STATUS) &&
> + (local->hw.conf.flags & IEEE80211_CONF_PS)) {
> + if (info->flags & IEEE80211_TX_STAT_ACK) {
> + local->ps_sdata->u.mgd.flags |=
> + IEEE80211_STA_NULLFUNC_ACKED;
> + ieee80211_queue_work(&local->hw,
> + &local->dynamic_ps_enable_work);
> + } else
> + mod_timer(&local->dynamic_ps_timer, jiffies +
> + msecs_to_jiffies(10));
> + }
> +
> /* this was a transmitted frame, but now we want to reuse it */
> skb_orphan(skb);
This code should be in ieee80211_frame_acked() where it doesn't need to
check the ACK status.
And the mod_timer case is completely useless there anyway, at least the
way you've done this now.
johannes