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

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

[Michal Kazior]

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

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: add active htc ul pipe polling support
  ath10k: introduce proper htt tx flow control

 drivers/net/wireless/ath/ath10k/htc.c    |  120 +++++++++---------------------
 drivers/net/wireless/ath/ath10k/htc.h    |    6 +-
 drivers/net/wireless/ath/ath10k/htt.c    |   18 -----
 drivers/net/wireless/ath/ath10k/htt.h    |   17 +----
 drivers/net/wireless/ath/ath10k/htt_tx.c |   61 ++++++++++++++-
 drivers/net/wireless/ath/ath10k/txrx.c   |    1 +
 6 files changed, 101 insertions(+), 122 deletions(-)

-- 
1.7.9.5

[ath9k-devel] [RFC/RFT 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] [RFC/RFT 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] [RFC/RFT 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] [RFC/RFT 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] [RFC/RFT 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] [RFC/RFT 6/7] ath10k: add active htc ul pipe polling support

[Michal Kazior]

Until now we depended solely on passive or lazy
polling for tx completion.

The passive was in htc rx handler. The lazy in
htc_send_work(). The lazy would fire only if there
are less than 50% resources free. However for HTT
tx we have 2047 resources and we never used more
than 512 (HTT_MAX_PENDING_TX).

It is a good idea to have a timer that polls for
tx completions "just in case". This could help if
mac80211 is waiting for tx status and there's no
tx/rx happening.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/htc.c |   14 ++++++++++++++
 drivers/net/wireless/ath/ath10k/htc.h |    2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 74363c9..fed83b0 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -33,6 +33,13 @@ static inline void ath10k_htc_send_complete_check(struct ath10k_htc_ep *ep,
 	ath10k_hif_send_complete_check(ep->htc->ar, ep->ul_pipe_id, force);
 }
 
+static void ath10k_htc_ul_poll(unsigned long ptr)
+{
+	struct ath10k_htc_ep *ep = (struct ath10k_htc_ep *)ptr;
+
+	ath10k_htc_send_complete_check(ep, 1);
+}
+
 static void ath10k_htc_control_tx_complete(struct ath10k *ar,
 					   struct sk_buff *skb)
 {
@@ -237,6 +244,10 @@ static void ath10k_htc_send_work(struct work_struct *work)
 		ret = ath10k_htc_issue_skb(htc, ep, skb, credits);
 		if (ret == -ENOSR)
 			break;
+
+		if (ret == 0 && ep->ul_is_polled)
+			mod_timer(&ep->ul_poll_timer, jiffies +
+				  msecs_to_jiffies(ATH10K_HTC_UL_POLL_DELAY_MS));
 	}
 }
 
@@ -308,6 +319,7 @@ static void ath10k_htc_flush_endpoint_tx(struct ath10k_htc *htc,
 	spin_unlock_bh(&htc->tx_lock);
 
 	cancel_work_sync(&ep->send_work);
+	del_timer_sync(&ep->ul_poll_timer);
 }
 
 /***********/
@@ -602,6 +614,8 @@ static void ath10k_htc_reset_endpoint_states(struct ath10k_htc *htc)
 		ep->htc = htc;
 		ep->tx_credit_flow_enabled = true;
 		INIT_WORK(&ep->send_work, ath10k_htc_send_work);
+		setup_timer(&ep->ul_poll_timer, ath10k_htc_ul_poll,
+			    (unsigned long)ep);
 	}
 }
 
diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h
index fa45844..7c09bf6 100644
--- a/drivers/net/wireless/ath/ath10k/htc.h
+++ b/drivers/net/wireless/ath/ath10k/htc.h
@@ -301,6 +301,7 @@ struct ath10k_htc_svc_conn_resp {
 #define ATH10K_HTC_CONTROL_BUFFER_SIZE (ATH10K_HTC_MAX_CTRL_MSG_LEN + \
 					sizeof(struct ath10k_htc_hdr))
 #define ATH10K_HTC_CONN_SVC_TIMEOUT_HZ (1*HZ)
+#define ATH10K_HTC_UL_POLL_DELAY_MS 20
 
 struct ath10k_htc_ep {
 	struct ath10k_htc *htc;
@@ -324,6 +325,7 @@ struct ath10k_htc_ep {
 	bool tx_credit_flow_enabled;
 
 	struct work_struct send_work;
+	struct timer_list ul_poll_timer;
 };
 
 struct ath10k_htc_svc_tx_credits {
-- 
1.7.9.5

[ath9k-devel] [RFC/RFT 6/7] ath10k: add active htc ul pipe polling support

[Kalle Valo]

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

> Until now we depended solely on passive or lazy
> polling for tx completion.
>
> The passive was in htc rx handler. The lazy in
> htc_send_work(). The lazy would fire only if there
> are less than 50% resources free. However for HTT
> tx we have 2047 resources and we never used more
> than 512 (HTT_MAX_PENDING_TX).
>
> It is a good idea to have a timer that polls for
> tx completions "just in case". This could help if
> mac80211 is waiting for tx status and there's no
> tx/rx happening.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

I haven't tested your patches yet, but I'm a bit concerned about the
timer. Timers usually create problems of their own and using them wrong
affect power consumption.

I don't have time to read the patches in detail, but can you give a
short summary how the timer works? How often is it fired? What happens
when the data path is idle?

And is it absolutely necessary to use a timer? Can't we use tx
completions or some other existing event from firmware to accomplish the
same?

-- 
Kalle Valo

[ath9k-devel] [RFC/RFT 6/7] ath10k: add active htc ul pipe polling support

[Michal Kazior]

On 15/05/13 14:37, Kalle Valo wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> Until now we depended solely on passive or lazy
>> polling for tx completion.
>>
>> The passive was in htc rx handler. The lazy in
>> htc_send_work(). The lazy would fire only if there
>> are less than 50% resources free. However for HTT
>> tx we have 2047 resources and we never used more
>> than 512 (HTT_MAX_PENDING_TX).
>>
>> It is a good idea to have a timer that polls for
>> tx completions "just in case". This could help if
>> mac80211 is waiting for tx status and there's no
>> tx/rx happening.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> I haven't tested your patches yet, but I'm a bit concerned about the
> timer. Timers usually create problems of their own and using them wrong
> affect power consumption.
>
> I don't have time to read the patches in detail, but can you give a
> short summary how the timer works? How often is it fired? What happens
> when the data path is idle?

The timer is fired after tx path has became idle after some tx had been 
done. This should make us report tx status to mac80211 more quickly 
(rather than waiting for a beacon, or other frame on HTT rx that would 
trigger the polling).

We could probably try a different scheme - to poll every 20ms as long as 
there is any tx pending. That would be a lot better wrt reporting tx 
status quickly.

Let's just drop this patch. It's not really necessary, at least for now.


> And is it absolutely necessary to use a timer? Can't we use tx
> completions or some other existing event from firmware to accomplish the
> same?

HTT tx is done on a pipe that has interrupts disabled. We need to poll 
for tx completions. We poll them in 2 cases:

  a) when we receive a frame on HTT rx (it's a different pipe, with 
interrupts)

  b) we submit a new HTC frame (so HTT tx request counts) and tx pipe 
resources are drained below 50% (which never happens, because we have 
512 msdu_ids limit, and there are 2047 tx resources on the HTT tx pipe)

The (b) case is solved in patch #7, although it could be done 
differently (by increasing msdu_ids limit to tx pipe limit which is 2047).


-- Pozdrawiam / Best regards, Michal Kazior.

[ath9k-devel] [RFC/RFT 7/7] ath10k: introduce proper htt tx flow control

[Michal Kazior]

We were missing proper frame tx flow control. The
driver would print 'tx failed' warnings if there
was a lot of small skbuff to be sent because we'd
run out of msdu_ids. This could happen because we
didn't have any flow control to prevent mac80211
from sending us more frames

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

We also actively poll ul pipe to prevent from
running out of msdu_ids.

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 |   59 ++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/txrx.c   |    1 +
 3 files changed, 62 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..3b78f42 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -18,9 +18,57 @@
 #include <linux/etherdevice.h>
 #include "htt.h"
 #include "mac.h"
+#include "hif.h"
 #include "txrx.h"
 #include "debug.h"
 
+static void ath10k_htt_tx_poll_completions(struct ath10k_htt *htt)
+{
+	u8 pipe_id = htt->ar->htc->endpoint[htt->eid].ul_pipe_id;
+
+	if (!htt->ar->htc->endpoint[htt->eid].ul_is_polled)
+		return;
+
+	ath10k_hif_send_complete_check(htt->ar, pipe_id, 1);
+}
+
+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);
+
+	if (htt->num_pending_tx >= HTT_MAX_NUM_PENDING_TX / 2)
+		ath10k_htt_tx_poll_completions(htt);
+
+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 +286,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 +348,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 +370,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 +489,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