[ath9k-devel] [PATCH 0/2] ath10k: locking cleanup

[ath9k-devel] [PATCH 0/2] ath10k: locking cleanup

[Michal Kazior]

This patchset kills vdev_mtx and scan.lock and
uses conf_mutex and data_lock respectively.

This simplifies and unifies locking in ath10k.

Michal Kazior (2):
  ath10k: kill vdev_mtx and use conf_mutex
  ath10k: refactor scan locking

 drivers/net/wireless/ath/ath10k/core.c |    2 -
 drivers/net/wireless/ath/ath10k/core.h |    2 -
 drivers/net/wireless/ath/ath10k/mac.c  |  111 +++++++++++++-------------------
 drivers/net/wireless/ath/ath10k/wmi.c  |   10 +--
 4 files changed, 50 insertions(+), 75 deletions(-)

-- 
1.7.9.5

[ath9k-devel] [PATCH 1/2] ath10k: kill vdev_mtx and use conf_mutex

[Michal Kazior]

The vdev_mtx doesn't serve any purpose anymore.
The locking should be done with conf_mutex (it was
already, implicitly anyway).

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.c |    1 -
 drivers/net/wireless/ath/ath10k/core.h |    1 -
 drivers/net/wireless/ath/ath10k/mac.c  |   55 ++++++++++----------------------
 3 files changed, 17 insertions(+), 40 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 39bc54c..a905f5e 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -498,7 +498,6 @@ struct ath10k *ath10k_core_create(void *hif_priv, struct device *dev,
 
 	init_completion(&ar->install_key_done);
 	init_completion(&ar->vdev_setup_done);
-	mutex_init(&ar->vdev_mtx);
 
 	setup_timer(&ar->scan.timeout, ath10k_ath10k_reset_scan, (unsigned long)ar);
 
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index de47b41..9874062 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -341,7 +341,6 @@ struct ath10k {
 	struct wmi_pdev_set_wmm_params_arg wmm_params;
 	struct completion install_key_done;
 
-	struct mutex vdev_mtx;
 	struct completion vdev_setup_done;
 
 	bool hw_v1_workaround;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 3499858..97481df 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -390,7 +390,7 @@ static int ath10k_vdev_start(struct ath10k_vif *arvif)
 	};
 	int ret = 0;
 
-	mutex_lock(&ar->vdev_mtx);
+	lockdep_assert_held(&ar->conf_mutex);
 
 	INIT_COMPLETION(ar->vdev_setup_done);
 
@@ -407,17 +407,15 @@ static int ath10k_vdev_start(struct ath10k_vif *arvif)
 	ret = ath10k_wmi_vdev_start(ar, &arg);
 	if (ret) {
 		ath10k_warn("WMI vdev start failed: ret %d\n", ret);
-		goto unlock;
+		return ret;
 	}
 
 	ret = ath10k_vdev_setup_sync(ar);
 	if (ret) {
 		ath10k_warn("vdev setup failed %d\n", ret);
-		goto unlock;
+		return ret;
 	}
 
-unlock:
-	mutex_unlock(&ar->vdev_mtx);
 	return ret;
 }
 
@@ -426,24 +424,22 @@ static int ath10k_vdev_stop(struct ath10k_vif *arvif)
 	struct ath10k *ar = arvif->ar;
 	int ret;
 
-	mutex_lock(&ar->vdev_mtx);
+	lockdep_assert_held(&ar->conf_mutex);
 
 	INIT_COMPLETION(ar->vdev_setup_done);
 
 	ret = ath10k_wmi_vdev_stop(ar, arvif->vdev_id);
 	if (ret) {
 		ath10k_warn("WMI vdev stop failed: ret %d\n", ret);
-		goto unlock;
+		return ret;
 	}
 
 	ret = ath10k_vdev_setup_sync(ar);
 	if (ret) {
 		ath10k_warn("vdev setup failed %d\n", ret);
-		goto unlock;
+		return ret;
 	}
 
-unlock:
-	mutex_unlock(&ar->vdev_mtx);
 	return ret;
 }
 
@@ -469,7 +465,7 @@ static int ath10k_monitor_start(struct ath10k *ar, int vdev_id)
 	};
 	int ret = 0;
 
-	lockdep_assert_held(&ar->vdev_mtx);
+	lockdep_assert_held(&ar->conf_mutex);
 
 	ret = ath10k_wmi_vdev_start(ar, &arg);
 	if (ret) {
@@ -506,7 +502,7 @@ static int ath10k_monitor_stop(struct ath10k *ar)
 {
 	int ret = 0;
 
-	lockdep_assert_held(&ar->vdev_mtx);
+	lockdep_assert_held(&ar->conf_mutex);
 
 	/* For some reasons, ath10k_wmi_vdev_down() here couse
 	 * often ath10k_wmi_vdev_stop() to fail. Next we could
@@ -531,18 +527,17 @@ static int ath10k_monitor_create(struct ath10k *ar)
 {
 	int bit, ret = 0;
 
-	mutex_lock(&ar->vdev_mtx);
+	lockdep_assert_held(&ar->conf_mutex);
 
 	if (ar->monitor_present) {
 		ath10k_warn("Monitor mode already enabled\n");
-		goto unlock;
+		return 0;
 	}
 
 	bit = ffs(ar->free_vdev_map);
 	if (bit == 0) {
 		ath10k_warn("No free VDEV slots\n");
-		ret = -ENOMEM;
-		goto unlock;
+		return -ENOMEM;
 	}
 
 	ar->monitor_vdev_id = bit - 1;
@@ -560,15 +555,13 @@ static int ath10k_monitor_create(struct ath10k *ar)
 		   ar->monitor_vdev_id);
 
 	ar->monitor_present = true;
-	goto unlock;
+	return 0;
 
 vdev_fail:
 	/*
 	 * Restore the ID to the global map.
 	 */
 	ar->free_vdev_map |= 1 << (ar->monitor_vdev_id);
-unlock:
-	mutex_unlock(&ar->vdev_mtx);
 	return ret;
 }
 
@@ -576,15 +569,15 @@ static int ath10k_monitor_destroy(struct ath10k *ar)
 {
 	int ret = 0;
 
-	mutex_lock(&ar->vdev_mtx);
+	lockdep_assert_held(&ar->conf_mutex);
 
 	if (!ar->monitor_present)
-		goto unlock;
+		return 0;
 
 	ret = ath10k_wmi_vdev_delete(ar, ar->monitor_vdev_id);
 	if (ret) {
 		ath10k_warn("WMI vdev monitor delete failed: %d\n", ret);
-		goto unlock;
+		return ret;
 	}
 
 	ar->free_vdev_map |= 1 << (ar->monitor_vdev_id);
@@ -592,8 +585,6 @@ static int ath10k_monitor_destroy(struct ath10k *ar)
 
 	ath10k_dbg(ATH10K_DBG_MAC, "Monitor interface destroyed, vdev id: %d\n",
 		   ar->monitor_vdev_id);
-unlock:
-	mutex_unlock(&ar->vdev_mtx);
 	return ret;
 }
 
@@ -1595,14 +1586,11 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
 	arvif->ar = ar;
 	arvif->vif = vif;
 
-	mutex_lock(&ar->vdev_mtx);
 	if ((vif->type == NL80211_IFTYPE_MONITOR) && ar->monitor_present) {
 		ath10k_warn("Only one monitor interface allowed\n");
-		mutex_unlock(&ar->vdev_mtx);
 		ret = -EBUSY;
 		goto exit;
 	}
-	mutex_unlock(&ar->vdev_mtx);
 
 	bit = ffs(ar->free_vdev_map);
 	if (bit == 0) {
@@ -1684,11 +1672,8 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
 			ath10k_warn("Failed to set PSPOLL count: %d\n", ret);
 	}
 
-	if (arvif->vdev_type == WMI_VDEV_TYPE_MONITOR) {
-		mutex_lock(&ar->vdev_mtx);
+	if (arvif->vdev_type == WMI_VDEV_TYPE_MONITOR)
 		ar->monitor_present = true;
-		mutex_unlock(&ar->vdev_mtx);
-	}
 
 exit:
 	mutex_unlock(&ar->conf_mutex);
@@ -1720,11 +1705,8 @@ static void ath10k_remove_interface(struct ieee80211_hw *hw,
 	if (ret)
 		ath10k_warn("WMI vdev delete failed: %d\n", ret);
 
-	if (arvif->vdev_type == WMI_VDEV_TYPE_MONITOR) {
-		mutex_lock(&ar->vdev_mtx);
+	if (arvif->vdev_type == WMI_VDEV_TYPE_MONITOR)
 		ar->monitor_present = false;
-		mutex_unlock(&ar->vdev_mtx);
-	}
 
 	mutex_unlock(&ar->conf_mutex);
 }
@@ -1756,8 +1738,6 @@ static void ath10k_configure_filter(struct ieee80211_hw *hw,
 	*total_flags &= SUPPORTED_FILTERS;
 	ar->filter_flags = *total_flags;
 
-	mutex_lock(&ar->vdev_mtx);
-
 	if ((ar->filter_flags & FIF_PROMISC_IN_BSS) &&
 	    !ar->monitor_enabled) {
 		ret = ath10k_monitor_start(ar, ar->monitor_vdev_id);
@@ -1774,7 +1754,6 @@ static void ath10k_configure_filter(struct ieee80211_hw *hw,
 			ath10k_dbg(ATH10K_DBG_MAC, "Monitor mode stopped\n");
 	}
 
-	mutex_unlock(&ar->vdev_mtx);
 	mutex_unlock(&ar->conf_mutex);
 }
 
-- 
1.7.9.5

[ath9k-devel] [PATCH 1/2] ath10k: kill vdev_mtx and use conf_mutex

[Kalle Valo]

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

> The vdev_mtx doesn't serve any purpose anymore.
> The locking should be done with conf_mutex (it was
> already, implicitly anyway).
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

Patch 1 applied, but I have a question with patch 2.

-- 
Kalle Valo

[ath9k-devel] [PATCH 2/2] ath10k: refactor scan locking

[Michal Kazior]

Since we have data_lock it is no longer necessary
to have scan.lock.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.c |    1 -
 drivers/net/wireless/ath/ath10k/core.h |    1 -
 drivers/net/wireless/ath/ath10k/mac.c  |   56 +++++++++++++++-----------------
 drivers/net/wireless/ath/ath10k/wmi.c  |   10 +++---
 4 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index a905f5e..527ae84 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -494,7 +494,6 @@ struct ath10k *ath10k_core_create(void *hif_priv, struct device *dev,
 
 	init_completion(&ar->scan.started);
 	init_completion(&ar->scan.completed);
-	spin_lock_init(&ar->scan.lock);
 
 	init_completion(&ar->install_key_done);
 	init_completion(&ar->vdev_setup_done);
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 9874062..cdf9674 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -316,7 +316,6 @@ struct ath10k {
 	} hw_params;
 
 	struct {
-		spinlock_t lock;
 		struct completion started;
 		struct completion completed;
 		struct timer_list timeout;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 97481df..b231992 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -1372,11 +1372,9 @@ void ath10k_ath10k_reset_scan(unsigned long ptr)
 {
 	struct ath10k *ar = (struct ath10k *)ptr;
 
-	spin_lock_bh(&ar->scan.lock);
-
+	spin_lock_bh(&ar->data_lock);
 	if (!ar->scan.in_progress) {
-		/* lucky! scan must've completed right before timeout */
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 		return;
 	}
 
@@ -1389,7 +1387,7 @@ void ath10k_ath10k_reset_scan(unsigned long ptr)
 
 	ar->scan.in_progress = false;
 	complete_all(&ar->scan.completed);
-	spin_unlock_bh(&ar->scan.lock);
+	spin_unlock_bh(&ar->data_lock);
 }
 
 static void ath10k_abort_scan(struct ath10k *ar)
@@ -1405,30 +1403,32 @@ static void ath10k_abort_scan(struct ath10k *ar)
 
 	del_timer_sync(&ar->scan.timeout);
 
-	spin_lock_bh(&ar->scan.lock);
+	spin_lock_bh(&ar->data_lock);
 	if (!ar->scan.in_progress) {
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 		return;
 	}
 
 	ar->scan.aborting = true;
-	spin_unlock_bh(&ar->scan.lock);
+	spin_unlock_bh(&ar->data_lock);
 
 	ret = ath10k_wmi_stop_scan(ar, &arg);
 	if (ret)
 		ath10k_warn("%s: ath10k_wmi_stop_scan failed (%d)\n", __func__, ret);
 
+	ath10k_wmi_flush_tx(ar);
+
 	ret = wait_for_completion_timeout(&ar->scan.completed, 3*HZ);
 	if (ret == 0)
 		ret = -ETIMEDOUT;
 
-	spin_lock_bh(&ar->scan.lock);
+	spin_lock_bh(&ar->data_lock);
 	if (ar->scan.in_progress) {
 		ath10k_warn("%s: could not stop scan (%d)\n", __func__, ret);
 		ar->scan.in_progress = false;
 		ath10k_ath10k_offchan_tx_purge(ar);
 	}
-	spin_unlock_bh(&ar->scan.lock);
+	spin_unlock_bh(&ar->data_lock);
 }
 
 static int ath10k_start_scan(struct ath10k *ar,
@@ -1436,9 +1436,11 @@ static int ath10k_start_scan(struct ath10k *ar,
 {
 	int ret;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	ret = ath10k_wmi_start_scan(ar, arg);
 	if (ret)
-		goto abort;
+		return ret;
 
 	/* make sure we submit the command so the completion
 	* timeout makes sense */
@@ -1448,7 +1450,7 @@ static int ath10k_start_scan(struct ath10k *ar,
 	if (ret == 0)
 		ret = -ETIMEDOUT;
 	if (ret < 0)
-		goto abort;
+		return ret;
 
 	/* the scan can complete earlier, before we even
 	 * start the timer. in that case the timer handler
@@ -1456,11 +1458,6 @@ static int ath10k_start_scan(struct ath10k *ar,
 	 * false. */
 	mod_timer(&ar->scan.timeout, jiffies + (arg->max_scan_time*HZ)/1000);
 	return 0;
-abort:
-	spin_lock_bh(&ar->scan.lock);
-	ar->scan.in_progress = false;
-	spin_unlock_bh(&ar->scan.lock);
-	return ret;
 }
 
 /**********************/
@@ -1495,10 +1492,10 @@ static void ath10k_tx(struct ieee80211_hw *hw,
 	ATH10K_SKB_CB(skb)->htt.vdev_id = vdev_id;
 
 	if (info->flags & IEEE80211_TX_CTL_TX_OFFCHAN) {
-		spin_lock_bh(&ar->scan.lock);
+		spin_lock_bh(&ar->data_lock);
 		ATH10K_SKB_CB(skb)->htt.is_offchan = true;
 		ATH10K_SKB_CB(skb)->htt.vdev_id = ar->scan.vdev_id;
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 
 		ath10k_dbg(ATH10K_DBG_MAC, "queued offchannel skb %p\n", skb);
 
@@ -1937,9 +1934,9 @@ static int ath10k_hw_scan(struct ieee80211_hw *hw,
 
 	mutex_lock(&ar->conf_mutex);
 
-	spin_lock_bh(&ar->scan.lock);
+	spin_lock_bh(&ar->data_lock);
 	if (ar->scan.in_progress) {
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 		ret = -EBUSY;
 		goto exit;
 	}
@@ -1950,7 +1947,7 @@ static int ath10k_hw_scan(struct ieee80211_hw *hw,
 	ar->scan.aborting = false;
 	ar->scan.is_roc = false;
 	ar->scan.vdev_id = arvif->vdev_id;
-	spin_unlock_bh(&ar->scan.lock);
+	spin_unlock_bh(&ar->data_lock);
 
 	memset(&arg, 0, sizeof(arg));
 	ath10k_wmi_start_scan_init(ar, &arg);
@@ -1981,9 +1978,10 @@ static int ath10k_hw_scan(struct ieee80211_hw *hw,
 
 	ret = ath10k_start_scan(ar, &arg);
 	if (ret) {
-		spin_lock_bh(&ar->scan.lock);
+		ath10k_warn("could not start hw scan (%d)\n", ret);
+		spin_lock_bh(&ar->data_lock);
 		ar->scan.in_progress = false;
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 	}
 
 exit:
@@ -2214,9 +2212,9 @@ static int ath10k_remain_on_channel(struct ieee80211_hw *hw,
 
 	mutex_lock(&ar->conf_mutex);
 
-	spin_lock_bh(&ar->scan.lock);
+	spin_lock_bh(&ar->data_lock);
 	if (ar->scan.in_progress) {
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 		ret = -EBUSY;
 		goto exit;
 	}
@@ -2227,7 +2225,7 @@ static int ath10k_remain_on_channel(struct ieee80211_hw *hw,
 	ar->scan.aborting = false;
 	ar->scan.is_roc = true;
 	ar->scan.vdev_id = arvif->vdev_id;
-	spin_unlock_bh(&ar->scan.lock);
+	spin_unlock_bh(&ar->data_lock);
 
 	memset(&arg, 0, sizeof(arg));
 	ath10k_wmi_start_scan_init(ar, &arg);
@@ -2244,9 +2242,9 @@ static int ath10k_remain_on_channel(struct ieee80211_hw *hw,
 	ret = ath10k_start_scan(ar, &arg);
 	if (ret) {
 		ath10k_warn("could not start roc scan (%d)\n", ret);
-		spin_lock_bh(&ar->scan.lock);
+		spin_lock_bh(&ar->data_lock);
 		ar->scan.in_progress = false;
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 	}
 
 exit:
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index d19d2cc..fe9d844 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -149,10 +149,12 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb)
 	case WMI_SCAN_EVENT_STARTED:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_STARTED\n");
 
-		if (ar->scan.is_roc)
+		spin_lock_bh(&ar->data_lock);
+		if (ar->scan.in_progress && ar->scan.is_roc)
 			ieee80211_ready_on_channel(ar->hw);
 
 		complete(&ar->scan.started);
+		spin_unlock_bh(&ar->data_lock);
 		break;
 	case WMI_SCAN_EVENT_COMPLETED:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_COMPLETED\n");
@@ -175,10 +177,10 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb)
 
 		rcu_assign_pointer(ar->scan_channel, NULL);
 
-		spin_lock_bh(&ar->scan.lock);
+		spin_lock_bh(&ar->data_lock);
 		if (!ar->scan.in_progress) {
+			spin_unlock_bh(&ar->data_lock);
 			ath10k_warn("no scan requested, ignoring\n");
-			spin_unlock_bh(&ar->scan.lock);
 			break;
 		}
 
@@ -194,7 +196,7 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb)
 		del_timer(&ar->scan.timeout);
 		complete_all(&ar->scan.completed);
 		ar->scan.in_progress = false;
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 		break;
 	case WMI_SCAN_EVENT_BSS_CHANNEL:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_BSS_CHANNEL\n");
-- 
1.7.9.5

[ath9k-devel] [PATCH 2/2] ath10k: refactor scan locking

[Kalle Valo]

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

> Since we have data_lock it is no longer necessary
> to have scan.lock.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---

[...]

>  	ret = ath10k_wmi_stop_scan(ar, &arg);
>  	if (ret)
>  		ath10k_warn("%s: ath10k_wmi_stop_scan failed (%d)\n", __func__, ret);
>  
> +	ath10k_wmi_flush_tx(ar);

Is this by accident?

-- 
Kalle Valo

[ath9k-devel] [PATCH 2/2] ath10k: refactor scan locking

[Michal Kazior]

On 22/04/13 10:08, Kalle Valo wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> Since we have data_lock it is no longer necessary
>> to have scan.lock.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>> ---
>
> [...]
>
>>   	ret = ath10k_wmi_stop_scan(ar, &arg);
>>   	if (ret)
>>   		ath10k_warn("%s: ath10k_wmi_stop_scan failed (%d)\n", __func__, ret);
>>
>> +	ath10k_wmi_flush_tx(ar);
>
> Is this by accident?

Oh. I noticed we were missing it. Must've mixed it into the patch. 
Should I resend the patches split?


-- Pozdrawiam / Best regards, Michal Kazior.

[ath9k-devel] [PATCH 2/2] ath10k: refactor scan locking

[Kalle Valo]

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

> On 22/04/13 10:08, Kalle Valo wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> Since we have data_lock it is no longer necessary
>>> to have scan.lock.
>>>
>>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>>> ---
>>
>> [...]
>>
>>>   	ret = ath10k_wmi_stop_scan(ar, &arg);
>>>   	if (ret)
>>>   		ath10k_warn("%s: ath10k_wmi_stop_scan failed (%d)\n", __func__, ret);
>>>
>>> +	ath10k_wmi_flush_tx(ar);
>>
>> Is this by accident?
>
> Oh. I noticed we were missing it. Must've mixed it into the patch.
> Should I resend the patches split?

Yes, please split patch 2. That makes it easier to bisect etc. And
please note that I have already applied patch 1.

But please wait an hour or two so that I have gone through my patch
backlog. Less conflicts that way.

-- 
Kalle Valo

[ath9k-devel] [PATCH 0/2] ath10k: scan fixes

[Michal Kazior]

This is a result of a patch split.

Michal Kazior (2):
  ath10k: sync wmi before starting scan timeout
  ath10k: refactor scan locking

 drivers/net/wireless/ath/ath10k/core.c |    1 -
 drivers/net/wireless/ath/ath10k/core.h |    1 -
 drivers/net/wireless/ath/ath10k/mac.c  |   56 +++++++++++++++-----------------
 drivers/net/wireless/ath/ath10k/wmi.c  |   10 +++---
 4 files changed, 33 insertions(+), 35 deletions(-)

-- 
1.7.9.5

[ath9k-devel] [PATCH 2/2] ath10k: refactor scan locking

[Michal Kazior]

Since we have data_lock it is no longer necessary
to have scan.lock.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.c |    1 -
 drivers/net/wireless/ath/ath10k/core.h |    1 -
 drivers/net/wireless/ath/ath10k/mac.c  |   54 +++++++++++++++-----------------
 drivers/net/wireless/ath/ath10k/wmi.c  |   10 +++---
 4 files changed, 31 insertions(+), 35 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 7036695..c9068b0 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -482,7 +482,6 @@ struct ath10k *ath10k_core_create(void *hif_priv, struct device *dev,
 
 	init_completion(&ar->scan.started);
 	init_completion(&ar->scan.completed);
-	spin_lock_init(&ar->scan.lock);
 
 	init_completion(&ar->install_key_done);
 	init_completion(&ar->vdev_setup_done);
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 4e19264..65b1d58 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -309,7 +309,6 @@ struct ath10k {
 	} hw_params;
 
 	struct {
-		spinlock_t lock;
 		struct completion started;
 		struct completion completed;
 		struct timer_list timeout;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 4589f6f..943c879 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -1372,11 +1372,9 @@ void ath10k_reset_scan(unsigned long ptr)
 {
 	struct ath10k *ar = (struct ath10k *)ptr;
 
-	spin_lock_bh(&ar->scan.lock);
-
+	spin_lock_bh(&ar->data_lock);
 	if (!ar->scan.in_progress) {
-		/* lucky! scan must've completed right before timeout */
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 		return;
 	}
 
@@ -1389,7 +1387,7 @@ void ath10k_reset_scan(unsigned long ptr)
 
 	ar->scan.in_progress = false;
 	complete_all(&ar->scan.completed);
-	spin_unlock_bh(&ar->scan.lock);
+	spin_unlock_bh(&ar->data_lock);
 }
 
 static void ath10k_abort_scan(struct ath10k *ar)
@@ -1405,14 +1403,14 @@ static void ath10k_abort_scan(struct ath10k *ar)
 
 	del_timer_sync(&ar->scan.timeout);
 
-	spin_lock_bh(&ar->scan.lock);
+	spin_lock_bh(&ar->data_lock);
 	if (!ar->scan.in_progress) {
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 		return;
 	}
 
 	ar->scan.aborting = true;
-	spin_unlock_bh(&ar->scan.lock);
+	spin_unlock_bh(&ar->data_lock);
 
 	ret = ath10k_wmi_stop_scan(ar, &arg);
 	if (ret)
@@ -1424,13 +1422,13 @@ static void ath10k_abort_scan(struct ath10k *ar)
 	if (ret == 0)
 		ret = -ETIMEDOUT;
 
-	spin_lock_bh(&ar->scan.lock);
+	spin_lock_bh(&ar->data_lock);
 	if (ar->scan.in_progress) {
 		ath10k_warn("%s: could not stop scan (%d)\n", __func__, ret);
 		ar->scan.in_progress = false;
 		ath10k_offchan_tx_purge(ar);
 	}
-	spin_unlock_bh(&ar->scan.lock);
+	spin_unlock_bh(&ar->data_lock);
 }
 
 static int ath10k_start_scan(struct ath10k *ar,
@@ -1438,9 +1436,11 @@ static int ath10k_start_scan(struct ath10k *ar,
 {
 	int ret;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
 	ret = ath10k_wmi_start_scan(ar, arg);
 	if (ret)
-		goto abort;
+		return ret;
 
 	/* make sure we submit the command so the completion
 	* timeout makes sense */
@@ -1450,7 +1450,7 @@ static int ath10k_start_scan(struct ath10k *ar,
 	if (ret == 0)
 		ret = -ETIMEDOUT;
 	if (ret < 0)
-		goto abort;
+		return ret;
 
 	/* the scan can complete earlier, before we even
 	 * start the timer. in that case the timer handler
@@ -1458,11 +1458,6 @@ static int ath10k_start_scan(struct ath10k *ar,
 	 * false. */
 	mod_timer(&ar->scan.timeout, jiffies + (arg->max_scan_time*HZ)/1000);
 	return 0;
-abort:
-	spin_lock_bh(&ar->scan.lock);
-	ar->scan.in_progress = false;
-	spin_unlock_bh(&ar->scan.lock);
-	return ret;
 }
 
 /**********************/
@@ -1509,10 +1504,10 @@ static void ath10k_tx(struct ieee80211_hw *hw,
 	ATH10K_SKB_CB(skb)->htt.tid = tid;
 
 	if (info->flags & IEEE80211_TX_CTL_TX_OFFCHAN) {
-		spin_lock_bh(&ar->scan.lock);
+		spin_lock_bh(&ar->data_lock);
 		ATH10K_SKB_CB(skb)->htt.is_offchan = true;
 		ATH10K_SKB_CB(skb)->htt.vdev_id = ar->scan.vdev_id;
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 
 		ath10k_dbg(ATH10K_DBG_MAC, "queued offchannel skb %p\n", skb);
 
@@ -1951,9 +1946,9 @@ static int ath10k_hw_scan(struct ieee80211_hw *hw,
 
 	mutex_lock(&ar->conf_mutex);
 
-	spin_lock_bh(&ar->scan.lock);
+	spin_lock_bh(&ar->data_lock);
 	if (ar->scan.in_progress) {
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 		ret = -EBUSY;
 		goto exit;
 	}
@@ -1964,7 +1959,7 @@ static int ath10k_hw_scan(struct ieee80211_hw *hw,
 	ar->scan.aborting = false;
 	ar->scan.is_roc = false;
 	ar->scan.vdev_id = arvif->vdev_id;
-	spin_unlock_bh(&ar->scan.lock);
+	spin_unlock_bh(&ar->data_lock);
 
 	memset(&arg, 0, sizeof(arg));
 	ath10k_wmi_start_scan_init(ar, &arg);
@@ -1995,9 +1990,10 @@ static int ath10k_hw_scan(struct ieee80211_hw *hw,
 
 	ret = ath10k_start_scan(ar, &arg);
 	if (ret) {
-		spin_lock_bh(&ar->scan.lock);
+		ath10k_warn("could not start hw scan (%d)\n", ret);
+		spin_lock_bh(&ar->data_lock);
 		ar->scan.in_progress = false;
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 	}
 
 exit:
@@ -2228,9 +2224,9 @@ static int ath10k_remain_on_channel(struct ieee80211_hw *hw,
 
 	mutex_lock(&ar->conf_mutex);
 
-	spin_lock_bh(&ar->scan.lock);
+	spin_lock_bh(&ar->data_lock);
 	if (ar->scan.in_progress) {
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 		ret = -EBUSY;
 		goto exit;
 	}
@@ -2241,7 +2237,7 @@ static int ath10k_remain_on_channel(struct ieee80211_hw *hw,
 	ar->scan.aborting = false;
 	ar->scan.is_roc = true;
 	ar->scan.vdev_id = arvif->vdev_id;
-	spin_unlock_bh(&ar->scan.lock);
+	spin_unlock_bh(&ar->data_lock);
 
 	memset(&arg, 0, sizeof(arg));
 	ath10k_wmi_start_scan_init(ar, &arg);
@@ -2258,9 +2254,9 @@ static int ath10k_remain_on_channel(struct ieee80211_hw *hw,
 	ret = ath10k_start_scan(ar, &arg);
 	if (ret) {
 		ath10k_warn("could not start roc scan (%d)\n", ret);
-		spin_lock_bh(&ar->scan.lock);
+		spin_lock_bh(&ar->data_lock);
 		ar->scan.in_progress = false;
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 	}
 
 exit:
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index fa521b0..ebb212e 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -149,10 +149,12 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb)
 	case WMI_SCAN_EVENT_STARTED:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_STARTED\n");
 
-		if (ar->scan.is_roc)
+		spin_lock_bh(&ar->data_lock);
+		if (ar->scan.in_progress && ar->scan.is_roc)
 			ieee80211_ready_on_channel(ar->hw);
 
 		complete(&ar->scan.started);
+		spin_unlock_bh(&ar->data_lock);
 		break;
 	case WMI_SCAN_EVENT_COMPLETED:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_COMPLETED\n");
@@ -175,10 +177,10 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb)
 
 		rcu_assign_pointer(ar->scan_channel, NULL);
 
-		spin_lock_bh(&ar->scan.lock);
+		spin_lock_bh(&ar->data_lock);
 		if (!ar->scan.in_progress) {
+			spin_unlock_bh(&ar->data_lock);
 			ath10k_warn("no scan requested, ignoring\n");
-			spin_unlock_bh(&ar->scan.lock);
 			break;
 		}
 
@@ -194,7 +196,7 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb)
 		del_timer(&ar->scan.timeout);
 		complete_all(&ar->scan.completed);
 		ar->scan.in_progress = false;
-		spin_unlock_bh(&ar->scan.lock);
+		spin_unlock_bh(&ar->data_lock);
 		break;
 	case WMI_SCAN_EVENT_BSS_CHANNEL:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_BSS_CHANNEL\n");
-- 
1.7.9.5