[PATCH 0/3] ath10k: fixes

[PATCH 0/3] 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.


Michal Kazior (3):
  ath10k: prevent HTC from being used after stopping
  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

[PATCH 1/3] ath10k: prevent HTC from being used after stopping

[Michal Kazior]

It was possible to submit new HTC commands
after/while HTC stopped. This led to memory
corruption in some rare cases.

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

diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 7d5a366..f5610f1 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -251,10 +251,14 @@ int ath10k_htc_send(struct ath10k_htc *htc,
 		return -ENOENT;
 	}
 
-	skb_push(skb, sizeof(struct ath10k_htc_hdr));
-
 	spin_lock_bh(&htc->tx_lock);
+	if (htc->stopped) {
+		spin_unlock_bh(&htc->tx_lock);
+		return -ESHUTDOWN;
+	}
+
 	__skb_queue_tail(&ep->tx_queue, skb);
+	skb_push(skb, sizeof(struct ath10k_htc_hdr));
 	spin_unlock_bh(&htc->tx_lock);
 
 	queue_work(htc->ar->workqueue, &ep->send_work);
@@ -267,23 +271,17 @@ static int ath10k_htc_tx_completion_handler(struct ath10k *ar,
 {
 	struct ath10k_htc *htc = &ar->htc;
 	struct ath10k_htc_ep *ep = &htc->endpoint[eid];
-	bool stopping;
 
 	ath10k_htc_notify_tx_completion(ep, skb);
 	/* the skb now belongs to the completion handler */
 
+	/* note: when using TX credit flow, the re-checking of queues happens
+	 * when credits flow back from the target.  in the non-TX credit case,
+	 * we recheck after the packet completes */
 	spin_lock_bh(&htc->tx_lock);
-	stopping = htc->stopping;
-	spin_unlock_bh(&htc->tx_lock);
-
-	if (!ep->tx_credit_flow_enabled && !stopping)
-		/*
-		 * note: when using TX credit flow, the re-checking of
-		 * queues happens when credits flow back from the target.
-		 * in the non-TX credit case, we recheck after the packet
-		 * completes
-		 */
+	if (!ep->tx_credit_flow_enabled && !htc->stopped)
 		queue_work(ar->workqueue, &ep->send_work);
+	spin_unlock_bh(&htc->tx_lock);
 
 	return 0;
 }
@@ -948,7 +946,7 @@ void ath10k_htc_stop(struct ath10k_htc *htc)
 	struct ath10k_htc_ep *ep;
 
 	spin_lock_bh(&htc->tx_lock);
-	htc->stopping = true;
+	htc->stopped = true;
 	spin_unlock_bh(&htc->tx_lock);
 
 	for (i = ATH10K_HTC_EP_0; i < ATH10K_HTC_EP_COUNT; i++) {
@@ -957,7 +955,6 @@ void ath10k_htc_stop(struct ath10k_htc *htc)
 	}
 
 	ath10k_hif_stop(htc->ar);
-	ath10k_htc_reset_endpoint_states(htc);
 }
 
 /* registered target arrival callback from the HIF layer */
@@ -969,6 +966,7 @@ int ath10k_htc_init(struct ath10k *ar)
 
 	spin_lock_init(&htc->tx_lock);
 
+	htc->stopped = false;
 	ath10k_htc_reset_endpoint_states(htc);
 
 	/* setup HIF layer callbacks */
diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h
index 1606c9f..e1dd8c7 100644
--- a/drivers/net/wireless/ath/ath10k/htc.h
+++ b/drivers/net/wireless/ath/ath10k/htc.h
@@ -335,7 +335,7 @@ struct ath10k_htc {
 	struct ath10k *ar;
 	struct ath10k_htc_ep endpoint[ATH10K_HTC_EP_COUNT];
 
-	/* protects endpoint and stopping fields */
+	/* protects endpoint and stopped fields */
 	spinlock_t tx_lock;
 
 	struct ath10k_htc_ops htc_ops;
@@ -349,7 +349,7 @@ struct ath10k_htc {
 	struct ath10k_htc_svc_tx_credits service_tx_alloc[ATH10K_HTC_EP_COUNT];
 	int target_credit_size;
 
-	bool stopping;
+	bool stopped;
 };
 
 int ath10k_htc_init(struct ath10k *ar);
-- 
1.7.9.5

[PATCH 2/3] ath10k: fix memleak in mac setup

[Michal Kazior]

In some cases channel arrays were never freed.

The patch also unifies error handling in the mac
setup function.

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

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 5082503..3d0f876 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3025,8 +3025,10 @@ int ath10k_mac_register(struct ath10k *ar)
 		channels = kmemdup(ath10k_2ghz_channels,
 				   sizeof(ath10k_2ghz_channels),
 				   GFP_KERNEL);
-		if (!channels)
-			return -ENOMEM;
+		if (!channels) {
+			ret = -ENOMEM;
+			goto err_free;
+		}
 
 		band = &ar->mac.sbands[IEEE80211_BAND_2GHZ];
 		band->n_channels = ARRAY_SIZE(ath10k_2ghz_channels);
@@ -3045,11 +3047,8 @@ int ath10k_mac_register(struct ath10k *ar)
 				   sizeof(ath10k_5ghz_channels),
 				   GFP_KERNEL);
 		if (!channels) {
-			if (ar->phy_capability & WHAL_WLAN_11G_CAPABILITY) {
-				band = &ar->mac.sbands[IEEE80211_BAND_2GHZ];
-				kfree(band->channels);
-			}
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto err_free;
 		}
 
 		band = &ar->mac.sbands[IEEE80211_BAND_5GHZ];
@@ -3113,25 +3112,30 @@ int ath10k_mac_register(struct ath10k *ar)
 			    ath10k_reg_notifier);
 	if (ret) {
 		ath10k_err("Regulatory initialization failed\n");
-		return ret;
+		goto err_free;
 	}
 
 	ret = ieee80211_register_hw(ar->hw);
 	if (ret) {
 		ath10k_err("ieee80211 registration failed: %d\n", ret);
-		return ret;
+		goto err_free;
 	}
 
 	if (!ath_is_world_regd(&ar->ath_common.regulatory)) {
 		ret = regulatory_hint(ar->hw->wiphy,
 				      ar->ath_common.regulatory.alpha2);
 		if (ret)
-			goto exit;
+			goto err_unregister;
 	}
 
 	return 0;
-exit:
+
+err_unregister:
 	ieee80211_unregister_hw(ar->hw);
+err_free:
+	kfree(ar->mac.sbands[IEEE80211_BAND_2GHZ].channels);
+	kfree(ar->mac.sbands[IEEE80211_BAND_5GHZ].channels);
+
 	return ret;
 }
 
-- 
1.7.9.5

[PATCH 3/3] ath10k: fix rts/fragmentation threshold setup

[Michal Kazior]

If RTS and fragmentation threshold values are
0xFFFFFFFF they should be considered disabled and
no min/max limits must be applied.

This fixes some issues with throughput issues,
especially with VHT.

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

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 3d0f876..019c6b8 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -331,6 +331,29 @@ static int ath10k_peer_create(struct ath10k *ar, u32 vdev_id, const u8 *addr)
 	return 0;
 }
 
+static int  ath10k_mac_set_rts(struct ath10k_vif *arvif, u32 value)
+{
+	if (value != 0xFFFFFFFF)
+		value = min_t(u32, arvif->ar->hw->wiphy->rts_threshold,
+			      ATH10K_RTS_MAX);
+
+	return ath10k_wmi_vdev_set_param(arvif->ar, arvif->vdev_id,
+					 WMI_VDEV_PARAM_RTS_THRESHOLD,
+					 value);
+}
+
+static int ath10k_mac_set_frag(struct ath10k_vif *arvif, u32 value)
+{
+	if (value != 0xFFFFFFFF)
+		value = clamp_t(u32, arvif->ar->hw->wiphy->frag_threshold,
+				ATH10K_FRAGMT_THRESHOLD_MIN,
+				ATH10K_FRAGMT_THRESHOLD_MAX);
+
+	return ath10k_wmi_vdev_set_param(arvif->ar, arvif->vdev_id,
+					 WMI_VDEV_PARAM_FRAGMENTATION_THRESHOLD,
+					 value);
+}
+
 static int ath10k_peer_delete(struct ath10k *ar, u32 vdev_id, const u8 *addr)
 {
 	int ret;
@@ -1803,7 +1826,7 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
 	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
 	enum wmi_sta_powersave_param param;
 	int ret = 0;
-	u32 value, rts, frag;
+	u32 value;
 	int bit;
 
 	mutex_lock(&ar->conf_mutex);
@@ -1906,20 +1929,12 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
 			ath10k_warn("Failed to set PSPOLL count: %d\n", ret);
 	}
 
-	rts = min_t(u32, ar->hw->wiphy->rts_threshold, ATH10K_RTS_MAX);
-	ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id,
-					 WMI_VDEV_PARAM_RTS_THRESHOLD,
-					 rts);
+	ret = ath10k_mac_set_rts(arvif, ar->hw->wiphy->rts_threshold);
 	if (ret)
 		ath10k_warn("failed to set rts threshold for vdev %d (%d)\n",
 			    arvif->vdev_id, ret);
 
-	frag = clamp_t(u32, ar->hw->wiphy->frag_threshold,
-		       ATH10K_FRAGMT_THRESHOLD_MIN,
-		       ATH10K_FRAGMT_THRESHOLD_MAX);
-	ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id,
-					WMI_VDEV_PARAM_FRAGMENTATION_THRESHOLD,
-					frag);
+	ret = ath10k_mac_set_frag(arvif, ar->hw->wiphy->frag_threshold);
 	if (ret)
 		ath10k_warn("failed to set frag threshold for vdev %d (%d)\n",
 			    arvif->vdev_id, ret);
@@ -2627,11 +2642,7 @@ static void ath10k_set_rts_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
 
 	lockdep_assert_held(&arvif->ar->conf_mutex);
 
-	rts = min_t(u32, rts, ATH10K_RTS_MAX);
-
-	ar_iter->ret = ath10k_wmi_vdev_set_param(ar_iter->ar, arvif->vdev_id,
-						 WMI_VDEV_PARAM_RTS_THRESHOLD,
-						 rts);
+	ar_iter->ret = ath10k_mac_set_rts(arvif, rts);
 	if (ar_iter->ret)
 		ath10k_warn("Failed to set RTS threshold for VDEV: %d\n",
 			    arvif->vdev_id);
@@ -2663,19 +2674,10 @@ static void ath10k_set_frag_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
 	struct ath10k_generic_iter *ar_iter = data;
 	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
 	u32 frag = ar_iter->ar->hw->wiphy->frag_threshold;
-	int ret;
 
 	lockdep_assert_held(&arvif->ar->conf_mutex);
 
-	frag = clamp_t(u32, frag,
-		       ATH10K_FRAGMT_THRESHOLD_MIN,
-		       ATH10K_FRAGMT_THRESHOLD_MAX);
-
-	ret = ath10k_wmi_vdev_set_param(ar_iter->ar, arvif->vdev_id,
-					WMI_VDEV_PARAM_FRAGMENTATION_THRESHOLD,
-					frag);
-
-	ar_iter->ret = ret;
+	ar_iter->ret = ath10k_mac_set_frag(arvif, frag);
 	if (ar_iter->ret)
 		ath10k_warn("Failed to set frag threshold for VDEV: %d\n",
 			    arvif->vdev_id);
-- 
1.7.9.5

Re: [PATCH 1/3] ath10k: prevent HTC from being used after stopping

[Kalle Valo]

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

> It was possible to submit new HTC commands
> after/while HTC stopped. This led to memory
> corruption in some rare cases.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> @@ -957,7 +955,6 @@ void ath10k_htc_stop(struct ath10k_htc *htc)
>  	}
>  
>  	ath10k_hif_stop(htc->ar);
> -	ath10k_htc_reset_endpoint_states(htc);
>  }

Is this on purpose? I can't fit it to the description.

-- 
Kalle Valo

Re: [PATCH 0/3] 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.
>
>
> Michal Kazior (3):
>   ath10k: prevent HTC from being used after stopping
>   ath10k: fix memleak in mac setup
>   ath10k: fix rts/fragmentation threshold setup

Doesn't apply:

Applying: ath10k: prevent HTC from being used after stopping
Applying: ath10k: fix memleak in mac setup
Applying: ath10k: fix rts/fragmentation threshold setup
fatal: sha1 information is lacking or useless (drivers/net/wireless/ath/ath10k/mac.c).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0003 ath10k: fix rts/fragmentation threshold setup


-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: prevent HTC from being used after stopping

[Michal Kazior]

On 19 July 2013 13:04, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> It was possible to submit new HTC commands
>> after/while HTC stopped. This led to memory
>> corruption in some rare cases.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> [...]
>
>> @@ -957,7 +955,6 @@ void ath10k_htc_stop(struct ath10k_htc *htc)
>>       }
>>
>>       ath10k_hif_stop(htc->ar);
>> -     ath10k_htc_reset_endpoint_states(htc);
>>  }
>
> Is this on purpose? I can't fit it to the description.

You're right. I should've mentioned that in the commit message.

This line is simply unnecessary. Do you prefer fixing the commit
message accordingly or should I post it as a separate patch?


Pozdrawiam / Best regards,
Michał Kazior.

Re: [PATCH 0/3] ath10k: fixes

[Michal Kazior]

On 19 July 2013 13:07, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> 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.
>>
>>
>> Michal Kazior (3):
>>   ath10k: prevent HTC from being used after stopping
>>   ath10k: fix memleak in mac setup
>>   ath10k: fix rts/fragmentation threshold setup
>
> Doesn't apply:
>
> Applying: ath10k: prevent HTC from being used after stopping
> Applying: ath10k: fix memleak in mac setup
> Applying: ath10k: fix rts/fragmentation threshold setup
> fatal: sha1 information is lacking or useless (drivers/net/wireless/ath/ath10k/mac.c).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0003 ath10k: fix rts/fragmentation threshold setup

Eww. Sorry about that (I did warn you though). Perhaps that's because
you skipped one of the recovery patches that git is confused now.

I'll resend the whole patchset rebased and with the HTC patch fixed.


Pozdrawiam / Best regards,
Michał Kazior.

Re: [PATCH 0/3] ath10k: fixes

[Kalle Valo]

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

> On 19 July 2013 13:07, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> 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.
>>>
>>>
>>> Michal Kazior (3):
>>>   ath10k: prevent HTC from being used after stopping
>>>   ath10k: fix memleak in mac setup
>>>   ath10k: fix rts/fragmentation threshold setup
>>
>> Doesn't apply:
>>
>> Applying: ath10k: prevent HTC from being used after stopping
>> Applying: ath10k: fix memleak in mac setup
>> Applying: ath10k: fix rts/fragmentation threshold setup
>> fatal: sha1 information is lacking or useless (drivers/net/wireless/ath/ath10k/mac.c).
>> Repository lacks necessary blobs to fall back on 3-way merge.
>> Cannot fall back to three-way merge.
>> Patch failed at 0003 ath10k: fix rts/fragmentation threshold setup
>
> Eww. Sorry about that (I did warn you though). Perhaps that's because
> you skipped one of the recovery patches that git is confused now.

No worries. Most likely I could have fixed the conflict myself, but if
3-way merge doesn't work it's too much work.

> I'll resend the whole patchset rebased and with the HTC patch fixed.

Thanks.

-- 
Kalle Valo

Re: [PATCH 1/3] ath10k: prevent HTC from being used after stopping

[Kalle Valo]

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

> On 19 July 2013 13:04, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> It was possible to submit new HTC commands
>>> after/while HTC stopped. This led to memory
>>> corruption in some rare cases.
>>>
>>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>>
>> [...]
>>
>>> @@ -957,7 +955,6 @@ void ath10k_htc_stop(struct ath10k_htc *htc)
>>>       }
>>>
>>>       ath10k_hif_stop(htc->ar);
>>> -     ath10k_htc_reset_endpoint_states(htc);
>>>  }
>>
>> Is this on purpose? I can't fit it to the description.
>
> You're right. I should've mentioned that in the commit message.
>
> This line is simply unnecessary. Do you prefer fixing the commit
> message accordingly or should I post it as a separate patch?

A separate patch, please. Makes it easier to bisect issues later on.

-- 
Kalle Valo

[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

[PATCH v2 1/4] ath10k: prevent HTC from being used after stopping

[Michal Kazior]

It was possible to submit new HTC commands
after/while HTC stopped. This led to memory
corruption in some rare cases.

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

diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 72e072c..47b7752 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -254,10 +254,14 @@ int ath10k_htc_send(struct ath10k_htc *htc,
 		return -ENOENT;
 	}
 
-	skb_push(skb, sizeof(struct ath10k_htc_hdr));
-
 	spin_lock_bh(&htc->tx_lock);
+	if (htc->stopped) {
+		spin_unlock_bh(&htc->tx_lock);
+		return -ESHUTDOWN;
+	}
+
 	__skb_queue_tail(&ep->tx_queue, skb);
+	skb_push(skb, sizeof(struct ath10k_htc_hdr));
 	spin_unlock_bh(&htc->tx_lock);
 
 	queue_work(htc->ar->workqueue, &ep->send_work);
@@ -270,23 +274,17 @@ static int ath10k_htc_tx_completion_handler(struct ath10k *ar,
 {
 	struct ath10k_htc *htc = &ar->htc;
 	struct ath10k_htc_ep *ep = &htc->endpoint[eid];
-	bool stopping;
 
 	ath10k_htc_notify_tx_completion(ep, skb);
 	/* the skb now belongs to the completion handler */
 
+	/* note: when using TX credit flow, the re-checking of queues happens
+	 * when credits flow back from the target.  in the non-TX credit case,
+	 * we recheck after the packet completes */
 	spin_lock_bh(&htc->tx_lock);
-	stopping = htc->stopping;
-	spin_unlock_bh(&htc->tx_lock);
-
-	if (!ep->tx_credit_flow_enabled && !stopping)
-		/*
-		 * note: when using TX credit flow, the re-checking of
-		 * queues happens when credits flow back from the target.
-		 * in the non-TX credit case, we recheck after the packet
-		 * completes
-		 */
+	if (!ep->tx_credit_flow_enabled && !htc->stopped)
 		queue_work(ar->workqueue, &ep->send_work);
+	spin_unlock_bh(&htc->tx_lock);
 
 	return 0;
 }
@@ -951,7 +949,7 @@ void ath10k_htc_stop(struct ath10k_htc *htc)
 	struct ath10k_htc_ep *ep;
 
 	spin_lock_bh(&htc->tx_lock);
-	htc->stopping = true;
+	htc->stopped = true;
 	spin_unlock_bh(&htc->tx_lock);
 
 	for (i = ATH10K_HTC_EP_0; i < ATH10K_HTC_EP_COUNT; i++) {
@@ -972,6 +970,7 @@ int ath10k_htc_init(struct ath10k *ar)
 
 	spin_lock_init(&htc->tx_lock);
 
+	htc->stopped = false;
 	ath10k_htc_reset_endpoint_states(htc);
 
 	/* setup HIF layer callbacks */
diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h
index 1606c9f..e1dd8c7 100644
--- a/drivers/net/wireless/ath/ath10k/htc.h
+++ b/drivers/net/wireless/ath/ath10k/htc.h
@@ -335,7 +335,7 @@ struct ath10k_htc {
 	struct ath10k *ar;
 	struct ath10k_htc_ep endpoint[ATH10K_HTC_EP_COUNT];
 
-	/* protects endpoint and stopping fields */
+	/* protects endpoint and stopped fields */
 	spinlock_t tx_lock;
 
 	struct ath10k_htc_ops htc_ops;
@@ -349,7 +349,7 @@ struct ath10k_htc {
 	struct ath10k_htc_svc_tx_credits service_tx_alloc[ATH10K_HTC_EP_COUNT];
 	int target_credit_size;
 
-	bool stopping;
+	bool stopped;
 };
 
 int ath10k_htc_init(struct ath10k *ar);
-- 
1.7.9.5

[PATCH v2 2/4] ath10k: don't reset HTC endpoints unnecessarily

[Michal Kazior]

Endpoints are re-initialized upon HTC start anyway
so there's no need to do that twice in case of
restarting HTC (i.e. in case of hardware
recovery).

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

diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 47b7752..ef3329e 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -958,7 +958,6 @@ void ath10k_htc_stop(struct ath10k_htc *htc)
 	}
 
 	ath10k_hif_stop(htc->ar);
-	ath10k_htc_reset_endpoint_states(htc);
 }
 
 /* registered target arrival callback from the HIF layer */
-- 
1.7.9.5

[PATCH v2 3/4] ath10k: fix memleak in mac setup

[Michal Kazior]

In some cases channel arrays were never freed.

The patch also unifies error handling in the mac
setup function.

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

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 07e5f7d..6144b3b 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3225,8 +3225,10 @@ int ath10k_mac_register(struct ath10k *ar)
 		channels = kmemdup(ath10k_2ghz_channels,
 				   sizeof(ath10k_2ghz_channels),
 				   GFP_KERNEL);
-		if (!channels)
-			return -ENOMEM;
+		if (!channels) {
+			ret = -ENOMEM;
+			goto err_free;
+		}
 
 		band = &ar->mac.sbands[IEEE80211_BAND_2GHZ];
 		band->n_channels = ARRAY_SIZE(ath10k_2ghz_channels);
@@ -3245,11 +3247,8 @@ int ath10k_mac_register(struct ath10k *ar)
 				   sizeof(ath10k_5ghz_channels),
 				   GFP_KERNEL);
 		if (!channels) {
-			if (ar->phy_capability & WHAL_WLAN_11G_CAPABILITY) {
-				band = &ar->mac.sbands[IEEE80211_BAND_2GHZ];
-				kfree(band->channels);
-			}
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto err_free;
 		}
 
 		band = &ar->mac.sbands[IEEE80211_BAND_5GHZ];
@@ -3313,25 +3312,30 @@ int ath10k_mac_register(struct ath10k *ar)
 			    ath10k_reg_notifier);
 	if (ret) {
 		ath10k_err("Regulatory initialization failed\n");
-		return ret;
+		goto err_free;
 	}
 
 	ret = ieee80211_register_hw(ar->hw);
 	if (ret) {
 		ath10k_err("ieee80211 registration failed: %d\n", ret);
-		return ret;
+		goto err_free;
 	}
 
 	if (!ath_is_world_regd(&ar->ath_common.regulatory)) {
 		ret = regulatory_hint(ar->hw->wiphy,
 				      ar->ath_common.regulatory.alpha2);
 		if (ret)
-			goto exit;
+			goto err_unregister;
 	}
 
 	return 0;
-exit:
+
+err_unregister:
 	ieee80211_unregister_hw(ar->hw);
+err_free:
+	kfree(ar->mac.sbands[IEEE80211_BAND_2GHZ].channels);
+	kfree(ar->mac.sbands[IEEE80211_BAND_5GHZ].channels);
+
 	return ret;
 }
 
-- 
1.7.9.5

[PATCH v2 4/4] ath10k: fix rts/fragmentation threshold setup

[Michal Kazior]

If RTS and fragmentation threshold values are
0xFFFFFFFF they should be considered disabled and
no min/max limits must be applied.

This fixes some issues with throughput issues,
especially with VHT.

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

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 6144b3b..d0a7761 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -332,6 +332,29 @@ static int ath10k_peer_create(struct ath10k *ar, u32 vdev_id, const u8 *addr)
 	return 0;
 }
 
+static int  ath10k_mac_set_rts(struct ath10k_vif *arvif, u32 value)
+{
+	if (value != 0xFFFFFFFF)
+		value = min_t(u32, arvif->ar->hw->wiphy->rts_threshold,
+			      ATH10K_RTS_MAX);
+
+	return ath10k_wmi_vdev_set_param(arvif->ar, arvif->vdev_id,
+					 WMI_VDEV_PARAM_RTS_THRESHOLD,
+					 value);
+}
+
+static int ath10k_mac_set_frag(struct ath10k_vif *arvif, u32 value)
+{
+	if (value != 0xFFFFFFFF)
+		value = clamp_t(u32, arvif->ar->hw->wiphy->frag_threshold,
+				ATH10K_FRAGMT_THRESHOLD_MIN,
+				ATH10K_FRAGMT_THRESHOLD_MAX);
+
+	return ath10k_wmi_vdev_set_param(arvif->ar, arvif->vdev_id,
+					 WMI_VDEV_PARAM_FRAGMENTATION_THRESHOLD,
+					 value);
+}
+
 static int ath10k_peer_delete(struct ath10k *ar, u32 vdev_id, const u8 *addr)
 {
 	int ret;
@@ -1897,7 +1920,7 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
 	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
 	enum wmi_sta_powersave_param param;
 	int ret = 0;
-	u32 value, rts, frag;
+	u32 value;
 	int bit;
 
 	mutex_lock(&ar->conf_mutex);
@@ -2000,20 +2023,12 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
 			ath10k_warn("Failed to set PSPOLL count: %d\n", ret);
 	}
 
-	rts = min_t(u32, ar->hw->wiphy->rts_threshold, ATH10K_RTS_MAX);
-	ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id,
-					 WMI_VDEV_PARAM_RTS_THRESHOLD,
-					 rts);
+	ret = ath10k_mac_set_rts(arvif, ar->hw->wiphy->rts_threshold);
 	if (ret)
 		ath10k_warn("failed to set rts threshold for vdev %d (%d)\n",
 			    arvif->vdev_id, ret);
 
-	frag = clamp_t(u32, ar->hw->wiphy->frag_threshold,
-		       ATH10K_FRAGMT_THRESHOLD_MIN,
-		       ATH10K_FRAGMT_THRESHOLD_MAX);
-	ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id,
-					WMI_VDEV_PARAM_FRAGMENTATION_THRESHOLD,
-					frag);
+	ret = ath10k_mac_set_frag(arvif, ar->hw->wiphy->frag_threshold);
 	if (ret)
 		ath10k_warn("failed to set frag threshold for vdev %d (%d)\n",
 			    arvif->vdev_id, ret);
@@ -2728,11 +2743,7 @@ static void ath10k_set_rts_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
 	if (ar_iter->ar->state == ATH10K_STATE_RESTARTED)
 		return;
 
-	rts = min_t(u32, rts, ATH10K_RTS_MAX);
-
-	ar_iter->ret = ath10k_wmi_vdev_set_param(ar_iter->ar, arvif->vdev_id,
-						 WMI_VDEV_PARAM_RTS_THRESHOLD,
-						 rts);
+	ar_iter->ret = ath10k_mac_set_rts(arvif, rts);
 	if (ar_iter->ret)
 		ath10k_warn("Failed to set RTS threshold for VDEV: %d\n",
 			    arvif->vdev_id);
@@ -2764,7 +2775,6 @@ static void ath10k_set_frag_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
 	struct ath10k_generic_iter *ar_iter = data;
 	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
 	u32 frag = ar_iter->ar->hw->wiphy->frag_threshold;
-	int ret;
 
 	lockdep_assert_held(&arvif->ar->conf_mutex);
 
@@ -2775,15 +2785,7 @@ static void ath10k_set_frag_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
 	if (ar_iter->ar->state == ATH10K_STATE_RESTARTED)
 		return;
 
-	frag = clamp_t(u32, frag,
-		       ATH10K_FRAGMT_THRESHOLD_MIN,
-		       ATH10K_FRAGMT_THRESHOLD_MAX);
-
-	ret = ath10k_wmi_vdev_set_param(ar_iter->ar, arvif->vdev_id,
-					WMI_VDEV_PARAM_FRAGMENTATION_THRESHOLD,
-					frag);
-
-	ar_iter->ret = ret;
+	ar_iter->ret = ath10k_mac_set_frag(arvif, frag);
 	if (ar_iter->ret)
 		ath10k_warn("Failed to set frag threshold for VDEV: %d\n",
 			    arvif->vdev_id);
-- 
1.7.9.5

Re: [PATCH v2 2/4] ath10k: don't reset HTC endpoints unnecessarily

[Kalle Valo]

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

> Endpoints are re-initialized upon HTC start anyway
> so there's no need to do that twice in case of
> restarting HTC (i.e. in case of hardware
> recovery).
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

This is much better, thanks.

-- 
Kalle Valo

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 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

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