There is a race on sending a data frame before the tx completion
of nullfunc frame for enabling power save. As the data quickly
follows the nullfunc frame, the AP thinks that the station is out
of power save and continues to send the frames. Whereas in the
station, the nullfunc ack will be processed after the tx completion
of data frame and mac80211 goes to powersave. Thus the power
save state mismatch between the station and the AP causes some
data loss and some applications fail because of that. This patch
fixes this issue.
Signed-off-by: Vivek Natarajan <[email protected]>
---
net/mac80211/ieee80211_i.h | 1 +
net/mac80211/mlme.c | 11 +++++++++--
net/mac80211/status.c | 5 ++++-
net/mac80211/tx.c | 8 ++++++++
4 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index f2ef15d..0933677 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -347,6 +347,7 @@ enum ieee80211_sta_flags {
IEEE80211_STA_UAPSD_ENABLED = BIT(7),
IEEE80211_STA_NULLFUNC_ACKED = BIT(8),
IEEE80211_STA_RESET_SIGNAL_AVE = BIT(9),
+ IEEE80211_STA_PS_PENDING = BIT(10),
};
struct ieee80211_if_managed {
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index d89e878..4c58769 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -738,16 +738,23 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
return;
if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
- (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)))
+ (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED) ||
+ !(ifmgd->flags & IEEE80211_STA_PS_PENDING))) {
+ ifmgd->flags |= IEEE80211_STA_PS_PENDING;
ieee80211_send_nullfunc(local, sdata, 1);
+ }
if (!((local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS) &&
(local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK)) ||
- (ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) {
+ ((ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED) &&
+ ifmgd->flags & IEEE80211_STA_PS_PENDING)) {
ifmgd->flags &= ~IEEE80211_STA_NULLFUNC_ACKED;
+ ifmgd->flags &= ~IEEE80211_STA_PS_PENDING;
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_timer(unsigned long data)
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 010a559..51caa0d 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -315,7 +315,10 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
(local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS) &&
!(info->flags & IEEE80211_TX_CTL_INJECTED) &&
local->ps_sdata && !(local->scanning)) {
- if (info->flags & IEEE80211_TX_STAT_ACK) {
+ if ((info->flags & IEEE80211_TX_STAT_ACK) &&
+ (local->ps_sdata->u.mgd.flags & IEEE80211_STA_PS_PENDING)) {
+ ieee80211_stop_queues_by_reason(&local->hw,
+ IEEE80211_QUEUE_STOP_REASON_PS);
local->ps_sdata->u.mgd.flags |=
IEEE80211_STA_NULLFUNC_ACKED;
ieee80211_queue_work(&local->hw,
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 17ef4f4..9850284 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -185,6 +185,7 @@ ieee80211_tx_h_dynamic_ps(struct ieee80211_tx_data *tx)
{
struct ieee80211_local *local = tx->local;
struct ieee80211_if_managed *ifmgd;
+ struct ieee80211_hdr *hdr;
/* driver doesn't support power save */
if (!(local->hw.flags & IEEE80211_HW_SUPPORTS_PS))
@@ -233,6 +234,13 @@ ieee80211_tx_h_dynamic_ps(struct ieee80211_tx_data *tx)
&& skb_get_queue_mapping(tx->skb) == 0)
return TX_CONTINUE;
+ hdr = (struct ieee80211_hdr *)tx->skb->data;
+
+ if (!(ieee80211_is_nullfunc(hdr->frame_control) &&
+ ieee80211_has_pm(hdr->frame_control)) &&
+ (ifmgd->flags & IEEE80211_STA_PS_PENDING))
+ ifmgd->flags &= ~IEEE80211_STA_PS_PENDING;
+
if (local->hw.conf.flags & IEEE80211_CONF_PS) {
ieee80211_stop_queues_by_reason(&local->hw,
IEEE80211_QUEUE_STOP_REASON_PS);
--
1.6.3.3
Vivek Natarajan wrote:
> if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
> - (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)))
> + (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED) ||
> + !(ifmgd->flags & IEEE80211_STA_PS_PENDING))) {
> + ifmgd->flags |= IEEE80211_STA_PS_PENDING;
> ieee80211_send_nullfunc(local, sdata, 1);
> + }
>
> if (!((local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS) &&
> (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK)) ||
> - (ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) {
> + ((ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED) &&
> + ifmgd->flags & IEEE80211_STA_PS_PENDING)) {
> ifmgd->flags &= ~IEEE80211_STA_NULLFUNC_ACKED;
> + ifmgd->flags &= ~IEEE80211_STA_PS_PENDING;
> 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);
> }
The queues are activated when enabling powersave ?
> void ieee80211_dynamic_ps_timer(unsigned long data)
> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
> index 010a559..51caa0d 100644
> --- a/net/mac80211/status.c
> +++ b/net/mac80211/status.c
> @@ -315,7 +315,10 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
> (local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS) &&
> !(info->flags & IEEE80211_TX_CTL_INJECTED) &&
> local->ps_sdata && !(local->scanning)) {
> - if (info->flags & IEEE80211_TX_STAT_ACK) {
> + if ((info->flags & IEEE80211_TX_STAT_ACK) &&
> + (local->ps_sdata->u.mgd.flags & IEEE80211_STA_PS_PENDING)) {
> + ieee80211_stop_queues_by_reason(&local->hw,
> + IEEE80211_QUEUE_STOP_REASON_PS);
I am not too familiar with the PS code, but why are the queues being stopped
after the nullfunc frame has been sent out and acked ? And it is a bit unclear why
the new flag IEEE80211_STA_PS_PENDING is required at all... Can't IEEE80211_STA_NULLFUNC_ACKED
be used to fix this race ?
Sujith
On Tue, Feb 15, 2011 at 12:46 PM, Sujith <[email protected]> wrote:
> Vivek Natarajan wrote:
>> ?void ieee80211_dynamic_ps_timer(unsigned long data)
>> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
>> index 010a559..51caa0d 100644
>> --- a/net/mac80211/status.c
>> +++ b/net/mac80211/status.c
>> @@ -315,7 +315,10 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
>> ? ? ? ? ? (local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS) &&
>> ? ? ? ? ? !(info->flags & IEEE80211_TX_CTL_INJECTED) &&
>> ? ? ? ? ? local->ps_sdata && !(local->scanning)) {
>> - ? ? ? ? ? ? if (info->flags & IEEE80211_TX_STAT_ACK) {
>> + ? ? ? ? ? ? if ((info->flags & IEEE80211_TX_STAT_ACK) &&
>> + ? ? ? ? ? ? ? ?(local->ps_sdata->u.mgd.flags & IEEE80211_STA_PS_PENDING)) {
>> + ? ? ? ? ? ? ? ? ? ? ieee80211_stop_queues_by_reason(&local->hw,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? IEEE80211_QUEUE_STOP_REASON_PS);
>
> I am not too familiar with the PS code, but why are the queues being stopped
> after the nullfunc frame has been sent out and acked ? And it is a bit unclear why
> the new flag IEEE80211_STA_PS_PENDING is required at all... Can't IEEE80211_STA_NULLFUNC_ACKED
> be used to fix this race ?
The new flag is required to prevent mac80211 to go into power save if
a frame is sent during the interval of sending nullfunc frame and
receiving ack for that. The queues are stopped to avoid sending any
frame between the interval of receiving ack and setting CONF_PS and it
mainly addresses the race that Johannes pointed out in the previous
version.
Please refer the previous thread too and the comments:
https://patchwork.kernel.org/patch/531711/
Vivek.
Vivek Natarajan wrote:
> On Tue, Feb 15, 2011 at 12:46 PM, Sujith <[email protected]> wrote:
> > Vivek Natarajan wrote:
> >> ?void ieee80211_dynamic_ps_timer(unsigned long data)
> >> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
> >> index 010a559..51caa0d 100644
> >> --- a/net/mac80211/status.c
> >> +++ b/net/mac80211/status.c
> >> @@ -315,7 +315,10 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
> >> ? ? ? ? ? (local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS) &&
> >> ? ? ? ? ? !(info->flags & IEEE80211_TX_CTL_INJECTED) &&
> >> ? ? ? ? ? local->ps_sdata && !(local->scanning)) {
> >> - ? ? ? ? ? ? if (info->flags & IEEE80211_TX_STAT_ACK) {
> >> + ? ? ? ? ? ? if ((info->flags & IEEE80211_TX_STAT_ACK) &&
> >> + ? ? ? ? ? ? ? ?(local->ps_sdata->u.mgd.flags & IEEE80211_STA_PS_PENDING)) {
> >> + ? ? ? ? ? ? ? ? ? ? ieee80211_stop_queues_by_reason(&local->hw,
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? IEEE80211_QUEUE_STOP_REASON_PS);
> >
> > I am not too familiar with the PS code, but why are the queues being stopped
> > after the nullfunc frame has been sent out and acked ? And it is a bit unclear why
> > the new flag IEEE80211_STA_PS_PENDING is required at all... Can't IEEE80211_STA_NULLFUNC_ACKED
> > be used to fix this race ?
>
> The new flag is required to prevent mac80211 to go into power save if
> a frame is sent during the interval of sending nullfunc frame and
> receiving ack for that. The queues are stopped to avoid sending any
> frame between the interval of receiving ack and setting CONF_PS and it
> mainly addresses the race that Johannes pointed out in the previous
> version.
Well ok, I still don't grok the details, though. :)
Sujith
In a highly noisy environment, a data frame which is queued before
a nullfunc frame on a different hw queue may be sent over the air
after the tx completion of nullfunc frame. This causes the station
to enter power save and the AP to see the station as awake and
continues to send the data traffic. Fix this by flushing all tx
queues before we send the null function frame with PM bit set.
Signed-off-by: Vivek Natarajan <[email protected]>
---
net/mac80211/mlme.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 4c58769..79e8f49 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -741,6 +741,9 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
(!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED) ||
!(ifmgd->flags & IEEE80211_STA_PS_PENDING))) {
ifmgd->flags |= IEEE80211_STA_PS_PENDING;
+ ieee80211_stop_queues_by_reason(&local->hw,
+ IEEE80211_QUEUE_STOP_REASON_PS);
+ drv_flush(local, false);
ieee80211_send_nullfunc(local, sdata, 1);
}
--
1.6.3.3