p54 problem with PS

p54 problem with PS

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 516 bytes --]

Right after associating, the AP sends us the EAPOL frame. We miss that
frame, due to a race condition (we add the STA info for the AP a little
too late and drop the frame because we don't know who it is from).

The AP of course retries the frame, a little later, but p54 misses the
TIM bit -- see the ps.pkt file.

We should probably make that race window smaller somehow (stop the
tasklet that is processing RX while processing assoc frames maybe?) but
this is clearly foremost a p54 problem.

johannes

[-- Attachment #1.2: ps.pkt --]
[-- Type: application/octet-stream, Size: 8286 bytes --]

[-- Attachment #1.3: nops.pkt --]
[-- Type: application/octet-stream, Size: 11484 bytes --]

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

Re: p54 problem with PS

[Christian Lamparter]

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

On Wednesday 15 April 2009 21:24:55 Johannes Berg wrote:
> Right after associating, the AP sends us the EAPOL frame. We miss that
> frame, due to a race condition (we add the STA info for the AP a little
> too late and drop the frame because we don't know who it is from).
> 
> The AP of course retries the frame, a little later, but p54 misses the
> TIM bit -- see the ps.pkt file.
> 
> We should probably make that race window smaller somehow (stop the
> tasklet that is processing RX while processing assoc frames maybe?) but
> this is clearly foremost a p54 problem.

Well, I guess I figured out what's wrong: the hw spec.
So this patch might work on PCI & USB, but could break SPI devices.

Johannes, can you please test the attached patch?

BTW: there might be two problems:
1. if the timeout is too low the device might be fast enough to go into ps:
  -  either before sending a probe request
   (firmware reports tx_status TX_PSM, so the frame wasn't sent at all)
  - or the AP got the request and responded... 
    But the frame was lost, because probe responds are not buffered...

2. there was something wrong in ieee80211_rx_mgmt_beacon
    Sometimes the TIM was parsed, but thanks to
    local->hw.conf.dynamic_ps_timeout == 0
    mac80211 didn't issue a wakeup => so no data => disconnect

    But this bug maybe comes from the old RFCs from yesterday,
    as I didn't tried the final version yet...

Oh and one question:
Some time ago, I found some useful function that converted beacon_int TUs <-> msec,
do we still have them somewhere around and I'm just (temp) blind ?

Regards,
	Chr

[-- Attachment #2: p54ps-test.diff --]
[-- Type: text/x-patch, Size: 1677 bytes --]

diff --git a/drivers/net/wireless/p54/p54.h b/drivers/net/wireless/p54/p54.h
index d6354fa..482071b 100644
--- a/drivers/net/wireless/p54/p54.h
+++ b/drivers/net/wireless/p54/p54.h
@@ -181,6 +181,7 @@ struct p54_common {
 	u32 tsf_low32, tsf_high32;
 	u32 basic_rate_mask;
 	u16 aid;
+	unsigned long beacon_timeout_guard;
 	struct sk_buff *cached_beacon;
 
 	/* cryptographic engine information */
diff --git a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
index ad2503a..0bd5e73 100644
--- a/drivers/net/wireless/p54/p54common.c
+++ b/drivers/net/wireless/p54/p54common.c
@@ -1058,7 +1058,8 @@ static void p54_rx_trap(struct ieee80211_hw *dev, struct sk_buff *skb)
 			wiphy_name(dev->wiphy), freq);
 		break;
 	case P54_TRAP_NO_BEACON:
-		if (priv->vif)
+		if (time_after(priv->beacon_timeout_guard, jiffies) &&
+		    priv->vif)
 			ieee80211_beacon_loss(priv->vif);
 		break;
 	case P54_TRAP_SCAN:
@@ -1941,10 +1942,12 @@ static int p54_set_ps(struct ieee80211_hw *dev)
 	u16 mode;
 	int i;
 
-	if (dev->conf.flags & IEEE80211_CONF_PS)
+	if (dev->conf.flags & IEEE80211_CONF_PS) {
 		mode = P54_PSM | P54_PSM_BEACON_TIMEOUT | P54_PSM_DTIM |
 		       P54_PSM_CHECKSUM | P54_PSM_MCBC;
-	else
+		priv->beacon_timeout_guard = jiffies + msecs_to_jiffies(
+						priv->hw->conf.beacon_int);
+	} else
 		mode = P54_PSM_CAM;
 
 	skb = p54_alloc_skb(dev, P54_HDR_FLAG_CONTROL_OPSET, sizeof(*psm),
@@ -1963,8 +1966,8 @@ static int p54_set_ps(struct ieee80211_hw *dev)
 
 	psm->beacon_rssi_skip_max = 200;
 	psm->rssi_delta_threshold = 0;
-	psm->nr = 10;
-	psm->exclude[0] = 0;
+	psm->nr = 1;
+	psm->exclude[0] = WLAN_EID_TIM;
 
 	priv->tx(dev, skb);
 

Re: p54 problem with PS

[Johannes Berg]

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


> Well, I guess I figured out what's wrong: the hw spec.
> So this patch might work on PCI & USB, but could break SPI devices.

Heh.

> Johannes, can you please test the attached patch?
> 
> BTW: there might be two problems:
> 1. if the timeout is too low the device might be fast enough to go into ps:
>   -  either before sending a probe request
>    (firmware reports tx_status TX_PSM, so the frame wasn't sent at all)
>   - or the AP got the request and responded... 
>     But the frame was lost, because probe responds are not buffered...

?

> 2. there was something wrong in ieee80211_rx_mgmt_beacon
>     Sometimes the TIM was parsed, but thanks to
>     local->hw.conf.dynamic_ps_timeout == 0
>     mac80211 didn't issue a wakeup => so no data => disconnect
> 
>     But this bug maybe comes from the old RFCs from yesterday,
>     as I didn't tried the final version yet...

I didn't change that -- Kalle did that, see the comment there. I guess
we need to change that then.

> Oh and one question:
> Some time ago, I found some useful function that converted beacon_int TUs <-> msec,
> do we still have them somewhere around and I'm just (temp) blind ?

I just added TU -> msec in one of my patches to include/linux/ieee8021.h

johannes

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

Re: p54 problem with PS

[Christian Lamparter]

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

On Thursday 16 April 2009 22:18:24 Johannes Berg wrote:
> 
> > Well, I guess I figured out what's wrong: the hw spec.
> > So this patch might work on PCI & USB, but could break SPI devices.
> 
> Heh.
> 
> > Johannes, can you please test the attached patch?
> > 

updated version.

[-- Attachment #2: p54ps-3.diff --]
[-- Type: text/x-patch, Size: 5282 bytes --]

diff --git a/drivers/net/wireless/p54/p54.h b/drivers/net/wireless/p54/p54.h
index 7fda1a9..1e999aa 100644
--- a/drivers/net/wireless/p54/p54.h
+++ b/drivers/net/wireless/p54/p54.h
@@ -181,6 +181,8 @@ struct p54_common {
 	u32 tsf_low32, tsf_high32;
 	u32 basic_rate_mask;
 	u16 aid;
+	bool allow_ps_filtering;
+	unsigned long beacon_timeout_guard;
 	struct sk_buff *cached_beacon;
 
 	/* cryptographic engine information */
diff --git a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
index 716c8a0..2c98904 100644
--- a/drivers/net/wireless/p54/p54common.c
+++ b/drivers/net/wireless/p54/p54common.c
@@ -739,6 +739,61 @@ static int p54_rssi_to_dbm(struct ieee80211_hw *dev, int rssi)
 			 priv->rssical_db[band].add) / 4;
 }
 
+static u8 *p54_find_ie_elem(u8 elem, u8 *ie, u8 *end)
+{
+	while (ie < end) {
+		if (ie + 2 + ie[1] > end)
+			return NULL;
+
+		if (ie[0] == elem)
+			return ie;
+
+		ie += 2 + ie[1];
+	}
+
+	return NULL;
+}
+
+static int p54_set_ps(struct ieee80211_hw *dev);
+
+static void p54_rx_ps_workaround(struct ieee80211_hw *dev, struct sk_buff *skb)
+{
+	struct p54_common *priv = dev->priv;
+	struct ieee80211_mgmt *mgmt = (void *)skb->data;
+	struct ieee80211_tim_ie *tim;
+	u8 *pos, *end, *ie;
+
+	if (skb->len <= sizeof(mgmt))
+		return ;
+
+	if (!ieee80211_is_beacon(mgmt->frame_control))
+		return ;
+
+	if (compare_ether_addr(priv->bssid, mgmt->bssid))
+		return ;
+
+	pos = (u8 *)mgmt->u.beacon.variable;
+	end = skb->data + skb->len;
+	ie = p54_find_ie_elem(WLAN_EID_TIM, pos, end);
+	if (!ie)
+		return ;
+
+	tim = (void *) &ie[2];
+	if (ieee80211_check_tim(tim, ie[1], priv->aid)) {
+		/*
+		 * disable powersave management, as long as
+		 * our TIM is set in the beacon
+		 * else we would miss PS-POLL frames
+		 */
+
+		priv->allow_ps_filtering = false;
+		p54_set_ps(dev);
+	} else {
+		priv->allow_ps_filtering = true;
+		p54_set_ps(dev);
+	}
+}
+
 static int p54_rx_data(struct ieee80211_hw *dev, struct sk_buff *skb)
 {
 	struct p54_common *priv = dev->priv;
@@ -794,6 +846,10 @@ static int p54_rx_data(struct ieee80211_hw *dev, struct sk_buff *skb)
 	skb_pull(skb, header_len);
 	skb_trim(skb, le16_to_cpu(hdr->len));
 
+	/* workaround for checksumming bug */
+	if (unlikely(dev->conf.flags & IEEE80211_CONF_PS))
+		p54_rx_ps_workaround(dev, skb);
+
 	ieee80211_rx_irqsafe(dev, skb, &rx_status);
 
 	queue_delayed_work(dev->workqueue, &priv->work,
@@ -1076,7 +1132,11 @@ static void p54_rx_trap(struct ieee80211_hw *dev, struct sk_buff *skb)
 			wiphy_name(dev->wiphy), freq);
 		break;
 	case P54_TRAP_NO_BEACON:
-		if (priv->vif)
+		if (time_before(priv->beacon_timeout_guard, jiffies))
+			printk(KERN_INFO "Fake beacon loss\n");
+
+		if (time_after(priv->beacon_timeout_guard, jiffies) &&
+		    priv->vif)
 			ieee80211_beacon_loss(priv->vif);
 		break;
 	case P54_TRAP_SCAN:
@@ -1959,10 +2019,14 @@ static int p54_set_ps(struct ieee80211_hw *dev)
 	u16 mode;
 	int i;
 
-	if (dev->conf.flags & IEEE80211_CONF_PS)
+	if (dev->conf.flags & IEEE80211_CONF_PS &&
+	    priv->allow_ps_filtering) {
 		mode = P54_PSM | P54_PSM_BEACON_TIMEOUT | P54_PSM_DTIM |
 		       P54_PSM_CHECKSUM | P54_PSM_MCBC;
-	else
+
+		priv->beacon_timeout_guard = jiffies + usecs_to_jiffies(
+			ieee80211_tu_to_usec(priv->hw->conf.beacon_int));
+	} else
 		mode = P54_PSM_CAM;
 
 	skb = p54_alloc_skb(dev, P54_HDR_FLAG_CONTROL_OPSET, sizeof(*psm),
@@ -1981,8 +2045,8 @@ static int p54_set_ps(struct ieee80211_hw *dev)
 
 	psm->beacon_rssi_skip_max = 200;
 	psm->rssi_delta_threshold = 0;
-	psm->nr = 10;
-	psm->exclude[0] = 0;
+	psm->nr = 1;
+	psm->exclude[0] = WLAN_EID_TIM;
 
 	priv->tx(dev, skb);
 
@@ -1998,42 +2062,38 @@ static int p54_beacon_tim(struct sk_buff *skb)
 	 */
 
 	struct ieee80211_mgmt *mgmt = (void *)skb->data;
-	u8 *pos, *end;
+	u8 *pos, *end, *tim_ie, *next;
+	u8 dtim_period, dtim_len;
 
 	if (skb->len <= sizeof(mgmt))
 		return -EINVAL;
 
 	pos = (u8 *)mgmt->u.beacon.variable;
 	end = skb->data + skb->len;
-	while (pos < end) {
-		if (pos + 2 + pos[1] > end)
-			return -EINVAL;
+	tim_ie = p54_find_ie_elem(WLAN_EID_TIM, pos, end);
+	if (!tim_ie)
+		return 0;
 
-		if (pos[0] == WLAN_EID_TIM) {
-			u8 dtim_len = pos[1];
-			u8 dtim_period = pos[3];
-			u8 *next = pos + 2 + dtim_len;
+	if (tim_ie[1] < 3)
+		return -EINVAL;
 
-			if (dtim_len < 3)
-				return -EINVAL;
+	dtim_len = tim_ie[1];
+	dtim_period = tim_ie[3];
+	next = pos + 2 + dtim_len;
 
-			memmove(pos, next, end - next);
+	memmove(pos, next, end - next);
 
-			if (dtim_len > 3)
-				skb_trim(skb, skb->len - (dtim_len - 3));
+	if (dtim_len > 3)
+		skb_trim(skb, skb->len - (dtim_len - 3));
 
-			pos = end - (dtim_len + 2);
+	pos = end - (dtim_len + 2);
 
-			/* add the dummy at the end */
-			pos[0] = WLAN_EID_TIM;
-			pos[1] = 3;
-			pos[2] = 0;
-			pos[3] = dtim_period;
-			pos[4] = 0;
-			return 0;
-		}
-		pos += 2 + pos[1];
-	}
+	/* add the dummy at the end */
+	pos[0] = WLAN_EID_TIM;
+	pos[1] = 3;
+	pos[2] = 0;
+	pos[3] = dtim_period;
+	pos[4] = 0;
 	return 0;
 }
 
@@ -2093,6 +2153,8 @@ static int p54_start(struct ieee80211_hw *dev)
 
 	queue_delayed_work(dev->workqueue, &priv->work, 0);
 
+	priv->allow_ps_filtering = true;
+	priv->beacon_timeout_guard = jiffies;
 	priv->softled_state = 0;
 	err = p54_set_leds(dev);
 

Re: p54 problem with PS

[Johannes Berg]

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

On Fri, 2009-04-17 at 13:56 +0200, Christian Lamparter wrote:
> On Thursday 16 April 2009 22:18:24 Johannes Berg wrote:
> > 
> > > Well, I guess I figured out what's wrong: the hw spec.
> > > So this patch might work on PCI & USB, but could break SPI devices.
> > 
> > Heh.
> > 
> > > Johannes, can you please test the attached patch?
> > > 
> 
> updated version.

Loads of "Fake beacon loss" but works. :)

johannes

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

Re: p54 problem with PS

[Johannes Berg]

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

On Fri, 2009-04-17 at 13:56 +0200, Christian Lamparter wrote:

> +       priv->beacon_timeout_guard = jiffies;

I wonder if you shouldn't do this when CONF_PS is (first?) enabled
instead? Or add_interface?

johannes

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