[ath9k-devel] [RFC 0/2] ath10k: kill off htt_tx_info wrapper

[ath9k-devel] [RFC 0/2] ath10k: kill off htt_tx_info wrapper

[Michal Kazior]

This patchset merges the htt_tx_info into
ath10k_skb_cb. This depends upon my 'htt tx
cleanup' patchset.

Michal Kazior (2):
  ath10k: simplify txi lifespan
  ath10k: merge htt_tx_info into ath10k_skb_cb

 drivers/net/wireless/ath/ath10k/core.h   |    7 +-
 drivers/net/wireless/ath/ath10k/htt.h    |   31 +---
 drivers/net/wireless/ath/ath10k/htt_tx.c |  263 ++++++++++++------------------
 drivers/net/wireless/ath/ath10k/mac.c    |   13 +-
 drivers/net/wireless/ath/ath10k/txrx.c   |   65 +++++---
 drivers/net/wireless/ath/ath10k/txrx.h   |    1 +
 6 files changed, 170 insertions(+), 210 deletions(-)

-- 
1.7.9.5

[ath9k-devel] [RFC 1/2] ath10k: simplify txi lifespan

[Michal Kazior]

Up until now we were using two booleans to keep
track of htt and htc tx completions. These can
come in no specific order.

Keep in mind that it was safe to access those
without any locks since tx completion processing
is done one endpoint at a time (that's the current
PCI backend implementation).

The new approach uses refcount. Since we need 2
completions we setup refcount=2 for mgmt and
data tx. This also depends on the tx completion
behaviour.

If the PCI tx completion scheme ever changes we'll
need locking to synchronize tx completions for
htt.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h   |    5 ++-
 drivers/net/wireless/ath/ath10k/htt.h    |    2 -
 drivers/net/wireless/ath/ath10k/htt_tx.c |   60 ++++++------------------------
 drivers/net/wireless/ath/ath10k/txrx.c   |   41 +++++++++++++-------
 drivers/net/wireless/ath/ath10k/txrx.h   |    1 +
 5 files changed, 45 insertions(+), 64 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 1aef940..0c3cfe4 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -59,13 +59,16 @@ struct ath10k_skb_cb {
 		u16 msdu_id;
 		bool is_offchan;
 		bool is_conf;
+		bool discard;
+		bool no_ack;
+		u8 refcount;
 	} __packed htt;
 
 	struct {
 		u8 credits_used;
 	} __packed htc;
 
-	/* 24 bytes left on 64bit arch */
+	/* 21 bytes left on 64bit arch */
 } __packed;
 
 static inline struct ath10k_skb_cb *ATH10K_SKB_CB(struct sk_buff *skb)
diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
index e98f079..8f85a42 100644
--- a/drivers/net/wireless/ath/ath10k/htt.h
+++ b/drivers/net/wireless/ath/ath10k/htt.h
@@ -1115,8 +1115,6 @@ struct htt_tx_info {
 	u16 msdu_id;
 
 	atomic_t is_used;
-	bool htc_tx_completed;
-	bool htt_tx_completed;
 
 	struct sk_buff *txdesc;
 	struct sk_buff *txfrag; /* not used for mgmt tx */
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index a4f1072..b20c386 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -18,6 +18,7 @@
 #include <linux/etherdevice.h>
 #include "htt.h"
 #include "mac.h"
+#include "txrx.h"
 #include "debug.h"
 
 static void ath10k_htt_tx_info_pool_setup(struct htt_struct *htt)
@@ -47,9 +48,6 @@ struct htt_tx_info *ath10k_htt_tx_info_alloc(struct htt_struct *htt)
 		ath10k_dbg(ATH10K_DBG_HTT, "htt txi alloc idx %d\n",
 			   txi->msdu_id);
 
-		txi->htc_tx_completed = false;
-		txi->htt_tx_completed = false;
-
 		atomic_inc(&htt->num_used_txi);
 	}
 
@@ -71,24 +69,6 @@ void ath10k_htt_tx_info_free(struct htt_struct *htt, struct htt_tx_info *txi)
 		wake_up(&htt->empty_tx_wq);
 }
 
-void ath10k_htt_tx_info_unref(struct htt_struct *htt, struct htt_tx_info *txi,
-			      struct sk_buff *skb)
-{
-	/* FIXME: we have to carefully synchronize completion of htt
-	 *        messages because those come in asynchronously
-	 *        and not necessarily in order.
-	 *        since htc_packet structure is no more this could perhaps
-	 *        be reworked in a more clean manner? */
-	if (!txi->htc_tx_completed)
-		return;
-
-	if (!txi->htt_tx_completed)
-		return;
-
-	dev_kfree_skb_any(skb);
-	ath10k_htt_tx_info_free(htt, txi);
-}
-
 void ath10k_htt_tx_attach(struct htt_struct *htt)
 {
 	ath10k_htt_tx_info_pool_setup(htt);
@@ -116,9 +96,7 @@ void ath10k_htt_htc_tx_complete(void *context, struct sk_buff *skb)
 {
 	struct ath10k_skb_cb *skb_cb = ATH10K_SKB_CB(skb);
 	struct htt_struct *htt = (struct htt_struct *)context;
-	struct device *dev = htt->ar->dev;
 	struct htt_tx_info *txi;
-	int ret;
 
 	if (skb_cb->htt.is_conf) {
 		dev_kfree_skb_any(skb);
@@ -132,34 +110,18 @@ void ath10k_htt_htc_tx_complete(void *context, struct sk_buff *skb)
 		return;
 	}
 
-	txi->htc_tx_completed = true;
-
 	if (skb_cb->is_aborted) {
-		/*
-		 * if a packet gets cancelled we need to make sure
-		 * to free skbs since htt mgmt tx completion indication
-		 * may have not came in yet
-		 */
-		if (!txi->htt_tx_completed) {
-			txi->htt_tx_completed = true;
-
-			if (txi->txfrag) {
-				ret = ath10k_skb_unmap(dev, txi->txfrag);
-				if (ret)
-					ath10k_warn("txfrag unmap failed (%d)\n", ret);
-
-				dev_kfree_skb_any(txi->txfrag);
-			}
-
-			ret = ath10k_skb_unmap(dev, txi->msdu);
-			if (ret)
-				ath10k_warn("data skb unmap failed (%d)\n", ret);
-
-			ieee80211_free_txskb(htt->ar->hw, txi->msdu);
-		}
+		skb_cb->htt.discard = true;
+
+		/* if the skbuff is aborted we need to make sure we'll free up
+		 * the tx resources, we can't simply run tx_unref() 2 times
+		 * because if htt tx completion came in earlier we'd access
+		 * unallocated memory */
+		if (skb_cb->htt.refcount > 1)
+			skb_cb->htt.refcount = 1;
 	}
 
-	ath10k_htt_tx_info_unref(htt, txi, skb);
+	ath10k_txrx_tx_unref(htt, txi);
 }
 
 int ath10k_htt_h2t_ver_req_msg(struct htt_struct *htt)
@@ -315,6 +277,7 @@ int ath10k_htt_mgmt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 
 	skb_cb = ATH10K_SKB_CB(txi->txdesc);
 	skb_cb->htt.msdu_id = txi->msdu_id;
+	skb_cb->htt.refcount = 2;
 
 	res = ath10k_htc_send(htt->htc_target, htt->ep_id, txi->txdesc);
 	if (res)
@@ -442,6 +405,7 @@ int ath10k_htt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 
 	skb_cb = ATH10K_SKB_CB(txi->txdesc);
 	skb_cb->htt.msdu_id = txi->msdu_id;
+	skb_cb->htt.refcount = 2;
 
 	res = ath10k_htc_send(htt->htc_target, htt->ep_id, txi->txdesc);
 	if (res)
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index 488216a..e69756f 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -44,22 +44,17 @@ out:
 	spin_unlock_bh(&ar->data_lock);
 }
 
-void ath10k_txrx_tx_completed(struct htt_struct *htt,
-			      const struct htt_tx_done *tx_done)
+void ath10k_txrx_tx_unref(struct htt_struct *htt, struct htt_tx_info *txi)
 {
 	struct device *dev = htt->ar->dev;
 	struct ieee80211_tx_info *info;
-	struct htt_tx_info *txi;
 	int ret;
 
-	ath10k_dbg(ATH10K_DBG_HTT, "htt tx completion msdu_id %u discard %d no_ack %d\n",
-		   tx_done->msdu_id, !!tx_done->discard, !!tx_done->no_ack);
+	if (ATH10K_SKB_CB(txi->txdesc)->htt.refcount == 0)
+		return;
 
-	txi = ath10k_htt_tx_info_lookup(htt, tx_done->msdu_id);
-	if (!txi) {
-		ath10k_dbg(ATH10K_DBG_HTT, "htt txi lookup fail\n");
+	if (--ATH10K_SKB_CB(txi->txdesc)->htt.refcount > 0)
 		return;
-	}
 
 	if (txi->txfrag) {
 		ret = ath10k_skb_unmap(dev, txi->txfrag);
@@ -78,7 +73,7 @@ void ath10k_txrx_tx_completed(struct htt_struct *htt,
 	info = IEEE80211_SKB_CB(txi->msdu);
 	memset(&info->status, 0, sizeof(info->status));
 
-	if (tx_done->discard) {
+	if (ATH10K_SKB_CB(txi->txdesc)->htt.discard) {
 		ieee80211_free_txskb(htt->ar->hw, txi->msdu);
 		goto exit;
 	}
@@ -86,15 +81,35 @@ void ath10k_txrx_tx_completed(struct htt_struct *htt,
 	if (!(info->flags & IEEE80211_TX_CTL_NO_ACK))
 		info->flags |= IEEE80211_TX_STAT_ACK;
 
-	if (tx_done->no_ack)
+	if (ATH10K_SKB_CB(txi->txdesc)->htt.no_ack)
 		info->flags &= ~IEEE80211_TX_STAT_ACK;
 
 	ieee80211_tx_status_irqsafe(htt->ar->hw, txi->msdu);
 	/* we do not own the msdu anymore */
 
 exit:
-	txi->htt_tx_completed = true;
-	ath10k_htt_tx_info_unref(htt, txi, txi->txdesc);
+	dev_kfree_skb_any(txi->txdesc);
+	ath10k_htt_tx_info_free(htt, txi);
+}
+
+void ath10k_txrx_tx_completed(struct htt_struct *htt,
+			      const struct htt_tx_done *tx_done)
+{
+	struct htt_tx_info *txi;
+
+	ath10k_dbg(ATH10K_DBG_HTT, "htt tx completion msdu_id %u discard %d no_ack %d\n",
+		   tx_done->msdu_id, !!tx_done->discard, !!tx_done->no_ack);
+
+	txi = ath10k_htt_tx_info_lookup(htt, tx_done->msdu_id);
+	if (!txi) {
+		ath10k_dbg(ATH10K_DBG_HTT, "htt txi lookup fail\n");
+		return;
+	}
+
+	ATH10K_SKB_CB(txi->txdesc)->htt.discard = tx_done->discard;
+	ATH10K_SKB_CB(txi->txdesc)->htt.no_ack = tx_done->no_ack;
+
+	ath10k_txrx_tx_unref(htt, txi);
 }
 
 static u8 rx_legacy_rate_idx[] = {
diff --git a/drivers/net/wireless/ath/ath10k/txrx.h b/drivers/net/wireless/ath/ath10k/txrx.h
index c80a472..7a1bbfe 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.h
+++ b/drivers/net/wireless/ath/ath10k/txrx.h
@@ -19,6 +19,7 @@
 
 #include "htt.h"
 
+void ath10k_txrx_tx_unref(struct htt_struct *htt, struct htt_tx_info *txi);
 void ath10k_txrx_tx_completed(struct htt_struct *htt,
 			      const struct htt_tx_done *tx_done);
 void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info);
-- 
1.7.9.5

[ath9k-devel] [RFC 2/2] ath10k: merge htt_tx_info into ath10k_skb_cb

[Michal Kazior]

This eliminates the wrapper structure htt_tx_info.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h   |    4 +-
 drivers/net/wireless/ath/ath10k/htt.h    |   29 ++---
 drivers/net/wireless/ath/ath10k/htt_tx.c |  205 ++++++++++++++----------------
 drivers/net/wireless/ath/ath10k/mac.c    |   13 +-
 drivers/net/wireless/ath/ath10k/txrx.c   |   52 ++++----
 drivers/net/wireless/ath/ath10k/txrx.h   |    2 +-
 6 files changed, 142 insertions(+), 163 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 0c3cfe4..e689212 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -62,13 +62,15 @@ struct ath10k_skb_cb {
 		bool discard;
 		bool no_ack;
 		u8 refcount;
+		struct sk_buff *txfrag;
+		struct sk_buff *msdu;
 	} __packed htt;
 
 	struct {
 		u8 credits_used;
 	} __packed htc;
 
-	/* 21 bytes left on 64bit arch */
+	/* 5 bytes left on 64bit arch */
 } __packed;
 
 static inline struct ath10k_skb_cb *ATH10K_SKB_CB(struct sk_buff *skb)
diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
index 8f85a42..bfb9c23 100644
--- a/drivers/net/wireless/ath/ath10k/htt.h
+++ b/drivers/net/wireless/ath/ath10k/htt.h
@@ -1109,18 +1109,6 @@ struct htt_rx_info {
 	bool fcs_err;
 };
 
-struct htt_tx_info {
-	struct htt_struct *htt;
-
-	u16 msdu_id;
-
-	atomic_t is_used;
-
-	struct sk_buff *txdesc;
-	struct sk_buff *txfrag; /* not used for mgmt tx */
-	struct sk_buff *msdu;
-};
-
 struct htt_struct {
 	struct ath10k *ar;
 
@@ -1206,9 +1194,10 @@ struct htt_struct {
 		int htc_err_cnt;
 	} stats;
 
-#define HTT_TX_INFO_POOL_SIZE 512 /* FIXME: find proper value? */
-	struct htt_tx_info txi_pool[HTT_TX_INFO_POOL_SIZE];
-	atomic_t num_used_txi;
+#define HTT_MAX_NUM_PENDING_TX 512 /* FIXME: find proper value? */
+	spinlock_t tx_lock;
+	struct sk_buff *pending_tx[HTT_MAX_NUM_PENDING_TX];
+	DECLARE_BITMAP(used_msdu_ids, HTT_MAX_NUM_PENDING_TX);
 	wait_queue_head_t empty_tx_wq;
 
 	/* set if host-fw communication goes haywire
@@ -1259,7 +1248,7 @@ struct htt_rx_desc {
  * changing conditions.
  * Hence, this queue depth threshold spec is mostly just a formality.
  */
-#define HTT_MAX_SEND_QUEUE_DEPTH (HTT_TX_INFO_POOL_SIZE)
+#define HTT_MAX_SEND_QUEUE_DEPTH (HTT_MAX_NUM_PENDING_TX)
 
 /*
  * FIX THIS
@@ -1292,11 +1281,9 @@ void ath10k_htt_t2h_msg_handler(void *context, struct sk_buff *skb);
 int ath10k_htt_h2t_ver_req_msg(struct htt_struct *htt);
 int ath10k_htt_send_rx_ring_cfg_ll(struct htt_struct *htt);
 
-struct htt_tx_info *ath10k_htt_tx_info_alloc(struct htt_struct *htt);
-void ath10k_htt_tx_info_free(struct htt_struct *htt, struct htt_tx_info *pkt);
-void ath10k_htt_tx_info_unref(struct htt_struct *htt, struct htt_tx_info *pkt,
-		       struct sk_buff *skb);
-struct htt_tx_info *ath10k_htt_tx_info_lookup(struct htt_struct *htt, u16 msdu_id);
+int ath10k_htt_tx_alloc_msdu_id(struct htt_struct *htt);
+void ath10k_htt_tx_free_msdu_id(struct htt_struct *htt, u16 msdu_id);
+
 void htt_t2h_stats_handler(struct htt_struct *htt, u8 *stats_msg_buf);
 int ath10k_htt_mgmt_tx(struct htt_struct *htt, struct sk_buff *);
 int ath10k_htt_tx(struct htt_struct *htt, struct sk_buff *);
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index b20c386..910b2e7 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -21,57 +21,35 @@
 #include "txrx.h"
 #include "debug.h"
 
-static void ath10k_htt_tx_info_pool_setup(struct htt_struct *htt)
+int ath10k_htt_tx_alloc_msdu_id(struct htt_struct *htt)
 {
-	int i;
+	int msdu_id;
 
-	memset(htt->txi_pool, 0, sizeof(htt->txi_pool));
-	for (i = 0; i < ARRAY_SIZE(htt->txi_pool); i++) {
-		htt->txi_pool[i].msdu_id = i;
-		htt->txi_pool[i].htt = htt;
-	}
-}
-
-struct htt_tx_info *ath10k_htt_tx_info_alloc(struct htt_struct *htt)
-{
-	struct htt_tx_info *txi = NULL;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(htt->txi_pool); i++) {
-		if (atomic_xchg(&htt->txi_pool[i].is_used, 1) == 0) {
-			txi = &htt->txi_pool[i];
-			break;
-		}
-	}
-
-	if (txi) {
-		ath10k_dbg(ATH10K_DBG_HTT, "htt txi alloc idx %d\n",
-			   txi->msdu_id);
+	lockdep_assert_held(&htt->tx_lock);
 
-		atomic_inc(&htt->num_used_txi);
-	}
+	msdu_id = find_first_zero_bit(htt->used_msdu_ids, HTT_MAX_NUM_PENDING_TX);
+	if (msdu_id == HTT_MAX_NUM_PENDING_TX)
+		return -ENOMEM;
 
-	return txi;
+	ath10k_dbg(ATH10K_DBG_HTT, "htt tx alloc msdu_id %d\n", msdu_id);
+	__set_bit(msdu_id, htt->used_msdu_ids);
+	return msdu_id;
 }
 
-void ath10k_htt_tx_info_free(struct htt_struct *htt, struct htt_tx_info *txi)
+void ath10k_htt_tx_free_msdu_id(struct htt_struct *htt, u16 msdu_id)
 {
-	int pending;
+	lockdep_assert_held(&htt->tx_lock);
 
-	ath10k_dbg(ATH10K_DBG_HTT, "htt txi free idx %d\n",
-		   txi->msdu_id);
+	if (!test_bit(msdu_id, htt->used_msdu_ids))
+		ath10k_warn("trying to free unallocated msdu_id %d\n", msdu_id);
 
-	WARN_ON(atomic_xchg(&txi->is_used, 0) == 0);
-
-	pending = atomic_sub_return(1, &htt->num_used_txi);
-	WARN_ON(pending < 0);
-	if (pending == 0)
-		wake_up(&htt->empty_tx_wq);
+	ath10k_dbg(ATH10K_DBG_HTT, "htt tx free msdu_id %hu\n", msdu_id);
+	__clear_bit(msdu_id, htt->used_msdu_ids);
 }
 
 void ath10k_htt_tx_attach(struct htt_struct *htt)
 {
-	ath10k_htt_tx_info_pool_setup(htt);
+	spin_lock_init(&htt->tx_lock);
 	init_waitqueue_head(&htt->empty_tx_wq);
 }
 
@@ -80,36 +58,16 @@ void ath10k_htt_tx_detach(struct htt_struct *htt)
 	return;
 }
 
-struct htt_tx_info *ath10k_htt_tx_info_lookup(struct htt_struct *htt,
-					      u16 msdu_id)
-{
-	if (WARN_ON(msdu_id >= ARRAY_SIZE(htt->txi_pool)))
-		return NULL;
-
-	if (WARN_ON(atomic_read(&htt->txi_pool[msdu_id].is_used) == 0))
-		return NULL;
-
-	return &htt->txi_pool[msdu_id];
-}
-
 void ath10k_htt_htc_tx_complete(void *context, struct sk_buff *skb)
 {
 	struct ath10k_skb_cb *skb_cb = ATH10K_SKB_CB(skb);
 	struct htt_struct *htt = (struct htt_struct *)context;
-	struct htt_tx_info *txi;
 
 	if (skb_cb->htt.is_conf) {
 		dev_kfree_skb_any(skb);
 		return;
 	}
 
-	txi = ath10k_htt_tx_info_lookup(htt, skb_cb->htt.msdu_id);
-	if (!txi) {
-		ath10k_warn("tx completion failure, wrong htt msdu_id %d\n",
-			    skb_cb->htt.msdu_id);
-		return;
-	}
-
 	if (skb_cb->is_aborted) {
 		skb_cb->htt.discard = true;
 
@@ -121,7 +79,7 @@ void ath10k_htt_htc_tx_complete(void *context, struct sk_buff *skb)
 			skb_cb->htt.refcount = 1;
 	}
 
-	ath10k_txrx_tx_unref(htt, txi);
+	ath10k_txrx_tx_unref(htt, skb);
 }
 
 int ath10k_htt_h2t_ver_req_msg(struct htt_struct *htt)
@@ -240,46 +198,52 @@ int ath10k_htt_mgmt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 {
 	struct device *dev = htt->ar->dev;
 	struct ath10k_skb_cb *skb_cb;
-	struct htt_tx_info *txi;
+	struct sk_buff *txdesc = NULL;
 	struct htt_cmd *cmd;
 	u8 vdev_id = ATH10K_SKB_CB(msdu)->htt.vdev_id;
 	int len = 0;
+	int msdu_id = -1;
 	int res;
 
-	txi = ath10k_htt_tx_info_alloc(htt);
-	if (!txi)
-		return -ENOMEM;
-
 	len += sizeof(cmd->hdr);
 	len += sizeof(cmd->mgmt_tx);
 
-	txi->txdesc = ath10k_htc_alloc_skb(len);
-	txi->msdu = msdu;
-
-	if (!txi->txdesc) {
+	txdesc = ath10k_htc_alloc_skb(len);
+	if (!txdesc) {
 		res = -ENOMEM;
 		goto err;
 	}
 
+	spin_lock_bh(&htt->tx_lock);
+	msdu_id = ath10k_htt_tx_alloc_msdu_id(htt);
+	if (msdu_id < 0) {
+		spin_unlock_bh(&htt->tx_lock);
+		res = msdu_id;
+		goto err;
+	}
+	htt->pending_tx[msdu_id] = txdesc;
+	spin_unlock_bh(&htt->tx_lock);
+
 	res = ath10k_skb_map(dev, msdu);
 	if (res)
 		goto err;
 
-	skb_put(txi->txdesc, len);
-	cmd = (struct htt_cmd *)txi->txdesc->data;
+	skb_put(txdesc, len);
+	cmd = (struct htt_cmd *)txdesc->data;
 	cmd->hdr.msg_type         = HTT_H2T_MSG_TYPE_MGMT_TX;
 	cmd->mgmt_tx.msdu_paddr = __cpu_to_le32(ATH10K_SKB_CB(msdu)->paddr);
 	cmd->mgmt_tx.len        = __cpu_to_le32(msdu->len);
-	cmd->mgmt_tx.desc_id    = __cpu_to_le32(txi->msdu_id);
+	cmd->mgmt_tx.desc_id    = __cpu_to_le32(msdu_id);
 	cmd->mgmt_tx.vdev_id    = __cpu_to_le32(vdev_id);
 	memcpy(cmd->mgmt_tx.hdr, msdu->data,
 	       min((int)msdu->len, HTT_MGMT_FRM_HDR_DOWNLOAD_LEN));
 
-	skb_cb = ATH10K_SKB_CB(txi->txdesc);
-	skb_cb->htt.msdu_id = txi->msdu_id;
+	skb_cb = ATH10K_SKB_CB(txdesc);
+	skb_cb->htt.msdu_id = msdu_id;
 	skb_cb->htt.refcount = 2;
+	skb_cb->htt.msdu = msdu;
 
-	res = ath10k_htc_send(htt->htc_target, htt->ep_id, txi->txdesc);
+	res = ath10k_htc_send(htt->htc_target, htt->ep_id, txdesc);
 	if (res)
 		goto err;
 
@@ -288,10 +252,14 @@ int ath10k_htt_mgmt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 err:
 	ath10k_skb_unmap(dev, msdu);
 
-	if (txi->txdesc)
-		dev_kfree_skb_any(txi->txdesc);
-
-	ath10k_htt_tx_info_free(htt, txi);
+	if (txdesc)
+		dev_kfree_skb_any(txdesc);
+	if (msdu_id >= 0) {
+		spin_lock_bh(&htt->tx_lock);
+		htt->pending_tx[msdu_id] = NULL;
+		ath10k_htt_tx_free_msdu_id(htt, msdu_id);
+		spin_unlock_bh(&htt->tx_lock);
+	}
 	return res;
 }
 
@@ -300,13 +268,15 @@ int ath10k_htt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 	struct device *dev = htt->ar->dev;
 	struct htt_cmd *cmd;
 	struct htt_data_tx_desc_frag *tx_frags;
-	struct htt_tx_info *txi;
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)msdu->data;
 	struct ath10k_skb_cb *skb_cb;
+	struct sk_buff *txdesc = NULL;
+	struct sk_buff *txfrag = NULL;
 	u8 vdev_id = ATH10K_SKB_CB(msdu)->htt.vdev_id;
 	u8 tid;
 	int prefetch_len, desc_len, frag_len;
 	dma_addr_t frags_paddr;
+	int msdu_id = -1;
 	int res;
 	u8 flags0;
 	u16 flags1;
@@ -314,61 +284,63 @@ int ath10k_htt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 	prefetch_len = min(htt->prefetch_len, msdu->len);
 	prefetch_len = roundup(prefetch_len, 4);
 
-	txi = ath10k_htt_tx_info_alloc(htt);
-	if (!txi)
-		return -ENOMEM;
-
 	desc_len = sizeof(cmd->hdr) + sizeof(cmd->data_tx) + prefetch_len;
 	frag_len = sizeof(*tx_frags) * 2;
 
-	txi->txdesc = ath10k_htc_alloc_skb(desc_len);
-	if (!txi->txdesc) {
+	txdesc = ath10k_htc_alloc_skb(desc_len);
+	if (!txdesc) {
 		res = -ENOMEM;
 		goto err;
 	}
 
-	txi->txfrag = dev_alloc_skb(frag_len);
-	if (!txi->txfrag) {
+	txfrag = dev_alloc_skb(frag_len);
+	if (!txfrag) {
 		res = -ENOMEM;
 		goto err;
 	}
 
-	txi->msdu = msdu;
-
-	if ((unsigned long)txi->txdesc->data & 0x3) {
+	if ((unsigned long)txdesc->data & 0x3) {
 		ath10k_warn("htt alignment check failed. dropping packet.\n");
 		res = -EIO;
 		goto err;
 	}
 
+	spin_lock_bh(&htt->tx_lock);
+	msdu_id = ath10k_htt_tx_alloc_msdu_id(htt);
+	if (msdu_id < 0) {
+		spin_unlock_bh(&htt->tx_lock);
+		res = msdu_id;
+		goto err;
+	}
+	htt->pending_tx[msdu_id] = txdesc;
+	spin_unlock_bh(&htt->tx_lock);
+
 	res = ath10k_skb_map(dev, msdu);
 	if (res)
 		goto err;
 
 	/* tx fragment list must be terminated with zero-entry */
-	skb_put(txi->txfrag, frag_len);
-	tx_frags = (struct htt_data_tx_desc_frag *)txi->txfrag->data;
+	skb_put(txfrag, frag_len);
+	tx_frags = (struct htt_data_tx_desc_frag *)txfrag->data;
 	tx_frags[0].paddr = __cpu_to_le32(ATH10K_SKB_CB(msdu)->paddr);
 	tx_frags[0].len   = __cpu_to_le32(msdu->len);
 	tx_frags[1].paddr = __cpu_to_le32(0);
 	tx_frags[1].len   = __cpu_to_le32(0);
 
-	res = ath10k_skb_map(dev, txi->txfrag);
+	res = ath10k_skb_map(dev, txfrag);
 	if (res)
 		goto err;
 
 	ath10k_dbg(ATH10K_DBG_HTT, "txfrag 0x%llx msdu 0x%llx\n",
-		   (unsigned long long) ATH10K_SKB_CB(txi->txfrag)->paddr,
-		   (unsigned long long) ATH10K_SKB_CB(txi->msdu)->paddr);
+		   (unsigned long long) ATH10K_SKB_CB(txfrag)->paddr,
+		   (unsigned long long) ATH10K_SKB_CB(msdu)->paddr);
 	ath10k_dbg_dump(ATH10K_DBG_HTT, NULL, "txfrag: ",
-			txi->txfrag->data,
-			frag_len);
+			txfrag->data, frag_len);
 	ath10k_dbg_dump(ATH10K_DBG_HTT, NULL, "msdu: ",
-			txi->msdu->data,
-			txi->msdu->len);
+			msdu->data, msdu->len);
 
-	skb_put(txi->txdesc, desc_len);
-	cmd = (struct htt_cmd *)txi->txdesc->data;
+	skb_put(txdesc, desc_len);
+	cmd = (struct htt_cmd *)txdesc->data;
 	memset(cmd, 0, desc_len);
 
 	tid = HTT_DATA_TX_EXT_TID_NON_QOS_MCAST_BCAST;
@@ -391,35 +363,42 @@ int ath10k_htt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 	flags1 |= SM((u16)vdev_id, HTT_DATA_TX_DESC_FLAGS1_VDEV_ID);
 	flags1 |= SM((u16)tid, HTT_DATA_TX_DESC_FLAGS1_EXT_TID);
 
-	frags_paddr = ATH10K_SKB_CB(txi->txfrag)->paddr;
+	frags_paddr = ATH10K_SKB_CB(txfrag)->paddr;
 
 	cmd->hdr.msg_type        = HTT_H2T_MSG_TYPE_TX_FRM;
 	cmd->data_tx.flags0      = flags0;
 	cmd->data_tx.flags1      = __cpu_to_le16(flags1);
 	cmd->data_tx.len         = __cpu_to_le16(msdu->len);
-	cmd->data_tx.id          = __cpu_to_le16(txi->msdu_id);
+	cmd->data_tx.id          = __cpu_to_le16(msdu_id);
 	cmd->data_tx.frags_paddr = __cpu_to_le32(frags_paddr);
 	cmd->data_tx.peerid      = __cpu_to_le32(HTT_INVALID_PEERID);
 
 	memcpy(cmd->data_tx.prefetch, msdu->data, prefetch_len);
 
-	skb_cb = ATH10K_SKB_CB(txi->txdesc);
-	skb_cb->htt.msdu_id = txi->msdu_id;
+	skb_cb = ATH10K_SKB_CB(txdesc);
+	skb_cb->htt.msdu_id = msdu_id;
 	skb_cb->htt.refcount = 2;
+	skb_cb->htt.txfrag = txfrag;
+	skb_cb->htt.msdu = msdu;
 
-	res = ath10k_htc_send(htt->htc_target, htt->ep_id, txi->txdesc);
+	res = ath10k_htc_send(htt->htc_target, htt->ep_id, txdesc);
 	if (res)
 		goto err;
 
 	return 0;
 err:
-	if (txi->txfrag)
-		ath10k_skb_unmap(dev, txi->txfrag);
-	if (txi->txdesc)
-		dev_kfree_skb_any(txi->txdesc);
-	if (txi->txfrag)
-		dev_kfree_skb_any(txi->txfrag);
-	ath10k_htt_tx_info_free(htt, txi);
+	if (txfrag)
+		ath10k_skb_unmap(dev, txfrag);
+	if (txdesc)
+		dev_kfree_skb_any(txdesc);
+	if (txfrag)
+		dev_kfree_skb_any(txfrag);
+	if (msdu_id >= 0) {
+		spin_lock_bh(&htt->tx_lock);
+		htt->pending_tx[msdu_id] = NULL;
+		ath10k_htt_tx_free_msdu_id(htt, msdu_id);
+		spin_unlock_bh(&htt->tx_lock);
+	}
 	ath10k_skb_unmap(dev, msdu);
 	return res;
 }
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 1d2e42e..1c9c048 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2376,12 +2376,15 @@ static void ath10k_flush(struct ieee80211_hw *hw, u32 queues, bool drop)
 	if (drop)
 		return;
 
-	ret = wait_event_timeout(ar->htt->empty_tx_wq,
-				 atomic_read(&ar->htt->num_used_txi) == 0,
-				 ATH10K_FLUSH_TIMEOUT_HZ);
+	ret = wait_event_timeout(ar->htt->empty_tx_wq, ({
+			bool empty;
+			spin_lock_bh(&ar->htt->tx_lock);
+			empty = bitmap_empty(ar->htt->used_msdu_ids, HTT_MAX_NUM_PENDING_TX);
+			spin_unlock_bh(&ar->htt->tx_lock);
+			(empty);
+		}), ATH10K_FLUSH_TIMEOUT_HZ);
 	if (ret <= 0)
-		ath10k_warn("tx not flushed (%d frames still pending)\n",
-			    atomic_read(&ar->htt->num_used_txi));
+		ath10k_warn("tx not flushed\n");
 }
 
 /* TODO: Implement this function properly
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index e69756f..d3a19b4 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -44,72 +44,80 @@ out:
 	spin_unlock_bh(&ar->data_lock);
 }
 
-void ath10k_txrx_tx_unref(struct htt_struct *htt, struct htt_tx_info *txi)
+void ath10k_txrx_tx_unref(struct htt_struct *htt, struct sk_buff *txdesc)
 {
 	struct device *dev = htt->ar->dev;
 	struct ieee80211_tx_info *info;
+	struct sk_buff *txfrag = ATH10K_SKB_CB(txdesc)->htt.txfrag;
+	struct sk_buff *msdu = ATH10K_SKB_CB(txdesc)->htt.msdu;
 	int ret;
 
-	if (ATH10K_SKB_CB(txi->txdesc)->htt.refcount == 0)
+	if (ATH10K_SKB_CB(txdesc)->htt.refcount == 0)
 		return;
 
-	if (--ATH10K_SKB_CB(txi->txdesc)->htt.refcount > 0)
+	if (--ATH10K_SKB_CB(txdesc)->htt.refcount > 0)
 		return;
 
-	if (txi->txfrag) {
-		ret = ath10k_skb_unmap(dev, txi->txfrag);
+	if (txfrag) {
+		ret = ath10k_skb_unmap(dev, txfrag);
 		if (ret)
 			ath10k_warn("txfrag unmap failed (%d)\n", ret);
 
-		dev_kfree_skb_any(txi->txfrag);
+		dev_kfree_skb_any(txfrag);
 	}
 
-	ret = ath10k_skb_unmap(dev, txi->msdu);
+	ret = ath10k_skb_unmap(dev, msdu);
 	if (ret)
 		ath10k_warn("data skb unmap failed (%d)\n", ret);
 
-	ath10k_report_offchan_tx(htt->ar, txi->msdu);
+	ath10k_report_offchan_tx(htt->ar, msdu);
 
-	info = IEEE80211_SKB_CB(txi->msdu);
+	info = IEEE80211_SKB_CB(msdu);
 	memset(&info->status, 0, sizeof(info->status));
 
-	if (ATH10K_SKB_CB(txi->txdesc)->htt.discard) {
-		ieee80211_free_txskb(htt->ar->hw, txi->msdu);
+	if (ATH10K_SKB_CB(txdesc)->htt.discard) {
+		ieee80211_free_txskb(htt->ar->hw, msdu);
 		goto exit;
 	}
 
 	if (!(info->flags & IEEE80211_TX_CTL_NO_ACK))
 		info->flags |= IEEE80211_TX_STAT_ACK;
 
-	if (ATH10K_SKB_CB(txi->txdesc)->htt.no_ack)
+	if (ATH10K_SKB_CB(txdesc)->htt.no_ack)
 		info->flags &= ~IEEE80211_TX_STAT_ACK;
 
-	ieee80211_tx_status_irqsafe(htt->ar->hw, txi->msdu);
+	ieee80211_tx_status_irqsafe(htt->ar->hw, msdu);
 	/* we do not own the msdu anymore */
 
 exit:
-	dev_kfree_skb_any(txi->txdesc);
-	ath10k_htt_tx_info_free(htt, txi);
+	spin_lock_bh(&htt->tx_lock);
+	ath10k_htt_tx_free_msdu_id(htt, ATH10K_SKB_CB(txdesc)->htt.msdu_id);
+	if (bitmap_empty(htt->used_msdu_ids, HTT_MAX_NUM_PENDING_TX))
+		wake_up(&htt->empty_tx_wq);
+	spin_unlock_bh(&htt->tx_lock);
+
+	dev_kfree_skb_any(txdesc);
 }
 
 void ath10k_txrx_tx_completed(struct htt_struct *htt,
 			      const struct htt_tx_done *tx_done)
 {
-	struct htt_tx_info *txi;
+	struct sk_buff *txdesc;
 
 	ath10k_dbg(ATH10K_DBG_HTT, "htt tx completion msdu_id %u discard %d no_ack %d\n",
 		   tx_done->msdu_id, !!tx_done->discard, !!tx_done->no_ack);
 
-	txi = ath10k_htt_tx_info_lookup(htt, tx_done->msdu_id);
-	if (!txi) {
-		ath10k_dbg(ATH10K_DBG_HTT, "htt txi lookup fail\n");
+	if (tx_done->msdu_id >= ARRAY_SIZE(htt->pending_tx)) {
+		ath10k_warn("warning: msdu_id %d too big, ignoring\n", tx_done->msdu_id);
 		return;
 	}
 
-	ATH10K_SKB_CB(txi->txdesc)->htt.discard = tx_done->discard;
-	ATH10K_SKB_CB(txi->txdesc)->htt.no_ack = tx_done->no_ack;
+	txdesc = htt->pending_tx[tx_done->msdu_id];
+
+	ATH10K_SKB_CB(txdesc)->htt.discard = tx_done->discard;
+	ATH10K_SKB_CB(txdesc)->htt.no_ack = tx_done->no_ack;
 
-	ath10k_txrx_tx_unref(htt, txi);
+	ath10k_txrx_tx_unref(htt, txdesc);
 }
 
 static u8 rx_legacy_rate_idx[] = {
diff --git a/drivers/net/wireless/ath/ath10k/txrx.h b/drivers/net/wireless/ath/ath10k/txrx.h
index 7a1bbfe..05bf9d8 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.h
+++ b/drivers/net/wireless/ath/ath10k/txrx.h
@@ -19,7 +19,7 @@
 
 #include "htt.h"
 
-void ath10k_txrx_tx_unref(struct htt_struct *htt, struct htt_tx_info *txi);
+void ath10k_txrx_tx_unref(struct htt_struct *htt, struct sk_buff *txdesc);
 void ath10k_txrx_tx_completed(struct htt_struct *htt,
 			      const struct htt_tx_done *tx_done);
 void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info);
-- 
1.7.9.5

[ath9k-devel] [PATCH 0/2] ath10k: kill off htt_tx_info wrapper

[Michal Kazior]

This patchset merges the htt_tx_info into
ath10k_skb_cb. This depends upon my 'htt tx
cleanup' patchset.

Michal Kazior (2):
  ath10k: simplify txi lifespan
  ath10k: merge htt_tx_info into ath10k_skb_cb

 drivers/net/wireless/ath/ath10k/core.h   |    7 +-
 drivers/net/wireless/ath/ath10k/htt.h    |   30 +---
 drivers/net/wireless/ath/ath10k/htt_tx.c |  263 ++++++++++++------------------
 drivers/net/wireless/ath/ath10k/mac.c    |   13 +-
 drivers/net/wireless/ath/ath10k/txrx.c   |   65 +++++---
 drivers/net/wireless/ath/ath10k/txrx.h   |    1 +
 6 files changed, 169 insertions(+), 210 deletions(-)

-- 
1.7.9.5

[ath9k-devel] [PATCH 1/2] ath10k: simplify txi lifespan

[Michal Kazior]

Up until now we were using two booleans to keep
track of htt and htc tx completions. These can
come in no specific order.

Keep in mind that it was safe to access those
without any locks since tx completion processing
is done one endpoint at a time (that's the current
PCI backend implementation).

The new approach uses refcount. Since we need 2
completions we setup refcount=2 for mgmt and
data tx. This also depends on the tx completion
behaviour.

If the PCI tx completion scheme ever changes we'll
need locking to synchronize tx completions for
htt.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h   |    5 ++-
 drivers/net/wireless/ath/ath10k/htt.h    |    2 -
 drivers/net/wireless/ath/ath10k/htt_tx.c |   60 ++++++------------------------
 drivers/net/wireless/ath/ath10k/txrx.c   |   41 +++++++++++++-------
 drivers/net/wireless/ath/ath10k/txrx.h   |    1 +
 5 files changed, 45 insertions(+), 64 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 4e19264..bb1eef1 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -59,13 +59,16 @@ struct ath10k_skb_cb {
 		u8 tid;
 		bool is_offchan;
 		bool is_conf;
+		bool discard;
+		bool no_ack;
+		u8 refcount;
 	} __packed htt;
 
 	struct {
 		u8 credits_used;
 	} __packed htc;
 
-	/* 23 bytes left on 64bit arch */
+	/* 20 bytes left on 64bit arch */
 } __packed;
 
 static inline struct ath10k_skb_cb *ATH10K_SKB_CB(struct sk_buff *skb)
diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
index 38762ca..dbd2912 100644
--- a/drivers/net/wireless/ath/ath10k/htt.h
+++ b/drivers/net/wireless/ath/ath10k/htt.h
@@ -1115,8 +1115,6 @@ struct htt_tx_info {
 	u16 msdu_id;
 
 	atomic_t is_used;
-	bool htc_tx_completed;
-	bool htt_tx_completed;
 
 	struct sk_buff *txdesc;
 	struct sk_buff *txfrag; /* not used for mgmt tx */
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index 3bb529e..6f46da3 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -18,6 +18,7 @@
 #include <linux/etherdevice.h>
 #include "htt.h"
 #include "mac.h"
+#include "txrx.h"
 #include "debug.h"
 
 static void ath10k_htt_tx_info_pool_setup(struct htt_struct *htt)
@@ -47,9 +48,6 @@ struct htt_tx_info *ath10k_htt_tx_info_alloc(struct htt_struct *htt)
 		ath10k_dbg(ATH10K_DBG_HTT, "htt txi alloc idx %d\n",
 			   txi->msdu_id);
 
-		txi->htc_tx_completed = false;
-		txi->htt_tx_completed = false;
-
 		atomic_inc(&htt->num_used_txi);
 	}
 
@@ -71,24 +69,6 @@ void ath10k_htt_tx_info_free(struct htt_struct *htt, struct htt_tx_info *txi)
 		wake_up(&htt->empty_tx_wq);
 }
 
-void ath10k_htt_tx_info_unref(struct htt_struct *htt, struct htt_tx_info *txi,
-			      struct sk_buff *skb)
-{
-	/* FIXME: we have to carefully synchronize completion of htt
-	 *        messages because those come in asynchronously
-	 *        and not necessarily in order.
-	 *        since htc_packet structure is no more this could perhaps
-	 *        be reworked in a more clean manner? */
-	if (!txi->htc_tx_completed)
-		return;
-
-	if (!txi->htt_tx_completed)
-		return;
-
-	dev_kfree_skb_any(skb);
-	ath10k_htt_tx_info_free(htt, txi);
-}
-
 void ath10k_htt_tx_attach(struct htt_struct *htt)
 {
 	ath10k_htt_tx_info_pool_setup(htt);
@@ -116,9 +96,7 @@ void ath10k_htt_htc_tx_complete(void *context, struct sk_buff *skb)
 {
 	struct ath10k_skb_cb *skb_cb = ATH10K_SKB_CB(skb);
 	struct htt_struct *htt = (struct htt_struct *)context;
-	struct device *dev = htt->ar->dev;
 	struct htt_tx_info *txi;
-	int ret;
 
 	if (skb_cb->htt.is_conf) {
 		dev_kfree_skb_any(skb);
@@ -132,34 +110,18 @@ void ath10k_htt_htc_tx_complete(void *context, struct sk_buff *skb)
 		return;
 	}
 
-	txi->htc_tx_completed = true;
-
 	if (skb_cb->is_aborted) {
-		/*
-		 * if a packet gets cancelled we need to make sure
-		 * to free skbs since htt mgmt tx completion indication
-		 * may have not came in yet
-		 */
-		if (!txi->htt_tx_completed) {
-			txi->htt_tx_completed = true;
-
-			if (txi->txfrag) {
-				ret = ath10k_skb_unmap(dev, txi->txfrag);
-				if (ret)
-					ath10k_warn("txfrag unmap failed (%d)\n", ret);
-
-				dev_kfree_skb_any(txi->txfrag);
-			}
-
-			ret = ath10k_skb_unmap(dev, txi->msdu);
-			if (ret)
-				ath10k_warn("data skb unmap failed (%d)\n", ret);
-
-			ieee80211_free_txskb(htt->ar->hw, txi->msdu);
-		}
+		skb_cb->htt.discard = true;
+
+		/* if the skbuff is aborted we need to make sure we'll free up
+		 * the tx resources, we can't simply run tx_unref() 2 times
+		 * because if htt tx completion came in earlier we'd access
+		 * unallocated memory */
+		if (skb_cb->htt.refcount > 1)
+			skb_cb->htt.refcount = 1;
 	}
 
-	ath10k_htt_tx_info_unref(htt, txi, skb);
+	ath10k_txrx_tx_unref(htt, txi);
 }
 
 int ath10k_htt_h2t_ver_req_msg(struct htt_struct *htt)
@@ -315,6 +277,7 @@ int ath10k_htt_mgmt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 
 	skb_cb = ATH10K_SKB_CB(txi->txdesc);
 	skb_cb->htt.msdu_id = txi->msdu_id;
+	skb_cb->htt.refcount = 2;
 
 	res = ath10k_htc_send(htt->htc, htt->eid, txi->txdesc);
 	if (res)
@@ -436,6 +399,7 @@ int ath10k_htt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 
 	skb_cb = ATH10K_SKB_CB(txi->txdesc);
 	skb_cb->htt.msdu_id = txi->msdu_id;
+	skb_cb->htt.refcount = 2;
 
 	res = ath10k_htc_send(htt->htc, htt->eid, txi->txdesc);
 	if (res)
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index e1317e3..9c521d6 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -44,22 +44,17 @@ out:
 	spin_unlock_bh(&ar->data_lock);
 }
 
-void ath10k_txrx_tx_completed(struct htt_struct *htt,
-			      const struct htt_tx_done *tx_done)
+void ath10k_txrx_tx_unref(struct htt_struct *htt, struct htt_tx_info *txi)
 {
 	struct device *dev = htt->ar->dev;
 	struct ieee80211_tx_info *info;
-	struct htt_tx_info *txi;
 	int ret;
 
-	ath10k_dbg(ATH10K_DBG_HTT, "htt tx completion msdu_id %u discard %d no_ack %d\n",
-		   tx_done->msdu_id, !!tx_done->discard, !!tx_done->no_ack);
+	if (ATH10K_SKB_CB(txi->txdesc)->htt.refcount == 0)
+		return;
 
-	txi = ath10k_htt_tx_info_lookup(htt, tx_done->msdu_id);
-	if (!txi) {
-		ath10k_dbg(ATH10K_DBG_HTT, "htt txi lookup fail\n");
+	if (--ATH10K_SKB_CB(txi->txdesc)->htt.refcount > 0)
 		return;
-	}
 
 	if (txi->txfrag) {
 		ret = ath10k_skb_unmap(dev, txi->txfrag);
@@ -78,7 +73,7 @@ void ath10k_txrx_tx_completed(struct htt_struct *htt,
 	info = IEEE80211_SKB_CB(txi->msdu);
 	memset(&info->status, 0, sizeof(info->status));
 
-	if (tx_done->discard) {
+	if (ATH10K_SKB_CB(txi->txdesc)->htt.discard) {
 		ieee80211_free_txskb(htt->ar->hw, txi->msdu);
 		goto exit;
 	}
@@ -86,15 +81,35 @@ void ath10k_txrx_tx_completed(struct htt_struct *htt,
 	if (!(info->flags & IEEE80211_TX_CTL_NO_ACK))
 		info->flags |= IEEE80211_TX_STAT_ACK;
 
-	if (tx_done->no_ack)
+	if (ATH10K_SKB_CB(txi->txdesc)->htt.no_ack)
 		info->flags &= ~IEEE80211_TX_STAT_ACK;
 
 	ieee80211_tx_status(htt->ar->hw, txi->msdu);
 	/* we do not own the msdu anymore */
 
 exit:
-	txi->htt_tx_completed = true;
-	ath10k_htt_tx_info_unref(htt, txi, txi->txdesc);
+	dev_kfree_skb_any(txi->txdesc);
+	ath10k_htt_tx_info_free(htt, txi);
+}
+
+void ath10k_txrx_tx_completed(struct htt_struct *htt,
+			      const struct htt_tx_done *tx_done)
+{
+	struct htt_tx_info *txi;
+
+	ath10k_dbg(ATH10K_DBG_HTT, "htt tx completion msdu_id %u discard %d no_ack %d\n",
+		   tx_done->msdu_id, !!tx_done->discard, !!tx_done->no_ack);
+
+	txi = ath10k_htt_tx_info_lookup(htt, tx_done->msdu_id);
+	if (!txi) {
+		ath10k_dbg(ATH10K_DBG_HTT, "htt txi lookup fail\n");
+		return;
+	}
+
+	ATH10K_SKB_CB(txi->txdesc)->htt.discard = tx_done->discard;
+	ATH10K_SKB_CB(txi->txdesc)->htt.no_ack = tx_done->no_ack;
+
+	ath10k_txrx_tx_unref(htt, txi);
 }
 
 static u8 rx_legacy_rate_idx[] = {
diff --git a/drivers/net/wireless/ath/ath10k/txrx.h b/drivers/net/wireless/ath/ath10k/txrx.h
index c80a472..7a1bbfe 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.h
+++ b/drivers/net/wireless/ath/ath10k/txrx.h
@@ -19,6 +19,7 @@
 
 #include "htt.h"
 
+void ath10k_txrx_tx_unref(struct htt_struct *htt, struct htt_tx_info *txi);
 void ath10k_txrx_tx_completed(struct htt_struct *htt,
 			      const struct htt_tx_done *tx_done);
 void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info);
-- 
1.7.9.5

[ath9k-devel] [PATCH 1/2] ath10k: simplify txi lifespan

[Kalle Valo]

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

> Up until now we were using two booleans to keep
> track of htt and htc tx completions. These can
> come in no specific order.
>
> Keep in mind that it was safe to access those
> without any locks since tx completion processing
> is done one endpoint at a time (that's the current
> PCI backend implementation).
>
> The new approach uses refcount. Since we need 2
> completions we setup refcount=2 for mgmt and
> data tx. This also depends on the tx completion
> behaviour.
>
> If the PCI tx completion scheme ever changes we'll
> need locking to synchronize tx completions for
> htt.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

Few comments, can you send a followup patch to address those separately,
please?

[...]

>  int ath10k_htt_h2t_ver_req_msg(struct htt_struct *htt)
> @@ -315,6 +277,7 @@ int ath10k_htt_mgmt_tx(struct htt_struct *htt, struct sk_buff *msdu)
>  
>  	skb_cb = ATH10K_SKB_CB(txi->txdesc);
>  	skb_cb->htt.msdu_id = txi->msdu_id;
> +	skb_cb->htt.refcount = 2;
>  
>  	res = ath10k_htc_send(htt->htc, htt->eid, txi->txdesc);
>  	if (res)
> @@ -436,6 +399,7 @@ int ath10k_htt_tx(struct htt_struct *htt, struct sk_buff *msdu)
>  
>  	skb_cb = ATH10K_SKB_CB(txi->txdesc);
>  	skb_cb->htt.msdu_id = txi->msdu_id;
> +	skb_cb->htt.refcount = 2;

It would be good to have a comment in the code why refcount needs to be
2. I know you had it in the commit log already, but people go through
commit logs only when they are really desparate :)


[...]

> --- a/drivers/net/wireless/ath/ath10k/txrx.c
> +++ b/drivers/net/wireless/ath/ath10k/txrx.c
> @@ -44,22 +44,17 @@ out:
>  	spin_unlock_bh(&ar->data_lock);
>  }
>  
> -void ath10k_txrx_tx_completed(struct htt_struct *htt,
> -			      const struct htt_tx_done *tx_done)
> +void ath10k_txrx_tx_unref(struct htt_struct *htt, struct htt_tx_info *txi)
>  {
>  	struct device *dev = htt->ar->dev;
>  	struct ieee80211_tx_info *info;
> -	struct htt_tx_info *txi;
>  	int ret;
>  
> -	ath10k_dbg(ATH10K_DBG_HTT, "htt tx completion msdu_id %u discard %d no_ack %d\n",
> -		   tx_done->msdu_id, !!tx_done->discard, !!tx_done->no_ack);
> +	if (ATH10K_SKB_CB(txi->txdesc)->htt.refcount == 0)
> +		return;
>  
> -	txi = ath10k_htt_tx_info_lookup(htt, tx_done->msdu_id);
> -	if (!txi) {
> -		ath10k_dbg(ATH10K_DBG_HTT, "htt txi lookup fail\n");
> +	if (--ATH10K_SKB_CB(txi->txdesc)->htt.refcount > 0)
>  		return;
> -	}

The 'if (--foo > 0)' is correct but IMHO bad style. It's easy to miss
the operator in front and that's why I would prefer:

foo--;

if (foo > 0)
   ...

-- 
Kalle Valo

[ath9k-devel] [PATCH 2/2] ath10k: merge htt_tx_info into ath10k_skb_cb

[Michal Kazior]

This eliminates the wrapper structure htt_tx_info.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h   |    4 +-
 drivers/net/wireless/ath/ath10k/htt.h    |   28 +---
 drivers/net/wireless/ath/ath10k/htt_tx.c |  205 ++++++++++++++----------------
 drivers/net/wireless/ath/ath10k/mac.c    |   13 +-
 drivers/net/wireless/ath/ath10k/txrx.c   |   52 ++++----
 drivers/net/wireless/ath/ath10k/txrx.h   |    2 +-
 6 files changed, 141 insertions(+), 163 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index bb1eef1..8648202 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -62,13 +62,15 @@ struct ath10k_skb_cb {
 		bool discard;
 		bool no_ack;
 		u8 refcount;
+		struct sk_buff *txfrag;
+		struct sk_buff *msdu;
 	} __packed htt;
 
 	struct {
 		u8 credits_used;
 	} __packed htc;
 
-	/* 20 bytes left on 64bit arch */
+	/* 4 bytes left on 64bit arch */
 } __packed;
 
 static inline struct ath10k_skb_cb *ATH10K_SKB_CB(struct sk_buff *skb)
diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
index dbd2912..c94f137 100644
--- a/drivers/net/wireless/ath/ath10k/htt.h
+++ b/drivers/net/wireless/ath/ath10k/htt.h
@@ -1109,18 +1109,6 @@ struct htt_rx_info {
 	bool fcs_err;
 };
 
-struct htt_tx_info {
-	struct htt_struct *htt;
-
-	u16 msdu_id;
-
-	atomic_t is_used;
-
-	struct sk_buff *txdesc;
-	struct sk_buff *txfrag; /* not used for mgmt tx */
-	struct sk_buff *msdu;
-};
-
 struct htt_struct {
 	struct ath10k *ar;
 	struct ath10k_htc *htc;
@@ -1205,9 +1193,10 @@ struct htt_struct {
 		int htc_err_cnt;
 	} stats;
 
-#define HTT_TX_INFO_POOL_SIZE 512 /* FIXME: find proper value? */
-	struct htt_tx_info txi_pool[HTT_TX_INFO_POOL_SIZE];
-	atomic_t num_used_txi;
+#define HTT_MAX_NUM_PENDING_TX 512 /* FIXME: find proper value? */
+	spinlock_t tx_lock;
+	struct sk_buff *pending_tx[HTT_MAX_NUM_PENDING_TX];
+	DECLARE_BITMAP(used_msdu_ids, HTT_MAX_NUM_PENDING_TX);
 	wait_queue_head_t empty_tx_wq;
 
 	/* set if host-fw communication goes haywire
@@ -1258,7 +1247,7 @@ struct htt_rx_desc {
  * changing conditions.
  * Hence, this queue depth threshold spec is mostly just a formality.
  */
-#define HTT_MAX_SEND_QUEUE_DEPTH (HTT_TX_INFO_POOL_SIZE)
+#define HTT_MAX_SEND_QUEUE_DEPTH (HTT_MAX_NUM_PENDING_TX)
 
 /*
  * FIX THIS
@@ -1291,11 +1280,8 @@ void ath10k_htt_t2h_msg_handler(void *context, struct sk_buff *skb);
 int ath10k_htt_h2t_ver_req_msg(struct htt_struct *htt);
 int ath10k_htt_send_rx_ring_cfg_ll(struct htt_struct *htt);
 
-struct htt_tx_info *ath10k_htt_tx_info_alloc(struct htt_struct *htt);
-void ath10k_htt_tx_info_free(struct htt_struct *htt, struct htt_tx_info *pkt);
-void ath10k_htt_tx_info_unref(struct htt_struct *htt, struct htt_tx_info *pkt,
-			      struct sk_buff *skb);
-struct htt_tx_info *ath10k_htt_tx_info_lookup(struct htt_struct *htt, u16 msdu_id);
+int ath10k_htt_tx_alloc_msdu_id(struct htt_struct *htt);
+void ath10k_htt_tx_free_msdu_id(struct htt_struct *htt, u16 msdu_id);
 int ath10k_htt_mgmt_tx(struct htt_struct *htt, struct sk_buff *);
 int ath10k_htt_tx(struct htt_struct *htt, struct sk_buff *);
 #endif
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index 6f46da3..5b3cad1 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -21,57 +21,35 @@
 #include "txrx.h"
 #include "debug.h"
 
-static void ath10k_htt_tx_info_pool_setup(struct htt_struct *htt)
+int ath10k_htt_tx_alloc_msdu_id(struct htt_struct *htt)
 {
-	int i;
+	int msdu_id;
 
-	memset(htt->txi_pool, 0, sizeof(htt->txi_pool));
-	for (i = 0; i < ARRAY_SIZE(htt->txi_pool); i++) {
-		htt->txi_pool[i].msdu_id = i;
-		htt->txi_pool[i].htt = htt;
-	}
-}
-
-struct htt_tx_info *ath10k_htt_tx_info_alloc(struct htt_struct *htt)
-{
-	struct htt_tx_info *txi = NULL;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(htt->txi_pool); i++) {
-		if (atomic_xchg(&htt->txi_pool[i].is_used, 1) == 0) {
-			txi = &htt->txi_pool[i];
-			break;
-		}
-	}
-
-	if (txi) {
-		ath10k_dbg(ATH10K_DBG_HTT, "htt txi alloc idx %d\n",
-			   txi->msdu_id);
+	lockdep_assert_held(&htt->tx_lock);
 
-		atomic_inc(&htt->num_used_txi);
-	}
+	msdu_id = find_first_zero_bit(htt->used_msdu_ids, HTT_MAX_NUM_PENDING_TX);
+	if (msdu_id == HTT_MAX_NUM_PENDING_TX)
+		return -ENOMEM;
 
-	return txi;
+	ath10k_dbg(ATH10K_DBG_HTT, "htt tx alloc msdu_id %d\n", msdu_id);
+	__set_bit(msdu_id, htt->used_msdu_ids);
+	return msdu_id;
 }
 
-void ath10k_htt_tx_info_free(struct htt_struct *htt, struct htt_tx_info *txi)
+void ath10k_htt_tx_free_msdu_id(struct htt_struct *htt, u16 msdu_id)
 {
-	int pending;
+	lockdep_assert_held(&htt->tx_lock);
 
-	ath10k_dbg(ATH10K_DBG_HTT, "htt txi free idx %d\n",
-		   txi->msdu_id);
+	if (!test_bit(msdu_id, htt->used_msdu_ids))
+		ath10k_warn("trying to free unallocated msdu_id %d\n", msdu_id);
 
-	WARN_ON(atomic_xchg(&txi->is_used, 0) == 0);
-
-	pending = atomic_sub_return(1, &htt->num_used_txi);
-	WARN_ON(pending < 0);
-	if (pending == 0)
-		wake_up(&htt->empty_tx_wq);
+	ath10k_dbg(ATH10K_DBG_HTT, "htt tx free msdu_id %hu\n", msdu_id);
+	__clear_bit(msdu_id, htt->used_msdu_ids);
 }
 
 void ath10k_htt_tx_attach(struct htt_struct *htt)
 {
-	ath10k_htt_tx_info_pool_setup(htt);
+	spin_lock_init(&htt->tx_lock);
 	init_waitqueue_head(&htt->empty_tx_wq);
 }
 
@@ -80,36 +58,16 @@ void ath10k_htt_tx_detach(struct htt_struct *htt)
 	return;
 }
 
-struct htt_tx_info *ath10k_htt_tx_info_lookup(struct htt_struct *htt,
-					      u16 msdu_id)
-{
-	if (WARN_ON(msdu_id >= ARRAY_SIZE(htt->txi_pool)))
-		return NULL;
-
-	if (WARN_ON(atomic_read(&htt->txi_pool[msdu_id].is_used) == 0))
-		return NULL;
-
-	return &htt->txi_pool[msdu_id];
-}
-
 void ath10k_htt_htc_tx_complete(void *context, struct sk_buff *skb)
 {
 	struct ath10k_skb_cb *skb_cb = ATH10K_SKB_CB(skb);
 	struct htt_struct *htt = (struct htt_struct *)context;
-	struct htt_tx_info *txi;
 
 	if (skb_cb->htt.is_conf) {
 		dev_kfree_skb_any(skb);
 		return;
 	}
 
-	txi = ath10k_htt_tx_info_lookup(htt, skb_cb->htt.msdu_id);
-	if (!txi) {
-		ath10k_warn("tx completion failure, wrong htt msdu_id %d\n",
-			    skb_cb->htt.msdu_id);
-		return;
-	}
-
 	if (skb_cb->is_aborted) {
 		skb_cb->htt.discard = true;
 
@@ -121,7 +79,7 @@ void ath10k_htt_htc_tx_complete(void *context, struct sk_buff *skb)
 			skb_cb->htt.refcount = 1;
 	}
 
-	ath10k_txrx_tx_unref(htt, txi);
+	ath10k_txrx_tx_unref(htt, skb);
 }
 
 int ath10k_htt_h2t_ver_req_msg(struct htt_struct *htt)
@@ -240,46 +198,52 @@ int ath10k_htt_mgmt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 {
 	struct device *dev = htt->ar->dev;
 	struct ath10k_skb_cb *skb_cb;
-	struct htt_tx_info *txi;
+	struct sk_buff *txdesc = NULL;
 	struct htt_cmd *cmd;
 	u8 vdev_id = ATH10K_SKB_CB(msdu)->htt.vdev_id;
 	int len = 0;
+	int msdu_id = -1;
 	int res;
 
-	txi = ath10k_htt_tx_info_alloc(htt);
-	if (!txi)
-		return -ENOMEM;
-
 	len += sizeof(cmd->hdr);
 	len += sizeof(cmd->mgmt_tx);
 
-	txi->txdesc = ath10k_htc_alloc_skb(len);
-	txi->msdu = msdu;
-
-	if (!txi->txdesc) {
+	txdesc = ath10k_htc_alloc_skb(len);
+	if (!txdesc) {
 		res = -ENOMEM;
 		goto err;
 	}
 
+	spin_lock_bh(&htt->tx_lock);
+	msdu_id = ath10k_htt_tx_alloc_msdu_id(htt);
+	if (msdu_id < 0) {
+		spin_unlock_bh(&htt->tx_lock);
+		res = msdu_id;
+		goto err;
+	}
+	htt->pending_tx[msdu_id] = txdesc;
+	spin_unlock_bh(&htt->tx_lock);
+
 	res = ath10k_skb_map(dev, msdu);
 	if (res)
 		goto err;
 
-	skb_put(txi->txdesc, len);
-	cmd = (struct htt_cmd *)txi->txdesc->data;
+	skb_put(txdesc, len);
+	cmd = (struct htt_cmd *)txdesc->data;
 	cmd->hdr.msg_type         = HTT_H2T_MSG_TYPE_MGMT_TX;
 	cmd->mgmt_tx.msdu_paddr = __cpu_to_le32(ATH10K_SKB_CB(msdu)->paddr);
 	cmd->mgmt_tx.len        = __cpu_to_le32(msdu->len);
-	cmd->mgmt_tx.desc_id    = __cpu_to_le32(txi->msdu_id);
+	cmd->mgmt_tx.desc_id    = __cpu_to_le32(msdu_id);
 	cmd->mgmt_tx.vdev_id    = __cpu_to_le32(vdev_id);
 	memcpy(cmd->mgmt_tx.hdr, msdu->data,
 	       min((int)msdu->len, HTT_MGMT_FRM_HDR_DOWNLOAD_LEN));
 
-	skb_cb = ATH10K_SKB_CB(txi->txdesc);
-	skb_cb->htt.msdu_id = txi->msdu_id;
+	skb_cb = ATH10K_SKB_CB(txdesc);
+	skb_cb->htt.msdu_id = msdu_id;
 	skb_cb->htt.refcount = 2;
+	skb_cb->htt.msdu = msdu;
 
-	res = ath10k_htc_send(htt->htc, htt->eid, txi->txdesc);
+	res = ath10k_htc_send(htt->htc, htt->eid, txdesc);
 	if (res)
 		goto err;
 
@@ -288,10 +252,14 @@ int ath10k_htt_mgmt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 err:
 	ath10k_skb_unmap(dev, msdu);
 
-	if (txi->txdesc)
-		dev_kfree_skb_any(txi->txdesc);
-
-	ath10k_htt_tx_info_free(htt, txi);
+	if (txdesc)
+		dev_kfree_skb_any(txdesc);
+	if (msdu_id >= 0) {
+		spin_lock_bh(&htt->tx_lock);
+		htt->pending_tx[msdu_id] = NULL;
+		ath10k_htt_tx_free_msdu_id(htt, msdu_id);
+		spin_unlock_bh(&htt->tx_lock);
+	}
 	return res;
 }
 
@@ -300,13 +268,15 @@ int ath10k_htt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 	struct device *dev = htt->ar->dev;
 	struct htt_cmd *cmd;
 	struct htt_data_tx_desc_frag *tx_frags;
-	struct htt_tx_info *txi;
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)msdu->data;
 	struct ath10k_skb_cb *skb_cb;
+	struct sk_buff *txdesc = NULL;
+	struct sk_buff *txfrag = NULL;
 	u8 vdev_id = ATH10K_SKB_CB(msdu)->htt.vdev_id;
 	u8 tid;
 	int prefetch_len, desc_len, frag_len;
 	dma_addr_t frags_paddr;
+	int msdu_id = -1;
 	int res;
 	u8 flags0;
 	u16 flags1;
@@ -314,61 +284,63 @@ int ath10k_htt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 	prefetch_len = min(htt->prefetch_len, msdu->len);
 	prefetch_len = roundup(prefetch_len, 4);
 
-	txi = ath10k_htt_tx_info_alloc(htt);
-	if (!txi)
-		return -ENOMEM;
-
 	desc_len = sizeof(cmd->hdr) + sizeof(cmd->data_tx) + prefetch_len;
 	frag_len = sizeof(*tx_frags) * 2;
 
-	txi->txdesc = ath10k_htc_alloc_skb(desc_len);
-	if (!txi->txdesc) {
+	txdesc = ath10k_htc_alloc_skb(desc_len);
+	if (!txdesc) {
 		res = -ENOMEM;
 		goto err;
 	}
 
-	txi->txfrag = dev_alloc_skb(frag_len);
-	if (!txi->txfrag) {
+	txfrag = dev_alloc_skb(frag_len);
+	if (!txfrag) {
 		res = -ENOMEM;
 		goto err;
 	}
 
-	txi->msdu = msdu;
-
-	if ((unsigned long)txi->txdesc->data & 0x3) {
+	if ((unsigned long)txdesc->data & 0x3) {
 		ath10k_warn("htt alignment check failed. dropping packet.\n");
 		res = -EIO;
 		goto err;
 	}
 
+	spin_lock_bh(&htt->tx_lock);
+	msdu_id = ath10k_htt_tx_alloc_msdu_id(htt);
+	if (msdu_id < 0) {
+		spin_unlock_bh(&htt->tx_lock);
+		res = msdu_id;
+		goto err;
+	}
+	htt->pending_tx[msdu_id] = txdesc;
+	spin_unlock_bh(&htt->tx_lock);
+
 	res = ath10k_skb_map(dev, msdu);
 	if (res)
 		goto err;
 
 	/* tx fragment list must be terminated with zero-entry */
-	skb_put(txi->txfrag, frag_len);
-	tx_frags = (struct htt_data_tx_desc_frag *)txi->txfrag->data;
+	skb_put(txfrag, frag_len);
+	tx_frags = (struct htt_data_tx_desc_frag *)txfrag->data;
 	tx_frags[0].paddr = __cpu_to_le32(ATH10K_SKB_CB(msdu)->paddr);
 	tx_frags[0].len   = __cpu_to_le32(msdu->len);
 	tx_frags[1].paddr = __cpu_to_le32(0);
 	tx_frags[1].len   = __cpu_to_le32(0);
 
-	res = ath10k_skb_map(dev, txi->txfrag);
+	res = ath10k_skb_map(dev, txfrag);
 	if (res)
 		goto err;
 
 	ath10k_dbg(ATH10K_DBG_HTT, "txfrag 0x%llx msdu 0x%llx\n",
-		   (unsigned long long) ATH10K_SKB_CB(txi->txfrag)->paddr,
-		   (unsigned long long) ATH10K_SKB_CB(txi->msdu)->paddr);
+		   (unsigned long long) ATH10K_SKB_CB(txfrag)->paddr,
+		   (unsigned long long) ATH10K_SKB_CB(msdu)->paddr);
 	ath10k_dbg_dump(ATH10K_DBG_HTT, NULL, "txfrag: ",
-			txi->txfrag->data,
-			frag_len);
+			txfrag->data, frag_len);
 	ath10k_dbg_dump(ATH10K_DBG_HTT, NULL, "msdu: ",
-			txi->msdu->data,
-			txi->msdu->len);
+			msdu->data, msdu->len);
 
-	skb_put(txi->txdesc, desc_len);
-	cmd = (struct htt_cmd *)txi->txdesc->data;
+	skb_put(txdesc, desc_len);
+	cmd = (struct htt_cmd *)txdesc->data;
 	memset(cmd, 0, desc_len);
 
 	tid = ATH10K_SKB_CB(msdu)->htt.tid;
@@ -385,35 +357,42 @@ int ath10k_htt_tx(struct htt_struct *htt, struct sk_buff *msdu)
 	flags1 |= SM((u16)vdev_id, HTT_DATA_TX_DESC_FLAGS1_VDEV_ID);
 	flags1 |= SM((u16)tid, HTT_DATA_TX_DESC_FLAGS1_EXT_TID);
 
-	frags_paddr = ATH10K_SKB_CB(txi->txfrag)->paddr;
+	frags_paddr = ATH10K_SKB_CB(txfrag)->paddr;
 
 	cmd->hdr.msg_type        = HTT_H2T_MSG_TYPE_TX_FRM;
 	cmd->data_tx.flags0      = flags0;
 	cmd->data_tx.flags1      = __cpu_to_le16(flags1);
 	cmd->data_tx.len         = __cpu_to_le16(msdu->len);
-	cmd->data_tx.id          = __cpu_to_le16(txi->msdu_id);
+	cmd->data_tx.id          = __cpu_to_le16(msdu_id);
 	cmd->data_tx.frags_paddr = __cpu_to_le32(frags_paddr);
 	cmd->data_tx.peerid      = __cpu_to_le32(HTT_INVALID_PEERID);
 
 	memcpy(cmd->data_tx.prefetch, msdu->data, prefetch_len);
 
-	skb_cb = ATH10K_SKB_CB(txi->txdesc);
-	skb_cb->htt.msdu_id = txi->msdu_id;
+	skb_cb = ATH10K_SKB_CB(txdesc);
+	skb_cb->htt.msdu_id = msdu_id;
 	skb_cb->htt.refcount = 2;
+	skb_cb->htt.txfrag = txfrag;
+	skb_cb->htt.msdu = msdu;
 
-	res = ath10k_htc_send(htt->htc, htt->eid, txi->txdesc);
+	res = ath10k_htc_send(htt->htc, htt->eid, txdesc);
 	if (res)
 		goto err;
 
 	return 0;
 err:
-	if (txi->txfrag)
-		ath10k_skb_unmap(dev, txi->txfrag);
-	if (txi->txdesc)
-		dev_kfree_skb_any(txi->txdesc);
-	if (txi->txfrag)
-		dev_kfree_skb_any(txi->txfrag);
-	ath10k_htt_tx_info_free(htt, txi);
+	if (txfrag)
+		ath10k_skb_unmap(dev, txfrag);
+	if (txdesc)
+		dev_kfree_skb_any(txdesc);
+	if (txfrag)
+		dev_kfree_skb_any(txfrag);
+	if (msdu_id >= 0) {
+		spin_lock_bh(&htt->tx_lock);
+		htt->pending_tx[msdu_id] = NULL;
+		ath10k_htt_tx_free_msdu_id(htt, msdu_id);
+		spin_unlock_bh(&htt->tx_lock);
+	}
 	ath10k_skb_unmap(dev, msdu);
 	return res;
 }
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 0f8869d..47a727e 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2365,12 +2365,15 @@ static void ath10k_flush(struct ieee80211_hw *hw, u32 queues, bool drop)
 	if (drop)
 		return;
 
-	ret = wait_event_timeout(ar->htt->empty_tx_wq,
-				 atomic_read(&ar->htt->num_used_txi) == 0,
-				 ATH10K_FLUSH_TIMEOUT_HZ);
+	ret = wait_event_timeout(ar->htt->empty_tx_wq, ({
+			bool empty;
+			spin_lock_bh(&ar->htt->tx_lock);
+			empty = bitmap_empty(ar->htt->used_msdu_ids, HTT_MAX_NUM_PENDING_TX);
+			spin_unlock_bh(&ar->htt->tx_lock);
+			(empty);
+		}), ATH10K_FLUSH_TIMEOUT_HZ);
 	if (ret <= 0)
-		ath10k_warn("tx not flushed (%d frames still pending)\n",
-			    atomic_read(&ar->htt->num_used_txi));
+		ath10k_warn("tx not flushed\n");
 }
 
 /* TODO: Implement this function properly
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index 9c521d6..6a66dc2 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -44,72 +44,80 @@ out:
 	spin_unlock_bh(&ar->data_lock);
 }
 
-void ath10k_txrx_tx_unref(struct htt_struct *htt, struct htt_tx_info *txi)
+void ath10k_txrx_tx_unref(struct htt_struct *htt, struct sk_buff *txdesc)
 {
 	struct device *dev = htt->ar->dev;
 	struct ieee80211_tx_info *info;
+	struct sk_buff *txfrag = ATH10K_SKB_CB(txdesc)->htt.txfrag;
+	struct sk_buff *msdu = ATH10K_SKB_CB(txdesc)->htt.msdu;
 	int ret;
 
-	if (ATH10K_SKB_CB(txi->txdesc)->htt.refcount == 0)
+	if (ATH10K_SKB_CB(txdesc)->htt.refcount == 0)
 		return;
 
-	if (--ATH10K_SKB_CB(txi->txdesc)->htt.refcount > 0)
+	if (--ATH10K_SKB_CB(txdesc)->htt.refcount > 0)
 		return;
 
-	if (txi->txfrag) {
-		ret = ath10k_skb_unmap(dev, txi->txfrag);
+	if (txfrag) {
+		ret = ath10k_skb_unmap(dev, txfrag);
 		if (ret)
 			ath10k_warn("txfrag unmap failed (%d)\n", ret);
 
-		dev_kfree_skb_any(txi->txfrag);
+		dev_kfree_skb_any(txfrag);
 	}
 
-	ret = ath10k_skb_unmap(dev, txi->msdu);
+	ret = ath10k_skb_unmap(dev, msdu);
 	if (ret)
 		ath10k_warn("data skb unmap failed (%d)\n", ret);
 
-	ath10k_report_offchan_tx(htt->ar, txi->msdu);
+	ath10k_report_offchan_tx(htt->ar, msdu);
 
-	info = IEEE80211_SKB_CB(txi->msdu);
+	info = IEEE80211_SKB_CB(msdu);
 	memset(&info->status, 0, sizeof(info->status));
 
-	if (ATH10K_SKB_CB(txi->txdesc)->htt.discard) {
-		ieee80211_free_txskb(htt->ar->hw, txi->msdu);
+	if (ATH10K_SKB_CB(txdesc)->htt.discard) {
+		ieee80211_free_txskb(htt->ar->hw, msdu);
 		goto exit;
 	}
 
 	if (!(info->flags & IEEE80211_TX_CTL_NO_ACK))
 		info->flags |= IEEE80211_TX_STAT_ACK;
 
-	if (ATH10K_SKB_CB(txi->txdesc)->htt.no_ack)
+	if (ATH10K_SKB_CB(txdesc)->htt.no_ack)
 		info->flags &= ~IEEE80211_TX_STAT_ACK;
 
-	ieee80211_tx_status(htt->ar->hw, txi->msdu);
+	ieee80211_tx_status(htt->ar->hw, msdu);
 	/* we do not own the msdu anymore */
 
 exit:
-	dev_kfree_skb_any(txi->txdesc);
-	ath10k_htt_tx_info_free(htt, txi);
+	spin_lock_bh(&htt->tx_lock);
+	ath10k_htt_tx_free_msdu_id(htt, ATH10K_SKB_CB(txdesc)->htt.msdu_id);
+	if (bitmap_empty(htt->used_msdu_ids, HTT_MAX_NUM_PENDING_TX))
+		wake_up(&htt->empty_tx_wq);
+	spin_unlock_bh(&htt->tx_lock);
+
+	dev_kfree_skb_any(txdesc);
 }
 
 void ath10k_txrx_tx_completed(struct htt_struct *htt,
 			      const struct htt_tx_done *tx_done)
 {
-	struct htt_tx_info *txi;
+	struct sk_buff *txdesc;
 
 	ath10k_dbg(ATH10K_DBG_HTT, "htt tx completion msdu_id %u discard %d no_ack %d\n",
 		   tx_done->msdu_id, !!tx_done->discard, !!tx_done->no_ack);
 
-	txi = ath10k_htt_tx_info_lookup(htt, tx_done->msdu_id);
-	if (!txi) {
-		ath10k_dbg(ATH10K_DBG_HTT, "htt txi lookup fail\n");
+	if (tx_done->msdu_id >= ARRAY_SIZE(htt->pending_tx)) {
+		ath10k_warn("warning: msdu_id %d too big, ignoring\n", tx_done->msdu_id);
 		return;
 	}
 
-	ATH10K_SKB_CB(txi->txdesc)->htt.discard = tx_done->discard;
-	ATH10K_SKB_CB(txi->txdesc)->htt.no_ack = tx_done->no_ack;
+	txdesc = htt->pending_tx[tx_done->msdu_id];
+
+	ATH10K_SKB_CB(txdesc)->htt.discard = tx_done->discard;
+	ATH10K_SKB_CB(txdesc)->htt.no_ack = tx_done->no_ack;
 
-	ath10k_txrx_tx_unref(htt, txi);
+	ath10k_txrx_tx_unref(htt, txdesc);
 }
 
 static u8 rx_legacy_rate_idx[] = {
diff --git a/drivers/net/wireless/ath/ath10k/txrx.h b/drivers/net/wireless/ath/ath10k/txrx.h
index 7a1bbfe..05bf9d8 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.h
+++ b/drivers/net/wireless/ath/ath10k/txrx.h
@@ -19,7 +19,7 @@
 
 #include "htt.h"
 
-void ath10k_txrx_tx_unref(struct htt_struct *htt, struct htt_tx_info *txi);
+void ath10k_txrx_tx_unref(struct htt_struct *htt, struct sk_buff *txdesc);
 void ath10k_txrx_tx_completed(struct htt_struct *htt,
 			      const struct htt_tx_done *tx_done);
 void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info);
-- 
1.7.9.5

[ath9k-devel] [PATCH 0/2] ath10k: kill off htt_tx_info wrapper

[Kalle Valo]

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

> This patchset merges the htt_tx_info into
> ath10k_skb_cb. This depends upon my 'htt tx
> cleanup' patchset.
>
> Michal Kazior (2):
>   ath10k: simplify txi lifespan
>   ath10k: merge htt_tx_info into ath10k_skb_cb

Thanks, both patches applied.

It's truly awesome that we can work directly with sk_buffs now. It's
pain in the butt to deal with struct htc_packet, struct htt_tx_info and
whatnot. Thank you very much for working on this.

-- 
Kalle Valo