[ath9k-devel] [PATCH 0/7] ath10k: tx flow control fixes

[ath9k-devel] [PATCH 0/7] ath10k: tx flow control fixes

[Michal Kazior]

This addresses some `tx failed` issues and minor
issues I've found along.

This is a resend as a PATCH rather than an RFC.

Updates include:
 * added `ath10k: detect htt pending tx limit at runtime`
 * dropped htc tx polling with a timer patch
 * dropped explicit htt tx polling from
   `ath10k: introduce proper htt tx flow control`
   (this was deadlocking, and is no longer necessary anyway)

Michal Kazior (7):
  ath10k: change errno if we run out of msdu_ids
  ath10k: ath10k_htc_prepare_tx_skb() never fails
  ath10k: add lockdep asserts to htc skb dequeuing
  ath10k: simplify htc flow control
  ath10k: remove unused queue limit
  ath10k: introduce proper htt tx flow control
  ath10k: detect htt pending tx limit at runtime

 drivers/net/wireless/ath/ath10k/htc.c    |  106 +++++++-----------------------
 drivers/net/wireless/ath/ath10k/htc.h    |    4 --
 drivers/net/wireless/ath/ath10k/htt.c    |   25 ++-----
 drivers/net/wireless/ath/ath10k/htt.h    |   26 ++------
 drivers/net/wireless/ath/ath10k/htt_tx.c |   83 +++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/mac.c    |    2 +-
 drivers/net/wireless/ath/ath10k/txrx.c   |    5 +-
 7 files changed, 116 insertions(+), 135 deletions(-)

-- 
1.7.9.5

[ath9k-devel] [PATCH 1/7] ath10k: change errno if we run out of msdu_ids

[Michal Kazior]

Since these are limited resources not strictly by
memory we should not report it as such. It's also
easier to distinguish `tx failed` warnings this
way.

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

diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index cde50a3..21add5d 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -30,7 +30,7 @@ int ath10k_htt_tx_alloc_msdu_id(struct ath10k_htt *htt)
 	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 -ENOBUFS;
 
 	ath10k_dbg(ATH10K_DBG_HTT, "htt tx alloc msdu_id %d\n", msdu_id);
 	__set_bit(msdu_id, htt->used_msdu_ids);
-- 
1.7.9.5

[ath9k-devel] [PATCH 2/7] ath10k: ath10k_htc_prepare_tx_skb() never fails

[Michal Kazior]

It shouldn't report any result.
ath10k_htc_restore_tx_skb() assumes
ath10k_htc_prepare_tx_skb() always succeeds. If
ath10k_htc_prepare_tx_skb() was to fail we'd
errornously skb_pull(). This patch is for clarity
and sanity.

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

diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index af95e87..820444b 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -139,8 +139,8 @@ static bool ath10k_htc_ep_need_credit_update(struct ath10k_htc_ep *ep)
 	return true;
 }
 
-static int ath10k_htc_prepare_tx_skb(struct ath10k_htc_ep *ep,
-				     struct sk_buff *skb)
+static void ath10k_htc_prepare_tx_skb(struct ath10k_htc_ep *ep,
+				      struct sk_buff *skb)
 {
 	struct ath10k_htc_hdr *hdr;
 
@@ -157,8 +157,6 @@ static int ath10k_htc_prepare_tx_skb(struct ath10k_htc_ep *ep,
 		hdr->flags |= ATH10K_HTC_FLAG_NEED_CREDIT_UPDATE;
 
 	spin_unlock_bh(&ep->htc->tx_lock);
-
-	return 0;
 }
 
 static int ath10k_htc_issue_skb(struct ath10k_htc *htc,
@@ -172,9 +170,7 @@ static int ath10k_htc_issue_skb(struct ath10k_htc *htc,
 	ath10k_dbg(ATH10K_DBG_HTC, "%s: ep %d skb %p\n", __func__,
 		   ep->eid, skb);
 
-	ret = ath10k_htc_prepare_tx_skb(ep, skb);
-	if (ret)
-		goto err;
+	ath10k_htc_prepare_tx_skb(ep, skb);
 
 	ret = ath10k_skb_map(htc->ar->dev, skb);
 	if (ret)
-- 
1.7.9.5

[ath9k-devel] [PATCH 3/7] ath10k: add lockdep asserts to htc skb dequeuing

[Michal Kazior]

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

diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 820444b..ac211f1 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -198,7 +198,6 @@ err:
 	return ret;
 }
 
-/* assumes tx_lock is held */
 static struct sk_buff *ath10k_htc_get_skb_credit_based(struct ath10k_htc *htc,
 						       struct ath10k_htc_ep *ep,
 						       u8 *credits)
@@ -209,6 +208,8 @@ static struct sk_buff *ath10k_htc_get_skb_credit_based(struct ath10k_htc *htc,
 	int remainder;
 	unsigned int transfer_len;
 
+	lockdep_assert_held(&htc->tx_lock);
+
 	skb = __skb_dequeue(&ep->tx_queue);
 	if (!skb)
 		return NULL;
@@ -241,7 +242,6 @@ static struct sk_buff *ath10k_htc_get_skb_credit_based(struct ath10k_htc *htc,
 	return skb;
 }
 
-/* assumes tx_lock is held */
 static struct sk_buff *ath10k_htc_get_skb(struct ath10k_htc *htc,
 					  struct ath10k_htc_ep *ep,
 					  int resources)
@@ -249,6 +249,8 @@ static struct sk_buff *ath10k_htc_get_skb(struct ath10k_htc *htc,
 	struct sk_buff *skb;
 	struct ath10k_skb_cb *skb_cb;
 
+	lockdep_assert_held(&htc->tx_lock);
+
 	if (!resources)
 		return NULL;
 
-- 
1.7.9.5

[ath9k-devel] [PATCH 4/7] ath10k: simplify htc flow control

[Michal Kazior]

Credit-based flow is designed around HTC, and HTC
is supposed to manage the credit count and
reservation.

For non credit-based endpoints we must depend on
the HIF pipe tx resources. This is CE ringbuffer
size/usage for PCI backend. This is not managed by
HTC but by the HIF. We could mirror the
tx_resources and sync it in HTC and perform
mirror-reservation in HTC but it doesn't really
make much sense. It's far simpler to reinsert skb
if we're out of resources.

Queue control in HTC is ineffective and does
virtually nothing. This will be replaced in the
future.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htc.c |   94 ++++++---------------------------
 drivers/net/wireless/ath/ath10k/htc.h |    4 --
 drivers/net/wireless/ath/ath10k/htt.c |   12 -----
 3 files changed, 17 insertions(+), 93 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index ac211f1..74363c9 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -23,48 +23,6 @@
 /* Send */
 /********/
 
-static inline void ath10k_htc_stop_queue(struct ath10k_htc_ep *ep)
-{
-	if (ep->tx_queue_stopped)
-		return;
-
-	if (ep->ep_ops.stop_queue)
-		ep->ep_ops.stop_queue(ep->htc->ar);
-
-	ath10k_dbg(ATH10K_DBG_HTC, "ep %d stop\n", ep->eid);
-	ep->tx_queue_stopped = true;
-}
-
-static inline void ath10k_htc_wake_queue(struct ath10k_htc_ep *ep)
-{
-	if (!ep->tx_queue_stopped)
-		return;
-
-	if (ep->ep_ops.wake_queue)
-		ep->ep_ops.wake_queue(ep->htc->ar);
-
-	ath10k_dbg(ATH10K_DBG_HTC, "ep %d wake\n", ep->eid);
-	ep->tx_queue_stopped = false;
-}
-
-static inline void ath10k_htc_recalc_queue(struct ath10k_htc_ep *ep, int delta)
-{
-	ath10k_dbg(ATH10K_DBG_HTC, "ep %d queue len %d +%d max %d\n",
-		   ep->eid, ep->tx_queue_len, delta, ep->max_tx_queue_depth);
-
-	if (ep->max_tx_queue_depth == 0)
-		return;
-
-	ep->tx_queue_len += delta;
-
-	if (ep->tx_queue_stopped) {
-		if (ep->tx_queue_len <= ep->max_tx_queue_depth/2)
-			ath10k_htc_wake_queue(ep);
-	} else if (ep->tx_queue_len >= ep->max_tx_queue_depth) {
-		ath10k_htc_stop_queue(ep);
-	}
-}
-
 static inline void ath10k_htc_send_complete_check(struct ath10k_htc_ep *ep,
 						  int force)
 {
@@ -192,6 +150,17 @@ err:
 	ep->tx_credits += credits;
 	spin_unlock_bh(&htc->tx_lock);
 
+	/* this is the simplest way to handle out-of-resources for non-credit
+	 * based endpoints. credit based endpoints can still get -ENOSR, but
+	 * this is highly unlikely as credit reservation should prevent that */
+	if (ret == -ENOSR) {
+		spin_lock_bh(&htc->tx_lock);
+		__skb_queue_head(&ep->tx_queue, skb);
+		spin_unlock_bh(&htc->tx_lock);
+
+		return ret;
+	}
+
 	skb_cb->is_aborted = true;
 	ath10k_htc_notify_tx_completion(ep, skb);
 
@@ -233,7 +202,6 @@ static struct sk_buff *ath10k_htc_get_skb_credit_based(struct ath10k_htc *htc,
 
 	if (ep->tx_credits < credits_required) {
 		__skb_queue_head(&ep->tx_queue, skb);
-		ath10k_htc_recalc_queue(ep, 1);
 		return NULL;
 	}
 
@@ -242,59 +210,33 @@ static struct sk_buff *ath10k_htc_get_skb_credit_based(struct ath10k_htc *htc,
 	return skb;
 }
 
-static struct sk_buff *ath10k_htc_get_skb(struct ath10k_htc *htc,
-					  struct ath10k_htc_ep *ep,
-					  int resources)
-{
-	struct sk_buff *skb;
-	struct ath10k_skb_cb *skb_cb;
-
-	lockdep_assert_held(&htc->tx_lock);
-
-	if (!resources)
-		return NULL;
-
-	skb = __skb_dequeue(&ep->tx_queue);
-	if (!skb)
-		return NULL;
-
-	skb_cb = ATH10K_SKB_CB(skb);
-	ath10k_htc_recalc_queue(ep, -1);
-
-	return skb;
-}
-
 static void ath10k_htc_send_work(struct work_struct *work)
 {
 	struct ath10k_htc_ep *ep = container_of(work,
 					struct ath10k_htc_ep, send_work);
 	struct ath10k_htc *htc = ep->htc;
 	struct sk_buff *skb;
-	int tx_resources = 0;
 	u8 credits = 0;
+	int ret;
 
 	while (true) {
-		if (!ep->tx_credit_flow_enabled)
-			tx_resources = ath10k_hif_get_free_queue_number
-					(htc->ar, ep->ul_pipe_id);
-
 		if (ep->ul_is_polled)
 			ath10k_htc_send_complete_check(ep, 0);
 
 		spin_lock_bh(&htc->tx_lock);
-
 		if (ep->tx_credit_flow_enabled)
 			skb = ath10k_htc_get_skb_credit_based(htc, ep,
 							      &credits);
 		else
-			skb = ath10k_htc_get_skb(htc, ep, tx_resources);
-
+			skb = __skb_dequeue(&ep->tx_queue);
 		spin_unlock_bh(&htc->tx_lock);
 
 		if (!skb)
-			break; /* tx_queue empty or out of resources */
+			break;
 
-		ath10k_htc_issue_skb(htc, ep, skb, credits);
+		ret = ath10k_htc_issue_skb(htc, ep, skb, credits);
+		if (ret == -ENOSR)
+			break;
 	}
 }
 
@@ -313,7 +255,6 @@ int ath10k_htc_send(struct ath10k_htc *htc,
 
 	spin_lock_bh(&htc->tx_lock);
 	__skb_queue_tail(&ep->tx_queue, skb);
-	ath10k_htc_recalc_queue(ep, 1);
 	spin_unlock_bh(&htc->tx_lock);
 
 	queue_work(htc->ar->workqueue, &ep->send_work);
@@ -658,7 +599,6 @@ static void ath10k_htc_reset_endpoint_states(struct ath10k_htc *htc)
 		ep->max_tx_queue_depth = 0;
 		ep->eid = i;
 		skb_queue_head_init(&ep->tx_queue);
-		ep->tx_queue_len = 0;
 		ep->htc = htc;
 		ep->tx_credit_flow_enabled = true;
 		INIT_WORK(&ep->send_work, ath10k_htc_send_work);
diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h
index 6b4f75e..fa45844 100644
--- a/drivers/net/wireless/ath/ath10k/htc.h
+++ b/drivers/net/wireless/ath/ath10k/htc.h
@@ -276,8 +276,6 @@ struct ath10k_htc_ops {
 struct ath10k_htc_ep_ops {
 	void (*ep_tx_complete)(struct ath10k *, struct sk_buff *);
 	void (*ep_rx_complete)(struct ath10k *, struct sk_buff *);
-	void (*stop_queue)(struct ath10k *);
-	void (*wake_queue)(struct ath10k *);
 };
 
 /* service connection information */
@@ -318,8 +316,6 @@ struct ath10k_htc_ep {
 	int dl_is_polled; /* call HIF to fetch rx (not implemented) */
 
 	struct sk_buff_head tx_queue;
-	int tx_queue_len;
-	bool tx_queue_stopped;
 
 	u8 seq_no; /* for debugging */
 	int tx_credits;
diff --git a/drivers/net/wireless/ath/ath10k/htt.c b/drivers/net/wireless/ath/ath10k/htt.c
index 4131cc6..e1fa279 100644
--- a/drivers/net/wireless/ath/ath10k/htt.c
+++ b/drivers/net/wireless/ath/ath10k/htt.c
@@ -21,16 +21,6 @@
 #include "core.h"
 #include "debug.h"
 
-static void ath10k_htt_stop_queue(struct ath10k *ar)
-{
-	ieee80211_stop_queues(ar->hw);
-}
-
-static void ath10k_htt_wake_queue(struct ath10k *ar)
-{
-	ieee80211_wake_queues(ar->hw);
-}
-
 static int ath10k_htt_htc_attach(struct ath10k_htt *htt)
 {
 	struct ath10k_htc_svc_conn_req conn_req;
@@ -42,8 +32,6 @@ static int ath10k_htt_htc_attach(struct ath10k_htt *htt)
 
 	conn_req.ep_ops.ep_tx_complete = ath10k_htt_htc_tx_complete;
 	conn_req.ep_ops.ep_rx_complete = ath10k_htt_t2h_msg_handler;
-	conn_req.ep_ops.stop_queue = ath10k_htt_stop_queue;
-	conn_req.ep_ops.wake_queue = ath10k_htt_wake_queue;
 
 	/*
 	 * Specify how deep to let a queue get before ath10k_htc_send will
-- 
1.7.9.5

[ath9k-devel] [PATCH 5/7] ath10k: remove unused queue limit

[Michal Kazior]

HTT isn't credit-based and we don't need to have
an artificial limit here.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt.c |    6 ------
 drivers/net/wireless/ath/ath10k/htt.h |   15 ---------------
 2 files changed, 21 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt.c b/drivers/net/wireless/ath/ath10k/htt.c
index e1fa279..b39f5de 100644
--- a/drivers/net/wireless/ath/ath10k/htt.c
+++ b/drivers/net/wireless/ath/ath10k/htt.c
@@ -33,12 +33,6 @@ static int ath10k_htt_htc_attach(struct ath10k_htt *htt)
 	conn_req.ep_ops.ep_tx_complete = ath10k_htt_htc_tx_complete;
 	conn_req.ep_ops.ep_rx_complete = ath10k_htt_t2h_msg_handler;
 
-	/*
-	 * Specify how deep to let a queue get before ath10k_htc_send will
-	 * call the ep_send_full function due to excessive send queue depth.
-	 */
-	conn_req.max_send_queue_depth = HTT_MAX_SEND_QUEUE_DEPTH;
-
 	/* connect to control service */
 	conn_req.service_id = ATH10K_HTC_SVC_ID_HTT_DATA_MSG;
 
diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
index f3c22df..85de0b3 100644
--- a/drivers/net/wireless/ath/ath10k/htt.h
+++ b/drivers/net/wireless/ath/ath10k/htt.h
@@ -1302,21 +1302,6 @@ struct htt_rx_desc {
 #define HTT_MAC_ADDR_LEN 6
 
 /*
- * HTT_MAX_SEND_QUEUE_DEPTH -
- * How many packets HTC should allow to accumulate in a send queue
- * before calling the ep_send_full callback to see whether to retain
- * or drop packets.
- * This is not relevant for LL, where tx descriptors should be immediately
- * downloaded to the target.
- * This is not very relevant for HL either, since it is anticipated that
- * the HL tx download scheduler will not work this far in advance - rather,
- * it will make its decisions just-in-time, so it can be responsive to
- * changing conditions.
- * Hence, this queue depth threshold spec is mostly just a formality.
- */
-#define HTT_MAX_SEND_QUEUE_DEPTH (HTT_MAX_NUM_PENDING_TX)
-
-/*
  * FIX THIS
  * Should be: sizeof(struct htt_host_rx_desc) + max rx MSDU size,
  * rounded up to a cache line size.
-- 
1.7.9.5

[ath9k-devel] [PATCH 6/7] ath10k: introduce proper htt tx flow control

[Michal Kazior]

There were cases where we'd run out of msdu_ids.
This can be fixed by telling mac80211 to stop
sending us more frames if we run out of resources.

It was possible to trigger with massive TCP RX
~300mbps (e.g. using iperf).

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt.h    |    2 ++
 drivers/net/wireless/ath/ath10k/htt_tx.c |   46 ++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/txrx.c   |    1 +
 3 files changed, 49 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
index 85de0b3..8ae1f03 100644
--- a/drivers/net/wireless/ath/ath10k/htt.h
+++ b/drivers/net/wireless/ath/ath10k/htt.h
@@ -1262,6 +1262,7 @@ struct ath10k_htt {
 
 	/* Protects access to %pending_tx, %used_msdu_ids */
 	spinlock_t tx_lock;
+	int num_pending_tx;
 	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;
@@ -1330,6 +1331,7 @@ void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb);
 int ath10k_htt_h2t_ver_req_msg(struct ath10k_htt *htt);
 int ath10k_htt_send_rx_ring_cfg_ll(struct ath10k_htt *htt);
 
+void __ath10k_htt_tx_dec_pending(struct ath10k_htt *htt);
 int ath10k_htt_tx_alloc_msdu_id(struct ath10k_htt *htt);
 void ath10k_htt_tx_free_msdu_id(struct ath10k_htt *htt, u16 msdu_id);
 int ath10k_htt_mgmt_tx(struct ath10k_htt *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 21add5d..7dd366f 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -18,9 +18,44 @@
 #include <linux/etherdevice.h>
 #include "htt.h"
 #include "mac.h"
+#include "hif.h"
 #include "txrx.h"
 #include "debug.h"
 
+void __ath10k_htt_tx_dec_pending(struct ath10k_htt *htt)
+{
+	htt->num_pending_tx--;
+	if (htt->num_pending_tx == HTT_MAX_NUM_PENDING_TX - 1)
+		ieee80211_wake_queues(htt->ar->hw);
+}
+
+static void ath10k_htt_tx_dec_pending(struct ath10k_htt *htt)
+{
+	spin_lock_bh(&htt->tx_lock);
+	__ath10k_htt_tx_dec_pending(htt);
+	spin_unlock_bh(&htt->tx_lock);
+}
+
+static int ath10k_htt_tx_inc_pending(struct ath10k_htt *htt)
+{
+	int ret = 0;
+
+	spin_lock_bh(&htt->tx_lock);
+
+	if (htt->num_pending_tx >= HTT_MAX_NUM_PENDING_TX) {
+		ret = -EBUSY;
+		goto exit;
+	}
+
+	htt->num_pending_tx++;
+	if (htt->num_pending_tx == HTT_MAX_NUM_PENDING_TX)
+		ieee80211_stop_queues(htt->ar->hw);
+
+exit:
+	spin_unlock_bh(&htt->tx_lock);
+	return ret;
+}
+
 int ath10k_htt_tx_alloc_msdu_id(struct ath10k_htt *htt)
 {
 	int msdu_id;
@@ -238,6 +273,11 @@ int ath10k_htt_mgmt_tx(struct ath10k_htt *htt, struct sk_buff *msdu)
 	int msdu_id = -1;
 	int res;
 
+
+	res = ath10k_htt_tx_inc_pending(htt);
+	if (res)
+		return res;
+
 	len += sizeof(cmd->hdr);
 	len += sizeof(cmd->mgmt_tx);
 
@@ -295,6 +335,7 @@ err:
 		ath10k_htt_tx_free_msdu_id(htt, msdu_id);
 		spin_unlock_bh(&htt->tx_lock);
 	}
+	ath10k_htt_tx_dec_pending(htt);
 	return res;
 }
 
@@ -316,6 +357,10 @@ int ath10k_htt_tx(struct ath10k_htt *htt, struct sk_buff *msdu)
 	u8 flags0;
 	u16 flags1;
 
+	res = ath10k_htt_tx_inc_pending(htt);
+	if (res)
+		return res;
+
 	prefetch_len = min(htt->prefetch_len, msdu->len);
 	prefetch_len = roundup(prefetch_len, 4);
 
@@ -431,6 +476,7 @@ err:
 		ath10k_htt_tx_free_msdu_id(htt, msdu_id);
 		spin_unlock_bh(&htt->tx_lock);
 	}
+	ath10k_htt_tx_dec_pending(htt);
 	ath10k_skb_unmap(dev, msdu);
 	return res;
 }
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index a32448c..ce44ab1 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -95,6 +95,7 @@ exit:
 	spin_lock_bh(&htt->tx_lock);
 	htt->pending_tx[ATH10K_SKB_CB(txdesc)->htt.msdu_id] = NULL;
 	ath10k_htt_tx_free_msdu_id(htt, ATH10K_SKB_CB(txdesc)->htt.msdu_id);
+	__ath10k_htt_tx_dec_pending(htt);
 	if (bitmap_empty(htt->used_msdu_ids, HTT_MAX_NUM_PENDING_TX))
 		wake_up(&htt->empty_tx_wq);
 	spin_unlock_bh(&htt->tx_lock);
-- 
1.7.9.5

[ath9k-devel] [PATCH 6/7] ath10k: introduce proper htt tx flow control

[Kalle Valo]

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

> There were cases where we'd run out of msdu_ids.
> This can be fixed by telling mac80211 to stop
> sending us more frames if we run out of resources.
>
> It was possible to trigger with massive TCP RX
> ~300mbps (e.g. using iperf).
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> +void __ath10k_htt_tx_dec_pending(struct ath10k_htt *htt)
> +{
> +	htt->num_pending_tx--;
> +	if (htt->num_pending_tx == HTT_MAX_NUM_PENDING_TX - 1)
> +		ieee80211_wake_queues(htt->ar->hw);
> +}

How often is this function called? I'm wondering if it should be an
inline function.

But that can be done in a followup patch (if needed).

-- 
Kalle Valo

[ath9k-devel] [PATCH 6/7] ath10k: introduce proper htt tx flow control

[Michal Kazior]

On 28/05/13 17:06, Kalle Valo wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> There were cases where we'd run out of msdu_ids.
>> This can be fixed by telling mac80211 to stop
>> sending us more frames if we run out of resources.
>>
>> It was possible to trigger with massive TCP RX
>> ~300mbps (e.g. using iperf).
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> [...]
>
>> +void __ath10k_htt_tx_dec_pending(struct ath10k_htt *htt)
>> +{
>> +	htt->num_pending_tx--;
>> +	if (htt->num_pending_tx == HTT_MAX_NUM_PENDING_TX - 1)
>> +		ieee80211_wake_queues(htt->ar->hw);
>> +}
>
> How often is this function called? I'm wondering if it should be an
> inline function.
>
> But that can be done in a followup patch (if needed).

It's called after each tx completion, so it might be a good idea to make 
it an inline function.


-- Pozdrawiam / Best regards, Michal Kazior.

[ath9k-devel] [PATCH 7/7] ath10k: detect htt pending tx limit at runtime

[Michal Kazior]

The real limit for sending htt tx is either
msdu_id storage type (u16 - 65536 different
values) or the HIF tx pipe queue length (2047 in
case of our PCI HIF).

The htt tx pipe does not use interrupts - it must
be polled. It is polled either on RX
unconditionally or on TX if HIF tx pipe has used
over 50% of the resources.

With this patch we should finally trigger the TX
polling properly. What this really means we should
have a smoother tx. Not because the tx limit is
greater, but simply because we'll be triggering
the polling properly in case of heavy tx.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htt.c    |    7 ++++-
 drivers/net/wireless/ath/ath10k/htt.h    |    9 +++----
 drivers/net/wireless/ath/ath10k/htt_tx.c |   41 +++++++++++++++++++++++++-----
 drivers/net/wireless/ath/ath10k/mac.c    |    2 +-
 drivers/net/wireless/ath/ath10k/txrx.c   |    4 +--
 5 files changed, 47 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt.c b/drivers/net/wireless/ath/ath10k/htt.c
index b39f5de..185a546 100644
--- a/drivers/net/wireless/ath/ath10k/htt.c
+++ b/drivers/net/wireless/ath/ath10k/htt.c
@@ -50,6 +50,7 @@ static int ath10k_htt_htc_attach(struct ath10k_htt *htt)
 struct ath10k_htt *ath10k_htt_attach(struct ath10k *ar)
 {
 	struct ath10k_htt *htt;
+	int ret;
 
 	htt = kzalloc(sizeof(*htt), GFP_KERNEL);
 	if (!htt)
@@ -67,7 +68,11 @@ struct ath10k_htt *ath10k_htt_attach(struct ath10k *ar)
 	if (ath10k_htt_htc_attach(htt))
 		goto err_htc_attach;
 
-	ath10k_htt_tx_attach(htt);
+	ret = ath10k_htt_tx_attach(htt);
+	if (ret) {
+		ath10k_err("could not attach htt tx (%d)\n", ret);
+		goto err_htc_attach;
+	}
 
 	if (ath10k_htt_rx_attach(htt))
 		goto err_rx_attach;
diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
index 8ae1f03..a7a7aa0 100644
--- a/drivers/net/wireless/ath/ath10k/htt.h
+++ b/drivers/net/wireless/ath/ath10k/htt.h
@@ -1184,8 +1184,6 @@ struct htt_rx_info {
 	bool fcs_err;
 };
 
-#define HTT_MAX_NUM_PENDING_TX 512 /* FIXME: find proper value? */
-
 struct ath10k_htt {
 	struct ath10k *ar;
 	enum ath10k_htc_ep_id eid;
@@ -1262,9 +1260,10 @@ struct ath10k_htt {
 
 	/* Protects access to %pending_tx, %used_msdu_ids */
 	spinlock_t tx_lock;
+	int max_num_pending_tx;
 	int num_pending_tx;
-	struct sk_buff *pending_tx[HTT_MAX_NUM_PENDING_TX];
-	DECLARE_BITMAP(used_msdu_ids, HTT_MAX_NUM_PENDING_TX);
+	struct sk_buff **pending_tx;
+	unsigned long *used_msdu_ids; /* bitmap */
 	wait_queue_head_t empty_tx_wq;
 
 	/* set if host-fw communication goes haywire
@@ -1322,7 +1321,7 @@ struct ath10k_htt *ath10k_htt_attach(struct ath10k *ar);
 int ath10k_htt_attach_target(struct ath10k_htt *htt);
 void ath10k_htt_detach(struct ath10k_htt *htt);
 
-void ath10k_htt_tx_attach(struct ath10k_htt *htt);
+int ath10k_htt_tx_attach(struct ath10k_htt *htt);
 void ath10k_htt_tx_detach(struct ath10k_htt *htt);
 int ath10k_htt_rx_attach(struct ath10k_htt *htt);
 void ath10k_htt_rx_detach(struct ath10k_htt *htt);
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index 7dd366f..bd84153 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -25,7 +25,7 @@
 void __ath10k_htt_tx_dec_pending(struct ath10k_htt *htt)
 {
 	htt->num_pending_tx--;
-	if (htt->num_pending_tx == HTT_MAX_NUM_PENDING_TX - 1)
+	if (htt->num_pending_tx == htt->max_num_pending_tx - 1)
 		ieee80211_wake_queues(htt->ar->hw);
 }
 
@@ -42,13 +42,13 @@ static int ath10k_htt_tx_inc_pending(struct ath10k_htt *htt)
 
 	spin_lock_bh(&htt->tx_lock);
 
-	if (htt->num_pending_tx >= HTT_MAX_NUM_PENDING_TX) {
+	if (htt->num_pending_tx >= htt->max_num_pending_tx) {
 		ret = -EBUSY;
 		goto exit;
 	}
 
 	htt->num_pending_tx++;
-	if (htt->num_pending_tx == HTT_MAX_NUM_PENDING_TX)
+	if (htt->num_pending_tx == htt->max_num_pending_tx)
 		ieee80211_stop_queues(htt->ar->hw);
 
 exit:
@@ -63,8 +63,8 @@ int ath10k_htt_tx_alloc_msdu_id(struct ath10k_htt *htt)
 	lockdep_assert_held(&htt->tx_lock);
 
 	msdu_id = find_first_zero_bit(htt->used_msdu_ids,
-				      HTT_MAX_NUM_PENDING_TX);
-	if (msdu_id == HTT_MAX_NUM_PENDING_TX)
+				      htt->max_num_pending_tx);
+	if (msdu_id == htt->max_num_pending_tx)
 		return -ENOBUFS;
 
 	ath10k_dbg(ATH10K_DBG_HTT, "htt tx alloc msdu_id %d\n", msdu_id);
@@ -83,10 +83,35 @@ void ath10k_htt_tx_free_msdu_id(struct ath10k_htt *htt, u16 msdu_id)
 	__clear_bit(msdu_id, htt->used_msdu_ids);
 }
 
-void ath10k_htt_tx_attach(struct ath10k_htt *htt)
+int ath10k_htt_tx_attach(struct ath10k_htt *htt)
 {
+	u8 pipe;
+
 	spin_lock_init(&htt->tx_lock);
 	init_waitqueue_head(&htt->empty_tx_wq);
+
+	/* At the beginning free queue number should hint us the maximum
+	 * queue length */
+	pipe = htt->ar->htc->endpoint[htt->eid].ul_pipe_id;
+	htt->max_num_pending_tx = ath10k_hif_get_free_queue_number(htt->ar, pipe);
+
+	ath10k_dbg(ATH10K_DBG_HTT, "htt tx max num pending tx %d\n",
+		   htt->max_num_pending_tx);
+
+	htt->pending_tx = kzalloc(sizeof(*htt->pending_tx) *
+				  htt->max_num_pending_tx, GFP_KERNEL);
+	if (!htt->pending_tx)
+		return -ENOMEM;
+
+	htt->used_msdu_ids = kzalloc(sizeof(unsigned long) *
+				     BITS_TO_LONGS(htt->max_num_pending_tx),
+				     GFP_KERNEL);
+	if (!htt->used_msdu_ids) {
+		kfree(htt->pending_tx);
+		return -ENOMEM;
+	}
+
+	return 0;
 }
 
 static void ath10k_htt_tx_cleanup_pending(struct ath10k_htt *htt)
@@ -97,7 +122,7 @@ static void ath10k_htt_tx_cleanup_pending(struct ath10k_htt *htt)
 	/* No locks needed. Called after communication with the device has
 	 * been stopped. */
 
-	for (msdu_id = 0; msdu_id < HTT_MAX_NUM_PENDING_TX; msdu_id++) {
+	for (msdu_id = 0; msdu_id < htt->max_num_pending_tx; msdu_id++) {
 		if (!test_bit(msdu_id, htt->used_msdu_ids))
 			continue;
 
@@ -119,6 +144,8 @@ static void ath10k_htt_tx_cleanup_pending(struct ath10k_htt *htt)
 void ath10k_htt_tx_detach(struct ath10k_htt *htt)
 {
 	ath10k_htt_tx_cleanup_pending(htt);
+	kfree(htt->pending_tx);
+	kfree(htt->used_msdu_ids);
 	return;
 }
 
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 974f992..05bf3e9 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2582,7 +2582,7 @@ static void ath10k_flush(struct ieee80211_hw *hw, u32 queues, bool drop)
 			bool empty;
 			spin_lock_bh(&ar->htt->tx_lock);
 			empty = bitmap_empty(ar->htt->used_msdu_ids,
-					     HTT_MAX_NUM_PENDING_TX);
+					     ar->htt->max_num_pending_tx);
 			spin_unlock_bh(&ar->htt->tx_lock);
 			(empty);
 		}), ATH10K_FLUSH_TIMEOUT_HZ);
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index ce44ab1..f9d05bd 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -96,7 +96,7 @@ exit:
 	htt->pending_tx[ATH10K_SKB_CB(txdesc)->htt.msdu_id] = NULL;
 	ath10k_htt_tx_free_msdu_id(htt, ATH10K_SKB_CB(txdesc)->htt.msdu_id);
 	__ath10k_htt_tx_dec_pending(htt);
-	if (bitmap_empty(htt->used_msdu_ids, HTT_MAX_NUM_PENDING_TX))
+	if (bitmap_empty(htt->used_msdu_ids, htt->max_num_pending_tx))
 		wake_up(&htt->empty_tx_wq);
 	spin_unlock_bh(&htt->tx_lock);
 
@@ -111,7 +111,7 @@ void ath10k_txrx_tx_completed(struct ath10k_htt *htt,
 	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 (tx_done->msdu_id >= ARRAY_SIZE(htt->pending_tx)) {
+	if (tx_done->msdu_id >= htt->max_num_pending_tx) {
 		ath10k_warn("warning: msdu_id %d too big, ignoring\n",
 			    tx_done->msdu_id);
 		return;
-- 
1.7.9.5

[ath9k-devel] [PATCH 7/7] ath10k: detect htt pending tx limit at runtime

[Felix Fietkau]

On 2013-05-16 11:09 AM, Michal Kazior wrote:
> The real limit for sending htt tx is either
> msdu_id storage type (u16 - 65536 different
> values) or the HIF tx pipe queue length (2047 in
> case of our PCI HIF).
> 
> The htt tx pipe does not use interrupts - it must
> be polled. It is polled either on RX
> unconditionally or on TX if HIF tx pipe has used
> over 50% of the resources.
> 
> With this patch we should finally trigger the TX
> polling properly. What this really means we should
> have a smoother tx. Not because the tx limit is
> greater, but simply because we'll be triggering
> the polling properly in case of heavy tx.
> 
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
I think a queue length of 2047 is completely excessive. It causes way
too much buffering and latency under load, and I suspect it may be
significantly hurting TCP throughput as well (if TCP manages to get the
queue filled up).
As long as there is no way to dynamically determine a *reasonable* queue
size, introducing an arbitrary limit is *much* better than just letting
the firmware gobble up as much as it wants.
For reference: ath9k limits the number of pending tx packets to 128 per
WMM queue, and even at the highest MCS rates with 3x3 and HT40 this is
much more than what's actually needed.

- Felix

[ath9k-devel] [PATCH 0/7] ath10k: tx flow control fixes

[Kalle Valo]

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

> This addresses some `tx failed` issues and minor
> issues I've found along.
>
> This is a resend as a PATCH rather than an RFC.
>
> Updates include:
>  * added `ath10k: detect htt pending tx limit at runtime`
>  * dropped htc tx polling with a timer patch
>  * dropped explicit htt tx polling from
>    `ath10k: introduce proper htt tx flow control`
>    (this was deadlocking, and is no longer necessary anyway)
>
> Michal Kazior (7):
>   ath10k: change errno if we run out of msdu_ids
>   ath10k: ath10k_htc_prepare_tx_skb() never fails
>   ath10k: add lockdep asserts to htc skb dequeuing
>   ath10k: simplify htc flow control
>   ath10k: remove unused queue limit
>   ath10k: introduce proper htt tx flow control
>   ath10k: detect htt pending tx limit at runtime

Thanks, all patches applied.

-- 
Kalle Valo

[ath9k-devel] [PATCH 0/7] ath10k: tx flow control fixes

[Kalle Valo]

Kalle Valo <kvalo@qca.qualcomm.com> writes:

> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> This addresses some `tx failed` issues and minor
>> issues I've found along.
>>
>> This is a resend as a PATCH rather than an RFC.
>>
>> Updates include:
>>  * added `ath10k: detect htt pending tx limit at runtime`
>>  * dropped htc tx polling with a timer patch
>>  * dropped explicit htt tx polling from
>>    `ath10k: introduce proper htt tx flow control`
>>    (this was deadlocking, and is no longer necessary anyway)
>>
>> Michal Kazior (7):
>>   ath10k: change errno if we run out of msdu_ids
>>   ath10k: ath10k_htc_prepare_tx_skb() never fails
>>   ath10k: add lockdep asserts to htc skb dequeuing
>>   ath10k: simplify htc flow control
>>   ath10k: remove unused queue limit
>>   ath10k: introduce proper htt tx flow control
>>   ath10k: detect htt pending tx limit at runtime
>
> Thanks, all patches applied.

I got a new warning, can you please send a followup patch for that:

drivers/net/wireless/ath/ath10k/htt_tx.c:96: WARNING: line over 80 characters

-- 
Kalle Valo

[ath9k-devel] [PATCH] ath10k: fix sparse warning

[Michal Kazior]

The line was over 80 characters long.

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

diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index bd84153..ef79106 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -93,7 +93,8 @@ int ath10k_htt_tx_attach(struct ath10k_htt *htt)
 	/* At the beginning free queue number should hint us the maximum
 	 * queue length */
 	pipe = htt->ar->htc->endpoint[htt->eid].ul_pipe_id;
-	htt->max_num_pending_tx = ath10k_hif_get_free_queue_number(htt->ar, pipe);
+	htt->max_num_pending_tx = ath10k_hif_get_free_queue_number(htt->ar,
+								   pipe);
 
 	ath10k_dbg(ATH10K_DBG_HTT, "htt tx max num pending tx %d\n",
 		   htt->max_num_pending_tx);
-- 
1.7.9.5

[ath9k-devel] [PATCH] ath10k: fix sparse warning

[Kalle Valo]

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

> The line was over 80 characters long.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

Thanks, applied.

-- 
Kalle Valo