[PATCH v3] mac80211: Retry null data frame for power save.

[PATCH v3] mac80211: Retry null data frame for power save.

[Vivek Natarajan]

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 <vnatarajan@atheros.com>
---
 include/net/mac80211.h     |    1 +
 net/mac80211/ieee80211_i.h |    1 +
 net/mac80211/mlme.c        |   20 +++++++++++++++-----
 net/mac80211/status.c      |   16 ++++++++++++++--
 4 files changed, 31 insertions(+), 7 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..ef4abe6 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -438,8 +438,11 @@ static void ieee80211_enable_ps(struct ieee80211_local *local,
 	} else {
 		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)) {
+			conf->flags |= IEEE80211_CONF_PS;
+			ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+		}
 	}
 }
 
@@ -545,6 +548,7 @@ 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)
@@ -553,11 +557,16 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
 	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;
+		local->hw.conf.flags |= IEEE80211_CONF_PS;
+		ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+	}
 }
 
 void ieee80211_dynamic_ps_timer(unsigned long data)
@@ -1904,6 +1913,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..796cf9f 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->ps_sdata && !(local->scanning)) {
+		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

[PATCH] mac80211: Reset dynamic ps timer in Rx path.

[Vivek Natarajan]

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 <vnatarajan@atheros.com>
CC: stable@kernel.org
---
 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

Re: [PATCH v3] mac80211: Retry null data frame for power save.

[Christian Lamparter]

On Monday 08 February 2010 13:17:00 Vivek Natarajan wrote:
> 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 <vnatarajan@atheros.com>
> ---
>  include/net/mac80211.h     |    1 +
>  net/mac80211/ieee80211_i.h |    1 +
>  net/mac80211/mlme.c        |   20 +++++++++++++++-----
>  net/mac80211/status.c      |   16 ++++++++++++++--
>  4 files changed, 31 insertions(+), 7 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,
>  };

It might be a good idea to add some documentation for new flags.

Otherwise this can easily lead to confusing (e.g.: Does
HW_TX_STATUS mean that the HW reports not just ACKs, but also
when if the frame was filtered by the HW, or if the HW reports
the rate control information (retries & retry index) as well?)

On a related issue: What about _inverting_ the flag, so it will
be set for devices which can't give any accurate tx_status
information. This has the advantage that we don't have to touch
other drivers?

Regards,
	Chr

Re: [PATCH v3] mac80211: Retry null data frame for power save.

[Vivek Natarajan]

On Mon, Feb 8, 2010 at 6:12 PM, Christian Lamparter
<chunkeey@googlemail.com> wrote:
> On Monday 08 February 2010 13:17:00 Vivek Natarajan wrote:
>> 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 <vnatarajan@atheros.com>
>> ---
>>  include/net/mac80211.h     |    1 +
>>  net/mac80211/ieee80211_i.h |    1 +
>>  net/mac80211/mlme.c        |   20 +++++++++++++++-----
>>  net/mac80211/status.c      |   16 ++++++++++++++--
>>  4 files changed, 31 insertions(+), 7 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,
>>  };
>
> It might be a good idea to add some documentation for new flags.
>
> Otherwise this can easily lead to confusing (e.g.: Does
> HW_TX_STATUS mean that the HW reports not just ACKs, but also
> when if the frame was filtered by the HW, or if the HW reports
> the rate control information (retries & retry index) as well?)
>
> On a related issue: What about _inverting_ the flag, so it will
> be set for devices which can't give any accurate tx_status
> information. This has the advantage that we don't have to touch
> other drivers?

Shall I rename it as HW_NO_TX_ACK_REPORT?
Looking at the other flags, they show some positively present
feature in the hw. In those lines, HW_REPORTS_TX_ACK_STATUS
might be better.

Thanks
Vivek.

Re: [PATCH v3] mac80211: Retry null data frame for power save.

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]

On Mon, 2010-02-08 at 18:43 +0530, Vivek Natarajan wrote:

> > On a related issue: What about _inverting_ the flag, so it will
> > be set for devices which can't give any accurate tx_status
> > information. This has the advantage that we don't have to touch
> > other drivers?
> 
> Shall I rename it as HW_NO_TX_ACK_REPORT?
> Looking at the other flags, they show some positively present
> feature in the hw. In those lines, HW_REPORTS_TX_ACK_STATUS
> might be better.

The positive feature flag has the advantage that we don't have to touch
any of the essentially unmaintained drivers, so I much prefer having it
that way so maintainers can enable the flag after testing etc.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3] mac80211: Retry null data frame for power save.

[Christian Lamparter]

On Monday 08 February 2010 17:24:39 Johannes Berg wrote:
> On Mon, 2010-02-08 at 18:43 +0530, Vivek Natarajan wrote:
> 
> > > On a related issue: What about _inverting_ the flag, so it will
> > > be set for devices which can't give any accurate tx_status
> > > information. This has the advantage that we don't have to touch
> > > other drivers?
> > 
> > Shall I rename it as HW_NO_TX_ACK_REPORT?
> > Looking at the other flags, they show some positively present
> > feature in the hw. In those lines, HW_REPORTS_TX_ACK_STATUS
> > might be better.
> 
> The positive feature flag has the advantage that we don't have to touch
> any of the essentially unmaintained drivers, so I much prefer having it
> that way so maintainers can enable the flag after testing etc.

Alright. I count 2:1 votes. So "positive feature flag" it is.

BTW: I added rt2x00 project maintainers to the CC, because
If I'm not totally wrong, this patch _reduces_ some PS
features of rt2x00. This is because not all of rt2x00
supported & PS-capable/enabled devices return a proper tx_status
and therefore these devices will be the most affected AFAICT.

Any word from you guys? Or have you scrubbed the PS features already?

Regards,
	Chr

Re: [PATCH v3] mac80211: Retry null data frame for power save.

[Christian Lamparter]

On Monday 08 February 2010 21:30:09 Christian Lamparter wrote:
> On Monday 08 February 2010 17:24:39 Johannes Berg wrote:
> > On Mon, 2010-02-08 at 18:43 +0530, Vivek Natarajan wrote:
> > 
> > > > On a related issue: What about _inverting_ the flag, so it will
> > > > be set for devices which can't give any accurate tx_status
> > > > information. This has the advantage that we don't have to touch
> > > > other drivers?
> > > 
> > > Shall I rename it as HW_NO_TX_ACK_REPORT?
> > > Looking at the other flags, they show some positively present
> > > feature in the hw. In those lines, HW_REPORTS_TX_ACK_STATUS
> > > might be better.
> > 
> > The positive feature flag has the advantage that we don't have to touch
> > any of the essentially unmaintained drivers, so I much prefer having it
> > that way so maintainers can enable the flag after testing etc.
>
> BTW: I added rt2x00 project maintainers to the CC, because
> If I'm not totally wrong, this patch _reduces_ some PS
> features of rt2x00. This is because not all of rt2x00
> supported & PS-capable/enabled devices return a proper tx_status
> and therefore these devices will be the most affected AFAICT.
> 
> Any word from you guys? Or have you scrubbed the PS features already?
>

Oops,

Johannes pointed out that the patch does not break rt2x00.
and indeed ... he IS right and I misread the check !HW_STATUS_CHECK
in ieee80211_enable_ps.

Sorry for all the noise!

Re: [PATCH v3] mac80211: Retry null data frame for power save.

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On Mon, 2010-02-08 at 17:47 +0530, Vivek Natarajan wrote:

> 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),

Good idea :)

> +	if (ieee80211_is_nullfunc(fc) && ieee80211_has_pm(fc) &&
> +		(local->hw.flags & IEEE80211_HW_TX_STATUS) &&
> +		local->ps_sdata && !(local->scanning)) {
> +		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));
> +	}

I think this needs to check against injected frames as well [and
personally, I'd prefer if you only indented the stuff belonging into the
first if by 4 spaces to match up with the "if ("].

Also, you're still missing documentation on IEEE80211_HW_TX_STATUS.

Thanks,
johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3] mac80211: Retry null data frame for power save.

[Vivek Natarajan]

On Tue, Feb 9, 2010 at 1:27 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2010-02-08 at 17:47 +0530, Vivek Natarajan wrote:
>> +     if (ieee80211_is_nullfunc(fc) && ieee80211_has_pm(fc) &&
>> +             (local->hw.flags & IEEE80211_HW_TX_STATUS) &&
>> +             local->ps_sdata && !(local->scanning)) {
>> +             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));
>> +     }
>
> I think this needs to check against injected frames as well
Any monitor mode injected frame should be dropped by the
check of ps_sdata which is set only during the station mode.

> personally, I'd prefer if you only indented the stuff belonging into the
> first if by 4 spaces to match up with the "if ("].
Thanks. I will modify it.

> Also, you're still missing documentation on IEEE80211_HW_TX_STATUS.
I think you missed the v4 with the name changed as
IEEE80211_HW_REPORTS_TX_ACK_STATUS.
I will send it once again with modified indentation.

Thanks
Vivek.

Re: [PATCH v3] mac80211: Retry null data frame for power save.

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]

On Tue, 2010-02-09 at 14:06 +0530, Vivek Natarajan wrote:
> On Tue, Feb 9, 2010 at 1:27 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Mon, 2010-02-08 at 17:47 +0530, Vivek Natarajan wrote:
> >> +     if (ieee80211_is_nullfunc(fc) && ieee80211_has_pm(fc) &&
> >> +             (local->hw.flags & IEEE80211_HW_TX_STATUS) &&
> >> +             local->ps_sdata && !(local->scanning)) {
> >> +             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));
> >> +     }
> >
> > I think this needs to check against injected frames as well

> Any monitor mode injected frame should be dropped by the
> check of ps_sdata which is set only during the station mode.

Yes, but you could have a monitor virtual interface and a station
virtual interface, and start injecting nullfunc packets. That'd be kinda
stupid, but we wouldn't want to act on them.

> > Also, you're still missing documentation on IEEE80211_HW_TX_STATUS.
> I think you missed the v4 with the name changed as
> IEEE80211_HW_REPORTS_TX_ACK_STATUS.
> I will send it once again with modified indentation.

Yeah I only saw v4 when I read the list, and replied to the version in
my inbox.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]