[PATCH] ath10k: handle attention flags correctly when A-MSDU

[PATCH] ath10k: handle attention flags correctly when A-MSDU

[Janusz Dziedzic]

In case of A-MSDU RX we should check flags for all
MSDU subframes to be sure we have correct FCS status.

Before we check attention flags only for first frame
where we didn't have correct info about FCS status in
case of A-MSDU. Next didn't mark RX_FLAG_FAILED_FCS_CRC
for skb. As a side efect we see big TP drop when TCP used.
This was easy to reproduce when AP interface was used
and added monitor interface run in promiscuous mode.

Reported-by: Denton Gentry <denton.gentry@gmail.com>
Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 6c102b1..9f704fe 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -307,7 +307,8 @@ static void ath10k_htt_rx_free_msdu_chain(struct sk_buff *skb)
 static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 				   u8 **fw_desc, int *fw_desc_len,
 				   struct sk_buff **head_msdu,
-				   struct sk_buff **tail_msdu)
+				   struct sk_buff **tail_msdu,
+				   u32 *attention)
 {
 	int msdu_len, msdu_chaining = 0;
 	struct sk_buff *msdu;
@@ -358,6 +359,11 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 			break;
 		}
 
+		*attention |= __le32_to_cpu(rx_desc->attention.flags) &
+					    (RX_ATTENTION_FLAGS_TKIP_MIC_ERR |
+					     RX_ATTENTION_FLAGS_DECRYPT_ERR |
+					     RX_ATTENTION_FLAGS_FCS_ERR |
+					     RX_ATTENTION_FLAGS_MGMT_TYPE);
 		/*
 		 * Copy the FW rx descriptor for this MSDU from the rx
 		 * indication message into the MSDU's netbuf. HL uses the
@@ -1233,13 +1239,15 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 		for (j = 0; j < mpdu_ranges[i].mpdu_count; j++) {
 			struct sk_buff *msdu_head, *msdu_tail;
 
+			attention = 0;
 			msdu_head = NULL;
 			msdu_tail = NULL;
 			ret = ath10k_htt_rx_amsdu_pop(htt,
 						      &fw_desc,
 						      &fw_desc_len,
 						      &msdu_head,
-						      &msdu_tail);
+						      &msdu_tail,
+						      &attention);
 
 			if (ret < 0) {
 				ath10k_warn("failed to pop amsdu from htt rx ring %d\n",
@@ -1251,7 +1259,6 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 			rxd = container_of((void *)msdu_head->data,
 					   struct htt_rx_desc,
 					   msdu_payload);
-			attention = __le32_to_cpu(rxd->attention.flags);
 
 			if (!ath10k_htt_rx_amsdu_allowed(htt, msdu_head,
 							 status,
@@ -1304,6 +1311,7 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 	u8 *fw_desc;
 	int fw_desc_len, hdrlen, paramlen;
 	int trim;
+	u32 attention = 0;
 
 	fw_desc_len = __le16_to_cpu(frag->fw_rx_desc_bytes);
 	fw_desc = (u8 *)frag->fw_msdu_rx_desc;
@@ -1313,7 +1321,8 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 
 	spin_lock_bh(&htt->rx_ring.lock);
 	ret = ath10k_htt_rx_amsdu_pop(htt, &fw_desc, &fw_desc_len,
-				      &msdu_head, &msdu_tail);
+				      &msdu_head, &msdu_tail,
+				      &attention);
 	spin_unlock_bh(&htt->rx_ring.lock);
 
 	ath10k_dbg(ATH10K_DBG_HTT_DUMP, "htt rx frag ahead\n");
@@ -1330,10 +1339,8 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 
 	hdr = (struct ieee80211_hdr *)msdu_head->data;
 	rxd = (void *)msdu_head->data - sizeof(*rxd);
-	tkip_mic_err = !!(__le32_to_cpu(rxd->attention.flags) &
-				RX_ATTENTION_FLAGS_TKIP_MIC_ERR);
-	decrypt_err = !!(__le32_to_cpu(rxd->attention.flags) &
-				RX_ATTENTION_FLAGS_DECRYPT_ERR);
+	tkip_mic_err = !!(attention & RX_ATTENTION_FLAGS_TKIP_MIC_ERR);
+	decrypt_err = !!(attention & RX_ATTENTION_FLAGS_DECRYPT_ERR);
 	fmt = MS(__le32_to_cpu(rxd->msdu_start.info1),
 			RX_MSDU_START_INFO1_DECAP_FORMAT);
 
-- 
1.7.9.5


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: handle attention flags correctly when A-MSDU

[Kalle Valo]

Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:

> In case of A-MSDU RX we should check flags for all
> MSDU subframes to be sure we have correct FCS status.
>
> Before we check attention flags only for first frame
> where we didn't have correct info about FCS status in
> case of A-MSDU. Next didn't mark RX_FLAG_FAILED_FCS_CRC
> for skb. As a side efect we see big TP drop when TCP used.
> This was easy to reproduce when AP interface was used
> and added monitor interface run in promiscuous mode.
>
> Reported-by: Denton Gentry <denton.gentry@gmail.com>
> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>

I'm having a hard time to understand the commit log. I think it would
make it easier to read if you first clearly describe the bug you are
fixing and then, in a separate paragraph, tell how you fix it.

So what is the bug Denton reported? "we see big TP drop when TCP used"
implies that this happens with all TCP traffic, but that can't be right.
Or did it happen with one of the reorder patches?

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: handle attention flags correctly when A-MSDU

[Janusz Dziedzic]

On 18 July 2014 14:50, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:
>
>> In case of A-MSDU RX we should check flags for all
>> MSDU subframes to be sure we have correct FCS status.
>>
>> Before we check attention flags only for first frame
>> where we didn't have correct info about FCS status in
>> case of A-MSDU. Next didn't mark RX_FLAG_FAILED_FCS_CRC
>> for skb. As a side efect we see big TP drop when TCP used.
>> This was easy to reproduce when AP interface was used
>> and added monitor interface run in promiscuous mode.
>>
>> Reported-by: Denton Gentry <denton.gentry@gmail.com>
>> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
>
> I'm having a hard time to understand the commit log. I think it would
> make it easier to read if you first clearly describe the bug you are
> fixing and then, in a separate paragraph, tell how you fix it.
>
Will change that.

> So what is the bug Denton reported? "we see big TP drop when TCP used"
> implies that this happens with all TCP traffic, but that can't be right.
> Or did it happen with one of the reorder patches?
>
TP drop is seen with/without reordering patches when TCP STA --> AP and:
- used ath10k STA
- I force to use AMSDU (7 frames) on STA
- not using cables and other traffic on the channel (often FCS errors)
- monitor iface used on AP

Without a patch we report AMSDU packets with wrong FCS as a correct to
the stack, next there is a lot of DUP ACK packets from AP --> STA.
TP drop depends on FCS errors for AMSDU.


This is example what I see without patch (without reordering patch):

echo "1 64" > htt_max_amsdu_ampdu
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0- 5.0 sec  56.6 MBytes  95.0 Mbits/sec
[  3]  5.0-10.0 sec  60.4 MBytes   101 Mbits/sec
[  3] 10.0-15.0 sec  60.2 MBytes   101 Mbits/sec
[  3] 15.0-20.0 sec  60.2 MBytes   101 Mbits/sec
[  3] 20.0-25.0 sec  63.8 MBytes   107 Mbits/sec
[  3] 25.0-30.0 sec  64.9 MBytes   109 Mbits/sec

echo "7 64" > htt_max_amsdu_ampdu  // lot of DUP ACK from AP --> STA seen
[  3] 30.0-35.0 sec  40.0 MBytes  67.1 Mbits/sec
[  3] 35.0-40.0 sec  35.9 MBytes  60.2 Mbits/sec
[  3] 40.0-45.0 sec  36.9 MBytes  61.9 Mbits/sec
[  3] 45.0-50.0 sec  37.9 MBytes  63.5 Mbits/sec
[  3] 50.0-55.0 sec  34.5 MBytes  57.9 Mbits/sec
[  3] 55.0-60.0 sec  25.4 MBytes  42.6 Mbits/sec
[  3] 60.0-65.0 sec  48.2 MBytes  81.0 Mbits/sec
[  3] 65.0-70.0 sec  28.8 MBytes  48.2 Mbits/sec
[  3] 70.0-75.0 sec  29.2 MBytes  49.1 Mbits/sec
[  3] 75.0-80.0 sec  22.9 MBytes  38.4 Mbits/sec
[  3] 80.0-85.0 sec  26.4 MBytes  44.2 Mbits/sec
[  3] 85.0-90.0 sec  31.5 MBytes  52.8 Mbits/sec


With patch:

echo "1 64" > htt_max_amsdu_ampdu
[  3] local 192.168.12.2 port 57512 connected with 192.168.12.1 port 5001
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0- 5.0 sec  60.8 MBytes   102 Mbits/sec
[  3]  5.0-10.0 sec  62.2 MBytes   104 Mbits/sec
[  3] 10.0-15.0 sec  60.9 MBytes   102 Mbits/sec

echo "7 64" > htt_max_amsdu_ampdu  // none DUP ACK form AP --> STA seen
[  3] 15.0-20.0 sec  68.1 MBytes   114 Mbits/sec
[  3] 20.0-25.0 sec  80.5 MBytes   135 Mbits/sec
[  3] 25.0-30.0 sec  83.0 MBytes   139 Mbits/sec
[  3] 30.0-35.0 sec  79.1 MBytes   133 Mbits/sec
[  3] 35.0-40.0 sec  77.1 MBytes   129 Mbits/sec
[  3] 40.0-45.0 sec  77.4 MBytes   130 Mbits/sec


BR
Janusz

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: handle attention flags correctly when A-MSDU

[Kalle Valo]

Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:

> On 18 July 2014 14:50, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:
>>
>>> In case of A-MSDU RX we should check flags for all
>>> MSDU subframes to be sure we have correct FCS status.
>>>
>>> Before we check attention flags only for first frame
>>> where we didn't have correct info about FCS status in
>>> case of A-MSDU. Next didn't mark RX_FLAG_FAILED_FCS_CRC
>>> for skb. As a side efect we see big TP drop when TCP used.
>>> This was easy to reproduce when AP interface was used
>>> and added monitor interface run in promiscuous mode.
>>>
>>> Reported-by: Denton Gentry <denton.gentry@gmail.com>
>>> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
>>
>> I'm having a hard time to understand the commit log. I think it would
>> make it easier to read if you first clearly describe the bug you are
>> fixing and then, in a separate paragraph, tell how you fix it.
>
> Will change that.

Thanks. What you wrote in this email was really good, I understand the
issue now and I'm sure so do others. I think you should put the same in
the commit log, there's no max limit for that anyway :)

Also add verb "using" to the title: "ath10k: handle attention flags
correctly when using A-MSDU"

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k