[PATCH v2 0/4] ath10k: fixes

[PATCH v2 0/4] ath10k: fixes

[Michal Kazior]

Hi,

Here are some fixes for ath10k. The rts threshold
patch addresses my mistake in commit
9aeb6fe53d1f0d6e58658484ce9ad7b59f0ef16b which
caused rts to be always enabled causing throughput
issues in some cases.

v2: first patch is split now into two as requested
    rebased on top of latest github master branch


Michal Kazior (4):
  ath10k: prevent HTC from being used after stopping
  ath10k: don't reset HTC endpoints unnecessarily
  ath10k: fix memleak in mac setup
  ath10k: fix rts/fragmentation threshold setup

 drivers/net/wireless/ath/ath10k/htc.c |   28 ++++++------
 drivers/net/wireless/ath/ath10k/htc.h |    4 +-
 drivers/net/wireless/ath/ath10k/mac.c |   80 ++++++++++++++++++---------------
 3 files changed, 58 insertions(+), 54 deletions(-)

-- 
1.7.9.5

Re: [PATCH v2 0/4] ath10k: fixes

[Kalle Valo]

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

> Hi,
>
> Here are some fixes for ath10k. The rts threshold
> patch addresses my mistake in commit
> 9aeb6fe53d1f0d6e58658484ce9ad7b59f0ef16b which
> caused rts to be always enabled causing throughput
> issues in some cases.
>
> v2: first patch is split now into two as requested
>     rebased on top of latest github master branch
>
>
> Michal Kazior (4):
>   ath10k: prevent HTC from being used after stopping
>   ath10k: don't reset HTC endpoints unnecessarily
>   ath10k: fix memleak in mac setup
>   ath10k: fix rts/fragmentation threshold setup

All four applied, thanks.

-- 
Kalle Valo

[PATCH 0/4] ath10k: fixes

[Michal Kazior]

Hi,

This patchset contains some bugfixes and
improvements.

Michal Kazior (4):
  ath10k: fix HTT service setup
  ath10k: fix HTC endpoint worker starvation
  ath10k: implement 802.3 SNAP rx decap type A-MSDU handling
  ath10k: plug possible memory leak in WMI

 drivers/net/wireless/ath/ath10k/htc.c    |   25 +++++++++++++------------
 drivers/net/wireless/ath/ath10k/htc.h    |    3 ++-
 drivers/net/wireless/ath/ath10k/htt_rx.c |   12 ++++++++++--
 drivers/net/wireless/ath/ath10k/wmi.c    |    1 +
 4 files changed, 26 insertions(+), 15 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] ath10k: fix HTT service setup

[Michal Kazior]

The "disable credit flow" flag was set too late
and it never was in the HTC service request
message.

This patch prevents firmware from reporting
(useless) HTC credits for HTT service. HTT service
doesn't use nor need credits.

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

diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index ef3329e..7d445d3 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -772,16 +772,16 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
 
 	flags |= SM(tx_alloc, ATH10K_HTC_CONN_FLAGS_RECV_ALLOC);
 
-	req_msg = &msg->connect_service;
-	req_msg->flags = __cpu_to_le16(flags);
-	req_msg->service_id = __cpu_to_le16(conn_req->service_id);
-
 	/* Only enable credit flow control for WMI ctrl service */
 	if (conn_req->service_id != ATH10K_HTC_SVC_ID_WMI_CONTROL) {
 		flags |= ATH10K_HTC_CONN_FLAGS_DISABLE_CREDIT_FLOW_CTRL;
 		disable_credit_flow_ctrl = true;
 	}
 
+	req_msg = &msg->connect_service;
+	req_msg->flags = __cpu_to_le16(flags);
+	req_msg->service_id = __cpu_to_le16(conn_req->service_id);
+
 	INIT_COMPLETION(htc->ctl_resp);
 
 	status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb);
-- 
1.7.9.5

[PATCH 2/4] ath10k: fix HTC endpoint worker starvation

[Michal Kazior]

HTC used a worker for each endpoint. This worked
until a slow host machine was flooded with massive
number of TX requests. HTT related worker would
remain active while WMI worker was starved. This
ended up with "could not send beacon" in AP mode.
It was even possible to see userspace being
starved.

The patch switches from using workers to using
tasklets for processing and submitting HTC frames
to HIF.

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

diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 7d445d3..4b1dd0e 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -15,6 +15,7 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
+#include <linux/interrupt.h>
 #include "core.h"
 #include "hif.h"
 #include "debug.h"
@@ -210,10 +211,9 @@ static struct sk_buff *ath10k_htc_get_skb_credit_based(struct ath10k_htc *htc,
 	return skb;
 }
 
-static void ath10k_htc_send_work(struct work_struct *work)
+static void ath10k_htc_send_task(unsigned long ptr)
 {
-	struct ath10k_htc_ep *ep = container_of(work,
-					struct ath10k_htc_ep, send_work);
+	struct ath10k_htc_ep *ep = (struct ath10k_htc_ep *)ptr;
 	struct ath10k_htc *htc = ep->htc;
 	struct sk_buff *skb;
 	u8 credits = 0;
@@ -264,7 +264,7 @@ int ath10k_htc_send(struct ath10k_htc *htc,
 	skb_push(skb, sizeof(struct ath10k_htc_hdr));
 	spin_unlock_bh(&htc->tx_lock);
 
-	queue_work(htc->ar->workqueue, &ep->send_work);
+	tasklet_schedule(&ep->send_task);
 	return 0;
 }
 
@@ -283,7 +283,7 @@ static int ath10k_htc_tx_completion_handler(struct ath10k *ar,
 	 * we recheck after the packet completes */
 	spin_lock_bh(&htc->tx_lock);
 	if (!ep->tx_credit_flow_enabled && !htc->stopped)
-		queue_work(ar->workqueue, &ep->send_work);
+		tasklet_schedule(&ep->send_task);
 	spin_unlock_bh(&htc->tx_lock);
 
 	return 0;
@@ -308,7 +308,7 @@ static void ath10k_htc_flush_endpoint_tx(struct ath10k_htc *htc,
 	}
 	spin_unlock_bh(&htc->tx_lock);
 
-	cancel_work_sync(&ep->send_work);
+	tasklet_kill(&ep->send_task);
 }
 
 /***********/
@@ -341,7 +341,7 @@ ath10k_htc_process_credit_report(struct ath10k_htc *htc,
 		ep->tx_credits += report->credits;
 
 		if (ep->tx_credits && !skb_queue_empty(&ep->tx_queue))
-			queue_work(htc->ar->workqueue, &ep->send_work);
+			tasklet_schedule(&ep->send_task);
 	}
 	spin_unlock_bh(&htc->tx_lock);
 }
@@ -602,7 +602,8 @@ static void ath10k_htc_reset_endpoint_states(struct ath10k_htc *htc)
 		skb_queue_head_init(&ep->tx_queue);
 		ep->htc = htc;
 		ep->tx_credit_flow_enabled = true;
-		INIT_WORK(&ep->send_work, ath10k_htc_send_work);
+		tasklet_init(&ep->send_task, ath10k_htc_send_task,
+			      (unsigned long)ep);
 	}
 }
 
diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h
index e1dd8c7..6afa175 100644
--- a/drivers/net/wireless/ath/ath10k/htc.h
+++ b/drivers/net/wireless/ath/ath10k/htc.h
@@ -24,6 +24,7 @@
 #include <linux/skbuff.h>
 #include <linux/semaphore.h>
 #include <linux/timer.h>
+#include <linux/interrupt.h>
 
 struct ath10k;
 
@@ -323,7 +324,7 @@ struct ath10k_htc_ep {
 	int tx_credits_per_max_message;
 	bool tx_credit_flow_enabled;
 
-	struct work_struct send_work;
+	struct tasklet_struct send_task;
 };
 
 struct ath10k_htc_svc_tx_credits {
-- 
1.7.9.5

[PATCH 3/4] ath10k: implement 802.3 SNAP rx decap type A-MSDU handling

[Michal Kazior]

This enables driver to rx another decapped a-msdu
frames. It should possibly help with throughputs
in some cases and reduce (or eliminate) number of
messages like this:

  ath10k: error processing msdus -524

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

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index e784c40..9b93aed 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -610,8 +610,7 @@ static int ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
 			RX_MPDU_START_INFO0_ENCRYPT_TYPE);
 
 	/* FIXME: No idea what assumptions are safe here. Need logs */
-	if ((fmt == RX_MSDU_DECAP_RAW && skb->next) ||
-	    (fmt == RX_MSDU_DECAP_8023_SNAP_LLC)) {
+	if ((fmt == RX_MSDU_DECAP_RAW && skb->next)) {
 		ath10k_htt_rx_free_msdu_chain(skb->next);
 		skb->next = NULL;
 		return -ENOTSUPP;
@@ -659,6 +658,15 @@ static int ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
 			decap_hdr += roundup(crypto_len, 4);
 		}
 
+		if (fmt == RX_MSDU_DECAP_8023_SNAP_LLC) {
+			/* SNAP 802.3 consists of:
+			 * [dst:6][src:6][len:2][dsap:1][ssap:1][ctl:1][snap:5]
+			 * [data][fcs:4].
+			 *
+			 * Since this overlaps with A-MSDU header (da, sa, len)
+			 * there's nothing extra to do. */
+		}
+
 		if (fmt == RX_MSDU_DECAP_ETHERNET2_DIX) {
 			/* Ethernet2 decap inserts ethernet header in place of
 			 * A-MSDU subframe header. */
-- 
1.7.9.5

[PATCH 4/4] ath10k: plug possible memory leak in WMI

[Michal Kazior]

There was a possible memory leak when WMI command
queue reached it's limit. Command buffers were not
freed.

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

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 55f90c7..775fedf 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -110,6 +110,7 @@ static int ath10k_wmi_cmd_send(struct ath10k *ar, struct sk_buff *skb,
 	if (atomic_add_return(1, &ar->wmi.pending_tx_count) >
 	    WMI_MAX_PENDING_TX_COUNT) {
 		/* avoid using up memory when FW hangs */
+		dev_kfree_skb(skb);
 		atomic_dec(&ar->wmi.pending_tx_count);
 		return -EBUSY;
 	}
-- 
1.7.9.5

Re: [PATCH 2/4] ath10k: fix HTC endpoint worker starvation

[Michal Kazior]

On 8 August 2013 10:16, Michal Kazior <michal.kazior@tieto.com> wrote:
> HTC used a worker for each endpoint. This worked
> until a slow host machine was flooded with massive
> number of TX requests. HTT related worker would
> remain active while WMI worker was starved. This
> ended up with "could not send beacon" in AP mode.
> It was even possible to see userspace being
> starved.
>
> The patch switches from using workers to using
> tasklets for processing and submitting HTC frames
> to HIF.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> (...)
>
> @@ -602,7 +602,8 @@ static void ath10k_htc_reset_endpoint_states(struct ath10k_htc *htc)
>                 skb_queue_head_init(&ep->tx_queue);
>                 ep->htc = htc;
>                 ep->tx_credit_flow_enabled = true;
> -               INIT_WORK(&ep->send_work, ath10k_htc_send_work);
> +               tasklet_init(&ep->send_task, ath10k_htc_send_task,
> +                             (unsigned long)ep);
>         }
>  }
>

Indentation is off by one here. Is it okay if I just resend this
single patch or is it easier for applying if I resend the whole
patchset?


Pozdrawiam / Best regards,
Michał Kazior.

Re: [PATCH 3/4] ath10k: implement 802.3 SNAP rx decap type A-MSDU handling

[Kalle Valo]

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

> This enables driver to rx another decapped a-msdu
> frames. It should possibly help with throughputs
> in some cases and reduce (or eliminate) number of
> messages like this:
>
>   ath10k: error processing msdus -524
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> @@ -659,6 +658,15 @@ static int ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
>  			decap_hdr += roundup(crypto_len, 4);
>  		}
>  
> +		if (fmt == RX_MSDU_DECAP_8023_SNAP_LLC) {
> +			/* SNAP 802.3 consists of:
> +			 * [dst:6][src:6][len:2][dsap:1][ssap:1][ctl:1][snap:5]
> +			 * [data][fcs:4].
> +			 *
> +			 * Since this overlaps with A-MSDU header (da, sa, len)
> +			 * there's nothing extra to do. */
> +		}

This block doesn't have any code, is that on purpose? Most likely a
static checker finds this later and we need to remove it.

If your idea is to document the LLC case (which is very good!) it's
better to do everything inside a comment, for example like this:

	/* When fmt == RX_MSDU_DECAP_8023_SNAP_LLC:
         *
         * SNAP 802.3 consists of:
	 * [dst:6][src:6][len:2][dsap:1][ssap:1][ctl:1][snap:5]
	 * [data][fcs:4].
	 *
	 * Since this overlaps with A-MSDU header (da, sa, len)
	 * there's nothing extra to do. */

-- 
Kalle Valo

Re: [PATCH 2/4] ath10k: fix HTC endpoint worker starvation

[Kalle Valo]

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

>> @@ -602,7 +602,8 @@ static void ath10k_htc_reset_endpoint_states(struct ath10k_htc *htc)
>>                 skb_queue_head_init(&ep->tx_queue);
>>                 ep->htc = htc;
>>                 ep->tx_credit_flow_enabled = true;
>> -               INIT_WORK(&ep->send_work, ath10k_htc_send_work);
>> +               tasklet_init(&ep->send_task, ath10k_htc_send_task,
>> +                             (unsigned long)ep);
>>         }
>>  }
>>
>
> Indentation is off by one here. Is it okay if I just resend this
> single patch or is it easier for applying if I resend the whole
> patchset?

It's errorprone if I start to pick patches from different sets. So
please resend the whole series. But take a look at my other comment
first.

-- 
Kalle Valo

Re: [PATCH 3/4] ath10k: implement 802.3 SNAP rx decap type A-MSDU handling

[Michal Kazior]

On 12 August 2013 16:55, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> This enables driver to rx another decapped a-msdu
>> frames. It should possibly help with throughputs
>> in some cases and reduce (or eliminate) number of
>> messages like this:
>>
>>   ath10k: error processing msdus -524
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> [...]
>
>> @@ -659,6 +658,15 @@ static int ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
>>                       decap_hdr += roundup(crypto_len, 4);
>>               }
>>
>> +             if (fmt == RX_MSDU_DECAP_8023_SNAP_LLC) {
>> +                     /* SNAP 802.3 consists of:
>> +                      * [dst:6][src:6][len:2][dsap:1][ssap:1][ctl:1][snap:5]
>> +                      * [data][fcs:4].
>> +                      *
>> +                      * Since this overlaps with A-MSDU header (da, sa, len)
>> +                      * there's nothing extra to do. */
>> +             }
>
> This block doesn't have any code, is that on purpose? Most likely a
> static checker finds this later and we need to remove it.
>
> If your idea is to document the LLC case (which is very good!) it's
> better to do everything inside a comment, for example like this:
>
>         /* When fmt == RX_MSDU_DECAP_8023_SNAP_LLC:
>          *
>          * SNAP 802.3 consists of:
>          * [dst:6][src:6][len:2][dsap:1][ssap:1][ctl:1][snap:5]
>          * [data][fcs:4].
>          *
>          * Since this overlaps with A-MSDU header (da, sa, len)
>          * there's nothing extra to do. */

Yes, it's on purpose. I'll make a comment out of it then.


Pozdrawiam / Best regards,
Michał Kazior.

[PATCH v2 0/4] ath10k: fixes

[Michal Kazior]

Hi,

This patchset contains some bugfixes and
improvements.

v2:
 * fix indentation
 * use a comment instead of a no-op if clause


Michal Kazior (4):
  ath10k: fix HTT service setup
  ath10k: fix HTC endpoint worker starvation
  ath10k: implement 802.3 SNAP rx decap type A-MSDU handling
  ath10k: plug possible memory leak in WMI

 drivers/net/wireless/ath/ath10k/htc.c    |   25 +++++++++++++------------
 drivers/net/wireless/ath/ath10k/htc.h    |    3 ++-
 drivers/net/wireless/ath/ath10k/htt_rx.c |   12 ++++++++++--
 drivers/net/wireless/ath/ath10k/wmi.c    |    1 +
 4 files changed, 26 insertions(+), 15 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/4] ath10k: fix HTT service setup

[Michal Kazior]

The "disable credit flow" flag was set too late
and it never was in the HTC service request
message.

This patch prevents firmware from reporting
(useless) HTC credits for HTT service. HTT service
doesn't use nor need credits.

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

diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index ef3329e..7d445d3 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -772,16 +772,16 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
 
 	flags |= SM(tx_alloc, ATH10K_HTC_CONN_FLAGS_RECV_ALLOC);
 
-	req_msg = &msg->connect_service;
-	req_msg->flags = __cpu_to_le16(flags);
-	req_msg->service_id = __cpu_to_le16(conn_req->service_id);
-
 	/* Only enable credit flow control for WMI ctrl service */
 	if (conn_req->service_id != ATH10K_HTC_SVC_ID_WMI_CONTROL) {
 		flags |= ATH10K_HTC_CONN_FLAGS_DISABLE_CREDIT_FLOW_CTRL;
 		disable_credit_flow_ctrl = true;
 	}
 
+	req_msg = &msg->connect_service;
+	req_msg->flags = __cpu_to_le16(flags);
+	req_msg->service_id = __cpu_to_le16(conn_req->service_id);
+
 	INIT_COMPLETION(htc->ctl_resp);
 
 	status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb);
-- 
1.7.9.5

[PATCH v2 2/4] ath10k: fix HTC endpoint worker starvation

[Michal Kazior]

HTC used a worker for each endpoint. This worked
until a slow host machine was flooded with massive
number of TX requests. HTT related worker would
remain active while WMI worker was starved. This
ended up with "could not send beacon" in AP mode.
It was even possible to see userspace being
starved.

The patch switches from using workers to using
tasklets for processing and submitting HTC frames
to HIF.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v2:
 * fix indentation

 drivers/net/wireless/ath/ath10k/htc.c |   17 +++++++++--------
 drivers/net/wireless/ath/ath10k/htc.h |    3 ++-
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 7d445d3..958e048 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -15,6 +15,7 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
+#include <linux/interrupt.h>
 #include "core.h"
 #include "hif.h"
 #include "debug.h"
@@ -210,10 +211,9 @@ static struct sk_buff *ath10k_htc_get_skb_credit_based(struct ath10k_htc *htc,
 	return skb;
 }
 
-static void ath10k_htc_send_work(struct work_struct *work)
+static void ath10k_htc_send_task(unsigned long ptr)
 {
-	struct ath10k_htc_ep *ep = container_of(work,
-					struct ath10k_htc_ep, send_work);
+	struct ath10k_htc_ep *ep = (struct ath10k_htc_ep *)ptr;
 	struct ath10k_htc *htc = ep->htc;
 	struct sk_buff *skb;
 	u8 credits = 0;
@@ -264,7 +264,7 @@ int ath10k_htc_send(struct ath10k_htc *htc,
 	skb_push(skb, sizeof(struct ath10k_htc_hdr));
 	spin_unlock_bh(&htc->tx_lock);
 
-	queue_work(htc->ar->workqueue, &ep->send_work);
+	tasklet_schedule(&ep->send_task);
 	return 0;
 }
 
@@ -283,7 +283,7 @@ static int ath10k_htc_tx_completion_handler(struct ath10k *ar,
 	 * we recheck after the packet completes */
 	spin_lock_bh(&htc->tx_lock);
 	if (!ep->tx_credit_flow_enabled && !htc->stopped)
-		queue_work(ar->workqueue, &ep->send_work);
+		tasklet_schedule(&ep->send_task);
 	spin_unlock_bh(&htc->tx_lock);
 
 	return 0;
@@ -308,7 +308,7 @@ static void ath10k_htc_flush_endpoint_tx(struct ath10k_htc *htc,
 	}
 	spin_unlock_bh(&htc->tx_lock);
 
-	cancel_work_sync(&ep->send_work);
+	tasklet_kill(&ep->send_task);
 }
 
 /***********/
@@ -341,7 +341,7 @@ ath10k_htc_process_credit_report(struct ath10k_htc *htc,
 		ep->tx_credits += report->credits;
 
 		if (ep->tx_credits && !skb_queue_empty(&ep->tx_queue))
-			queue_work(htc->ar->workqueue, &ep->send_work);
+			tasklet_schedule(&ep->send_task);
 	}
 	spin_unlock_bh(&htc->tx_lock);
 }
@@ -602,7 +602,8 @@ static void ath10k_htc_reset_endpoint_states(struct ath10k_htc *htc)
 		skb_queue_head_init(&ep->tx_queue);
 		ep->htc = htc;
 		ep->tx_credit_flow_enabled = true;
-		INIT_WORK(&ep->send_work, ath10k_htc_send_work);
+		tasklet_init(&ep->send_task, ath10k_htc_send_task,
+			     (unsigned long)ep);
 	}
 }
 
diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h
index e1dd8c7..6afa175 100644
--- a/drivers/net/wireless/ath/ath10k/htc.h
+++ b/drivers/net/wireless/ath/ath10k/htc.h
@@ -24,6 +24,7 @@
 #include <linux/skbuff.h>
 #include <linux/semaphore.h>
 #include <linux/timer.h>
+#include <linux/interrupt.h>
 
 struct ath10k;
 
@@ -323,7 +324,7 @@ struct ath10k_htc_ep {
 	int tx_credits_per_max_message;
 	bool tx_credit_flow_enabled;
 
-	struct work_struct send_work;
+	struct tasklet_struct send_task;
 };
 
 struct ath10k_htc_svc_tx_credits {
-- 
1.7.9.5

[PATCH v2 3/4] ath10k: implement 802.3 SNAP rx decap type A-MSDU handling

[Michal Kazior]

This enables driver to rx another decapped a-msdu
frames. It should possibly help with throughputs
in some cases and reduce (or eliminate) number of
messages like this:

  ath10k: error processing msdus -524

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v2:
 * use a comment instead of a no-op if clause

 drivers/net/wireless/ath/ath10k/htt_rx.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index e784c40..9bb0ae89 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -610,8 +610,7 @@ static int ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
 			RX_MPDU_START_INFO0_ENCRYPT_TYPE);
 
 	/* FIXME: No idea what assumptions are safe here. Need logs */
-	if ((fmt == RX_MSDU_DECAP_RAW && skb->next) ||
-	    (fmt == RX_MSDU_DECAP_8023_SNAP_LLC)) {
+	if ((fmt == RX_MSDU_DECAP_RAW && skb->next)) {
 		ath10k_htt_rx_free_msdu_chain(skb->next);
 		skb->next = NULL;
 		return -ENOTSUPP;
@@ -659,6 +658,15 @@ static int ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
 			decap_hdr += roundup(crypto_len, 4);
 		}
 
+		/* When fmt == RX_MSDU_DECAP_8023_SNAP_LLC:
+		 *
+		 * SNAP 802.3 consists of:
+		 * [dst:6][src:6][len:2][dsap:1][ssap:1][ctl:1][snap:5]
+		 * [data][fcs:4].
+		 *
+		 * Since this overlaps with A-MSDU header (da, sa, len)
+		 * there's nothing extra to do. */
+
 		if (fmt == RX_MSDU_DECAP_ETHERNET2_DIX) {
 			/* Ethernet2 decap inserts ethernet header in place of
 			 * A-MSDU subframe header. */
-- 
1.7.9.5

[PATCH v2 4/4] ath10k: plug possible memory leak in WMI

[Michal Kazior]

There was a possible memory leak when WMI command
queue reached it's limit. Command buffers were not
freed.

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

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 55f90c7..775fedf 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -110,6 +110,7 @@ static int ath10k_wmi_cmd_send(struct ath10k *ar, struct sk_buff *skb,
 	if (atomic_add_return(1, &ar->wmi.pending_tx_count) >
 	    WMI_MAX_PENDING_TX_COUNT) {
 		/* avoid using up memory when FW hangs */
+		dev_kfree_skb(skb);
 		atomic_dec(&ar->wmi.pending_tx_count);
 		return -EBUSY;
 	}
-- 
1.7.9.5

Re: [PATCH v2 2/4] ath10k: fix HTC endpoint worker starvation

[Michal Kazior]

On 13 August 2013 07:59, Michal Kazior <michal.kazior@tieto.com> wrote:
> HTC used a worker for each endpoint. This worked
> until a slow host machine was flooded with massive
> number of TX requests. HTT related worker would
> remain active while WMI worker was starved. This
> ended up with "could not send beacon" in AP mode.
> It was even possible to see userspace being
> starved.
>
> The patch switches from using workers to using
> tasklets for processing and submitting HTC frames
> to HIF.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

I'd like drop this patch, at least for now. I'm having doubts this is
the right way to solve the problem.


Pozdrawiam / Best regards,
Michał Kazior.

Re: [PATCH v2 0/4] ath10k: fixes

[Kalle Valo]

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

> Hi,
>
> This patchset contains some bugfixes and
> improvements.
>
> v2:
>  * fix indentation
>  * use a comment instead of a no-op if clause
>

Thanks, these three patches applied:

>   ath10k: fix HTT service setup
>   ath10k: implement 802.3 SNAP rx decap type A-MSDU handling
>   ath10k: plug possible memory leak in WMI

This patch dropped per your request:

>   ath10k: fix HTC endpoint worker starvation

-- 
Kalle Valo

[PATCH v2 0/4] ath10k: fixes

[Michal Kazior]

Hi,

This patchset addresses some throughput/stability
issues.

v2:
 * add new patch
 * fix WARNING due to wrong ieee80211_rx() usage


Pozdrawiam / Best regards,
Michal Kazior.


Michal Kazior (4):
  ath10k: synchronize tx/rx reporting to mac80211
  ath10k: make the workqueue multithreaded
  ath10k: move htt rx processing to worker
  ath10k: fix issues on non-preemptible systems

 drivers/net/wireless/ath/ath10k/core.c   |    5 ++-
 drivers/net/wireless/ath/ath10k/core.h   |   10 ++++++
 drivers/net/wireless/ath/ath10k/htc.c    |    4 +++
 drivers/net/wireless/ath/ath10k/htt.h    |    3 ++
 drivers/net/wireless/ath/ath10k/htt_rx.c |   58 ++++++++++++++++++++++++++----
 drivers/net/wireless/ath/ath10k/txrx.c   |    6 +++-
 drivers/net/wireless/ath/ath10k/wmi.c    |    4 +++
 7 files changed, 81 insertions(+), 9 deletions(-)

-- 
1.7.9.5

Re: [PATCH v2 0/4] ath10k: fixes

[Luis R. Rodriguez]

On Mon, Aug 26, 2013 at 1:53 AM, Michal Kazior <michal.kazior@tieto.com> wrote:
>   ath10k: fix issues on non-preemptible systems

This patch looks like a stable candidate fix. Please annotate as such
if you confirm. Also, I reviewed other ath10k "fixes" and I see no
practice of propagating any patches to stable yet. Can you please
start doing that? If there were patches which are already merged
upstream that should be propagated to stable then they can be
submitted as stable candidate patches.

  Luis

Re: [PATCH v2 0/4] ath10k: fixes

[Michal Kazior]

On 26 August 2013 22:20, Luis R. Rodriguez <mcgrof@do-not-panic.com> wrote:
> On Mon, Aug 26, 2013 at 1:53 AM, Michal Kazior <michal.kazior@tieto.com> wrote:
>>   ath10k: fix issues on non-preemptible systems
>
> This patch looks like a stable candidate fix. Please annotate as such
> if you confirm. Also, I reviewed other ath10k "fixes" and I see no
> practice of propagating any patches to stable yet. Can you please
> start doing that? If there were patches which are already merged
> upstream that should be propagated to stable then they can be
> submitted as stable candidate patches.

Good point.

If this patch is to be propagated to stable the patch(set) needs some
adjustments to avoid conflicts. Right now the patch touches a place
that was introduced with "ath10k: move htt rx processing to worker" so
the offending patch hunk should be moved to that patch.

Actually the "ath10k: make the workqueue multithreaded" could also be
a candidate for stable as it fixes AP beaconing during heavy TX
traffic. I should've mentioned that in the commit log.

I'll resend an updated patchset later today.


Michał.

Re: [PATCH v2 0/4] ath10k: fixes

[Kalle Valo]

"Luis R. Rodriguez" <mcgrof@do-not-panic.com> writes:

> On Mon, Aug 26, 2013 at 1:53 AM, Michal Kazior <michal.kazior@tieto.com> wrote:
>>   ath10k: fix issues on non-preemptible systems
>
> This patch looks like a stable candidate fix. Please annotate as such
> if you confirm. Also, I reviewed other ath10k "fixes" and I see no
> practice of propagating any patches to stable yet. Can you please
> start doing that? If there were patches which are already merged
> upstream that should be propagated to stable then they can be
> submitted as stable candidate patches.

I disagree. The point of linux-stable is _not_ that we send all possible
fixes to stable. Instead we should send fixes only which really matter
to users and for which we have received bug reports. I haven't yet seen
any fix for ath10k which should be a candidate for stable releases.

If we start sending all ath10k fixes to stable it's just extra churn for
both Greg and people working on ath10k.

-- 
Kalle Valo

Re: [PATCH v2 0/4] ath10k: fixes

[Luis R. Rodriguez]

On Tue, Aug 27, 2013 at 07:42:39AM +0200, Michal Kazior wrote:
> On 26 August 2013 22:20, Luis R. Rodriguez <mcgrof@do-not-panic.com> wrote:
> > On Mon, Aug 26, 2013 at 1:53 AM, Michal Kazior <michal.kazior@tieto.com> wrote:
> >>   ath10k: fix issues on non-preemptible systems
> >
> > This patch looks like a stable candidate fix. Please annotate as such
> > if you confirm. Also, I reviewed other ath10k "fixes" and I see no
> > practice of propagating any patches to stable yet. Can you please
> > start doing that? If there were patches which are already merged
> > upstream that should be propagated to stable then they can be
> > submitted as stable candidate patches.
> 
> Good point.
> 
> If this patch is to be propagated to stable the patch(set) needs some
> adjustments to avoid conflicts. Right now the patch touches a place
> that was introduced with "ath10k: move htt rx processing to worker" so
> the offending patch hunk should be moved to that patch.
> 
> Actually the "ath10k: make the workqueue multithreaded" could also be
> a candidate for stable as it fixes AP beaconing during heavy TX
> traffic. I should've mentioned that in the commit log.
> 
> I'll resend an updated patchset later today.

That one doesn't seem like a stable fix, it does quite a big change.

  Luis

Re: [PATCH v2 0/4] ath10k: fixes

[Luis R. Rodriguez]

On Tue, Aug 27, 2013 at 09:57:22AM +0300, Kalle Valo wrote:
> "Luis R. Rodriguez" <mcgrof@do-not-panic.com> writes:
> 
> > On Mon, Aug 26, 2013 at 1:53 AM, Michal Kazior <michal.kazior@tieto.com> wrote:
> >>   ath10k: fix issues on non-preemptible systems
> >
> > This patch looks like a stable candidate fix. Please annotate as such
> > if you confirm. Also, I reviewed other ath10k "fixes" and I see no
> > practice of propagating any patches to stable yet. Can you please
> > start doing that? If there were patches which are already merged
> > upstream that should be propagated to stable then they can be
> > submitted as stable candidate patches.
> 
> I disagree. The point of linux-stable is _not_ that we send all possible
> fixes to stable. Instead we should send fixes only which really matter
> to users and for which we have received bug reports. I haven't yet seen
> any fix for ath10k which should be a candidate for stable releases.

You don't need to wait for an issue to happen to consider it serious,
the description of the symptoms seem pretty bad to me, but its your
call in the end.

> If we start sending all ath10k fixes to stable it's just extra churn for
> both Greg and people working on ath10k.

I'm not asking for anything that has the word "fix" to be sent, I'm
asking them to be reviewed for stable consideration.

  Luis

Re: [PATCH v2 0/4] ath10k: fixes

[Kalle Valo]

"Luis R. Rodriguez" <rodrigue@qca.qualcomm.com> writes:

>> I disagree. The point of linux-stable is _not_ that we send all possible
>> fixes to stable. Instead we should send fixes only which really matter
>> to users and for which we have received bug reports. I haven't yet seen
>> any fix for ath10k which should be a candidate for stable releases.
>
> You don't need to wait for an issue to happen to consider it serious,
> the description of the symptoms seem pretty bad to me, but its your
> call in the end.
>
>> If we start sending all ath10k fixes to stable it's just extra churn for
>> both Greg and people working on ath10k.
>
> I'm not asking for anything that has the word "fix" to be sent, I'm
> asking them to be reviewed for stable consideration.

I think a good rule is that we should send fixes to stable only if a
user (!= developer) has reported the problem.

-- 
Kalle Valo