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]>
---
net/mac80211/status.c | 15 +++++++++++++--
1 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index e57ad6b..a0e18b4 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,18 @@ 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.conf.flags & IEEE80211_CONF_PS)) {
+ if (!(info->flags & IEEE80211_TX_STAT_ACK)) {
+ ieee80211_stop_queues_by_reason(&local->hw,
+ IEEE80211_QUEUE_STOP_REASON_PS);
+ ieee80211_queue_work(&local->hw,
+ &local->dynamic_ps_disable_work);
+ 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
Vivek Natarajan <[email protected]> writes:
> If there is a continuous Rx UDP traffic with power save enabled,
> there is some loss of packets with ath9k as Atheros chipsets
> need to be awake to do Rx, unlike other vendor chipsets.
Most probably this problem is related to ath9k staying awake after
PS-Poll frame. Maybe we need to change mac80211 disable PS (but not
send nullfunc!) when sending PS-Poll frames. But that might break p54,
I don't know.
But I want to emphasise that this patch is just a workaround, it
doesn't solve the actual, hidden, problem (whatever that is). I
recommmend finding and fixing the real problem first. This patch just
hides the real problem and makes the probability for it to happen
lower.
> The current mac80211 implementation enables power save if there is
> no Tx traffic for a specific timeout. This adversely affects ath9k
> when there is a continuous Rx UDP traffic going on since it depends
> only on the tim bit in the next beacon to awake. Fix this by
> restarting the dynamic ps timer on receiving every data packet.
I don't oppose changing the dynamic powersave to also wakeup from
received unicast frames destined to the client. Not because it solves
some problems with ath9k but because it decreases latency in some use
case, naturally with the cost of increased power consumption. But I
think we can manage with that for now.
But received multicast frames powersave should not disable powersave,
otherwise on a busy network we would wakeup way too often.
> + * @IEEE80211_HW_NEEDS_RX_PS_RESET:
> + * Hardware requires the stack to reset the dynamic PS timer
> + * on receiving a data frame.
> */
> enum ieee80211_hw_flags {
> IEEE80211_HW_HAS_RATE_CONTROL = 1<<0,
> @@ -978,6 +982,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_NEEDS_RX_PS_RESET = 1<<18,
This should be dropped. I agree with Johannes, I don't see why we need
a new hw flag.
> 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,15 @@ ieee80211_rx_h_data(struct ieee80211_rx_data *rx)
> dev->stats.rx_packets++;
> dev->stats.rx_bytes += rx->skb->len;
>
> + if ((local->hw.flags & IEEE80211_HW_NEEDS_RX_PS_RESET) &&
> + ieee80211_is_data(hdr->frame_control) &&
> + !is_multicast_ether_addr(hdr->addr1)) {
> + if (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));
> + }
> + }
What if CONF_PS is set? You need to take that into account here and
disable powersave when it's enabled.
I'm busy now and I only took a quick peek of the patch. I need to
review this in detail later today.
--
Kalle Valo
On Thu, Feb 4, 2010 at 2:29 PM, Kalle Valo <[email protected]> wrote:
>> If there is a continuous Rx UDP traffic with power save enabled,
>> there is some loss of packets with ath9k as Atheros chipsets
>> need to be awake to do Rx, unlike other vendor chipsets.
>
> Most probably this problem is related to ath9k staying awake after
> PS-Poll frame. Maybe we need to change mac80211 disable PS (but not
> send nullfunc!) when sending PS-Poll frames. But that might break p54,
> I don't know.
>
The issue I observed is related to dynamic power save getting triggered
by mac80211 eventhough there might be a continuous Rx traffic.
>> The current mac80211 implementation enables power save if there is
>> no Tx traffic for a specific timeout. This adversely affects ath9k
>> when there is a continuous Rx UDP traffic going on since it depends
>> only on the tim bit in the next beacon to awake. Fix this by
>> restarting the dynamic ps timer on receiving every data packet.
> But received multicast frames powersave should not disable powersave,
> otherwise on a busy network we would wakeup way too often.
Yes, this has been taken care in the patch.
>> ?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,15 @@ ieee80211_rx_h_data(struct ieee80211_rx_data *rx)
>> ? ? ? dev->stats.rx_packets++;
>> ? ? ? dev->stats.rx_bytes += rx->skb->len;
>>
>> + ? ? if ((local->hw.flags & IEEE80211_HW_NEEDS_RX_PS_RESET) &&
>> + ? ? ? ? ieee80211_is_data(hdr->frame_control) &&
>> + ? ? ? ? !is_multicast_ether_addr(hdr->addr1)) {
>> + ? ? ? ? ? ? if (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));
>> + ? ? ? ? ? ? }
>> + ? ? }
>
> What if CONF_PS is set? You need to take that into account here and
> disable powersave when it's enabled.
With ath9k, for a data frame(non-multicast) to reach mac80211, CONF_PS
should have been disabled. in other words,a data packet will not reach mac80211
when CONF_PS is set. Hence that check might be redundant here.
Thanks
Vivek.
If there is a continuous Rx UDP traffic with power save enabled,
there is some loss of packets with ath9k as Atheros chipsets
need to be awake to do Rx, unlike other vendor chipsets.
The current mac80211 implementation enables power save if there
is no Tx traffic for a specific timeout. This adversely affects
ath9k when there is a continuous Rx UDP traffic going on since it
depends only on the tim bit in the next beacon to awake.
Fix this by restarting the dynamic ps timer on receiving every
data packet.
Signed-off-by: Vivek Natarajan <[email protected]>
---
include/net/mac80211.h | 5 +++++
net/mac80211/rx.c | 10 ++++++++++
2 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 74ccf30..6ecc065 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -958,6 +958,10 @@ enum ieee80211_tkip_key_type {
* Hardware supports Unscheduled Automatic Power Save Delivery
* (U-APSD) in managed mode. The mode is configured with
* conf_tx() operation.
+ *
+ * @IEEE80211_HW_NEEDS_RX_PS_RESET:
+ * Hardware requires the stack to reset the dynamic PS timer
+ * on receiving a data frame.
*/
enum ieee80211_hw_flags {
IEEE80211_HW_HAS_RATE_CONTROL = 1<<0,
@@ -978,6 +982,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_NEEDS_RX_PS_RESET = 1<<18,
};
/**
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 5709307..0bb6169 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,15 @@ ieee80211_rx_h_data(struct ieee80211_rx_data *rx)
dev->stats.rx_packets++;
dev->stats.rx_bytes += rx->skb->len;
+ if ((local->hw.flags & IEEE80211_HW_NEEDS_RX_PS_RESET) &&
+ ieee80211_is_data(hdr->frame_control) &&
+ !is_multicast_ether_addr(hdr->addr1)) {
+ if (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
Johannes Berg <[email protected]> writes:
> On the other hand, there is hardware (even from Atheros/Zydas!) that
> will not provide status reports for every frame.
Also at76c50x-usb doesn't report status AFAIK. I think we need to have
a new hw flag to indicate if driver reports frame status.
> Also, I'm not entirely sure why the AP would lose the frame? It's a
> unicast frame that will be retransmitted quite a number of times. Is it
> really actually going out on the air or is there something else wrong?
> Maybe just the queue priority inversion issue since this frame goes out
> as higher priority as most data frames?
I have received reports that in a busy office networks nullfunc frames
get lost, I'm assuming due to collisions on the air. For example,
wl1251 (in which firmware sends the nullfunc frame) sends an event to
the host if nullfunc frame fails for some reason. I think we need to
take into account in mac80211 the case when nullfunc frames get lost.
> If you really need to provide this functionality I think you should
> probably do it "properly" in the sense that you react to the "frame
> acked" by enabling PS, rather than the other way around.
This is much better. Also I'm worried about the test for nullfunc
frame being a racy. I would prefer to make it sure that the nullfunc
frame really is for the powersave transition.
> That requires a hardware flag though that enables mac80211 to know
> that all frames will get a status report unless something is
> seriously wrong, since otherwise we'd *never* go into PS.
Agreed.
--
Kalle Valo
On Thu, Feb 4, 2010 at 1:44 PM, Johannes Berg <[email protected]> wrote:
> On Thu, 2010-02-04 at 12:24 +0530, Vivek Natarajan wrote:
>> If there is a continuous Rx UDP traffic with power save enabled,
>> there is some loss of packets with ath9k as Atheros chipsets
>> need to be awake to do Rx, unlike other vendor chipsets.
>
> That sentence doesn't make a whole lot of sense, all chips need to be
> awake to RX ;)
>
> IOW, I think the flag is probably useless?
I believe, in wl12xx or p54 chipsets, Rx engine is always awake eventhough
power save might have been enabled. But with Atheros, it is completely
shut off. Hence I had to use a flag in order to avoid disturbing the
other drivers. Without the flag,it may end up in increasing the average power
consumption for other drivers.
Thanks
Vivek
Vivek Natarajan <[email protected]> writes:
> 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.
Yes, this is definitely needed. I like the idea, but I will check the
patch in detail later today.
--
Kalle Valo
On Thu, 2010-02-04 at 12:24 +0530, Vivek Natarajan wrote:
> @@ -275,6 +274,18 @@ 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.conf.flags & IEEE80211_CONF_PS)) {
> + if (!(info->flags & IEEE80211_TX_STAT_ACK)) {
> + ieee80211_stop_queues_by_reason(&local->hw,
> + IEEE80211_QUEUE_STOP_REASON_PS);
> + ieee80211_queue_work(&local->hw,
> + &local->dynamic_ps_disable_work);
> + mod_timer(&local->dynamic_ps_timer, jiffies +
> + msecs_to_jiffies(10));
> + }
> + }
> +
This will still lose packets in the time between frame tx and status
report. And in fact, it'll also lose packets from lower-priority queues,
since ath9k doesn't implement flushing yet. It really should :)
On the other hand, there is hardware (even from Atheros/Zydas!) that
will not provide status reports for every frame.
Also, I'm not entirely sure why the AP would lose the frame? It's a
unicast frame that will be retransmitted quite a number of times. Is it
really actually going out on the air or is there something else wrong?
Maybe just the queue priority inversion issue since this frame goes out
as higher priority as most data frames?
If you really need to provide this functionality I think you should
probably do it "properly" in the sense that you react to the "frame
acked" by enabling PS, rather than the other way around. That requires a
hardware flag though that enables mac80211 to know that all frames will
get a status report unless something is seriously wrong, since otherwise
we'd *never* go into PS.
johannes
On Thu, 2010-02-04 at 12:24 +0530, Vivek Natarajan wrote:
> If there is a continuous Rx UDP traffic with power save enabled,
> there is some loss of packets with ath9k as Atheros chipsets
> need to be awake to do Rx, unlike other vendor chipsets.
That sentence doesn't make a whole lot of sense, all chips need to be
awake to RX ;)
IOW, I think the flag is probably useless?
> + if ((local->hw.flags & IEEE80211_HW_NEEDS_RX_PS_RESET) &&
> + ieee80211_is_data(hdr->frame_control) &&
> + !is_multicast_ether_addr(hdr->addr1)) {
> + if (local->hw.conf.dynamic_ps_timeout > 0 && local->ps_sdata) {
Why nest two if statements instead of just using a single one?
johannes
Vivek Natarajan <[email protected]> writes:
>>> + ? ? if ((local->hw.flags & IEEE80211_HW_NEEDS_RX_PS_RESET) &&
>>> + ? ? ? ? ieee80211_is_data(hdr->frame_control) &&
>>> + ? ? ? ? !is_multicast_ether_addr(hdr->addr1)) {
>>> + ? ? ? ? ? ? if (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));
>>> + ? ? ? ? ? ? }
>>> + ? ? }
>>
>> What if CONF_PS is set? You need to take that into account here and
>> disable powersave when it's enabled.
>
> With ath9k, for a data frame(non-multicast) to reach mac80211,
> CONF_PS should have been disabled. in other words,a data packet will
> not reach mac80211 when CONF_PS is set. Hence that check might be
> redundant here.
I checked this now, and that indeed is the case. So no need to check
CONF_PS here.
--
Kalle Valo