[PATCH 0/7] ath10k: rework rx path

[PATCH 0/7] ath10k: rework rx path

[Michal Kazior]

Hi,

This patchset unifies rx path. Instead of
branching for A-MSDU and MSDU cases a single
codepath is introduced.

This yields:

   text    data     bss     dec     hex filename
  14068       0       0   14068    36f4 before/htt_rx.o
  13308       3       0   13311    33ff after/htt_rx.o

I haven't measured any Rx performance loss in my
setup but I may be biased. In theory this could
reduce i-cache pressure and improve Rx throughput
on CPU contrained systems by a few mbps.

This patchset serves two purposes though:
 - clean up and unify the Rx path a bit
 - make it possible to reuse code more easily in
   the future


Michal Kazior (7):
  ath10k: start using sk_buff_head
  ath10k: simplify Rx loop
  ath10k: refactor htt->rx_confused
  ath10k: unify rx undecapping
  ath10k: remove unused function argument
  ath10k: use rx descriptor for ppdu status extraction
  ath10k: report rx rate and signal for fragmented Rx

 drivers/net/wireless/ath/ath10k/htt_rx.c | 1063 ++++++++++++++++--------------
 1 file changed, 577 insertions(+), 486 deletions(-)

-- 
1.8.5.3

[PATCH 1/7] ath10k: start using sk_buff_head

[Michal Kazior]

Instead of using manual sk_buff linking via ->next
use sk_buff_head. It's more robust, cleaner and
there's plenty of helper functions in kernel
already to manage sk_buff_head.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 213 +++++++++++++------------------
 1 file changed, 91 insertions(+), 122 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index a691fdf..85f11e6 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -303,27 +303,15 @@ static inline struct sk_buff *ath10k_htt_rx_netbuf_pop(struct ath10k_htt *htt)
 	return msdu;
 }
 
-static void ath10k_htt_rx_free_msdu_chain(struct sk_buff *skb)
-{
-	struct sk_buff *next;
-
-	while (skb) {
-		next = skb->next;
-		dev_kfree_skb_any(skb);
-		skb = next;
-	}
-}
-
 /* return: < 0 fatal error, 0 - non chained msdu, 1 chained msdu */
 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_head *amsdu,
 				   u32 *attention)
 {
 	struct ath10k *ar = htt->ar;
 	int msdu_len, msdu_chaining = 0;
-	struct sk_buff *msdu, *next;
+	struct sk_buff *msdu;
 	struct htt_rx_desc *rx_desc;
 
 	lockdep_assert_held(&htt->rx_ring.lock);
@@ -333,10 +321,19 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 		return -1;
 	}
 
-	msdu = *head_msdu = ath10k_htt_rx_netbuf_pop(htt);
-	while (msdu) {
+	for (;;) {
 		int last_msdu, msdu_len_invalid, msdu_chained;
 
+		msdu = ath10k_htt_rx_netbuf_pop(htt);
+		if (!msdu) {
+			ath10k_err(ar, "failed to pop msdu\n");
+			__skb_queue_purge(amsdu);
+			htt->rx_confused = true;
+			break;
+		}
+
+		__skb_queue_tail(amsdu, msdu);
+
 		rx_desc = (struct htt_rx_desc *)msdu->data;
 
 		/* FIXME: we must report msdu payload since this is what caller
@@ -354,10 +351,8 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 		 */
 		if (!(__le32_to_cpu(rx_desc->attention.flags)
 				& RX_ATTENTION_FLAGS_MSDU_DONE)) {
-			ath10k_htt_rx_free_msdu_chain(*head_msdu);
-			*head_msdu = NULL;
-			msdu = NULL;
-			ath10k_err(ar, "htt rx stopped. cannot recover\n");
+			ath10k_err(ar, "popped an incomplete msdu\n");
+			__skb_queue_purge(amsdu);
 			htt->rx_confused = true;
 			break;
 		}
@@ -423,25 +418,20 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 		skb_put(msdu, min(msdu_len, HTT_RX_MSDU_SIZE));
 		msdu_len -= msdu->len;
 
-		/* FIXME: Do chained buffers include htt_rx_desc or not? */
+		/* Note: Chained buffers do not contain rx descriptor */
 		while (msdu_chained--) {
-			struct sk_buff *next = ath10k_htt_rx_netbuf_pop(htt);
-
-			if (!next) {
+			msdu = ath10k_htt_rx_netbuf_pop(htt);
+			if (!msdu) {
 				ath10k_warn(ar, "failed to pop chained msdu\n");
-				ath10k_htt_rx_free_msdu_chain(*head_msdu);
-				*head_msdu = NULL;
-				msdu = NULL;
+				__skb_queue_purge(amsdu);
 				htt->rx_confused = true;
 				break;
 			}
 
-			skb_trim(next, 0);
-			skb_put(next, min(msdu_len, HTT_RX_BUF_SIZE));
-			msdu_len -= next->len;
-
-			msdu->next = next;
-			msdu = next;
+			__skb_queue_tail(amsdu, msdu);
+			skb_trim(msdu, 0);
+			skb_put(msdu, min(msdu_len, HTT_RX_BUF_SIZE));
+			msdu_len -= msdu->len;
 			msdu_chaining = 1;
 		}
 
@@ -450,18 +440,12 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 
 		trace_ath10k_htt_rx_desc(ar, &rx_desc->attention,
 					 sizeof(*rx_desc) - sizeof(u32));
-		if (last_msdu) {
-			msdu->next = NULL;
-			break;
-		}
 
-		next = ath10k_htt_rx_netbuf_pop(htt);
-		msdu->next = next;
-		msdu = next;
+		if (last_msdu)
+			break;
 	}
-	*tail_msdu = msdu;
 
-	if (*head_msdu == NULL)
+	if (skb_queue_empty(amsdu))
 		msdu_chaining = -1;
 
 	/*
@@ -915,11 +899,11 @@ static int ath10k_htt_rx_nwifi_hdrlen(struct ieee80211_hdr *hdr)
 
 static void ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
 				struct ieee80211_rx_status *rx_status,
-				struct sk_buff *skb_in)
+				struct sk_buff_head *amsdu)
 {
 	struct ath10k *ar = htt->ar;
 	struct htt_rx_desc *rxd;
-	struct sk_buff *skb = skb_in;
+	struct sk_buff *skb;
 	struct sk_buff *first;
 	enum rx_msdu_decap_format fmt;
 	enum htt_rx_mpdu_encrypt_type enctype;
@@ -927,7 +911,9 @@ static void ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
 	u8 hdr_buf[64], da[ETH_ALEN], sa[ETH_ALEN], *qos;
 	unsigned int hdr_len;
 
-	rxd = (void *)skb->data - sizeof(*rxd);
+	first = skb_peek(amsdu);
+
+	rxd = (void *)first->data - sizeof(*rxd);
 	enctype = MS(__le32_to_cpu(rxd->mpdu_start.info0),
 		     RX_MPDU_START_INFO0_ENCRYPT_TYPE);
 
@@ -936,8 +922,7 @@ static void ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
 	memcpy(hdr_buf, hdr, hdr_len);
 	hdr = (struct ieee80211_hdr *)hdr_buf;
 
-	first = skb;
-	while (skb) {
+	while ((skb = __skb_dequeue(amsdu))) {
 		void *decap_hdr;
 		int len;
 
@@ -1005,18 +990,15 @@ static void ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
 			break;
 		}
 
-		skb_in = skb;
-		ath10k_htt_rx_h_protected(htt, rx_status, skb_in, enctype, fmt,
+		ath10k_htt_rx_h_protected(htt, rx_status, skb, enctype, fmt,
 					  false);
-		skb = skb->next;
-		skb_in->next = NULL;
 
-		if (skb)
-			rx_status->flag |= RX_FLAG_AMSDU_MORE;
-		else
+		if (skb_queue_empty(amsdu))
 			rx_status->flag &= ~RX_FLAG_AMSDU_MORE;
+		else
+			rx_status->flag |= RX_FLAG_AMSDU_MORE;
 
-		ath10k_process_rx(htt->ar, rx_status, skb_in);
+		ath10k_process_rx(htt->ar, rx_status, skb);
 	}
 
 	/* FIXME: It might be nice to re-assemble the A-MSDU when there's a
@@ -1035,13 +1017,6 @@ static void ath10k_htt_rx_msdu(struct ath10k_htt *htt,
 	int hdr_len;
 	void *rfc1042;
 
-	/* This shouldn't happen. If it does than it may be a FW bug. */
-	if (skb->next) {
-		ath10k_warn(ar, "htt rx received chained non A-MSDU frame\n");
-		ath10k_htt_rx_free_msdu_chain(skb->next);
-		skb->next = NULL;
-	}
-
 	rxd = (void *)skb->data - sizeof(*rxd);
 	fmt = MS(__le32_to_cpu(rxd->msdu_start.info1),
 		 RX_MSDU_START_INFO1_DECAP_FORMAT);
@@ -1127,10 +1102,9 @@ static int ath10k_htt_rx_get_csum_state(struct sk_buff *skb)
 	return CHECKSUM_UNNECESSARY;
 }
 
-static int ath10k_unchain_msdu(struct sk_buff *msdu_head)
+static int ath10k_unchain_msdu(struct sk_buff_head *amsdu)
 {
-	struct sk_buff *next = msdu_head->next;
-	struct sk_buff *to_free = next;
+	struct sk_buff *skb, *first;
 	int space;
 	int total_len = 0;
 
@@ -1141,40 +1115,33 @@ static int ath10k_unchain_msdu(struct sk_buff *msdu_head)
 	 * skb?
 	 */
 
-	msdu_head->next = NULL;
+	first = __skb_dequeue(amsdu);
 
 	/* Allocate total length all at once. */
-	while (next) {
-		total_len += next->len;
-		next = next->next;
-	}
+	skb_queue_walk(amsdu, skb)
+		total_len += skb->len;
 
-	space = total_len - skb_tailroom(msdu_head);
+	space = total_len - skb_tailroom(first);
 	if ((space > 0) &&
-	    (pskb_expand_head(msdu_head, 0, space, GFP_ATOMIC) < 0)) {
+	    (pskb_expand_head(first, 0, space, GFP_ATOMIC) < 0)) {
 		/* TODO:  bump some rx-oom error stat */
 		/* put it back together so we can free the
 		 * whole list at once.
 		 */
-		msdu_head->next = to_free;
+		__skb_queue_head(amsdu, first);
 		return -1;
 	}
 
 	/* Walk list again, copying contents into
 	 * msdu_head
 	 */
-	next = to_free;
-	while (next) {
-		skb_copy_from_linear_data(next, skb_put(msdu_head, next->len),
-					  next->len);
-		next = next->next;
+	while ((skb = __skb_dequeue(amsdu))) {
+		skb_copy_from_linear_data(skb, skb_put(first, skb->len),
+					  skb->len);
+		dev_kfree_skb_any(skb);
 	}
 
-	/* If here, we have consolidated skb.  Free the
-	 * fragments and pass the main skb on up the
-	 * stack.
-	 */
-	ath10k_htt_rx_free_msdu_chain(to_free);
+	__skb_queue_head(amsdu, first);
 	return 0;
 }
 
@@ -1223,6 +1190,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 	struct ath10k *ar = htt->ar;
 	struct ieee80211_rx_status *rx_status = &htt->rx_status;
 	struct htt_rx_indication_mpdu_range *mpdu_ranges;
+	struct sk_buff_head amsdu;
 	struct ieee80211_hdr *hdr;
 	int num_mpdu_ranges;
 	u32 attention;
@@ -1271,35 +1239,28 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 
 	for (i = 0; i < num_mpdu_ranges; i++) {
 		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,
+			__skb_queue_head_init(&amsdu);
+			ret = ath10k_htt_rx_amsdu_pop(htt, &fw_desc,
+						      &fw_desc_len, &amsdu,
 						      &attention);
 
 			if (ret < 0) {
 				ath10k_warn(ar, "failed to pop amsdu from htt rx ring %d\n",
 					    ret);
-				ath10k_htt_rx_free_msdu_chain(msdu_head);
+				__skb_queue_purge(&amsdu);
 				continue;
 			}
 
-			if (!ath10k_htt_rx_amsdu_allowed(htt, msdu_head,
+			if (!ath10k_htt_rx_amsdu_allowed(htt, skb_peek(&amsdu),
 							 channel_set,
 							 attention)) {
-				ath10k_htt_rx_free_msdu_chain(msdu_head);
+				__skb_queue_purge(&amsdu);
 				continue;
 			}
 
-			if (ret > 0 &&
-			    ath10k_unchain_msdu(msdu_head) < 0) {
-				ath10k_htt_rx_free_msdu_chain(msdu_head);
+			if (ret > 0 && ath10k_unchain_msdu(&amsdu) < 0) {
+				__skb_queue_purge(&amsdu);
 				continue;
 			}
 
@@ -1313,12 +1274,13 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 			else
 				rx_status->flag &= ~RX_FLAG_MMIC_ERROR;
 
-			hdr = ath10k_htt_rx_skb_get_hdr(msdu_head);
+			hdr = ath10k_htt_rx_skb_get_hdr(skb_peek(&amsdu));
 
 			if (ath10k_htt_rx_hdr_is_amsdu(hdr))
-				ath10k_htt_rx_amsdu(htt, rx_status, msdu_head);
+				ath10k_htt_rx_amsdu(htt, rx_status, &amsdu);
 			else
-				ath10k_htt_rx_msdu(htt, rx_status, msdu_head);
+				ath10k_htt_rx_msdu(htt, rx_status,
+						   __skb_dequeue(&amsdu));
 		}
 	}
 
@@ -1329,12 +1291,13 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 				       struct htt_rx_fragment_indication *frag)
 {
 	struct ath10k *ar = htt->ar;
-	struct sk_buff *msdu_head, *msdu_tail;
+	struct sk_buff *msdu;
 	enum htt_rx_mpdu_encrypt_type enctype;
 	struct htt_rx_desc *rxd;
 	enum rx_msdu_decap_format fmt;
 	struct ieee80211_rx_status *rx_status = &htt->rx_status;
 	struct ieee80211_hdr *hdr;
+	struct sk_buff_head amsdu;
 	int ret;
 	bool tkip_mic_err;
 	bool decrypt_err;
@@ -1346,13 +1309,11 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 	fw_desc_len = __le16_to_cpu(frag->fw_rx_desc_bytes);
 	fw_desc = (u8 *)frag->fw_msdu_rx_desc;
 
-	msdu_head = NULL;
-	msdu_tail = NULL;
+	__skb_queue_head_init(&amsdu);
 
 	spin_lock_bh(&htt->rx_ring.lock);
 	ret = ath10k_htt_rx_amsdu_pop(htt, &fw_desc, &fw_desc_len,
-				      &msdu_head, &msdu_tail,
-				      &attention);
+				      &amsdu, &attention);
 	spin_unlock_bh(&htt->rx_ring.lock);
 
 	tasklet_schedule(&htt->rx_replenish_task);
@@ -1362,15 +1323,23 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 	if (ret) {
 		ath10k_warn(ar, "failed to pop amsdu from httr rx ring for fragmented rx %d\n",
 			    ret);
-		ath10k_htt_rx_free_msdu_chain(msdu_head);
+		__skb_queue_purge(&amsdu);
 		return;
 	}
 
+	if (skb_queue_len(&amsdu) != 1) {
+		ath10k_warn(ar, "failed to pop frag amsdu: too many msdus\n");
+		__skb_queue_purge(&amsdu);
+		return;
+	}
+
+	msdu = __skb_dequeue(&amsdu);
+
 	/* FIXME: implement signal strength */
 	rx_status->flag |= RX_FLAG_NO_SIGNAL_VAL;
 
-	hdr = (struct ieee80211_hdr *)msdu_head->data;
-	rxd = (void *)msdu_head->data - sizeof(*rxd);
+	hdr = (struct ieee80211_hdr *)msdu->data;
+	rxd = (void *)msdu->data - sizeof(*rxd);
 	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),
@@ -1378,22 +1347,22 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 
 	if (fmt != RX_MSDU_DECAP_RAW) {
 		ath10k_warn(ar, "we dont support non-raw fragmented rx yet\n");
-		dev_kfree_skb_any(msdu_head);
+		dev_kfree_skb_any(msdu);
 		goto end;
 	}
 
 	enctype = MS(__le32_to_cpu(rxd->mpdu_start.info0),
 		     RX_MPDU_START_INFO0_ENCRYPT_TYPE);
-	ath10k_htt_rx_h_protected(htt, rx_status, msdu_head, enctype, fmt,
+	ath10k_htt_rx_h_protected(htt, rx_status, msdu, enctype, fmt,
 				  true);
-	msdu_head->ip_summed = ath10k_htt_rx_get_csum_state(msdu_head);
+	msdu->ip_summed = ath10k_htt_rx_get_csum_state(msdu);
 
 	if (tkip_mic_err)
 		ath10k_warn(ar, "tkip mic error\n");
 
 	if (decrypt_err) {
 		ath10k_warn(ar, "decryption err in fragmented rx\n");
-		dev_kfree_skb_any(msdu_head);
+		dev_kfree_skb_any(msdu);
 		goto end;
 	}
 
@@ -1402,11 +1371,11 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 		paramlen = ath10k_htt_rx_crypto_param_len(ar, enctype);
 
 		/* It is more efficient to move the header than the payload */
-		memmove((void *)msdu_head->data + paramlen,
-			(void *)msdu_head->data,
+		memmove((void *)msdu->data + paramlen,
+			(void *)msdu->data,
 			hdrlen);
-		skb_pull(msdu_head, paramlen);
-		hdr = (struct ieee80211_hdr *)msdu_head->data;
+		skb_pull(msdu, paramlen);
+		hdr = (struct ieee80211_hdr *)msdu->data;
 	}
 
 	/* remove trailing FCS */
@@ -1420,17 +1389,17 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 	    enctype == HTT_RX_MPDU_ENCRYPT_TKIP_WPA)
 		trim += MICHAEL_MIC_LEN;
 
-	if (trim > msdu_head->len) {
+	if (trim > msdu->len) {
 		ath10k_warn(ar, "htt rx fragment: trailer longer than the frame itself? drop\n");
-		dev_kfree_skb_any(msdu_head);
+		dev_kfree_skb_any(msdu);
 		goto end;
 	}
 
-	skb_trim(msdu_head, msdu_head->len - trim);
+	skb_trim(msdu, msdu->len - trim);
 
 	ath10k_dbg_dump(ar, ATH10K_DBG_HTT_DUMP, NULL, "htt rx frag mpdu: ",
-			msdu_head->data, msdu_head->len);
-	ath10k_process_rx(htt->ar, rx_status, msdu_head);
+			msdu->data, msdu->len);
+	ath10k_process_rx(htt->ar, rx_status, msdu);
 
 end:
 	if (fw_desc_len > 0) {
-- 
1.8.5.3

[PATCH 2/7] ath10k: simplify Rx loop

[Michal Kazior]

Since htt_rx_mpdu_status isn't used anymore
(instead attention flags are used) simplify the
loop.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 80 ++++++++++++++++----------------
 1 file changed, 39 insertions(+), 41 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 85f11e6..cba2f3b 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1197,8 +1197,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 	int fw_desc_len;
 	u8 *fw_desc;
 	bool channel_set;
-	int i, j;
-	int ret;
+	int i, ret, mpdu_count = 0;
 
 	lockdep_assert_held(&htt->rx_ring.lock);
 
@@ -1237,51 +1236,50 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 			(sizeof(struct htt_rx_indication_mpdu_range) *
 				num_mpdu_ranges));
 
-	for (i = 0; i < num_mpdu_ranges; i++) {
-		for (j = 0; j < mpdu_ranges[i].mpdu_count; j++) {
-			attention = 0;
-			__skb_queue_head_init(&amsdu);
-			ret = ath10k_htt_rx_amsdu_pop(htt, &fw_desc,
-						      &fw_desc_len, &amsdu,
-						      &attention);
-
-			if (ret < 0) {
-				ath10k_warn(ar, "failed to pop amsdu from htt rx ring %d\n",
-					    ret);
-				__skb_queue_purge(&amsdu);
-				continue;
-			}
+	for (i = 0; i < num_mpdu_ranges; i++)
+		mpdu_count += mpdu_ranges[i].mpdu_count;
+
+	while (mpdu_count--) {
+		attention = 0;
+		__skb_queue_head_init(&amsdu);
+		ret = ath10k_htt_rx_amsdu_pop(htt, &fw_desc,
+					      &fw_desc_len, &amsdu,
+					      &attention);
+		if (ret < 0) {
+			ath10k_warn(ar, "failed to pop amsdu from htt rx ring %d\n",
+				    ret);
+			__skb_queue_purge(&amsdu);
+			continue;
+		}
 
-			if (!ath10k_htt_rx_amsdu_allowed(htt, skb_peek(&amsdu),
-							 channel_set,
-							 attention)) {
-				__skb_queue_purge(&amsdu);
-				continue;
-			}
+		if (!ath10k_htt_rx_amsdu_allowed(htt, skb_peek(&amsdu),
+						 channel_set, attention)) {
+			__skb_queue_purge(&amsdu);
+			continue;
+		}
 
-			if (ret > 0 && ath10k_unchain_msdu(&amsdu) < 0) {
-				__skb_queue_purge(&amsdu);
-				continue;
-			}
+		if (ret > 0 && ath10k_unchain_msdu(&amsdu) < 0) {
+			__skb_queue_purge(&amsdu);
+			continue;
+		}
 
-			if (attention & RX_ATTENTION_FLAGS_FCS_ERR)
-				rx_status->flag |= RX_FLAG_FAILED_FCS_CRC;
-			else
-				rx_status->flag &= ~RX_FLAG_FAILED_FCS_CRC;
+		if (attention & RX_ATTENTION_FLAGS_FCS_ERR)
+			rx_status->flag |= RX_FLAG_FAILED_FCS_CRC;
+		else
+			rx_status->flag &= ~RX_FLAG_FAILED_FCS_CRC;
 
-			if (attention & RX_ATTENTION_FLAGS_TKIP_MIC_ERR)
-				rx_status->flag |= RX_FLAG_MMIC_ERROR;
-			else
-				rx_status->flag &= ~RX_FLAG_MMIC_ERROR;
+		if (attention & RX_ATTENTION_FLAGS_TKIP_MIC_ERR)
+			rx_status->flag |= RX_FLAG_MMIC_ERROR;
+		else
+			rx_status->flag &= ~RX_FLAG_MMIC_ERROR;
 
-			hdr = ath10k_htt_rx_skb_get_hdr(skb_peek(&amsdu));
+		hdr = ath10k_htt_rx_skb_get_hdr(skb_peek(&amsdu));
 
-			if (ath10k_htt_rx_hdr_is_amsdu(hdr))
-				ath10k_htt_rx_amsdu(htt, rx_status, &amsdu);
-			else
-				ath10k_htt_rx_msdu(htt, rx_status,
-						   __skb_dequeue(&amsdu));
-		}
+		if (ath10k_htt_rx_hdr_is_amsdu(hdr))
+			ath10k_htt_rx_amsdu(htt, rx_status, &amsdu);
+		else
+			ath10k_htt_rx_msdu(htt, rx_status,
+					   __skb_dequeue(&amsdu));
 	}
 
 	tasklet_schedule(&htt->rx_replenish_task);
-- 
1.8.5.3

[PATCH 3/7] ath10k: refactor htt->rx_confused

[Michal Kazior]

Make the rx_confused be handled by the rx
indication handlers instead of the buffer popping
function.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index cba2f3b..651028e 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -316,20 +316,13 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 
 	lockdep_assert_held(&htt->rx_ring.lock);
 
-	if (htt->rx_confused) {
-		ath10k_warn(ar, "htt is confused. refusing rx\n");
-		return -1;
-	}
-
 	for (;;) {
 		int last_msdu, msdu_len_invalid, msdu_chained;
 
 		msdu = ath10k_htt_rx_netbuf_pop(htt);
 		if (!msdu) {
-			ath10k_err(ar, "failed to pop msdu\n");
 			__skb_queue_purge(amsdu);
-			htt->rx_confused = true;
-			break;
+			return -ENOENT;
 		}
 
 		__skb_queue_tail(amsdu, msdu);
@@ -351,10 +344,8 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 		 */
 		if (!(__le32_to_cpu(rx_desc->attention.flags)
 				& RX_ATTENTION_FLAGS_MSDU_DONE)) {
-			ath10k_err(ar, "popped an incomplete msdu\n");
 			__skb_queue_purge(amsdu);
-			htt->rx_confused = true;
-			break;
+			return -EIO;
 		}
 
 		*attention |= __le32_to_cpu(rx_desc->attention.flags) &
@@ -422,10 +413,8 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 		while (msdu_chained--) {
 			msdu = ath10k_htt_rx_netbuf_pop(htt);
 			if (!msdu) {
-				ath10k_warn(ar, "failed to pop chained msdu\n");
 				__skb_queue_purge(amsdu);
-				htt->rx_confused = true;
-				break;
+				return -ENOENT;
 			}
 
 			__skb_queue_tail(amsdu, msdu);
@@ -1201,6 +1190,9 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 
 	lockdep_assert_held(&htt->rx_ring.lock);
 
+	if (htt->rx_confused)
+		return;
+
 	fw_desc_len = __le16_to_cpu(rx->prefix.fw_rx_desc_bytes);
 	fw_desc = (u8 *)&rx->fw_desc;
 
@@ -1246,10 +1238,13 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 					      &fw_desc_len, &amsdu,
 					      &attention);
 		if (ret < 0) {
-			ath10k_warn(ar, "failed to pop amsdu from htt rx ring %d\n",
-				    ret);
+			ath10k_warn(ar, "rx ring became corrupted: %d\n", ret);
 			__skb_queue_purge(&amsdu);
-			continue;
+			/* FIXME: It's probably a good idea to reboot the
+			 * device instead of leaving it inoperable.
+			 */
+			htt->rx_confused = true;
+			break;
 		}
 
 		if (!ath10k_htt_rx_amsdu_allowed(htt, skb_peek(&amsdu),
-- 
1.8.5.3

[PATCH 4/7] ath10k: unify rx undecapping

[Michal Kazior]

This creates a single, common path for MSDU,
A-MSDU and fragmented Rx.

Hopefully this will make it easier to understand
Rx path and make it easier to work with.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 738 +++++++++++++++++--------------
 1 file changed, 412 insertions(+), 326 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 651028e..6abfea7 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -603,35 +603,6 @@ static int ath10k_htt_rx_crypto_tail_len(struct ath10k *ar,
 	return 0;
 }
 
-/* Applies for first msdu in chain, before altering it. */
-static struct ieee80211_hdr *ath10k_htt_rx_skb_get_hdr(struct sk_buff *skb)
-{
-	struct htt_rx_desc *rxd;
-	enum rx_msdu_decap_format fmt;
-
-	rxd = (void *)skb->data - sizeof(*rxd);
-	fmt = MS(__le32_to_cpu(rxd->msdu_start.info1),
-		 RX_MSDU_START_INFO1_DECAP_FORMAT);
-
-	if (fmt == RX_MSDU_DECAP_RAW)
-		return (void *)skb->data;
-
-	return (void *)skb->data - RX_HTT_HDR_STATUS_LEN;
-}
-
-/* This function only applies for first msdu in an msdu chain */
-static bool ath10k_htt_rx_hdr_is_amsdu(struct ieee80211_hdr *hdr)
-{
-	u8 *qc;
-
-	if (ieee80211_is_data_qos(hdr->frame_control)) {
-		qc = ieee80211_get_qos_ctl(hdr);
-		if (qc[0] & 0x80)
-			return true;
-	}
-	return false;
-}
-
 struct rfc1042_hdr {
 	u8 llc_dsap;
 	u8 llc_ssap;
@@ -757,41 +728,6 @@ static void ath10k_htt_rx_h_rates(struct ath10k *ar,
 	}
 }
 
-static void ath10k_htt_rx_h_protected(struct ath10k_htt *htt,
-				      struct ieee80211_rx_status *rx_status,
-				      struct sk_buff *skb,
-				      enum htt_rx_mpdu_encrypt_type enctype,
-				      enum rx_msdu_decap_format fmt,
-				      bool dot11frag)
-{
-	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
-
-	rx_status->flag &= ~(RX_FLAG_DECRYPTED |
-			     RX_FLAG_IV_STRIPPED |
-			     RX_FLAG_MMIC_STRIPPED);
-
-	if (enctype == HTT_RX_MPDU_ENCRYPT_NONE)
-		return;
-
-	/*
-	 * There's no explicit rx descriptor flag to indicate whether a given
-	 * frame has been decrypted or not. We're forced to use the decap
-	 * format as an implicit indication. However fragmentation rx is always
-	 * raw and it probably never reports undecrypted raws.
-	 *
-	 * This makes sure sniffed frames are reported as-is without stripping
-	 * the protected flag.
-	 */
-	if (fmt == RX_MSDU_DECAP_RAW && !dot11frag)
-		return;
-
-	rx_status->flag |= RX_FLAG_DECRYPTED |
-			   RX_FLAG_IV_STRIPPED |
-			   RX_FLAG_MMIC_STRIPPED;
-	hdr->frame_control = __cpu_to_le16(__le16_to_cpu(hdr->frame_control) &
-					   ~IEEE80211_FCTL_PROTECTED);
-}
-
 static bool ath10k_htt_rx_h_channel(struct ath10k *ar,
 				    struct ieee80211_rx_status *status)
 {
@@ -886,178 +822,262 @@ static int ath10k_htt_rx_nwifi_hdrlen(struct ieee80211_hdr *hdr)
 	return round_up(ieee80211_hdrlen(hdr->frame_control), 4);
 }
 
-static void ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
-				struct ieee80211_rx_status *rx_status,
-				struct sk_buff_head *amsdu)
+static void ath10k_htt_rx_h_undecap_raw(struct ath10k *ar,
+					struct sk_buff *msdu,
+					struct ieee80211_rx_status *status,
+					enum htt_rx_mpdu_encrypt_type enctype,
+					bool is_decrypted)
 {
-	struct ath10k *ar = htt->ar;
+	struct ieee80211_hdr *hdr;
 	struct htt_rx_desc *rxd;
-	struct sk_buff *skb;
-	struct sk_buff *first;
-	enum rx_msdu_decap_format fmt;
-	enum htt_rx_mpdu_encrypt_type enctype;
+	size_t hdr_len;
+	size_t crypto_len;
+	bool is_first;
+	bool is_last;
+
+	rxd = (void *)msdu->data - sizeof(*rxd);
+	is_first = !!(rxd->msdu_end.info0 &
+		      __cpu_to_le32(RX_MSDU_END_INFO0_FIRST_MSDU));
+	is_last = !!(rxd->msdu_end.info0 &
+		     __cpu_to_le32(RX_MSDU_END_INFO0_LAST_MSDU));
+
+	/* Delivered decapped frame:
+	 * [802.11 header]
+	 * [crypto param] <-- can be trimmed if !fcs_err &&
+	 *                    !decrypt_err && !peer_idx_invalid
+	 * [amsdu header] <-- only if A-MSDU
+	 * [rfc1042/llc]
+	 * [payload]
+	 * [FCS] <-- at end, needs to be trimmed
+	 */
+
+	/* This probably shouldn't happen but warn just in case */
+	if (unlikely(WARN_ON_ONCE(!is_first)))
+		return;
+
+	/* This probably shouldn't happen but warn just in case */
+	if (unlikely(WARN_ON_ONCE(!(is_first && is_last))))
+		return;
+
+	skb_trim(msdu, msdu->len - FCS_LEN);
+
+	/* In most cases this will be true for sniffed frames. It makes sense
+	 * to deliver them as-is without stripping the crypto param. This would
+	 * also make sense for software based decryption (which is not
+	 * implemented in ath10k).
+	 *
+	 * If there's no error then the frame is decrypted. At least that is
+	 * the case for frames that come in via fragmented rx indication.
+	 */
+	if (!is_decrypted)
+		return;
+
+	/* The payload is decrypted so strip crypto params. Start from tail
+	 * since hdr is used to compute some stuff.
+	 */
+
+	hdr = (void *)msdu->data;
+
+	/* Tail */
+	skb_trim(msdu, msdu->len - ath10k_htt_rx_crypto_tail_len(ar, enctype));
+
+	/* MMIC */
+	if (!ieee80211_has_morefrags(hdr->frame_control) &&
+	    enctype == HTT_RX_MPDU_ENCRYPT_TKIP_WPA)
+		skb_trim(msdu, msdu->len - 8);
+
+	/* Head */
+	hdr_len = ieee80211_hdrlen(hdr->frame_control);
+	crypto_len = ath10k_htt_rx_crypto_param_len(ar, enctype);
+
+	memmove((void *)msdu->data + crypto_len,
+		(void *)msdu->data, hdr_len);
+	skb_pull(msdu, crypto_len);
+}
+
+static void ath10k_htt_rx_h_undecap_nwifi(struct ath10k *ar,
+					  struct sk_buff *msdu,
+					  struct ieee80211_rx_status *status,
+					  const u8 first_hdr[64])
+{
 	struct ieee80211_hdr *hdr;
-	u8 hdr_buf[64], da[ETH_ALEN], sa[ETH_ALEN], *qos;
-	unsigned int hdr_len;
+	size_t hdr_len;
+	u8 da[ETH_ALEN];
+	u8 sa[ETH_ALEN];
 
-	first = skb_peek(amsdu);
+	/* Delivered decapped frame:
+	 * [nwifi 802.11 header] <-- replaced with 802.11 hdr
+	 * [rfc1042/llc]
+	 *
+	 * Note: The nwifi header doesn't have QoS Control and is
+	 * (always?) a 3addr frame.
+	 *
+	 * Note2: There's no A-MSDU subframe header. Even if it's part
+	 * of an A-MSDU.
+	 */
 
-	rxd = (void *)first->data - sizeof(*rxd);
-	enctype = MS(__le32_to_cpu(rxd->mpdu_start.info0),
-		     RX_MPDU_START_INFO0_ENCRYPT_TYPE);
+	/* pull decapped header and copy SA & DA */
+	hdr = (struct ieee80211_hdr *)msdu->data;
+	hdr_len = ath10k_htt_rx_nwifi_hdrlen(hdr);
+	ether_addr_copy(da, ieee80211_get_DA(hdr));
+	ether_addr_copy(sa, ieee80211_get_SA(hdr));
+	skb_pull(msdu, hdr_len);
 
-	hdr = (struct ieee80211_hdr *)rxd->rx_hdr_status;
+	/* push original 802.11 header */
+	hdr = (struct ieee80211_hdr *)first_hdr;
 	hdr_len = ieee80211_hdrlen(hdr->frame_control);
-	memcpy(hdr_buf, hdr, hdr_len);
-	hdr = (struct ieee80211_hdr *)hdr_buf;
+	memcpy(skb_push(msdu, hdr_len), hdr, hdr_len);
 
-	while ((skb = __skb_dequeue(amsdu))) {
-		void *decap_hdr;
-		int len;
-
-		rxd = (void *)skb->data - sizeof(*rxd);
-		fmt = MS(__le32_to_cpu(rxd->msdu_start.info1),
-			 RX_MSDU_START_INFO1_DECAP_FORMAT);
-		decap_hdr = (void *)rxd->rx_hdr_status;
-
-		skb->ip_summed = ath10k_htt_rx_get_csum_state(skb);
-
-		/* First frame in an A-MSDU chain has more decapped data. */
-		if (skb == first) {
-			len = round_up(ieee80211_hdrlen(hdr->frame_control), 4);
-			len += round_up(ath10k_htt_rx_crypto_param_len(ar,
-						enctype), 4);
-			decap_hdr += len;
-		}
+	/* original 802.11 header has a different DA and in
+	 * case of 4addr it may also have different SA
+	 */
+	hdr = (struct ieee80211_hdr *)msdu->data;
+	ether_addr_copy(ieee80211_get_DA(hdr), da);
+	ether_addr_copy(ieee80211_get_SA(hdr), sa);
+}
 
-		switch (fmt) {
-		case RX_MSDU_DECAP_RAW:
-			/* remove trailing FCS */
-			skb_trim(skb, skb->len - FCS_LEN);
-			break;
-		case RX_MSDU_DECAP_NATIVE_WIFI:
-			/* pull decapped header and copy SA & DA */
-			hdr = (struct ieee80211_hdr *)skb->data;
-			hdr_len = ath10k_htt_rx_nwifi_hdrlen(hdr);
-			ether_addr_copy(da, ieee80211_get_DA(hdr));
-			ether_addr_copy(sa, ieee80211_get_SA(hdr));
-			skb_pull(skb, hdr_len);
-
-			/* push original 802.11 header */
-			hdr = (struct ieee80211_hdr *)hdr_buf;
-			hdr_len = ieee80211_hdrlen(hdr->frame_control);
-			memcpy(skb_push(skb, hdr_len), hdr, hdr_len);
-
-			/* original A-MSDU header has the bit set but we're
-			 * not including A-MSDU subframe header */
-			hdr = (struct ieee80211_hdr *)skb->data;
-			qos = ieee80211_get_qos_ctl(hdr);
-			qos[0] &= ~IEEE80211_QOS_CTL_A_MSDU_PRESENT;
-
-			/* original 802.11 header has a different DA and in
-			 * case of 4addr it may also have different SA
-			 */
-			ether_addr_copy(ieee80211_get_DA(hdr), da);
-			ether_addr_copy(ieee80211_get_SA(hdr), sa);
-			break;
-		case RX_MSDU_DECAP_ETHERNET2_DIX:
-			/* strip ethernet header and insert decapped 802.11
-			 * header, amsdu subframe header and rfc1042 header */
+static void *ath10k_htt_rx_h_find_rfc1042(struct ath10k *ar,
+					  struct sk_buff *msdu,
+					  enum htt_rx_mpdu_encrypt_type enctype)
+{
+	struct ieee80211_hdr *hdr;
+	struct htt_rx_desc *rxd;
+	size_t hdr_len, crypto_len;
+	void *rfc1042;
+	bool is_first, is_last, is_amsdu;
 
-			len = 0;
-			len += sizeof(struct rfc1042_hdr);
-			len += sizeof(struct amsdu_subframe_hdr);
+	rxd = (void *)msdu->data - sizeof(*rxd);
+	hdr = (void *)rxd->rx_hdr_status;
 
-			skb_pull(skb, sizeof(struct ethhdr));
-			memcpy(skb_push(skb, len), decap_hdr, len);
-			memcpy(skb_push(skb, hdr_len), hdr, hdr_len);
-			break;
-		case RX_MSDU_DECAP_8023_SNAP_LLC:
-			/* insert decapped 802.11 header making a singly
-			 * A-MSDU */
-			memcpy(skb_push(skb, hdr_len), hdr, hdr_len);
-			break;
-		}
+	is_first = !!(rxd->msdu_end.info0 &
+		      __cpu_to_le32(RX_MSDU_END_INFO0_FIRST_MSDU));
+	is_last = !!(rxd->msdu_end.info0 &
+		     __cpu_to_le32(RX_MSDU_END_INFO0_LAST_MSDU));
+	is_amsdu = !(is_first && is_last);
 
-		ath10k_htt_rx_h_protected(htt, rx_status, skb, enctype, fmt,
-					  false);
+	rfc1042 = hdr;
 
-		if (skb_queue_empty(amsdu))
-			rx_status->flag &= ~RX_FLAG_AMSDU_MORE;
-		else
-			rx_status->flag |= RX_FLAG_AMSDU_MORE;
+	if (is_first) {
+		hdr_len = ieee80211_hdrlen(hdr->frame_control);
+		crypto_len = ath10k_htt_rx_crypto_param_len(ar, enctype);
 
-		ath10k_process_rx(htt->ar, rx_status, skb);
+		rfc1042 += round_up(hdr_len, 4) +
+			   round_up(crypto_len, 4);
 	}
 
-	/* FIXME: It might be nice to re-assemble the A-MSDU when there's a
-	 * monitor interface active for sniffing purposes. */
+	if (is_amsdu)
+		rfc1042 += sizeof(struct amsdu_subframe_hdr);
+
+	return rfc1042;
 }
 
-static void ath10k_htt_rx_msdu(struct ath10k_htt *htt,
-			       struct ieee80211_rx_status *rx_status,
-			       struct sk_buff *skb)
+static void ath10k_htt_rx_h_undecap_eth(struct ath10k *ar,
+					struct sk_buff *msdu,
+					struct ieee80211_rx_status *status,
+					const u8 first_hdr[64],
+					enum htt_rx_mpdu_encrypt_type enctype)
 {
-	struct ath10k *ar = htt->ar;
-	struct htt_rx_desc *rxd;
 	struct ieee80211_hdr *hdr;
-	enum rx_msdu_decap_format fmt;
-	enum htt_rx_mpdu_encrypt_type enctype;
-	int hdr_len;
+	struct ethhdr *eth;
+	size_t hdr_len;
 	void *rfc1042;
+	u8 da[ETH_ALEN];
+	u8 sa[ETH_ALEN];
 
-	rxd = (void *)skb->data - sizeof(*rxd);
-	fmt = MS(__le32_to_cpu(rxd->msdu_start.info1),
-		 RX_MSDU_START_INFO1_DECAP_FORMAT);
-	enctype = MS(__le32_to_cpu(rxd->mpdu_start.info0),
-		     RX_MPDU_START_INFO0_ENCRYPT_TYPE);
-	hdr = (struct ieee80211_hdr *)rxd->rx_hdr_status;
+	/* Delivered decapped frame:
+	 * [eth header] <-- replaced with 802.11 hdr & rfc1042/llc
+	 * [payload]
+	 */
+
+	rfc1042 = ath10k_htt_rx_h_find_rfc1042(ar, msdu, enctype);
+	if (WARN_ON_ONCE(!rfc1042))
+		return;
+
+	/* pull decapped header and copy SA & DA */
+	eth = (struct ethhdr *)msdu->data;
+	ether_addr_copy(da, eth->h_dest);
+	ether_addr_copy(sa, eth->h_source);
+	skb_pull(msdu, sizeof(struct ethhdr));
+
+	/* push rfc1042/llc/snap */
+	memcpy(skb_push(msdu, sizeof(struct rfc1042_hdr)), rfc1042,
+	       sizeof(struct rfc1042_hdr));
+
+	/* push original 802.11 header */
+	hdr = (struct ieee80211_hdr *)first_hdr;
+	hdr_len = ieee80211_hdrlen(hdr->frame_control);
+	memcpy(skb_push(msdu, hdr_len), hdr, hdr_len);
+
+	/* original 802.11 header has a different DA and in
+	 * case of 4addr it may also have different SA
+	 */
+	hdr = (struct ieee80211_hdr *)msdu->data;
+	ether_addr_copy(ieee80211_get_DA(hdr), da);
+	ether_addr_copy(ieee80211_get_SA(hdr), sa);
+}
+
+static void ath10k_htt_rx_h_undecap_snap(struct ath10k *ar,
+					 struct sk_buff *msdu,
+					 struct ieee80211_rx_status *status,
+					 const u8 first_hdr[64])
+{
+	struct ieee80211_hdr *hdr;
+	size_t hdr_len;
+
+	/* Delivered decapped frame:
+	 * [amsdu header] <-- replaced with 802.11 hdr
+	 * [rfc1042/llc]
+	 * [payload]
+	 */
+
+	skb_pull(msdu, sizeof(struct amsdu_subframe_hdr));
+
+	hdr = (struct ieee80211_hdr *)first_hdr;
 	hdr_len = ieee80211_hdrlen(hdr->frame_control);
+	memcpy(skb_push(msdu, hdr_len), hdr, hdr_len);
+}
 
-	skb->ip_summed = ath10k_htt_rx_get_csum_state(skb);
+static void ath10k_htt_rx_h_undecap(struct ath10k *ar,
+				    struct sk_buff *msdu,
+				    struct ieee80211_rx_status *status,
+				    u8 first_hdr[64],
+				    enum htt_rx_mpdu_encrypt_type enctype,
+				    bool is_decrypted)
+{
+	struct htt_rx_desc *rxd;
+	enum rx_msdu_decap_format decap;
+	struct ieee80211_hdr *hdr;
 
-	switch (fmt) {
+	/* First msdu's decapped header:
+	 * [802.11 header] <-- padded to 4 bytes long
+	 * [crypto param] <-- padded to 4 bytes long
+	 * [amsdu header] <-- only if A-MSDU
+	 * [rfc1042/llc]
+	 *
+	 * Other (2nd, 3rd, ..) msdu's decapped header:
+	 * [amsdu header] <-- only if A-MSDU
+	 * [rfc1042/llc]
+	 */
+
+	rxd = (void *)msdu->data - sizeof(*rxd);
+	hdr = (void *)rxd->rx_hdr_status;
+	decap = MS(__le32_to_cpu(rxd->msdu_start.info1),
+		   RX_MSDU_START_INFO1_DECAP_FORMAT);
+
+	switch (decap) {
 	case RX_MSDU_DECAP_RAW:
-		/* remove trailing FCS */
-		skb_trim(skb, skb->len - FCS_LEN);
+		ath10k_htt_rx_h_undecap_raw(ar, msdu, status, enctype, is_decrypted);
 		break;
 	case RX_MSDU_DECAP_NATIVE_WIFI:
-		/* Pull decapped header */
-		hdr = (struct ieee80211_hdr *)skb->data;
-		hdr_len = ath10k_htt_rx_nwifi_hdrlen(hdr);
-		skb_pull(skb, hdr_len);
-
-		/* Push original header */
-		hdr = (struct ieee80211_hdr *)rxd->rx_hdr_status;
-		hdr_len = ieee80211_hdrlen(hdr->frame_control);
-		memcpy(skb_push(skb, hdr_len), hdr, hdr_len);
+		ath10k_htt_rx_h_undecap_nwifi(ar, msdu, status, first_hdr);
 		break;
 	case RX_MSDU_DECAP_ETHERNET2_DIX:
-		/* strip ethernet header and insert decapped 802.11 header and
-		 * rfc1042 header */
-
-		rfc1042 = hdr;
-		rfc1042 += roundup(hdr_len, 4);
-		rfc1042 += roundup(ath10k_htt_rx_crypto_param_len(ar,
-					enctype), 4);
-
-		skb_pull(skb, sizeof(struct ethhdr));
-		memcpy(skb_push(skb, sizeof(struct rfc1042_hdr)),
-		       rfc1042, sizeof(struct rfc1042_hdr));
-		memcpy(skb_push(skb, hdr_len), hdr, hdr_len);
+		ath10k_htt_rx_h_undecap_eth(ar, msdu, status, first_hdr, enctype);
 		break;
 	case RX_MSDU_DECAP_8023_SNAP_LLC:
-		/* remove A-MSDU subframe header and insert
-		 * decapped 802.11 header. rfc1042 header is already there */
-
-		skb_pull(skb, sizeof(struct amsdu_subframe_hdr));
-		memcpy(skb_push(skb, hdr_len), hdr, hdr_len);
+		ath10k_htt_rx_h_undecap_snap(ar, msdu, status, first_hdr);
 		break;
 	}
-
-	ath10k_htt_rx_h_protected(htt, rx_status, skb, enctype, fmt, false);
-
-	ath10k_process_rx(htt->ar, rx_status, skb);
 }
 
 static int ath10k_htt_rx_get_csum_state(struct sk_buff *skb)
@@ -1091,6 +1111,126 @@ static int ath10k_htt_rx_get_csum_state(struct sk_buff *skb)
 	return CHECKSUM_UNNECESSARY;
 }
 
+static void ath10k_htt_rx_h_csum_offload(struct sk_buff *msdu)
+{
+	msdu->ip_summed = ath10k_htt_rx_get_csum_state(msdu);
+}
+
+static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
+				 struct sk_buff_head *amsdu,
+				 struct ieee80211_rx_status *status)
+{
+	struct sk_buff *first;
+	struct sk_buff *last;
+	struct sk_buff *msdu;
+	struct htt_rx_desc *rxd;
+	struct ieee80211_hdr *hdr;
+	enum htt_rx_mpdu_encrypt_type enctype;
+	u8 first_hdr[64];
+	u8 *qos;
+	size_t hdr_len;
+	bool has_fcs_err;
+	bool has_crypto_err;
+	bool has_tkip_err;
+	bool has_peer_idx_invalid;
+	bool is_decrypted;
+
+	if (skb_queue_empty(amsdu))
+		return;
+
+	first = skb_peek(amsdu);
+	rxd = (void *)first->data - sizeof(*rxd);
+
+	enctype = MS(__le32_to_cpu(rxd->mpdu_start.info0),
+		     RX_MPDU_START_INFO0_ENCRYPT_TYPE);
+
+	/* First MSDU's Rx descriptor in an A-MSDU contains full 802.11
+	 * decapped header. It'll be used for undecapping of each MSDU.
+	 */
+	hdr = (void *)rxd->rx_hdr_status;
+	hdr_len = ieee80211_hdrlen(hdr->frame_control);
+	memcpy(first_hdr, hdr, hdr_len);
+
+	/* Each A-MSDU subframe will use the original header as the base and be
+	 * reported as a separate MSDU so strip the A-MSDU bit from QoS Ctl.
+	 */
+	hdr = (void *)first_hdr;
+	qos = ieee80211_get_qos_ctl(hdr);
+	qos[0] &= ~IEEE80211_QOS_CTL_A_MSDU_PRESENT;
+
+	/* Some attention flags are valid only in the last MSDU. */
+	last = skb_peek_tail(amsdu);
+	rxd = (void *)last->data - sizeof(*rxd);
+	has_fcs_err = !!(rxd->attention.flags &
+			 __cpu_to_le32(RX_ATTENTION_FLAGS_FCS_ERR));
+	has_crypto_err = !!(rxd->attention.flags &
+			    __cpu_to_le32(RX_ATTENTION_FLAGS_DECRYPT_ERR));
+	has_tkip_err = !!(rxd->attention.flags &
+			  __cpu_to_le32(RX_ATTENTION_FLAGS_TKIP_MIC_ERR));
+	has_peer_idx_invalid = !!(rxd->attention.flags &
+			          __cpu_to_le32(RX_ATTENTION_FLAGS_PEER_IDX_INVALID));
+
+	/* Note: If hardware captures an encrypted frame that it can't decrypt,
+	 * e.g. due to fcs error, missing peer or invalid key data it will
+	 * report the frame as raw.
+	 */
+	is_decrypted = (enctype != HTT_RX_MPDU_ENCRYPT_NONE &&
+			!has_fcs_err &&
+			!has_crypto_err &&
+			!has_peer_idx_invalid);
+
+	/* Clear per-MPDU flags while leaving per-PPDU flags intact. */
+	status->flag &= ~(RX_FLAG_FAILED_FCS_CRC |
+			  RX_FLAG_MMIC_ERROR |
+			  RX_FLAG_DECRYPTED |
+			  RX_FLAG_IV_STRIPPED |
+			  RX_FLAG_MMIC_STRIPPED);
+
+	if (has_fcs_err)
+		status->flag |= RX_FLAG_FAILED_FCS_CRC;
+
+	if (has_tkip_err)
+		status->flag |= RX_FLAG_MMIC_ERROR;
+
+	if (is_decrypted)
+		status->flag |= RX_FLAG_DECRYPTED |
+				RX_FLAG_IV_STRIPPED |
+				RX_FLAG_MMIC_STRIPPED;
+
+	skb_queue_walk(amsdu, msdu) {
+		ath10k_htt_rx_h_csum_offload(msdu);
+		ath10k_htt_rx_h_undecap(ar, msdu, status, first_hdr, enctype,
+					is_decrypted);
+
+		/* Undecapping involves copying the original 802.11 header back
+		 * to sk_buff. If frame is protected and hardware has decrypted
+		 * it then remove the protected bit.
+		 */
+		if (!is_decrypted)
+			continue;
+
+		hdr = (void *)msdu->data;
+		hdr->frame_control &= ~__cpu_to_le16(IEEE80211_FCTL_PROTECTED);
+	}
+}
+
+static void ath10k_htt_rx_h_deliver(struct ath10k *ar,
+				    struct sk_buff_head *amsdu,
+				    struct ieee80211_rx_status *status)
+{
+	struct sk_buff *msdu;
+
+	while ((msdu = __skb_dequeue(amsdu))) {
+		/* Setup per-MSDU flags */
+		if (skb_queue_empty(amsdu))
+			status->flag &= ~RX_FLAG_AMSDU_MORE;
+		else
+			status->flag |= RX_FLAG_AMSDU_MORE;
+
+		ath10k_process_rx(ar, status, msdu);
+	}
+}
+
 static int ath10k_unchain_msdu(struct sk_buff_head *amsdu)
 {
 	struct sk_buff *skb, *first;
@@ -1134,45 +1274,86 @@ static int ath10k_unchain_msdu(struct sk_buff_head *amsdu)
 	return 0;
 }
 
-static bool ath10k_htt_rx_amsdu_allowed(struct ath10k_htt *htt,
-					struct sk_buff *head,
-					bool channel_set,
-					u32 attention)
+static void ath10k_htt_rx_h_unchain(struct ath10k *ar,
+				    struct sk_buff_head *amsdu,
+				    bool chained)
 {
-	struct ath10k *ar = htt->ar;
+	struct sk_buff *first;
+	struct htt_rx_desc *rxd;
+	enum rx_msdu_decap_format decap;
 
-	if (head->len == 0) {
-		ath10k_dbg(ar, ATH10K_DBG_HTT,
-			   "htt rx dropping due to zero-len\n");
-		return false;
-	}
+	first = skb_peek(amsdu);
+	rxd = (void *)first->data - sizeof(*rxd);
+	decap = MS(__le32_to_cpu(rxd->msdu_start.info1),
+		   RX_MSDU_START_INFO1_DECAP_FORMAT);
 
-	if (attention & RX_ATTENTION_FLAGS_DECRYPT_ERR) {
-		ath10k_dbg(ar, ATH10K_DBG_HTT,
-			   "htt rx dropping due to decrypt-err\n");
-		return false;
+	if (!chained)
+		return;
+
+	/* FIXME: Current unchaining logic can only handle simple case of raw
+	 * msdu chaining. If decapping is other than raw the chaining may be
+	 * more complex and this isn't handled by the current code. Don't even
+	 * try re-constructing such frames - it'll be pretty much garbage.
+	 */
+	if (decap != RX_MSDU_DECAP_RAW ||
+	    skb_queue_len(amsdu) != 1 + rxd->frag_info.ring2_more_count) {
+		__skb_queue_purge(amsdu);
+		return;
 	}
 
-	if (!channel_set) {
-		ath10k_warn(ar, "no channel configured; ignoring frame!\n");
+	ath10k_unchain_msdu(amsdu);
+}
+
+static bool ath10k_htt_rx_amsdu_allowed(struct ath10k *ar,
+					struct sk_buff_head *amsdu,
+					struct ieee80211_rx_status *rx_status)
+{
+	struct sk_buff *msdu;
+	struct htt_rx_desc *rxd;
+
+	msdu = skb_peek(amsdu);
+	rxd = (void *)msdu->data - sizeof(*rxd);
+
+	/* FIXME: It might be a good idea to do some fuzzy-testing to drop
+	 * invalid/dangerous frames.
+	 */
+
+	if (!rx_status->freq) {
+		ath10k_warn(ar, "no channel configured; ignoring frame(s)!\n");
 		return false;
 	}
 
-	/* Skip mgmt frames while we handle this in WMI */
-	if (attention & RX_ATTENTION_FLAGS_MGMT_TYPE) {
+	/* Management frames are handled via WMI events. The pros of such
+	 * approach is that channel is explicitly provided in WMI events
+	 * whereas HTT doesn't provide channel information for Rxed frames.
+	 */
+	if (rxd->attention.flags &
+	    __cpu_to_le32(RX_ATTENTION_FLAGS_MGMT_TYPE)) {
 		ath10k_dbg(ar, ATH10K_DBG_HTT, "htt rx mgmt ctrl\n");
 		return false;
 	}
 
-	if (test_bit(ATH10K_CAC_RUNNING, &htt->ar->dev_flags)) {
-		ath10k_dbg(ar, ATH10K_DBG_HTT,
-			   "htt rx CAC running\n");
+	if (test_bit(ATH10K_CAC_RUNNING, &ar->dev_flags)) {
+		ath10k_dbg(ar, ATH10K_DBG_HTT, "htt rx cac running\n");
 		return false;
 	}
 
 	return true;
 }
 
+static void ath10k_htt_rx_h_filter(struct ath10k *ar,
+				   struct sk_buff_head *amsdu,
+				   struct ieee80211_rx_status *rx_status)
+{
+	if (skb_queue_empty(amsdu))
+		return;
+
+	if (ath10k_htt_rx_amsdu_allowed(ar, amsdu, rx_status))
+		return;
+
+	__skb_queue_purge(amsdu);
+}
+
 static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 				  struct htt_rx_indication *rx)
 {
@@ -1180,7 +1361,6 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 	struct ieee80211_rx_status *rx_status = &htt->rx_status;
 	struct htt_rx_indication_mpdu_range *mpdu_ranges;
 	struct sk_buff_head amsdu;
-	struct ieee80211_hdr *hdr;
 	int num_mpdu_ranges;
 	u32 attention;
 	int fw_desc_len;
@@ -1247,34 +1427,10 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 			break;
 		}
 
-		if (!ath10k_htt_rx_amsdu_allowed(htt, skb_peek(&amsdu),
-						 channel_set, attention)) {
-			__skb_queue_purge(&amsdu);
-			continue;
-		}
-
-		if (ret > 0 && ath10k_unchain_msdu(&amsdu) < 0) {
-			__skb_queue_purge(&amsdu);
-			continue;
-		}
-
-		if (attention & RX_ATTENTION_FLAGS_FCS_ERR)
-			rx_status->flag |= RX_FLAG_FAILED_FCS_CRC;
-		else
-			rx_status->flag &= ~RX_FLAG_FAILED_FCS_CRC;
-
-		if (attention & RX_ATTENTION_FLAGS_TKIP_MIC_ERR)
-			rx_status->flag |= RX_FLAG_MMIC_ERROR;
-		else
-			rx_status->flag &= ~RX_FLAG_MMIC_ERROR;
-
-		hdr = ath10k_htt_rx_skb_get_hdr(skb_peek(&amsdu));
-
-		if (ath10k_htt_rx_hdr_is_amsdu(hdr))
-			ath10k_htt_rx_amsdu(htt, rx_status, &amsdu);
-		else
-			ath10k_htt_rx_msdu(htt, rx_status,
-					   __skb_dequeue(&amsdu));
+		ath10k_htt_rx_h_unchain(ar, &amsdu, ret > 0);
+		ath10k_htt_rx_h_filter(ar, &amsdu, rx_status);
+		ath10k_htt_rx_h_mpdu(ar, &amsdu, rx_status);
+		ath10k_htt_rx_h_deliver(ar, &amsdu, rx_status);
 	}
 
 	tasklet_schedule(&htt->rx_replenish_task);
@@ -1284,19 +1440,11 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 				       struct htt_rx_fragment_indication *frag)
 {
 	struct ath10k *ar = htt->ar;
-	struct sk_buff *msdu;
-	enum htt_rx_mpdu_encrypt_type enctype;
-	struct htt_rx_desc *rxd;
-	enum rx_msdu_decap_format fmt;
 	struct ieee80211_rx_status *rx_status = &htt->rx_status;
-	struct ieee80211_hdr *hdr;
 	struct sk_buff_head amsdu;
 	int ret;
-	bool tkip_mic_err;
-	bool decrypt_err;
 	u8 *fw_desc;
-	int fw_desc_len, hdrlen, paramlen;
-	int trim;
+	int fw_desc_len;
 	u32 attention = 0;
 
 	fw_desc_len = __le16_to_cpu(frag->fw_rx_desc_bytes);
@@ -1326,75 +1474,13 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 		return;
 	}
 
-	msdu = __skb_dequeue(&amsdu);
-
 	/* FIXME: implement signal strength */
 	rx_status->flag |= RX_FLAG_NO_SIGNAL_VAL;
 
-	hdr = (struct ieee80211_hdr *)msdu->data;
-	rxd = (void *)msdu->data - sizeof(*rxd);
-	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);
-
-	if (fmt != RX_MSDU_DECAP_RAW) {
-		ath10k_warn(ar, "we dont support non-raw fragmented rx yet\n");
-		dev_kfree_skb_any(msdu);
-		goto end;
-	}
-
-	enctype = MS(__le32_to_cpu(rxd->mpdu_start.info0),
-		     RX_MPDU_START_INFO0_ENCRYPT_TYPE);
-	ath10k_htt_rx_h_protected(htt, rx_status, msdu, enctype, fmt,
-				  true);
-	msdu->ip_summed = ath10k_htt_rx_get_csum_state(msdu);
-
-	if (tkip_mic_err)
-		ath10k_warn(ar, "tkip mic error\n");
-
-	if (decrypt_err) {
-		ath10k_warn(ar, "decryption err in fragmented rx\n");
-		dev_kfree_skb_any(msdu);
-		goto end;
-	}
-
-	if (enctype != HTT_RX_MPDU_ENCRYPT_NONE) {
-		hdrlen = ieee80211_hdrlen(hdr->frame_control);
-		paramlen = ath10k_htt_rx_crypto_param_len(ar, enctype);
-
-		/* It is more efficient to move the header than the payload */
-		memmove((void *)msdu->data + paramlen,
-			(void *)msdu->data,
-			hdrlen);
-		skb_pull(msdu, paramlen);
-		hdr = (struct ieee80211_hdr *)msdu->data;
-	}
-
-	/* remove trailing FCS */
-	trim  = 4;
-
-	/* remove crypto trailer */
-	trim += ath10k_htt_rx_crypto_tail_len(ar, enctype);
-
-	/* last fragment of TKIP frags has MIC */
-	if (!ieee80211_has_morefrags(hdr->frame_control) &&
-	    enctype == HTT_RX_MPDU_ENCRYPT_TKIP_WPA)
-		trim += MICHAEL_MIC_LEN;
-
-	if (trim > msdu->len) {
-		ath10k_warn(ar, "htt rx fragment: trailer longer than the frame itself? drop\n");
-		dev_kfree_skb_any(msdu);
-		goto end;
-	}
-
-	skb_trim(msdu, msdu->len - trim);
-
-	ath10k_dbg_dump(ar, ATH10K_DBG_HTT_DUMP, NULL, "htt rx frag mpdu: ",
-			msdu->data, msdu->len);
-	ath10k_process_rx(htt->ar, rx_status, msdu);
+	ath10k_htt_rx_h_filter(ar, &amsdu, rx_status);
+	ath10k_htt_rx_h_mpdu(ar, &amsdu, rx_status);
+	ath10k_htt_rx_h_deliver(ar, &amsdu, rx_status);
 
-end:
 	if (fw_desc_len > 0) {
 		ath10k_dbg(ar, ATH10K_DBG_HTT,
 			   "expecting more fragmented rx in one indication %d\n",
-- 
1.8.5.3

[PATCH 5/7] ath10k: remove unused function argument

[Michal Kazior]

The original fix has been moved into a different
place in the code.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 6abfea7..55e4568 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -306,8 +306,7 @@ static inline struct sk_buff *ath10k_htt_rx_netbuf_pop(struct ath10k_htt *htt)
 /* return: < 0 fatal error, 0 - non chained msdu, 1 chained msdu */
 static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 				   u8 **fw_desc, int *fw_desc_len,
-				   struct sk_buff_head *amsdu,
-				   u32 *attention)
+				   struct sk_buff_head *amsdu)
 {
 	struct ath10k *ar = htt->ar;
 	int msdu_len, msdu_chaining = 0;
@@ -348,11 +347,6 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 			return -EIO;
 		}
 
-		*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
@@ -1362,7 +1356,6 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 	struct htt_rx_indication_mpdu_range *mpdu_ranges;
 	struct sk_buff_head amsdu;
 	int num_mpdu_ranges;
-	u32 attention;
 	int fw_desc_len;
 	u8 *fw_desc;
 	bool channel_set;
@@ -1412,11 +1405,9 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 		mpdu_count += mpdu_ranges[i].mpdu_count;
 
 	while (mpdu_count--) {
-		attention = 0;
 		__skb_queue_head_init(&amsdu);
 		ret = ath10k_htt_rx_amsdu_pop(htt, &fw_desc,
-					      &fw_desc_len, &amsdu,
-					      &attention);
+					      &fw_desc_len, &amsdu);
 		if (ret < 0) {
 			ath10k_warn(ar, "rx ring became corrupted: %d\n", ret);
 			__skb_queue_purge(&amsdu);
@@ -1445,7 +1436,6 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 	int ret;
 	u8 *fw_desc;
 	int fw_desc_len;
-	u32 attention = 0;
 
 	fw_desc_len = __le16_to_cpu(frag->fw_rx_desc_bytes);
 	fw_desc = (u8 *)frag->fw_msdu_rx_desc;
@@ -1454,7 +1444,7 @@ 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,
-				      &amsdu, &attention);
+				      &amsdu);
 	spin_unlock_bh(&htt->rx_ring.lock);
 
 	tasklet_schedule(&htt->rx_replenish_task);
-- 
1.8.5.3

[PATCH 6/7] ath10k: use rx descriptor for ppdu status extraction

[Michal Kazior]

This makes it more in line with the new Rx path.
It also makes the code more reusable because Rx
descriptor is more accessible.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 137 ++++++++++++++++++++++---------
 1 file changed, 96 insertions(+), 41 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 55e4568..9073a0e 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -631,23 +631,34 @@ static const u8 rx_legacy_rate_idx[] = {
 };
 
 static void ath10k_htt_rx_h_rates(struct ath10k *ar,
-				  enum ieee80211_band band,
-				  u8 info0, u32 info1, u32 info2,
-				  struct ieee80211_rx_status *status)
+				  struct ieee80211_rx_status *status,
+				  struct htt_rx_desc *rxd)
 {
+	enum ieee80211_band band;
 	u8 cck, rate, rate_idx, bw, sgi, mcs, nss;
 	u8 preamble = 0;
+	u32 info1, info2, info3;
 
-	/* Check if valid fields */
-	if (!(info0 & HTT_RX_INDICATION_INFO0_START_VALID))
+	/* Band value can't be set as undefined but freq can be 0 - use that to
+	 * determine whether band is provided.
+	 *
+	 * FIXME: Perhaps this can go away if CCK rate reporting is a little
+	 * reworked?
+	 */
+	if (!status->freq)
 		return;
 
-	preamble = MS(info1, HTT_RX_INDICATION_INFO1_PREAMBLE_TYPE);
+	band = status->band;
+	info1 = __le32_to_cpu(rxd->ppdu_start.info1);
+	info2 = __le32_to_cpu(rxd->ppdu_start.info2);
+	info3 = __le32_to_cpu(rxd->ppdu_start.info3);
+
+	preamble = MS(info1, RX_PPDU_START_INFO1_PREAMBLE_TYPE);
 
 	switch (preamble) {
 	case HTT_RX_LEGACY:
-		cck = info0 & HTT_RX_INDICATION_INFO0_LEGACY_RATE_CCK;
-		rate = MS(info0, HTT_RX_INDICATION_INFO0_LEGACY_RATE);
+		cck = info1 & RX_PPDU_START_INFO1_L_SIG_RATE_SELECT;
+		rate = MS(info1, RX_PPDU_START_INFO1_L_SIG_RATE);
 		rate_idx = 0;
 
 		if (rate < 0x08 || rate > 0x0F)
@@ -674,11 +685,11 @@ static void ath10k_htt_rx_h_rates(struct ath10k *ar,
 		break;
 	case HTT_RX_HT:
 	case HTT_RX_HT_WITH_TXBF:
-		/* HT-SIG - Table 20-11 in info1 and info2 */
-		mcs = info1 & 0x1F;
+		/* HT-SIG - Table 20-11 in info2 and info3 */
+		mcs = info2 & 0x1F;
 		nss = mcs >> 3;
-		bw = (info1 >> 7) & 1;
-		sgi = (info2 >> 7) & 1;
+		bw = (info2 >> 7) & 1;
+		sgi = (info3 >> 7) & 1;
 
 		status->rate_idx = mcs;
 		status->flag |= RX_FLAG_HT;
@@ -689,12 +700,12 @@ static void ath10k_htt_rx_h_rates(struct ath10k *ar,
 		break;
 	case HTT_RX_VHT:
 	case HTT_RX_VHT_WITH_TXBF:
-		/* VHT-SIG-A1 in info 1, VHT-SIG-A2 in info2
+		/* VHT-SIG-A1 in info2, VHT-SIG-A2 in info3
 		   TODO check this */
-		mcs = (info2 >> 4) & 0x0F;
-		nss = ((info1 >> 10) & 0x07) + 1;
-		bw = info1 & 3;
-		sgi = info2 & 1;
+		mcs = (info3 >> 4) & 0x0F;
+		nss = ((info2 >> 10) & 0x07) + 1;
+		bw = info2 & 3;
+		sgi = info3 & 1;
 
 		status->rate_idx = mcs;
 		status->vht_nss = nss;
@@ -742,6 +753,73 @@ static bool ath10k_htt_rx_h_channel(struct ath10k *ar,
 	return true;
 }
 
+static void ath10k_htt_rx_h_signal(struct ath10k *ar,
+				   struct ieee80211_rx_status *status,
+				   struct htt_rx_desc *rxd)
+{
+	/* FIXME: Get real NF */
+	status->signal = ATH10K_DEFAULT_NOISE_FLOOR +
+			 rxd->ppdu_start.rssi_comb;
+	status->flag &= ~RX_FLAG_NO_SIGNAL_VAL;
+}
+
+static void ath10k_htt_rx_h_mactime(struct ath10k *ar,
+				    struct ieee80211_rx_status *status,
+				    struct htt_rx_desc *rxd)
+{
+	/* FIXME: TSF is known only at the end of PPDU, in the last MPDU. This
+	 * means all prior MSDUs in a PPDU are reported to mac80211 without the
+	 * TSF. Is it worth holding frames until end of PPDU is known?
+	 *
+	 * FIXME: Can we get/compute 64bit TSF?
+	 */
+	status->mactime = __le32_to_cpu(rxd->ppdu_end.tsf_timestamp);
+	status->flag |= RX_FLAG_MACTIME_END;
+}
+
+static void ath10k_htt_rx_h_ppdu(struct ath10k *ar,
+				 struct sk_buff_head *amsdu,
+				 struct ieee80211_rx_status *status)
+{
+	struct sk_buff *first;
+	struct htt_rx_desc *rxd;
+	bool is_first_ppdu;
+	bool is_last_ppdu;
+
+	if (skb_queue_empty(amsdu))
+		return;
+
+	first = skb_peek(amsdu);
+	rxd = (void *)first->data - sizeof(*rxd);
+
+	is_first_ppdu = !!(rxd->attention.flags &
+			   __cpu_to_le32(RX_ATTENTION_FLAGS_FIRST_MPDU));
+	is_last_ppdu = !!(rxd->attention.flags &
+			  __cpu_to_le32(RX_ATTENTION_FLAGS_LAST_MPDU));
+
+	if (is_first_ppdu) {
+		/* New PPDU starts so clear out the old per-PPDU status. */
+		status->freq = 0;
+		status->rate_idx = 0;
+		status->vht_nss = 0;
+		status->vht_flag &= ~RX_VHT_FLAG_80MHZ;
+		status->flag &= ~(RX_FLAG_HT |
+				  RX_FLAG_VHT |
+				  RX_FLAG_SHORT_GI |
+				  RX_FLAG_40MHZ |
+				  RX_FLAG_MACTIME_END);
+		status->flag |= RX_FLAG_NO_SIGNAL_VAL;
+
+		ath10k_htt_rx_h_signal(ar, status, rxd);
+		ath10k_htt_rx_h_channel(ar, status);
+		ath10k_htt_rx_h_rates(ar, status, rxd);
+	}
+
+	if (is_last_ppdu) {
+		ath10k_htt_rx_h_mactime(ar, status, rxd);
+	}
+}
+
 static const char * const tid_to_ac[] = {
 	"BE",
 	"BK",
@@ -1358,7 +1436,6 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 	int num_mpdu_ranges;
 	int fw_desc_len;
 	u8 *fw_desc;
-	bool channel_set;
 	int i, ret, mpdu_count = 0;
 
 	lockdep_assert_held(&htt->rx_ring.lock);
@@ -1373,29 +1450,6 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 			     HTT_RX_INDICATION_INFO1_NUM_MPDU_RANGES);
 	mpdu_ranges = htt_rx_ind_get_mpdu_ranges(rx);
 
-	/* Fill this once, while this is per-ppdu */
-	if (rx->ppdu.info0 & HTT_RX_INDICATION_INFO0_START_VALID) {
-		memset(rx_status, 0, sizeof(*rx_status));
-		rx_status->signal  = ATH10K_DEFAULT_NOISE_FLOOR +
-				     rx->ppdu.combined_rssi;
-	}
-
-	if (rx->ppdu.info0 & HTT_RX_INDICATION_INFO0_END_VALID) {
-		/* TSF available only in 32-bit */
-		rx_status->mactime = __le32_to_cpu(rx->ppdu.tsf) & 0xffffffff;
-		rx_status->flag |= RX_FLAG_MACTIME_END;
-	}
-
-	channel_set = ath10k_htt_rx_h_channel(htt->ar, rx_status);
-
-	if (channel_set) {
-		ath10k_htt_rx_h_rates(htt->ar, rx_status->band,
-				      rx->ppdu.info0,
-				      __le32_to_cpu(rx->ppdu.info1),
-				      __le32_to_cpu(rx->ppdu.info2),
-				      rx_status);
-	}
-
 	ath10k_dbg_dump(ar, ATH10K_DBG_HTT_DUMP, NULL, "htt rx ind: ",
 			rx, sizeof(*rx) +
 			(sizeof(struct htt_rx_indication_mpdu_range) *
@@ -1418,6 +1472,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 			break;
 		}
 
+		ath10k_htt_rx_h_ppdu(ar, &amsdu, rx_status);
 		ath10k_htt_rx_h_unchain(ar, &amsdu, ret > 0);
 		ath10k_htt_rx_h_filter(ar, &amsdu, rx_status);
 		ath10k_htt_rx_h_mpdu(ar, &amsdu, rx_status);
-- 
1.8.5.3

[PATCH 7/7] ath10k: report rx rate and signal for fragmented Rx

[Michal Kazior]

Fragmented Rx wasn't reporting everything. With
the reworked Rx code it's very easy to fix it.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 9073a0e..a22e832 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1519,9 +1519,7 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 		return;
 	}
 
-	/* FIXME: implement signal strength */
-	rx_status->flag |= RX_FLAG_NO_SIGNAL_VAL;
-
+	ath10k_htt_rx_h_ppdu(ar, &amsdu, rx_status);
 	ath10k_htt_rx_h_filter(ar, &amsdu, rx_status);
 	ath10k_htt_rx_h_mpdu(ar, &amsdu, rx_status);
 	ath10k_htt_rx_h_deliver(ar, &amsdu, rx_status);
-- 
1.8.5.3

Re: [PATCH 4/7] ath10k: unify rx undecapping

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> This creates a single, common path for MSDU,
> A-MSDU and fragmented Rx.
>
> Hopefully this will make it easier to understand
> Rx path and make it easier to work with.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

This patch had few checkpatch warnings. I fixed them with the folded
patch and full patch here:

https://github.com/kvalo/ath/commit/71fbd07d43e54f5f9f442bc5f2f4f9ef83aead63

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 6abfea768173..08963439891b 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1066,7 +1066,8 @@ static void ath10k_htt_rx_h_undecap(struct ath10k *ar,
 
 	switch (decap) {
 	case RX_MSDU_DECAP_RAW:
-		ath10k_htt_rx_h_undecap_raw(ar, msdu, status, enctype, is_decrypted);
+		ath10k_htt_rx_h_undecap_raw(ar, msdu, status, enctype,
+					    is_decrypted);
 		break;
 	case RX_MSDU_DECAP_NATIVE_WIFI:
 		ath10k_htt_rx_h_undecap_nwifi(ar, msdu, status, first_hdr);
@@ -1132,7 +1133,7 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
 	bool has_fcs_err;
 	bool has_crypto_err;
 	bool has_tkip_err;
-	bool has_peer_idx_invalid;
+	bool has_idx_invalid;
 	bool is_decrypted;
 
 	if (skb_queue_empty(amsdu))
@@ -1167,8 +1168,8 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
 			    __cpu_to_le32(RX_ATTENTION_FLAGS_DECRYPT_ERR));
 	has_tkip_err = !!(rxd->attention.flags &
 			  __cpu_to_le32(RX_ATTENTION_FLAGS_TKIP_MIC_ERR));
-	has_peer_idx_invalid = !!(rxd->attention.flags &
-			          __cpu_to_le32(RX_ATTENTION_FLAGS_PEER_IDX_INVALID));
+	has_idx_invalid = !!(rxd->attention.flags &
+			     __cpu_to_le32(RX_ATTENTION_FLAGS_PEER_IDX_INVALID));
 
 	/* Note: If hardware captures an encrypted frame that it can't decrypt,
 	 * e.g. due to fcs error, missing peer or invalid key data it will
@@ -1177,7 +1178,7 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
 	is_decrypted = (enctype != HTT_RX_MPDU_ENCRYPT_NONE &&
 			!has_fcs_err &&
 			!has_crypto_err &&
-			!has_peer_idx_invalid);
+			!has_idx_invalid);
 
 	/* Clear per-MPDU flags while leaving per-PPDU flags intact. */
 	status->flag &= ~(RX_FLAG_FAILED_FCS_CRC |


-- 
Kalle Valo

Re: [PATCH 6/7] ath10k: use rx descriptor for ppdu status extraction

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> This makes it more in line with the new Rx path.
> It also makes the code more reusable because Rx
> descriptor is more accessible.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

This patch also had a checkpatch warning. Full patch here:

https://github.com/kvalo/ath/commit/d01090f45a5891f811bfaadb9a15c7e47ef0ef26

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 41dce044e3e8..bf82cb756548 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -815,9 +815,8 @@ static void ath10k_htt_rx_h_ppdu(struct ath10k *ar,
 		ath10k_htt_rx_h_rates(ar, status, rxd);
 	}
 
-	if (is_last_ppdu) {
+	if (is_last_ppdu)
 		ath10k_htt_rx_h_mactime(ar, status, rxd);
-	}
 }
 
 static const char * const tid_to_ac[] = {


-- 
Kalle Valo

Re: [PATCH 4/7] ath10k: unify rx undecapping

[Michal Kazior]

On 17 November 2014 15:32, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> This creates a single, common path for MSDU,
>> A-MSDU and fragmented Rx.
>>
>> Hopefully this will make it easier to understand
>> Rx path and make it easier to work with.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> This patch had few checkpatch warnings. I fixed them with the folded
> patch and full patch here:
>
> https://github.com/kvalo/ath/commit/71fbd07d43e54f5f9f442bc5f2f4f9ef83aead63

Thanks!

[...]
> @@ -1132,7 +1133,7 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
>         bool has_fcs_err;
>         bool has_crypto_err;
>         bool has_tkip_err;
> -       bool has_peer_idx_invalid;
> +       bool has_idx_invalid;
>         bool is_decrypted;

I don't really like the has_idx_invalid. Perhaps has_peer_err conveys
a bit more of the original meaning?

As much as I'd like to leave the original var name I'd like to be
checkpatch warning free. Sigh..


Michał

Re: [PATCH 4/7] ath10k: unify rx undecapping

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 17 November 2014 15:32, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> This creates a single, common path for MSDU,
>>> A-MSDU and fragmented Rx.
>>>
>>> Hopefully this will make it easier to understand
>>> Rx path and make it easier to work with.
>>>
>>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>>
>> This patch had few checkpatch warnings. I fixed them with the folded
>> patch and full patch here:
>>
>> https://github.com/kvalo/ath/commit/71fbd07d43e54f5f9f442bc5f2f4f9ef83aead63
>
> Thanks!
>
> [...]
>> @@ -1132,7 +1133,7 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
>>         bool has_fcs_err;
>>         bool has_crypto_err;
>>         bool has_tkip_err;
>> -       bool has_peer_idx_invalid;
>> +       bool has_idx_invalid;
>>         bool is_decrypted;
>
> I don't really like the has_idx_invalid. Perhaps has_peer_err conveys
> a bit more of the original meaning?

What about just peer_idx_invalid? IMHO we really don't need the has_
prefix in that relatively small function.

> As much as I'd like to leave the original var name I'd like to be
> checkpatch warning free. Sigh..

Same here. The checkpatch is just so useful tool to keep the style
unified.

-- 
Kalle Valo

Re: [PATCH 4/7] ath10k: unify rx undecapping

[Michal Kazior]

On 17 November 2014 16:11, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>> On 17 November 2014 15:32, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
[...]
>>> This patch had few checkpatch warnings. I fixed them with the folded
>>> patch and full patch here:
>>>
>>> https://github.com/kvalo/ath/commit/71fbd07d43e54f5f9f442bc5f2f4f9ef83aead63
[...]
>>> @@ -1132,7 +1133,7 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
>>>         bool has_fcs_err;
>>>         bool has_crypto_err;
>>>         bool has_tkip_err;
>>> -       bool has_peer_idx_invalid;
>>> +       bool has_idx_invalid;
>>>         bool is_decrypted;
>>
>> I don't really like the has_idx_invalid. Perhaps has_peer_err conveys
>> a bit more of the original meaning?
>
> What about just peer_idx_invalid? IMHO we really don't need the has_
> prefix in that relatively small function.

Good point but the assignment line with `peer_idx_invalid` will still
be over 80 chars long, no?

So perhaps instead of doing `rxd->attention.flags &
__cpu_to_le32(FLAG)` it could be `attention =
__le32_to_cpu(rxd->attention.flags)` and then `attention & FLAG` ?
This way it shouldn't exceed the 80 char limit and var names won't
need to be changed. Hopefully compiler will optimize it away.


Michał

Re: [PATCH 4/7] ath10k: unify rx undecapping

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 17 November 2014 16:11, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>> On 17 November 2014 15:32, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> [...]
>>>> This patch had few checkpatch warnings. I fixed them with the folded
>>>> patch and full patch here:
>>>>
>>>> https://github.com/kvalo/ath/commit/71fbd07d43e54f5f9f442bc5f2f4f9ef83aead63
> [...]
>>>> @@ -1132,7 +1133,7 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
>>>>         bool has_fcs_err;
>>>>         bool has_crypto_err;
>>>>         bool has_tkip_err;
>>>> -       bool has_peer_idx_invalid;
>>>> +       bool has_idx_invalid;
>>>>         bool is_decrypted;
>>>
>>> I don't really like the has_idx_invalid. Perhaps has_peer_err conveys
>>> a bit more of the original meaning?
>>
>> What about just peer_idx_invalid? IMHO we really don't need the has_
>> prefix in that relatively small function.
>
> Good point but the assignment line with `peer_idx_invalid` will still
> be over 80 chars long, no?

I actually test with 83 char limit nowadays. I should add the checkpatch
command line to the wiki.

> So perhaps instead of doing `rxd->attention.flags &
> __cpu_to_le32(FLAG)` it could be `attention =
> __le32_to_cpu(rxd->attention.flags)` and then `attention & FLAG` ?
> This way it shouldn't exceed the 80 char limit and var names won't
> need to be changed. Hopefully compiler will optimize it away.

That's much better and actually has a lot less calls to cpu_le32(). I
folded the patch below.

But aren't the '!!' operators useless? 'has_*' variables are booleans,
doesn't compiler handle that automatically?

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 6f58b7f912cd..6abb3b6a348f 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1133,8 +1133,9 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
 	bool has_fcs_err;
 	bool has_crypto_err;
 	bool has_tkip_err;
-	bool has_idx_invalid;
+	bool has_peer_idx_invalid;
 	bool is_decrypted;
+	u32 attention;
 
 	if (skb_queue_empty(amsdu))
 		return;
@@ -1162,14 +1163,12 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
 	/* Some attention flags are valid only in the last MSDU. */
 	last = skb_peek_tail(amsdu);
 	rxd = (void *)last->data - sizeof(*rxd);
-	has_fcs_err = !!(rxd->attention.flags &
-			 __cpu_to_le32(RX_ATTENTION_FLAGS_FCS_ERR));
-	has_crypto_err = !!(rxd->attention.flags &
-			    __cpu_to_le32(RX_ATTENTION_FLAGS_DECRYPT_ERR));
-	has_tkip_err = !!(rxd->attention.flags &
-			  __cpu_to_le32(RX_ATTENTION_FLAGS_TKIP_MIC_ERR));
-	has_idx_invalid = !!(rxd->attention.flags &
-			     __cpu_to_le32(RX_ATTENTION_FLAGS_PEER_IDX_INVALID));
+	attention = __le32_to_cpu(rxd->attention.flags);
+
+	has_fcs_err = !!(attention & RX_ATTENTION_FLAGS_FCS_ERR);
+	has_crypto_err = !!(attention & RX_ATTENTION_FLAGS_DECRYPT_ERR);
+	has_tkip_err = !!(attention & RX_ATTENTION_FLAGS_TKIP_MIC_ERR);
+	has_peer_idx_invalid = !!(attention & RX_ATTENTION_FLAGS_PEER_IDX_INVALID);
 
 	/* Note: If hardware captures an encrypted frame that it can't decrypt,
 	 * e.g. due to fcs error, missing peer or invalid key data it will
@@ -1178,7 +1177,7 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
 	is_decrypted = (enctype != HTT_RX_MPDU_ENCRYPT_NONE &&
 			!has_fcs_err &&
 			!has_crypto_err &&
-			!has_idx_invalid);
+			!has_peer_idx_invalid);
 
 	/* Clear per-MPDU flags while leaving per-PPDU flags intact. */
 	status->flag &= ~(RX_FLAG_FAILED_FCS_CRC |




-- 
Kalle Valo

Re: [PATCH 0/7] ath10k: rework rx path

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> This patchset unifies rx path. Instead of
> branching for A-MSDU and MSDU cases a single
> codepath is introduced.
>
> This yields:
>
>    text    data     bss     dec     hex filename
>   14068       0       0   14068    36f4 before/htt_rx.o
>   13308       3       0   13311    33ff after/htt_rx.o
>
> I haven't measured any Rx performance loss in my
> setup but I may be biased. In theory this could
> reduce i-cache pressure and improve Rx throughput
> on CPU contrained systems by a few mbps.
>
> This patchset serves two purposes though:
>  - clean up and unify the Rx path a bit
>  - make it possible to reuse code more easily in
>    the future
>
>
> Michal Kazior (7):
>   ath10k: start using sk_buff_head
>   ath10k: simplify Rx loop
>   ath10k: refactor htt->rx_confused
>   ath10k: unify rx undecapping
>   ath10k: remove unused function argument
>   ath10k: use rx descriptor for ppdu status extraction
>   ath10k: report rx rate and signal for fragmented Rx

Thanks, all seven applied.

-- 
Kalle Valo