carl9170: Fix memory accounting when sta is in power-save mode.

RFC In review Nicolas Cavallari 4 participants 16 messages Feb 23, 2012 – Feb 27, 2012 CARL9170 Linux community wireless driver

lore

Share & tools

lore

Nicolas Cavallari

author

2012-02-23 15:53 UTC #0 lore raw

RFC 1/1 carl9170: Fix memory accounting when sta is in power-save mode.

On Access Point mode, when transmitting a packet, if the destination station is in powersave mode, we abort transmitting the packet to the device queue, but we do not reclaim the allocated memory. Given enough packets, we can go in a state where there is no packet on the device queue, but we think the device has no memory left, so no packet gets transmitted, connections breaks and the AP stops working.

This undo the allocation done in the TX path when the station is in power-save mode.

Signed-off-by Nicolas Cavallari <[email protected]>

---
While this seems to fix my problem, there could still be problems remaining in this code path. Particularly, carl9170_tx_status will do lots of things like incrementing counters and waking up mac80211 queues...

drivers/net/wireless/ath/carl9170/tx.c +1 -0

index 771e1a9..8d52ddd 100644
@@ -1253,6 +1253,7 @@ static bool carl9170_tx_ps_drop(struct ar9170 *ar, struct sk_buff *skb)
 			atomic_dec(&ar->tx_ampdu_upload);
 
 		tx_info->flags |= IEEE80211_TX_STAT_TX_FILTERED;
+		carl9170_release_dev_space(ar, skb);
 		carl9170_tx_status(ar, skb, false);
 		return true;
 	}

Signature · 1 line

1.7.9

Christian Lamparter

maintainer Acked-by on v1 1/1

2012-02-23 16:52 UTC #1 lore raw

Quoted 11 lines · Nicolas Cavallari

On Access Point mode, when transmitting a packet, if the destination station is in powersave mode, we abort transmitting the packet to the device queue, but we do not reclaim the allocated memory. Given enough packets, we can go in a state where there is no packet on the device queue, but we think the device has no memory left, so no packet gets transmitted, connections breaks and the AP stops working.
This undo the allocation done in the TX path when the station is in power-save mode.
Signed-off-by Nicolas Cavallari <[email protected]>

Acked-by Christian Lamparter <[email protected]>

Cc stable <[email protected]>

Quoted 4 lines

---
While this seems to fix my problem, there could still be problems remaining in this code path. Particularly carl9170_tx_status will do lots of things like incrementing counters and waking up mac80211 queues...

That should be alright. As a rule of thumb, for every call to carl9170_tx_accounting [called by carl9170_op_tx tx handler] we need to call carl9170_tx_status. If the station is still in ps mode, mac80211 will cache the station's frames and sets the TIM bits.

Quoted 14 lines

drivers/net/wireless/ath/carl9170/tx.c +1 -0

index 771e1a9..8d52ddd 100644
@@ -1253,6 +1253,7 @@ static bool carl9170_tx_ps_drop(struct ar9170 *ar, struct sk_buff *skb)
 			atomic_dec(&ar->tx_ampdu_upload);
 
 		tx_info->flags |= IEEE80211_TX_STAT_TX_FILTERED;
+		carl9170_release_dev_space(ar, skb);
 		carl9170_tx_status(ar, skb, false);
 		return true;
 	}

Nicolas Cavallari

author

2012-02-23 17:28 UTC #2 lore raw

Quoted 9 lines · Christian Lamparter

Quoted 3 lines · Nicolas Cavallari
While this seems to fix my problem, there could still be problems remaining in this code path. Particularly carl9170_tx_status will do lots of things like incrementing counters and waking up mac80211 queues...
That should be alright. As a rule of thumb, for every call to carl9170_tx_accounting [called by carl9170_op_tx tx handler] we need to call carl9170_tx_status. If the station is still in ps mode, mac80211 will cache the station's frames and sets the TIM bits.

Part of the reason i wrote this, is because, while this patch fixes the memory accounting problem and prevent the AP from stopping working, the powersaving station sometimes cannot communicate with the AP anymore (other stations can). The station uses ath9k, and is just really doing a background scan. The first traces that i have just indicate that the station entered powersave mode, then sends probe requests and probe requests directed to the AP, then sends authentication requests to the AP (?), but the AP does not respond to any of them...

I did thoses traces with an external device in monitor mode, so i may have missed some packets. I'll try with a monitor vif on the AP interface.

Christian Lamparter

maintainer

2012-02-23 18:08 UTC #3 lore raw

Quoted 11 lines · Nicolas Cavallari

Quoted 9 lines · Christian Lamparter
Quoted 3 lines · Nicolas Cavallari
While this seems to fix my problem, there could still be problems remaining in this code path. Particularly carl9170_tx_status will do lots of things like incrementing counters and waking up mac80211 queues...
That should be alright. As a rule of thumb, for every call to carl9170_tx_accounting [called by carl9170_op_tx tx handler] we need to call carl9170_tx_status. If the station is still in ps mode, mac80211 will cache the station's frames and sets the TIM bits.

Quoted 9 lines

Part of the reason i wrote this, is because, while this patch fixes the memory accounting problem and prevent the AP from stopping working, the powersaving station sometimes cannot communicate with the AP anymore (other stations can). The station uses ath9k, and is just really doing a background scan. The first traces that i have just indicate that the station entered powersave mode, then sends probe requests and probe requests directed to the AP, then sends authentication requests to the AP (?), but the AP does not respond to any of them...

Do you run an 802.11n ap on carl9170?

Regards,
Chr

Nicolas Cavallari

author

2012-02-23 18:52 UTC #4 lore raw

Quoted 20 lines · Christian Lamparter

Quoted 10 lines · Nicolas Cavallari
Quoted 9 lines · Christian Lamparter
Quoted 3 lines · Nicolas Cavallari
While this seems to fix my problem, there could still be problems remaining in this code path. Particularly carl9170_tx_status will do lots of things like incrementing counters and waking up mac80211 queues...
That should be alright. As a rule of thumb, for every call to carl9170_tx_accounting [called by carl9170_op_tx tx handler] we need to call carl9170_tx_status. If the station is still in ps mode, mac80211 will cache the station's frames and sets the TIM bits.
Quoted 9 lines
Part of the reason i wrote this, is because, while this patch fixes the memory accounting problem and prevent the AP from stopping working, the powersaving station sometimes cannot communicate with the AP anymore (other stations can). The station uses ath9k, and is just really doing a background scan. The first traces that i have just indicate that the station entered powersave mode, then sends probe requests and probe requests directed to the AP, then sends authentication requests to the AP (?), but the AP does not respond to any of them...

Quoted 1 line

Do you run an 802.11n ap on carl9170?

No, it's a 802.11g currently.
My current wild guess is that, the responses to those requests are somehow still queued ...

Nicolas Cavallari

author

2012-02-24 13:35 UTC #5 lore raw

Quoted 21 lines · Nicolas Cavallari

Quoted 20 lines · Christian Lamparter
Quoted 19 lines · Nicolas Cavallari
Quoted 9 lines · Christian Lamparter
Quoted 3 lines · Nicolas Cavallari
While this seems to fix my problem, there could still be problems remaining in this code path. Particularly carl9170_tx_status will do lots of things like incrementing counters and waking up mac80211 queues...
That should be alright. As a rule of thumb, for every call to carl9170_tx_accounting [called by carl9170_op_tx tx handler] we need to call carl9170_tx_status. If the station is still in ps mode, mac80211 will cache the station's frames and sets the TIM bits.
Part of the reason i wrote this, is because, while this patch fixes the memory accounting problem and prevent the AP from stopping working, the powersaving station sometimes cannot communicate with the AP anymore (other stations can). The station uses ath9k, and is just really doing a background scan. The first traces that i have just indicate that the station entered powersave mode, then sends probe requests and probe requests directed to the AP, then sends authentication requests to the AP (?), but the AP does not respond to any of them...

Quoted 4 lines

Quoted 1 line
Do you run an 802.11n ap on carl9170?
No, it's a 802.11g currently.
My current wild guess is that, the responses to those requests are somehow still queued ...

And i was right...

/*
* Ignore doze->wake transitions that are
* indicated by non-data frames, the standard
* is unclear here, but for example going to
* PS mode and then scanning would cause a
* doze->wake transition for the probe request,
* and that is clearly undesirable.
*/
if (ieee80211_is_data(hdr->frame_control) &&

    !ieee80211_has_pm(hdr->frame_control))
	ap_sta_ps_end(sta);

As (re)associations requests and Auth frames are not data frames, mac80211 still considers that the station is still in powersave mode, so it still queues frames, and even queues the responses to the auth frames. Except that the station disassociated silently, and is trying to re-associate after a short time.

Shouldn't mac80211 transition the sta to "awake" when receiving auth/assoc frames ?

Johannes Berg

2012-02-24 13:39 UTC #6 lore raw

Quoted 41 lines · Nicolas Cavallari

Quoted 21 lines · Nicolas Cavallari
Quoted 20 lines · Christian Lamparter
Quoted 19 lines · Nicolas Cavallari
Quoted 9 lines · Christian Lamparter
Quoted 3 lines · Nicolas Cavallari
While this seems to fix my problem, there could still be problems remaining in this code path. Particularly carl9170_tx_status will do lots of things like incrementing counters and waking up mac80211 queues...
That should be alright. As a rule of thumb, for every call to carl9170_tx_accounting [called by carl9170_op_tx tx handler] we need to call carl9170_tx_status. If the station is still in ps mode, mac80211 will cache the station's frames and sets the TIM bits.
Part of the reason i wrote this, is because, while this patch fixes the memory accounting problem and prevent the AP from stopping working, the powersaving station sometimes cannot communicate with the AP anymore (other stations can). The station uses ath9k, and is just really doing a background scan. The first traces that i have just indicate that the station entered powersave mode, then sends probe requests and probe requests directed to the AP, then sends authentication requests to the AP (?), but the AP does not respond to any of them...
Quoted 4 lines
Quoted 1 line
Do you run an 802.11n ap on carl9170?
No, it's a 802.11g currently.
My current wild guess is that, the responses to those requests are somehow still queued ...
And i was right...
/*
* Ignore doze->wake transitions that are
* indicated by non-data frames, the standard
* is unclear here, but for example going to
* PS mode and then scanning would cause a
* doze->wake transition for the probe request,
* and that is clearly undesirable.
*/
if (ieee80211_is_data(hdr->frame_control) &&
    !ieee80211_has_pm(hdr->frame_control))
	ap_sta_ps_end(sta);

Quoted 8 lines

As (re)associations requests and Auth frames are not data frames, mac80211 still considers that the station is still in powersave mode, so it still queues frames, and even queues the responses to the auth frames. Except that the station disassociated silently, and is trying to re-associate after a short time.
Shouldn't mac80211 transition the sta to "awake" when receiving auth/assoc frames ?

I don't think so, that would make us send frames to the station it no longer deserves to have. Hostapd should instead delete & re-create the station entry, I think?

johannes

Nicolas Cavallari

author

2012-02-24 14:01 UTC #7 lore raw

Quoted 48 lines · Johannes Berg

Quoted 47 lines · Nicolas Cavallari
Quoted 21 lines · Nicolas Cavallari
Quoted 20 lines · Christian Lamparter
Quoted 19 lines · Nicolas Cavallari
Quoted 9 lines · Christian Lamparter
Quoted 3 lines · Nicolas Cavallari
While this seems to fix my problem, there could still be problems remaining in this code path. Particularly carl9170_tx_status will do lots of things like incrementing counters and waking up mac80211 queues...
That should be alright. As a rule of thumb, for every call to carl9170_tx_accounting [called by carl9170_op_tx tx handler] we need to call carl9170_tx_status. If the station is still in ps mode, mac80211 will cache the station's frames and sets the TIM bits.
Part of the reason i wrote this, is because, while this patch fixes the memory accounting problem and prevent the AP from stopping working, the powersaving station sometimes cannot communicate with the AP anymore (other stations can). The station uses ath9k, and is just really doing a background scan. The first traces that i have just indicate that the station entered powersave mode, then sends probe requests and probe requests directed to the AP, then sends authentication requests to the AP (?), but the AP does not respond to any of them...
Quoted 4 lines
Quoted 1 line
Do you run an 802.11n ap on carl9170?
No, it's a 802.11g currently.
My current wild guess is that, the responses to those requests are somehow still queued ...
And i was right...
/*
* Ignore doze->wake transitions that are
* indicated by non-data frames, the standard
* is unclear here, but for example going to
* PS mode and then scanning would cause a
* doze->wake transition for the probe request,
* and that is clearly undesirable.
*/
if (ieee80211_is_data(hdr->frame_control) &&
    !ieee80211_has_pm(hdr->frame_control))
	ap_sta_ps_end(sta);
As (re)associations requests and Auth frames are not data frames, mac80211 still considers that the station is still in powersave mode, so it still queues frames, and even queues the responses to the auth frames. Except that the station disassociated silently, and is trying to re-associate after a short time.

Quoted 9 lines

Quoted 2 lines
Shouldn't mac80211 transition the sta to "awake" when receiving auth/assoc frames ?
I don't think so, that would make us send frames to the station it no longer deserves to have. Hostapd should instead delete & re-create the station entry, I think?
johannes

Hostapd already does that¹ ... after the association response is acked.

Will send a patch for hostapd then.

[¹] :
/*
* Remove the STA entry in order to make sure the STA PS state gets
* cleared and configuration gets updated in case of reassociation back
* to the same AP.
*/
hostapd_drv_sta_remove(hapd, sta->addr);

Johannes Berg

2012-02-24 14:09 UTC #8 lore raw

Quoted 9 lines · Nicolas Cavallari

Quoted 6 lines
Quoted 2 lines
Shouldn't mac80211 transition the sta to "awake" when receiving auth/assoc frames ?
I don't think so, that would make us send frames to the station it no longer deserves to have. Hostapd should instead delete & re-create the station entry, I think?
Hostapd already does that¹ ... after the association response is acked.

Ok maybe then we should send out assoc resp etc. frames and not subject them to PS queueing? Jouni?

johannes

Jouni Malinen

2012-02-25 10:34 UTC #9 lore raw

Quoted 5 lines · Johannes Berg

Quoted 1 line · Nicolas Cavallari
Hostapd already does that¹ ... after the association response is acked.
Ok maybe then we should send out assoc resp etc. frames and not subject them to PS queueing? Jouni?

We should try to follow the clarified description of management frame buffering in P802.11-REVmb/D12.0 (i.e., soon to be published IEEE Std 802.11-2012). (Re)Association Response frame is not a bufferable management frame and as such, it needs to be sent out without being subject to PS queuing.

Signature · 1 line

Jouni Malinen                                            PGP id EFC895FA

Johannes Berg

2012-02-25 20:12 UTC #10 lore raw

Quoted 4 lines · Jouni Malinen

Quoted 3 lines · Johannes Berg
Quoted 1 line · Nicolas Cavallari
Hostapd already does that¹ ... after the association response is acked.

Quoted 8 lines

Quoted 2 lines
Ok maybe then we should send out assoc resp etc. frames and not subject them to PS queueing? Jouni?
We should try to follow the clarified description of management frame buffering in P802.11-REVmb/D12.0 (i.e., soon to be published IEEE Std 802.11-2012). (Re)Association Response frame is not a bufferable management frame and as such, it needs to be sent out without being subject to PS queuing.

Ah yes, you mentioned before that this was being clarified but I forgot about it again. I'll take a look.

johannes

Christian Lamparter

maintainer

2012-02-25 20:20 UTC #11 lore raw

Quoted 8 lines · On Saturday 25 February 2012 21:12:52 Johannes Berg wrote:

Quoted 7 lines · Jouni Malinen
Quoted 5 lines · Johannes Berg
Quoted 1 line · Nicolas Cavallari
Hostapd already does that¹ ... after the association response is acked.
Ok maybe then we should send out assoc resp etc. frames and not subject them to PS queueing? Jouni?

Quoted 9 lines

Quoted 5 lines
We should try to follow the clarified description of management frame buffering in P802.11-REVmb/D12.0 (i.e., soon to be published IEEE Std 802.11-2012). (Re)Association Response frame is not a bufferable management frame and as such, it needs to be sent out without being subject to PS queuing.
Ah yes, you mentioned before that this was being clarified but I forgot about it again. I'll take a look.

Actually, I think I found the problem in carl9170 [writing a patch atm]. [I forgot to check IEEE80211_TX_CTL_POLL_RESPONSE and IEEE80211_TX_CTL_CLEAR_PS_FILT flags].

[A simple fix: we could set something like TX_CTL_IGNORE_PS (now known as TX_CTL_POLL_RESPONSE) on outgoing auth & (re-)assoc frames]

Regards,
Chr

Christian Lamparter

maintainer

2012-02-24 14:07 UTC #12 lore raw

In reply to Nicolas Cavallari

Quoted 38 lines · Nicolas Cavallari

Quoted 21 lines · Nicolas Cavallari
Quoted 20 lines · Christian Lamparter
Quoted 19 lines · Nicolas Cavallari
Quoted 9 lines · Christian Lamparter
Quoted 3 lines · Nicolas Cavallari
While this seems to fix my problem, there could still be problems remaining in this code path. Particularly carl9170_tx_status will do lots of things like incrementing counters and waking up mac80211 queues...
That should be alright. As a rule of thumb, for every call to carl9170_tx_accounting [called by carl9170_op_tx tx handler] we need to call carl9170_tx_status. If the station is still in ps mode, mac80211 will cache the station's frames and sets the TIM bits.
Part of the reason i wrote this, is because, while this patch fixes the memory accounting problem and prevent the AP from stopping working, the powersaving station sometimes cannot communicate with the AP anymore (other stations can). The station uses ath9k, and is just really doing a background scan. The first traces that i have just indicate that the station entered powersave mode, then sends probe requests and probe requests directed to the AP, then sends authentication requests to the AP (?), but the AP does not respond to any of them...
Quoted 4 lines
Quoted 1 line
Do you run an 802.11n ap on carl9170?
No, it's a 802.11g currently.
My current wild guess is that, the responses to those requests are somehow still queued ...
And i was right...
/*
* Ignore doze->wake transitions that are
* indicated by non-data frames, the standard
* is unclear here, but for example going to
* PS mode and then scanning would cause a
* doze->wake transition for the probe request,
* and that is clearly undesirable.
*/
if (ieee80211_is_data(hdr->frame_control) &&

Quoted 8 lines

    !ieee80211_has_pm(hdr->frame_control))
	ap_sta_ps_end(sta);
As (re)associations requests and Auth frames are not data frames, mac80211 still considers that the station is still in powersave mode, so it still queues frames, and even queues the responses to the auth frames. Except that the station disassociated silently, and is trying to re-associate after a short time.

That's really odd... Doesn't your ath9k client say why it deauthed from the ap in the first place? Furthermore, [I think] there's no such thing as a "silent disassociation", I bet we will find the missing deauth frames stuck in a queue as well...

Regards,
Chr

Christian Lamparter

maintainer

2012-02-24 20:43 UTC #13 lore raw

In reply to Nicolas Cavallari

Quoted 11 lines · On Thursday 23 February 2012 18:27:59 Nicolas Cavallari wrote:

Quoted 9 lines · Christian Lamparter
Quoted 3 lines · Nicolas Cavallari
While this seems to fix my problem, there could still be problems remaining in this code path. Particularly carl9170_tx_status will do lots of things like incrementing counters and waking up mac80211 queues...
That should be alright. As a rule of thumb, for every call to carl9170_tx_accounting [called by carl9170_op_tx tx handler] we need to call carl9170_tx_status. If the station is still in ps mode, mac80211 will cache the station's frames and sets the TIM bits.

Quoted 8 lines

Part of the reason i wrote this, is because, while this patch fixes the memory accounting problem and prevent the AP from stopping working, the powersaving station sometimes cannot communicate with the AP anymore (other stations can). The station uses ath9k, and is just really doing a background scan. The first traces that i have just indicate that the station entered powersave mode, then sends probe requests and probe requests directed to the AP, then sends authentication requests to the AP (?), but the AP does not respond to any of them...

understood.

In the meantime, I think we can put this fix into the kernel queue right away.

John,
would you please do the honors? :-D

Regards,
Chr

Christian Lamparter

maintainer

2012-02-25 20:36 UTC #14 lore raw

PATCH 1/1 carl9170: fix frame delivery if sta is in powersave mode

In reply to Nicolas Cavallari

Nicolas Cavallari discovered that carl9170 has some serious problems delivering data to sleeping stations.

It turns out that the driver was not honoring two important flags (IEEE80211_TX_CTL_POLL_RESPONSE and IEEE80211_TX_CTL_CLEAR_PS_FILT) which are set on frames that should be sent although the receiving station is still in powersave mode.

Cc stable <[email protected]>

Reported-by Nicolas Cavallari <[email protected]>

Signed-off-by Christian Lamparter <[email protected]>

---

Quoted 9 lines

Part of the reason i wrote this, is because, while this patch fixes the memory accounting problem and prevent the AP from stopping working, the powersaving station sometimes cannot communicate with the AP anymore (other stations can). The station uses ath9k, and is just really doing a background scan. The first traces that i have just indicate that the station entered powersave mode, then sends probe requests and probe requests directed to the AP, then sends authentication requests to the AP (?), but the AP does not respond to any of them...

Ok, I think this patch [on top of yours] should fix the problem.
---

drivers/net/wireless/ath/carl9170/tx.c +5 -3

index 771e1a9..0f00721 100644
@@ -1236,6 +1236,7 @@ static bool carl9170_tx_ps_drop(struct ar9170 *ar, struct sk_buff *skb)
 {
 	struct ieee80211_sta *sta;
 	struct carl9170_sta_info *sta_info;
+	struct ieee80211_tx_info *tx_info;
 
 	rcu_read_lock();
 	sta = __carl9170_get_tx_sta(ar, skb);

@@ -1243,17 +1244,18 @@ static bool carl9170_tx_ps_drop(struct ar9170 *ar, struct sk_buff *skb)
 		goto out_rcu;
 
 	sta_info = (void *) sta->drv_priv;
-	if (unlikely(sta_info->sleeping)) {
-		struct ieee80211_tx_info *tx_info;
+	tx_info = IEEE80211_SKB_CB(skb);
 
+	if (unlikely(sta_info->sleeping) &&
+	    !(tx_info->flags & (IEEE80211_TX_CTL_POLL_RESPONSE |
+				IEEE80211_TX_CTL_CLEAR_PS_FILT))) {
 		rcu_read_unlock();
 
-		tx_info = IEEE80211_SKB_CB(skb);
 		if (tx_info->flags & IEEE80211_TX_CTL_AMPDU)
 			atomic_dec(&ar->tx_ampdu_upload);
 
 		tx_info->flags |= IEEE80211_TX_STAT_TX_FILTERED;
 		carl9170_release_dev_space(ar, skb);
 		carl9170_tx_status(ar, skb, false);
 		return true;
 	}

Nicolas Cavallari

author

2012-02-27 12:00 UTC #15 lore raw

Re: [PATCH -stable] carl9170: fix frame delivery if sta is in powersave mode

Quoted 22 lines · Christian Lamparter

Nicolas Cavallari discovered that carl9170 has some serious problems delivering data to sleeping stations.
It turns out that the driver was not honoring two important flags (IEEE80211_TX_CTL_POLL_RESPONSE and IEEE80211_TX_CTL_CLEAR_PS_FILT) which are set on frames that should be sent although the receiving station is still in powersave mode.
Cc stable <[email protected]>
Reported-by Nicolas Cavallari <[email protected]>
Signed-off-by Christian Lamparter <[email protected]>
---
Quoted 9 lines
Part of the reason i wrote this, is because, while this patch fixes the memory accounting problem and prevent the AP from stopping working, the powersaving station sometimes cannot communicate with the AP anymore (other stations can). The station uses ath9k, and is just really doing a background scan. The first traces that i have just indicate that the station entered powersave mode, then sends probe requests and probe requests directed to the AP, then sends authentication requests to the AP (?), but the AP does not respond to any of them...

Quoted 1 line

Ok, I think this patch [on top of yours] should fix the problem.

This effectively fixes the problem.

Thanks !