Skip to contentSkip to conversation

ath10k: fixes

PATCH New Michal Kazior 2 participants 12 messages Jun 12, 2013 – Jun 14, 2013 Qualcomm Atheros ATH10K wireless driver

lore
Share & tools
lore
#0 lore raw

cover ath10k: fixes

This includes 2 patches from my previous patchset
that had conflicts. It is now rebased and should
apply cleanly on the github master branch.

Michal Kazior (6):
ath10k: embed HTC struct inside ath10k
ath10k: embed HTT struct inside ath10k
ath10k: improve locking
ath10k: abort scan properly if wmi_scan_stop fails
ath10k: wait for CE to drain when shutting down
ath10k: add missing debug prints

 drivers/net/wireless/ath/ath10k/core.c   |   40 +++++++---------
 drivers/net/wireless/ath/ath10k/core.h   |    9 ++--
 drivers/net/wireless/ath/ath10k/htc.c    |   23 ++-------
 drivers/net/wireless/ath/ath10k/htc.h    |    4 +-
 drivers/net/wireless/ath/ath10k/htt.c    |   27 ++++++-----
 drivers/net/wireless/ath/ath10k/htt.h    |    3 +-
 drivers/net/wireless/ath/ath10k/htt_rx.c |    3 +-
 drivers/net/wireless/ath/ath10k/htt_tx.c |   12 ++---
 drivers/net/wireless/ath/ath10k/mac.c    |   77 ++++++++++++++++++++++++++----
 drivers/net/wireless/ath/ath10k/pci.c    |   21 ++++++++
 drivers/net/wireless/ath/ath10k/pci.h    |    2 +-
 drivers/net/wireless/ath/ath10k/wmi.c    |   10 +++-
 12 files changed, 148 insertions(+), 83 deletions(-)
Signature · 1 line
1.7.9.5
#1 lore raw

PATCH 1/6 ath10k: embed HTC struct inside ath10k

This reduces number of allocations and simplifies
memory managemnt.

---
 drivers/net/wireless/ath/ath10k/core.c   |   28 ++++++++++++----------------
 drivers/net/wireless/ath/ath10k/core.h   |    2 +-
 drivers/net/wireless/ath/ath10k/htc.c    |   23 +++++------------------
 drivers/net/wireless/ath/ath10k/htc.h    |    4 +---
 drivers/net/wireless/ath/ath10k/htt.c    |    2 +-
 drivers/net/wireless/ath/ath10k/htt_tx.c |   10 +++++-----
 drivers/net/wireless/ath/ath10k/wmi.c    |    4 ++--
 7 files changed, 27 insertions(+), 46 deletions(-)
drivers/net/wireless/ath/ath10k/core.c +12 -16
index aabe166..4199560 100644
@@ -100,7 +100,7 @@ static int ath10k_init_connect_htc(struct ath10k *ar)
 		goto conn_fail;
 
 	/* Start HTC */
-	status = ath10k_htc_start(ar->htc);
+	status = ath10k_htc_start(&ar->htc);
 	if (status)
 		goto conn_fail;
 
@@ -116,7 +116,7 @@ static int ath10k_init_connect_htc(struct ath10k *ar)
 	return 0;
 
 timeout:
-	ath10k_htc_stop(ar->htc);
+	ath10k_htc_stop(&ar->htc);
 conn_fail:
 	return status;
 }
@@ -505,7 +505,6 @@ EXPORT_SYMBOL(ath10k_core_destroy);
 
 int ath10k_core_register(struct ath10k *ar)
 {
-	struct ath10k_htc_ops htc_ops;
 	struct bmi_target_info target_info;
 	int status;
 
@@ -534,26 +533,26 @@ int ath10k_core_register(struct ath10k *ar)
 	if (status)
 		goto err;
 
-	htc_ops.target_send_suspend_complete = ath10k_send_suspend_complete;
+	ar->htc.htc_ops.target_send_suspend_complete =
+		ath10k_send_suspend_complete;
 
-	ar->htc = ath10k_htc_create(ar, &htc_ops);
-	if (IS_ERR(ar->htc)) {
-		status = PTR_ERR(ar->htc);
-		ath10k_err("could not create HTC (%d)\n", status);
+	status = ath10k_htc_init(ar);
+	if (status) {
+		ath10k_err("could not init HTC (%d)\n", status);
 		goto err;
 	}
 
 	status = ath10k_bmi_done(ar);
 	if (status)
-		goto err_htc_destroy;
+		goto err;
 
 	status = ath10k_wmi_attach(ar);
 	if (status) {
 		ath10k_err("WMI attach failed: %d\n", status);
-		goto err_htc_destroy;
+		goto err;
 	}
 
-	status = ath10k_htc_wait_target(ar->htc);
+	status = ath10k_htc_wait_target(&ar->htc);
 	if (status)
 		goto err_wmi_detach;
 
@@ -605,13 +604,11 @@ int ath10k_core_register(struct ath10k *ar)
 err_unregister_mac:
 	ath10k_mac_unregister(ar);
 err_disconnect_htc:
-	ath10k_htc_stop(ar->htc);
+	ath10k_htc_stop(&ar->htc);
 err_htt_detach:
 	ath10k_htt_detach(ar->htt);
 err_wmi_detach:
 	ath10k_wmi_detach(ar);
-err_htc_destroy:
-	ath10k_htc_destroy(ar->htc);
 err:
 	return status;
 }
@@ -623,10 +620,9 @@ void ath10k_core_unregister(struct ath10k *ar)
 	 * Otherwise we will fail to submit commands to FW and mac80211 will be
 	 * unhappy about callback failures. */
 	ath10k_mac_unregister(ar);
-	ath10k_htc_stop(ar->htc);
+	ath10k_htc_stop(&ar->htc);
 	ath10k_htt_detach(ar->htt);
 	ath10k_wmi_detach(ar);
-	ath10k_htc_destroy(ar->htc);
 }
 EXPORT_SYMBOL(ath10k_core_unregister);
 
drivers/net/wireless/ath/ath10k/core.h +1 -1
index 7489770..dfd4fe1 100644
@@ -279,8 +279,8 @@ struct ath10k {
 	bool is_target_paused;
 
 	struct ath10k_bmi bmi;
+	struct ath10k_htc htc;
 
-	struct ath10k_htc *htc;
 	struct ath10k_htt *htt;
 
 	struct ath10k_hw_params {
drivers/net/wireless/ath/ath10k/htc.c +5 -18
index f37a6e1..7d5a366 100644
@@ -265,7 +265,7 @@ static int ath10k_htc_tx_completion_handler(struct ath10k *ar,
 					    struct sk_buff *skb,
 					    unsigned int eid)
 {
-	struct ath10k_htc *htc = ar->htc;
+	struct ath10k_htc *htc = &ar->htc;
 	struct ath10k_htc_ep *ep = &htc->endpoint[eid];
 	bool stopping;
 
@@ -414,7 +414,7 @@ static int ath10k_htc_rx_completion_handler(struct ath10k *ar,
 					    u8 pipe_id)
 {
 	int status = 0;
-	struct ath10k_htc *htc = ar->htc;
+	struct ath10k_htc *htc = &ar->htc;
 	struct ath10k_htc_hdr *hdr;
 	struct ath10k_htc_ep *ep;
 	u16 payload_len;
@@ -961,22 +961,14 @@ void ath10k_htc_stop(struct ath10k_htc *htc)
 }
 
 /* registered target arrival callback from the HIF layer */
-struct ath10k_htc *ath10k_htc_create(struct ath10k *ar,
-				     struct ath10k_htc_ops *htc_ops)
+int ath10k_htc_init(struct ath10k *ar)
 {
 	struct ath10k_hif_cb htc_callbacks;
 	struct ath10k_htc_ep *ep = NULL;
-	struct ath10k_htc *htc = NULL;
-
-	/* FIXME: use struct ath10k instead */
-	htc = kzalloc(sizeof(struct ath10k_htc), GFP_KERNEL);
-	if (!htc)
-		return ERR_PTR(-ENOMEM);
+	struct ath10k_htc *htc = &ar->htc;
 
 	spin_lock_init(&htc->tx_lock);
 
-	memcpy(&htc->htc_ops, htc_ops, sizeof(struct ath10k_htc_ops));
-
 	ath10k_htc_reset_endpoint_states(htc);
 
 	/* setup HIF layer callbacks */
@@ -992,10 +984,5 @@ struct ath10k_htc *ath10k_htc_create(struct ath10k *ar,
 
 	init_completion(&htc->ctl_resp);
 
-	return htc;
-}
-
-void ath10k_htc_destroy(struct ath10k_htc *htc)
-{
-	kfree(htc);
+	return 0;
 }
drivers/net/wireless/ath/ath10k/htc.h +1 -3
index fa45844..1606c9f 100644
@@ -352,8 +352,7 @@ struct ath10k_htc {
 	bool stopping;
 };
 
-struct ath10k_htc *ath10k_htc_create(struct ath10k *ar,
-				     struct ath10k_htc_ops *htc_ops);
+int ath10k_htc_init(struct ath10k *ar);
 int ath10k_htc_wait_target(struct ath10k_htc *htc);
 int ath10k_htc_start(struct ath10k_htc *htc);
 int ath10k_htc_connect_service(struct ath10k_htc *htc,
@@ -362,7 +361,6 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
 int ath10k_htc_send(struct ath10k_htc *htc, enum ath10k_htc_ep_id eid,
 		    struct sk_buff *packet);
 void ath10k_htc_stop(struct ath10k_htc *htc);
-void ath10k_htc_destroy(struct ath10k_htc *htc);
 struct sk_buff *ath10k_htc_alloc_skb(int size);
 
 #endif
drivers/net/wireless/ath/ath10k/htt.c +1 -1
index 185a546..2bfb9b4 100644
@@ -36,7 +36,7 @@ static int ath10k_htt_htc_attach(struct ath10k_htt *htt)
 	/* connect to control service */
 	conn_req.service_id = ATH10K_HTC_SVC_ID_HTT_DATA_MSG;
 
-	status = ath10k_htc_connect_service(htt->ar->htc, &conn_req,
+	status = ath10k_htc_connect_service(&htt->ar->htc, &conn_req,
 					    &conn_resp);
 
 	if (status)
drivers/net/wireless/ath/ath10k/htt_tx.c +5 -5
index ef79106..fa568f1 100644
@@ -92,7 +92,7 @@ int ath10k_htt_tx_attach(struct ath10k_htt *htt)
 
 	/* At the beginning free queue number should hint us the maximum
 	 * queue length */
-	pipe = htt->ar->htc->endpoint[htt->eid].ul_pipe_id;
+	pipe = htt->ar->htc.endpoint[htt->eid].ul_pipe_id;
 	htt->max_num_pending_tx = ath10k_hif_get_free_queue_number(htt->ar,
 								   pipe);
 
@@ -194,7 +194,7 @@ int ath10k_htt_h2t_ver_req_msg(struct ath10k_htt *htt)
 
 	ATH10K_SKB_CB(skb)->htt.is_conf = true;
 
-	ret = ath10k_htc_send(htt->ar->htc, htt->eid, skb);
+	ret = ath10k_htc_send(&htt->ar->htc, htt->eid, skb);
 	if (ret) {
 		dev_kfree_skb_any(skb);
 		return ret;
@@ -281,7 +281,7 @@ int ath10k_htt_send_rx_ring_cfg_ll(struct ath10k_htt *htt)
 
 	ATH10K_SKB_CB(skb)->htt.is_conf = true;
 
-	ret = ath10k_htc_send(htt->ar->htc, htt->eid, skb);
+	ret = ath10k_htc_send(&htt->ar->htc, htt->eid, skb);
 	if (ret) {
 		dev_kfree_skb_any(skb);
 		return ret;
@@ -346,7 +346,7 @@ int ath10k_htt_mgmt_tx(struct ath10k_htt *htt, struct sk_buff *msdu)
 	skb_cb->htt.refcount = 2;
 	skb_cb->htt.msdu = msdu;
 
-	res = ath10k_htc_send(htt->ar->htc, htt->eid, txdesc);
+	res = ath10k_htc_send(&htt->ar->htc, htt->eid, txdesc);
 	if (res)
 		goto err;
 
@@ -486,7 +486,7 @@ int ath10k_htt_tx(struct ath10k_htt *htt, struct sk_buff *msdu)
 	skb_cb->htt.txfrag = txfrag;
 	skb_cb->htt.msdu = msdu;
 
-	res = ath10k_htc_send(htt->ar->htc, htt->eid, txdesc);
+	res = ath10k_htc_send(&htt->ar->htc, htt->eid, txdesc);
 	if (res)
 		goto err;
 
drivers/net/wireless/ath/ath10k/wmi.c +2 -2
index 1b5312d..681e941 100644
@@ -111,7 +111,7 @@ static int ath10k_wmi_cmd_send(struct ath10k *ar, struct sk_buff *skb,
 
 	trace_ath10k_wmi_cmd(cmd_id, skb->data, skb->len);
 
-	status = ath10k_htc_send(ar->htc, ar->wmi.eid, skb);
+	status = ath10k_htc_send(&ar->htc, ar->wmi.eid, skb);
 	if (status) {
 		dev_kfree_skb_any(skb);
 		atomic_dec(&ar->wmi.pending_tx_count);
@@ -1114,7 +1114,7 @@ int ath10k_wmi_connect_htc_service(struct ath10k *ar)
 	/* connect to control service */
 	conn_req.service_id = ATH10K_HTC_SVC_ID_WMI_CONTROL;
 
-	status = ath10k_htc_connect_service(ar->htc, &conn_req, &conn_resp);
+	status = ath10k_htc_connect_service(&ar->htc, &conn_req, &conn_resp);
 	if (status) {
 		ath10k_warn("failed to connect to WMI CONTROL service status: %d\n",
 			    status);
Signature · 1 line
1.7.9.5
#2 lore raw

PATCH 2/6 ath10k: embed HTT struct inside ath10k

In reply to Michal Kazior

This reduces number of allocations and simplifies
memory managemnt.

---
 drivers/net/wireless/ath/ath10k/core.c   |   12 ++++++------
 drivers/net/wireless/ath/ath10k/core.h   |    7 +++----
 drivers/net/wireless/ath/ath10k/htt.c    |   25 +++++++++++++------------
 drivers/net/wireless/ath/ath10k/htt.h    |    3 +--
 drivers/net/wireless/ath/ath10k/htt_rx.c |    3 ++-
 drivers/net/wireless/ath/ath10k/htt_tx.c |    2 +-
 drivers/net/wireless/ath/ath10k/mac.c    |   16 ++++++++--------
 7 files changed, 34 insertions(+), 34 deletions(-)
drivers/net/wireless/ath/ath10k/core.c +6 -6
index 4199560..f1312fa 100644
@@ -556,9 +556,9 @@ int ath10k_core_register(struct ath10k *ar)
 	if (status)
 		goto err_wmi_detach;
 
-	ar->htt = ath10k_htt_attach(ar);
-	if (!ar->htt) {
-		status = -ENOMEM;
+	status = ath10k_htt_attach(ar);
+	if (status) {
+		ath10k_err("could not attach htt (%d)\n", status);
 		goto err_wmi_detach;
 	}
 
@@ -585,7 +585,7 @@ int ath10k_core_register(struct ath10k *ar)
 		goto err_disconnect_htc;
 	}
 
-	status = ath10k_htt_attach_target(ar->htt);
+	status = ath10k_htt_attach_target(&ar->htt);
 	if (status)
 		goto err_disconnect_htc;
 
@@ -606,7 +606,7 @@ err_unregister_mac:
 err_disconnect_htc:
 	ath10k_htc_stop(&ar->htc);
 err_htt_detach:
-	ath10k_htt_detach(ar->htt);
+	ath10k_htt_detach(&ar->htt);
 err_wmi_detach:
 	ath10k_wmi_detach(ar);
 err:
@@ -621,7 +621,7 @@ void ath10k_core_unregister(struct ath10k *ar)
 	 * unhappy about callback failures. */
 	ath10k_mac_unregister(ar);
 	ath10k_htc_stop(&ar->htc);
-	ath10k_htt_detach(ar->htt);
+	ath10k_htt_detach(&ar->htt);
 	ath10k_wmi_detach(ar);
 }
 EXPORT_SYMBOL(ath10k_core_unregister);
drivers/net/wireless/ath/ath10k/core.h +3 -4
index dfd4fe1..2f7065c 100644
@@ -23,6 +23,7 @@
 #include <linux/types.h>
 #include <linux/pci.h>
 
+#include "htt.h"
 #include "htc.h"
 #include "hw.h"
 #include "targaddrs.h"
@@ -273,15 +274,13 @@ struct ath10k {
 		const struct ath10k_hif_ops *ops;
 	} hif;
 
-	struct ath10k_wmi wmi;
-
 	wait_queue_head_t event_queue;
 	bool is_target_paused;
 
 	struct ath10k_bmi bmi;
+	struct ath10k_wmi wmi;
 	struct ath10k_htc htc;
-
-	struct ath10k_htt *htt;
+	struct ath10k_htt htt;
 
 	struct ath10k_hw_params {
 		u32 id;
drivers/net/wireless/ath/ath10k/htt.c +13 -12
index 2bfb9b4..39342c5 100644
@@ -16,6 +16,7 @@
  */
 
 #include <linux/slab.h>
+#include <linux/if_ether.h>
 
 #include "htt.h"
 #include "core.h"
@@ -47,15 +48,11 @@ static int ath10k_htt_htc_attach(struct ath10k_htt *htt)
 	return 0;
 }
 
-struct ath10k_htt *ath10k_htt_attach(struct ath10k *ar)
+int ath10k_htt_attach(struct ath10k *ar)
 {
-	struct ath10k_htt *htt;
+	struct ath10k_htt *htt = &ar->htt;
 	int ret;
 
-	htt = kzalloc(sizeof(*htt), GFP_KERNEL);
-	if (!htt)
-		return NULL;
-
 	htt->ar = ar;
 	htt->max_throughput_mbps = 800;
 
@@ -65,8 +62,11 @@ struct ath10k_htt *ath10k_htt_attach(struct ath10k *ar)
 	 * since ath10k_htt_rx_attach involves sending a rx ring configure
 	 * message to the target.
 	 */
-	if (ath10k_htt_htc_attach(htt))
+	ret = ath10k_htt_htc_attach(htt);
+	if (ret) {
+		ath10k_err("could not attach htt htc (%d)\n", ret);
 		goto err_htc_attach;
+	}
 
 	ret = ath10k_htt_tx_attach(htt);
 	if (ret) {
@@ -74,8 +74,11 @@ struct ath10k_htt *ath10k_htt_attach(struct ath10k *ar)
 		goto err_htc_attach;
 	}
 
-	if (ath10k_htt_rx_attach(htt))
+	ret = ath10k_htt_rx_attach(htt);
+	if (ret) {
+		ath10k_err("could not attach htt rx (%d)\n", ret);
 		goto err_rx_attach;
+	}
 
 	/*
 	 * Prefetch enough data to satisfy target
@@ -89,13 +92,12 @@ struct ath10k_htt *ath10k_htt_attach(struct ath10k *ar)
 		8 + /* llc snap */
 		2; /* ip4 dscp or ip6 priority */
 
-	return htt;
+	return 0;
 
 err_rx_attach:
 	ath10k_htt_tx_detach(htt);
 err_htc_attach:
-	kfree(htt);
-	return NULL;
+	return ret;
 }
 
 #define HTT_TARGET_VERSION_TIMEOUT_HZ (3*HZ)
@@ -148,5 +150,4 @@ void ath10k_htt_detach(struct ath10k_htt *htt)
 {
 	ath10k_htt_rx_detach(htt);
 	ath10k_htt_tx_detach(htt);
-	kfree(htt);
 }
drivers/net/wireless/ath/ath10k/htt.h +1 -2
index a7a7aa0..318be46 100644
@@ -20,7 +20,6 @@
 
 #include <linux/bug.h>
 
-#include "core.h"
 #include "htc.h"
 #include "rx_desc.h"
 
@@ -1317,7 +1316,7 @@ struct htt_rx_desc {
 #define HTT_LOG2_MAX_CACHE_LINE_SIZE 7	/* 2^7 = 128 */
 #define HTT_MAX_CACHE_LINE_SIZE_MASK ((1 << HTT_LOG2_MAX_CACHE_LINE_SIZE) - 1)
 
-struct ath10k_htt *ath10k_htt_attach(struct ath10k *ar);
+int ath10k_htt_attach(struct ath10k *ar);
 int ath10k_htt_attach_target(struct ath10k_htt *htt);
 void ath10k_htt_detach(struct ath10k_htt *htt);
 
drivers/net/wireless/ath/ath10k/htt_rx.c +2 -1
index de058d7..04f08d9 100644
@@ -15,6 +15,7 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
+#include "core.h"
 #include "htc.h"
 #include "htt.h"
 #include "txrx.h"
@@ -1036,7 +1037,7 @@ end:
 
 void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
 {
-	struct ath10k_htt *htt = ar->htt;
+	struct ath10k_htt *htt = &ar->htt;
 	struct htt_resp *resp = (struct htt_resp *)skb->data;
 
 	/* confirm alignment */
drivers/net/wireless/ath/ath10k/htt_tx.c +1 -1
index fa568f1..dc3f3e8 100644
@@ -153,7 +153,7 @@ void ath10k_htt_tx_detach(struct ath10k_htt *htt)
 void ath10k_htt_htc_tx_complete(struct ath10k *ar, struct sk_buff *skb)
 {
 	struct ath10k_skb_cb *skb_cb = ATH10K_SKB_CB(skb);
-	struct ath10k_htt *htt = ar->htt;
+	struct ath10k_htt *htt = &ar->htt;
 
 	if (skb_cb->htt.is_conf) {
 		dev_kfree_skb_any(skb);
drivers/net/wireless/ath/ath10k/mac.c +8 -8
index cdee800..7bc746f 100644
@@ -1394,15 +1394,15 @@ static void ath10k_tx_htt(struct ath10k *ar, struct sk_buff *skb)
 	int ret;
 
 	if (ieee80211_is_mgmt(hdr->frame_control))
-		ret = ath10k_htt_mgmt_tx(ar->htt, skb);
+		ret = ath10k_htt_mgmt_tx(&ar->htt, skb);
 	else if (ieee80211_is_nullfunc(hdr->frame_control))
 		/* FW does not report tx status properly for NullFunc frames
 		 * unless they are sent through mgmt tx path. mac80211 sends
 		 * those frames when it detects link/beacon loss and depends on
 		 * the tx status to be correct. */
-		ret = ath10k_htt_mgmt_tx(ar->htt, skb);
+		ret = ath10k_htt_mgmt_tx(&ar->htt, skb);
 	else
-		ret = ath10k_htt_tx(ar->htt, skb);
+		ret = ath10k_htt_tx(&ar->htt, skb);
 
 	if (ret) {
 		ath10k_warn("tx failed (%d). dropping packet.\n", ret);
@@ -2656,12 +2656,12 @@ static void ath10k_flush(struct ieee80211_hw *hw, u32 queues, bool drop)
 	if (drop)
 		return;
 
-	ret = wait_event_timeout(ar->htt->empty_tx_wq, ({
+	ret = wait_event_timeout(ar->htt.empty_tx_wq, ({
 			bool empty;
-			spin_lock_bh(&ar->htt->tx_lock);
-			empty = bitmap_empty(ar->htt->used_msdu_ids,
-					     ar->htt->max_num_pending_tx);
-			spin_unlock_bh(&ar->htt->tx_lock);
+			spin_lock_bh(&ar->htt.tx_lock);
+			empty = bitmap_empty(ar->htt.used_msdu_ids,
+					     ar->htt.max_num_pending_tx);
+			spin_unlock_bh(&ar->htt.tx_lock);
 			(empty);
 		}), ATH10K_FLUSH_TIMEOUT_HZ);
 	if (ret <= 0)
Signature · 1 line
1.7.9.5
#3 lore raw

PATCH 3/6 ath10k: improve locking

In reply to Michal Kazior

Add more lockdep asserts and a few conf_mutex
locks. It's better to be on the safe side.

---
 drivers/net/wireless/ath/ath10k/mac.c |   57 +++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)
drivers/net/wireless/ath/ath10k/mac.c +55 -2
index 7bc746f..def1e68 100644
@@ -43,6 +43,8 @@ static int ath10k_send_key(struct ath10k_vif *arvif,
 		.macaddr = macaddr,
 	};
 
+	lockdep_assert_held(&arvif->ar->conf_mutex);
+
 	if (key->flags & IEEE80211_KEY_FLAG_PAIRWISE)
 		arg.key_flags = WMI_KEY_PAIRWISE;
 	else
@@ -88,6 +90,8 @@ static int ath10k_install_key(struct ath10k_vif *arvif,
 	struct ath10k *ar = arvif->ar;
 	int ret;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	INIT_COMPLETION(ar->install_key_done);
 
 	ret = ath10k_send_key(arvif, key, cmd, macaddr);
@@ -369,6 +373,8 @@ static inline int ath10k_vdev_setup_sync(struct ath10k *ar)
 {
 	int ret;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	ret = wait_for_completion_timeout(&ar->vdev_setup_done,
 					  ATH10K_VDEV_SETUP_TIMEOUT_HZ);
 	if (ret == 0)
@@ -602,6 +608,8 @@ static void ath10k_control_beaconing(struct ath10k_vif *arvif,
 {
 	int ret = 0;
 
+	lockdep_assert_held(&arvif->ar->conf_mutex);
+
 	if (!info->enable_beacon) {
 		ath10k_vdev_stop(arvif);
 		return;
@@ -628,6 +636,8 @@ static void ath10k_control_ibss(struct ath10k_vif *arvif,
 {
 	int ret = 0;
 
+	lockdep_assert_held(&arvif->ar->conf_mutex);
+
 	if (!info->ibss_joined) {
 		ret = ath10k_peer_delete(arvif->ar, arvif->vdev_id, self_peer);
 		if (ret)
@@ -677,6 +687,8 @@ static void ath10k_ps_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
 	enum wmi_sta_ps_mode psmode;
 	int ret;
 
+	lockdep_assert_held(&arvif->ar->conf_mutex);
+
 	if (vif->type != NL80211_IFTYPE_STATION)
 		return;
 
@@ -719,6 +731,8 @@ static void ath10k_peer_assoc_h_basic(struct ath10k *ar,
 				      struct ieee80211_bss_conf *bss_conf,
 				      struct wmi_peer_assoc_complete_arg *arg)
 {
+	lockdep_assert_held(&ar->conf_mutex);
+
 	memcpy(arg->addr, sta->addr, ETH_ALEN);
 	arg->vdev_id = arvif->vdev_id;
 	arg->peer_aid = sta->aid;
@@ -761,6 +775,8 @@ static void ath10k_peer_assoc_h_crypto(struct ath10k *ar,
 	const u8 *rsnie = NULL;
 	const u8 *wpaie = NULL;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	bss = cfg80211_get_bss(ar->hw->wiphy, ar->hw->conf.chandef.chan,
 			       info->bssid, NULL, 0, 0, 0);
 	if (bss) {
@@ -801,6 +817,8 @@ static void ath10k_peer_assoc_h_rates(struct ath10k *ar,
 	u32 ratemask;
 	int i;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	sband = ar->hw->wiphy->bands[ar->hw->conf.chandef.chan->band];
 	ratemask = sta->supp_rates[ar->hw->conf.chandef.chan->band];
 	rates = sband->bitrates;
@@ -824,6 +842,8 @@ static void ath10k_peer_assoc_h_ht(struct ath10k *ar,
 	int smps;
 	int i, n;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	if (!ht_cap->ht_supported)
 		return;
 
@@ -902,6 +922,8 @@ static void ath10k_peer_assoc_h_qos_ap(struct ath10k *ar,
 	u32 uapsd = 0;
 	u32 max_sp = 0;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	if (sta->wme)
 		arg->peer_flags |= WMI_PEER_QOS;
 
@@ -1053,6 +1075,8 @@ static int ath10k_peer_assoc(struct ath10k *ar,
 {
 	struct wmi_peer_assoc_complete_arg arg;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	memset(&arg, 0, sizeof(struct wmi_peer_assoc_complete_arg));
 
 	ath10k_peer_assoc_h_basic(ar, arvif, sta, bss_conf, &arg);
@@ -1076,6 +1100,8 @@ static void ath10k_bss_assoc(struct ieee80211_hw *hw,
 	struct ieee80211_sta *ap_sta;
 	int ret;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	rcu_read_lock();
 
 	ap_sta = ieee80211_find_sta(vif, bss_conf->bssid);
@@ -1116,6 +1142,8 @@ static void ath10k_bss_disassoc(struct ieee80211_hw *hw,
 	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
 	int ret;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	/*
 	 * For some reason, calling VDEV-DOWN before VDEV-STOP
 	 * makes the FW to send frames via HTT after disassociation.
@@ -1149,6 +1177,8 @@ static int ath10k_station_assoc(struct ath10k *ar, struct ath10k_vif *arvif,
 {
 	int ret = 0;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	ret = ath10k_peer_assoc(ar, arvif, sta, NULL);
 	if (ret) {
 		ath10k_warn("WMI peer assoc failed for %pM\n", sta->addr);
@@ -1169,6 +1199,8 @@ static int ath10k_station_disassoc(struct ath10k *ar, struct ath10k_vif *arvif,
 {
 	int ret = 0;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	ret = ath10k_clear_peer_keys(arvif, sta->addr);
 	if (ret) {
 		ath10k_warn("could not clear all peer wep keys (%d)\n", ret);
@@ -1195,6 +1227,8 @@ static int ath10k_update_channel_list(struct ath10k *ar)
 	int ret;
 	int i;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	bands = hw->wiphy->bands;
 	for (band = 0; band < IEEE80211_NUM_BANDS; band++) {
 		if (!bands[band])
@@ -1281,6 +1315,8 @@ static void ath10k_reg_notifier(struct wiphy *wiphy,
 	struct ath10k *ar = hw->priv;
 	int ret;
 
+	mutex_lock(&ar->conf_mutex);
+
 	ath_reg_notifier_apply(wiphy, request, &ar->ath_common.regulatory);
 
 	ret = ath10k_update_channel_list(ar);
@@ -1298,6 +1334,8 @@ static void ath10k_reg_notifier(struct wiphy *wiphy,
 					    regpair->reg_5ghz_ctl);
 	if (ret)
 		ath10k_warn("could not set pdev regdomain (%d)\n", ret);
+
+	mutex_unlock(&ar->conf_mutex);
 }
 
 /***************/
@@ -1675,6 +1713,8 @@ static int ath10k_start(struct ieee80211_hw *hw)
 	struct ath10k *ar = hw->priv;
 	int ret;
 
+	mutex_lock(&ar->conf_mutex);
+
 	ret = ath10k_wmi_pdev_set_param(ar, WMI_PDEV_PARAM_PMF_QOS, 1);
 	if (ret)
 		ath10k_warn("could not enable WMI_PDEV_PARAM_PMF_QOS (%d)\n",
@@ -1685,6 +1725,7 @@ static int ath10k_start(struct ieee80211_hw *hw)
 		ath10k_warn("could not init WMI_PDEV_PARAM_DYNAMIC_BW (%d)\n",
 			    ret);
 
+	mutex_unlock(&ar->conf_mutex);
 	return 0;
 }
 
@@ -1692,9 +1733,11 @@ static void ath10k_stop(struct ieee80211_hw *hw)
 {
 	struct ath10k *ar = hw->priv;
 
-	/* avoid leaks in case FW never confirms scan for offchannel */
-	cancel_work_sync(&ar->offchan_tx_work);
+	mutex_lock(&ar->conf_mutex);
 	ath10k_offchan_tx_purge(ar);
+	mutex_unlock(&ar->conf_mutex);
+
+	cancel_work_sync(&ar->offchan_tx_work);
 }
 
 static int ath10k_config(struct ieee80211_hw *hw, u32 changed)
@@ -2378,6 +2421,8 @@ static int ath10k_conf_tx_uapsd(struct ath10k *ar, struct ieee80211_vif *vif,
 	u32 value = 0;
 	int ret = 0;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	if (arvif->vdev_type != WMI_VDEV_TYPE_STA)
 		return 0;
 
@@ -2573,6 +2618,8 @@ static void ath10k_set_rts_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
 	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
 	u32 rts = ar_iter->ar->hw->wiphy->rts_threshold;
 
+	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,
@@ -2611,6 +2658,8 @@ static void ath10k_set_frag_iter(void *data, u8 *mac, struct ieee80211_vif *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);
@@ -2656,6 +2705,8 @@ static void ath10k_flush(struct ieee80211_hw *hw, u32 queues, bool drop)
 	if (drop)
 		return;
 
+	mutex_lock(&ar->conf_mutex);
+
 	ret = wait_event_timeout(ar->htt.empty_tx_wq, ({
 			bool empty;
 			spin_lock_bh(&ar->htt.tx_lock);
@@ -2666,6 +2717,8 @@ static void ath10k_flush(struct ieee80211_hw *hw, u32 queues, bool drop)
 		}), ATH10K_FLUSH_TIMEOUT_HZ);
 	if (ret <= 0)
 		ath10k_warn("tx not flushed\n");
+
+	mutex_unlock(&ar->conf_mutex);
 }
 
 /* TODO: Implement this function properly
Signature · 1 line
1.7.9.5
#4 lore raw

PATCH 4/6 ath10k: abort scan properly if wmi_scan_stop fails

In reply to Michal Kazior
---
 drivers/net/wireless/ath/ath10k/mac.c |    4 ++++
 1 file changed, 4 insertions(+)
drivers/net/wireless/ath/ath10k/mac.c +4 -0
index def1e68..4867f30 100644
@@ -1587,6 +1587,10 @@ static int ath10k_abort_scan(struct ath10k *ar)
 	ret = ath10k_wmi_stop_scan(ar, &arg);
 	if (ret) {
 		ath10k_warn("could not submit wmi stop scan (%d)\n", ret);
+		spin_lock_bh(&ar->data_lock);
+		ar->scan.in_progress = false;
+		ath10k_offchan_tx_purge(ar);
+		spin_unlock_bh(&ar->data_lock);
 		return -EIO;
 	}
 
Signature · 1 line
1.7.9.5
#5 lore raw

PATCH 5/6 ath10k: wait for CE to drain when shutting down

In reply to Michal Kazior

ath10k_pci_process_ce() is used to process completions. Only one thread can do that though.

If one thread starts handling completions then the other one (i.e. possibly PCI shutdown) would exit immediatelely and free up memory while completions are being processed leading to corruption.

---
 drivers/net/wireless/ath/ath10k/pci.c |   21 +++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/pci.h |    2 +-
 2 files changed, 22 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/ath10k/pci.c +21 -0
index 1a59638..06124a4 100644
@@ -765,6 +765,7 @@ static int ath10k_pci_start_ce(struct ath10k *ar)
 	int i, pipe_num, completions, disable_interrupts;
 
 	spin_lock_init(&ar_pci->compl_lock);
+	init_waitqueue_head(&ar_pci->compl_wq);
 	INIT_LIST_HEAD(&ar_pci->compl_process);
 
 	for (pipe_num = 0; pipe_num < ar_pci->ce_count; pipe_num++) {
@@ -953,9 +954,28 @@ static void ath10k_pci_process_ce(struct ath10k *ar)
 
 	spin_lock_bh(&ar_pci->compl_lock);
 	ar_pci->compl_processing = false;
+	wake_up(&ar_pci->compl_wq);
 	spin_unlock_bh(&ar_pci->compl_lock);
 }
 
+/* This function assumes no new data is going to be submitted/completed. It is
+ * mainly intended to flush out completions when stopping the device. */
+static void ath10k_pci_wait_for_ce_drain(struct ath10k *ar)
+{
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	int ret;
+
+	ret = wait_event_timeout(ar_pci->compl_wq, ({
+			bool processing;
+			spin_lock_bh(&ar_pci->compl_lock);
+			processing = ar_pci->compl_processing;
+			spin_unlock_bh(&ar_pci->compl_lock);
+			(!processing);
+		}), 5*HZ);
+	if (ret == 0)
+		ath10k_warn("timed out while waiting for completions to be processed\n");
+}
+
 /* TODO - temporary mapping while we have too few CE's */
 static int ath10k_pci_hif_map_service_to_pipe(struct ath10k *ar,
 					      u16 service_id, u8 *ul_pipe,
@@ -1261,6 +1281,7 @@ static void ath10k_pci_hif_stop(struct ath10k *ar)
 	 * everything else up. */
 
 	ath10k_pci_process_ce(ar);
+	ath10k_pci_wait_for_ce_drain(ar);
 	ath10k_pci_cleanup_ce(ar);
 	ath10k_pci_buffer_cleanup(ar);
 }
drivers/net/wireless/ath/ath10k/pci.h +1 -1
index d3a2e6c..0403cea 100644
@@ -215,8 +215,8 @@ struct ath10k_pci {
 
 	/* protects compl_processing and compl_process */
 	spinlock_t compl_lock;
-
 	bool compl_processing;
+	wait_queue_head_t compl_wq;
 
 	struct hif_ce_pipe_info pipe_info[CE_COUNT_MAX];
 
Signature · 1 line
1.7.9.5
#6 lore raw

PATCH 5/6 ath10k: wait for CE to drain when shutting down

Quoted 9 lines · Michal Kazior

ath10k_pci_process_ce() is used to process completions. Only one thread can do that though.

If one thread starts handling completions then the other one (i.e. possibly PCI shutdown) would exit immediatelely and free up memory while completions are being processed leading to corruption.

[...]

Quoted 7 lines
drivers/net/wireless/ath/ath10k/pci.c +7 -0
+/* This function assumes no new data is going to be submitted/completed. It is
+ * mainly intended to flush out completions when stopping the device. */
+static void ath10k_pci_wait_for_ce_drain(struct ath10k *ar)
+{
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	int ret;
+

drivers/net/wireless/ath/ath10k/pci.c static void ath10k_pci_process_ce(struct ath10k *ar) · line 977

Quoted 10 lines
drivers/net/wireless/ath/ath10k/pci.c +10 -0
+	ret = wait_event_timeout(ar_pci->compl_wq, ({
+			bool processing;
+			spin_lock_bh(&ar_pci->compl_lock);
+			processing = ar_pci->compl_processing;
+			spin_unlock_bh(&ar_pci->compl_lock);
+			(!processing);
+		}), 5*HZ);
+	if (ret == 0)
+		ath10k_warn("timed out while waiting for completions to be processed\n");
+}

This looks like a hack to me. Wouldn't it be a better to fix make sure that all threads/tasklets are stopped, for example with tasklet_kill() and cancel_work_sync()? (And of course first making sure that we don't fire new instances). That way we could be sure that there is no other thread running while we shutdown.

Signature · 1 line
Kalle Valo
#7 lore raw

PATCH 5/6 ath10k: wait for CE to drain when shutting down

Quoted 29 lines · Kalle Valo
Quoted 9 lines · Michal Kazior

ath10k_pci_process_ce() is used to process completions. Only one thread can do that though.

If one thread starts handling completions then the other one (i.e. possibly PCI shutdown) would exit immediatelely and free up memory while completions are being processed leading to corruption.

[...]

Quoted 15 lines
drivers/net/wireless/ath/ath10k/pci.c +15 -0
+/* This function assumes no new data is going to be submitted/completed. It is
+ * mainly intended to flush out completions when stopping the device. */
+static void ath10k_pci_wait_for_ce_drain(struct ath10k *ar)
+{
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	int ret;
+
+	ret = wait_event_timeout(ar_pci->compl_wq, ({
+			bool processing;
+			spin_lock_bh(&ar_pci->compl_lock);
+			processing = ar_pci->compl_processing;
+			spin_unlock_bh(&ar_pci->compl_lock);
+			(!processing);
+		}), 5*HZ);
+	if (ret == 0)
Quoted 8 lines

drivers/net/wireless/ath/ath10k/pci.c static void ath10k_pci_process_ce(struct ath10k *ar) · line 977

Quoted 2 lines
drivers/net/wireless/ath/ath10k/pci.c +2 -0
+		ath10k_warn("timed out while waiting for completions to be processed\n");
+}

This looks like a hack to me. Wouldn't it be a better to fix make sure that all threads/tasklets are stopped, for example with tasklet_kill() and cancel_work_sync()? (And of course first making sure that we don't fire new instances). That way we could be sure that there is no other thread running while we shutdown.

Apparently I can't reproduce this bug anymore. I can't really locate where the other thread could be comming from. Maybe an interrupt? We don't unregister interrupt handlers at the point where the issue happens (we only stop CE interrupts via registers).

Perhaps we can drop this patch for now and look for a proper solution later on?

-- Pozdrawiam / Best regards, Michal Kazior.

#8 lore raw

PATCH 5/6 ath10k: wait for CE to drain when shutting down

Quoted 39 lines · Michal Kazior
Quoted 29 lines · Kalle Valo
Quoted 9 lines · Michal Kazior

ath10k_pci_process_ce() is used to process completions. Only one thread can do that though.

If one thread starts handling completions then the other one (i.e. possibly PCI shutdown) would exit immediatelely and free up memory while completions are being processed leading to corruption.

[...]

Quoted 15 lines
drivers/net/wireless/ath/ath10k/pci.c +15 -0
+/* This function assumes no new data is going to be submitted/completed. It is
+ * mainly intended to flush out completions when stopping the device. */
+static void ath10k_pci_wait_for_ce_drain(struct ath10k *ar)
+{
+	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
+	int ret;
+
+	ret = wait_event_timeout(ar_pci->compl_wq, ({
+			bool processing;
+			spin_lock_bh(&ar_pci->compl_lock);
+			processing = ar_pci->compl_processing;
+			spin_unlock_bh(&ar_pci->compl_lock);
+			(!processing);
+		}), 5*HZ);
+	if (ret == 0)
Quoted 8 lines

drivers/net/wireless/ath/ath10k/pci.c static void ath10k_pci_process_ce(struct ath10k *ar) · line 977

Quoted 2 lines
drivers/net/wireless/ath/ath10k/pci.c +2 -0
+		ath10k_warn("timed out while waiting for completions to be processed\n");
+}

This looks like a hack to me. Wouldn't it be a better to fix make sure that all threads/tasklets are stopped, for example with tasklet_kill() and cancel_work_sync()? (And of course first making sure that we don't fire new instances). That way we could be sure that there is no other thread running while we shutdown.

Quoted 4 lines

Apparently I can't reproduce this bug anymore. I can't really locate where the other thread could be comming from. Maybe an interrupt? We don't unregister interrupt handlers at the point where the issue happens (we only stop CE interrupts via registers).

In longer term I think it would be more robust to disable interrupts from host as well, just in case something goes wrong.

Quoted 2 lines

Perhaps we can drop this patch for now and look for a proper solution later on?

Sounds good to me.

Signature · 1 line
Kalle Valo
#9 lore raw

PATCH 6/6 ath10k: add missing debug prints

In reply to Michal Kazior
---
 drivers/net/wireless/ath/ath10k/wmi.c |    6 ++++++
 1 file changed, 6 insertions(+)
drivers/net/wireless/ath/ath10k/wmi.c +6 -0
index 681e941..b7e7e45 100644
@@ -1748,6 +1748,9 @@ int ath10k_wmi_vdev_install_key(struct ath10k *ar,
 	if (arg->key_data)
 		memcpy(cmd->key_data, arg->key_data, arg->key_len);
 
+	ath10k_dbg(ATH10K_DBG_WMI,
+		   "wmi vdev install key idx %d cipher %d len %d\n",
+		   arg->key_idx, arg->key_cipher, arg->key_len);
 	return ath10k_wmi_cmd_send(ar, skb, WMI_VDEV_INSTALL_KEY_CMDID);
 }
 
@@ -2011,6 +2014,9 @@ int ath10k_wmi_peer_assoc(struct ath10k *ar,
 	cmd->peer_vht_rates.tx_mcs_set =
 		__cpu_to_le32(arg->peer_vht_rates.tx_mcs_set);
 
+	ath10k_dbg(ATH10K_DBG_WMI,
+		   "wmi peer assoc vdev %d addr %pM\n",
+		   arg->vdev_id, arg->addr);
 	return ath10k_wmi_cmd_send(ar, skb, WMI_PEER_ASSOC_CMDID);
 }
 
Signature · 1 line
1.7.9.5
#10 lore raw

cover ath10k: fixes

In reply to Michal Kazior
Quoted 11 lines · Michal Kazior

This includes 2 patches from my previous patchset
that had conflicts. It is now rebased and should
apply cleanly on the github master branch.

Michal Kazior (6):
ath10k: embed HTC struct inside ath10k
ath10k: embed HTT struct inside ath10k
ath10k: improve locking
ath10k: abort scan properly if wmi_scan_stop fails
ath10k: wait for CE to drain when shutting down
ath10k: add missing debug prints

Patches 1 and 2 applied, will review patches 3-6 later.

Please don't added new unrelated patches to the patchset while in review. It just confuses everyone, especially me.

Signature · 1 line
Kalle Valo
#11 lore raw

cover ath10k: fixes

Quoted 14 lines · Kalle Valo
Quoted 11 lines · Michal Kazior

This includes 2 patches from my previous patchset
that had conflicts. It is now rebased and should
apply cleanly on the github master branch.

Michal Kazior (6):
ath10k: embed HTC struct inside ath10k
ath10k: embed HTT struct inside ath10k
ath10k: improve locking
ath10k: abort scan properly if wmi_scan_stop fails
ath10k: wait for CE to drain when shutting down
ath10k: add missing debug prints

Quoted 1 line

Patches 1 and 2 applied, will review patches 3-6 later.

Patches 3, 4 and 6 applied. Let's discuss more about patch 5.

Signature · 1 line
Kalle Valo