Re: p54: AP mode: no data frame despite traffic indication set in TIM

[Christian Lamparter <chunkeey@web.de>]

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

On Monday 24 November 2008 14:37:54 Johannes Berg wrote:
> On Fri, 2008-11-21 at 15:12 +0100, Stefan Steuerwald wrote:
> 
> > - frame 7: http request
> > - frame 13: iPod goes to sleep
> > - frame 23: AP beacon indicates traffic for iPod (AID 1 in TIM)
> 
> in 24 too, the ipod probably didn't see the beacon in frame 23 even
> though 23 was a dtim beacon (which is a bit odd, but maybe the ipod
> doesn't care about mcast at this point)
> 
> > - frame 25: iPod wakes up
> 
> 26: ack from the AP
> 
> > - in between: MISSING DATA ???
> > - frame 27: AP beacon with no traffic indicated ???
> > - frame 29: iPod goes to sleep again
> > - subsequent frames: repetitions of this, until the TCP connection is closed
> > 
> > My understanding is that the AP would not indicate any traffic without
> > actually having some ready to send? Wrong assumption?
> 
> Indeed. Christian, is it possible that the p54 device is actually
> filtering these frames? I'm pretty sure mac80211 behaves correctly, and
> it unsetting the TIM bit means that it must no longer have traffic
> buffered.
As far as I know it works like this:
If a frame with a the PS-Bit in the FC set is received, the firmware
will mark the source mac / aid as "sleeping". And every frame from
this moment on for this device will be buffered.

To remove the "mark" again, the driver has to call p54_sta_unlock.
And the firmware will send out all buffered frames.
Or if we only need for one frame (e.g: probe resp) we have a tx_flag (_NO_CANCEL)

If for some reason the "mark" doesn't get removed the firmware will eventually timeout
the stuck frame and sets a the P54_TX_PSM_CANCELLED flag in tx_status.
And we pass this on to the mac with IEEE80211_TX_STAT_TX_FILTERED.

one thing: p54 reports the tx_status through the rx-ring-buffer.
so I hope there's no rx/tx race here since everything is in the same "boat" here.

based on that:
I made two different patches to address the problem.

one fiddles with mac80211 only (set-and-clear.diff).
It assumes that if a station comes out of PS, we have to set
the CLEAR_PS_FILT on the same time we clear the WLAN_STA_PS.

the other one is only for p54 (p54-sta-flags.diff)... Doesn't do very much,
it just checks if the CLEAR_PS_FILT is set and then sets the
NO_CANCEL flag on that frame, so the firmware won't buffer it.

Of course you can test both patches on the same time, if it doesn't help.

And finally of course, I could be totally wrong and this is all nothing but useless gibberish.

Regards,
	Chr

BTW: I couldn't test the patches, so it may OOps

[-- Attachment #2: set-and-clear.diff --]
[-- Type: text/x-diff, Size: 930 bytes --]

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 5a1a60f..077fdb7 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -657,7 +657,8 @@ static void ap_sta_ps_start(struct sta_info *sta)
 	DECLARE_MAC_BUF(mac);
 
 	atomic_inc(&sdata->bss->num_sta_ps);
-	set_and_clear_sta_flags(sta, WLAN_STA_PS, WLAN_STA_PSPOLL);
+	set_and_clear_sta_flags(sta, WLAN_STA_PS, WLAN_STA_PSPOLL |
+				WLAN_STA_CLEAR_PS_FILT);
 #ifdef CONFIG_MAC80211_VERBOSE_PS_DEBUG
 	printk(KERN_DEBUG "%s: STA %s aid %d enters power save mode\n",
 	       sdata->dev->name, print_mac(mac, sta->sta.addr), sta->sta.aid);
@@ -674,7 +675,8 @@ static int ap_sta_ps_end(struct sta_info *sta)
 
 	atomic_dec(&sdata->bss->num_sta_ps);
 
-	clear_sta_flags(sta, WLAN_STA_PS | WLAN_STA_PSPOLL);
+	set_and_clear_sta_flags(sta, WLAN_STA_CLEAR_PS_FILT,
+				WLAN_STA_PS | WLAN_STA_PSPOLL);
 
 	if (!skb_queue_empty(&sta->ps_tx_buf))
 		sta_info_clear_tim_bit(sta);

[-- Attachment #3: p54-sta-flags.diff --]
[-- Type: text/x-diff, Size: 1973 bytes --]

diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
--- a/drivers/net/wireless/p54/p54common.c	2008-11-24 13:07:09.053832187 +0100
+++ b/drivers/net/wireless/p54/p54common.c	2008-11-24 16:03:26.862045588 +0100
@@ -1067,9 +1067,15 @@ static int p54_tx_fill(struct ieee80211_
 			*queue = 3;
 			return 0;
 		}
-		if (info->control.sta)
+		if (info->control.sta) {
+			if (info->flags & IEEE80211_TX_CTL_CLEAR_PS_FILT) {
+				ret = p54_sta_unlock(dev, info->control.sta->addr);
+				if (ret)
+					return ret;
+				*flags |= P54_HDR_FLAG_DATA_OUT_NOCANCEL;
+			}
 			*aid = info->control.sta->aid;
-		else
+		} else
 			*flags = P54_HDR_FLAG_DATA_OUT_NOCANCEL;
 	}
 	return ret;
@@ -1083,7 +1089,7 @@ static int p54_tx(struct ieee80211_hw *d
 	struct p54_hdr *hdr;
 	struct p54_tx_data *txhdr;
 	size_t padding, len, tim_len = 0;
-	int i, j, ridx;
+	int i, j, ridx, ret;
 	u16 hdr_flags = 0, aid = 0;
 	u8 rate, queue;
 	u8 cts_rate = 0x20;
@@ -1093,7 +1099,10 @@ static int p54_tx(struct ieee80211_hw *d
 
 	queue = skb_get_queue_mapping(skb);
 
-	if (p54_tx_fill(dev, skb, info, &queue, &tim_len, &hdr_flags, &aid)) {
+	ret = p54_tx_fill(dev, skb, info, &queue, &tim_len, &hdr_flags, &aid);
+	if (ret < 0)
+		return NETDEV_TX_BUSY;
+	if (ret) {
 		current_queue = &priv->tx_stats[queue];
 		if (unlikely(current_queue->len > current_queue->limit))
 			return NETDEV_TX_BUSY;
@@ -1106,17 +1115,6 @@ static int p54_tx(struct ieee80211_hw *d
 	padding = (unsigned long)(skb->data - (sizeof(*hdr) + sizeof(*txhdr))) & 3;
 	len = skb->len;
 
-	if (info->flags & IEEE80211_TX_CTL_CLEAR_PS_FILT) {
-		if (info->control.sta)
-			if (p54_sta_unlock(dev, info->control.sta->addr)) {
-				if (current_queue) {
-					current_queue->len--;
-					current_queue->count--;
-				}
-				return NETDEV_TX_BUSY;
-			}
-	}
-
 	txhdr = (struct p54_tx_data *) skb_push(skb, sizeof(*txhdr) + padding);
 	hdr = (struct p54_hdr *) skb_push(skb, sizeof(*hdr));