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

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

[Michal Kazior]

Michal Kazior (5):
  ath10k: use data_lock to protect {scan,rx}_channel
  ath10k: simplify scan event locking
  ath10k: increase scan timeout
  ath10k: wait for remain on channel chan switch
  ath10k: abort scan if start completion never came

 drivers/net/wireless/ath/ath10k/core.c |    1 +
 drivers/net/wireless/ath/ath10k/core.h |    6 ++++--
 drivers/net/wireless/ath/ath10k/mac.c  |   26 ++++++++++++++++++++++----
 drivers/net/wireless/ath/ath10k/txrx.c |    9 ++++-----
 drivers/net/wireless/ath/ath10k/wmi.c  |   21 ++++++++++-----------
 5 files changed, 41 insertions(+), 22 deletions(-)

-- 
1.7.9.5

[ath9k-devel] [PATCH 1/5] ath10k: use data_lock to protect {scan, rx}_channel

[Michal Kazior]

We used RCU in a funky way.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h |    4 ++--
 drivers/net/wireless/ath/ath10k/mac.c  |    4 +++-
 drivers/net/wireless/ath/ath10k/txrx.c |    9 ++++-----
 drivers/net/wireless/ath/ath10k/wmi.c  |   12 +++++++-----
 4 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index fbf4d11..1110551 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -315,10 +315,10 @@ struct ath10k {
 	} mac;
 
 	/* should never be NULL; needed for regular htt rx */
-	struct ieee80211_channel __rcu *rx_channel;
+	struct ieee80211_channel *rx_channel;
 
 	/* valid during scan; needed for mgmt rx during scan */
-	struct ieee80211_channel __rcu *scan_channel;
+	struct ieee80211_channel *scan_channel;
 
 	int free_vdev_map;
 	int monitor_vdev_id;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index b5cc3c8..3a27f35 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -1589,7 +1589,9 @@ static int ath10k_config(struct ieee80211_hw *hw, u32 changed)
 	if (changed & IEEE80211_CONF_CHANGE_CHANNEL) {
 		ath10k_dbg(ATH10K_DBG_MAC, "Config channel %d mhz\n",
 			   conf->chandef.chan->center_freq);
-		rcu_assign_pointer(ar->rx_channel, conf->chandef.chan);
+		spin_lock_bh(&ar->data_lock);
+		ar->rx_channel = conf->chandef.chan;
+		spin_unlock_bh(&ar->data_lock);
 	}
 
 	if (changed & IEEE80211_CONF_CHANGE_PS) {
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index 9d0b1e2..a32448c 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -261,13 +261,13 @@ void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info)
 
 	status->signal = info->signal;
 
-	rcu_read_lock();
-	ch = rcu_dereference(ar->scan_channel);
+	spin_lock_bh(&ar->data_lock);
+	ch = ar->scan_channel;
 	if (!ch)
-		ch = rcu_dereference(ar->rx_channel);
+		ch = ar->rx_channel;
+	spin_unlock_bh(&ar->data_lock);
 
 	if (!ch) {
-		rcu_read_unlock();
 		ath10k_warn("no channel configured; ignoring frame!\n");
 		dev_kfree_skb_any(info->skb);
 		return;
@@ -276,7 +276,6 @@ void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info)
 	process_rx_rates(ar, info, ch->band, status);
 	status->band = ch->band;
 	status->freq = ch->center_freq;
-	rcu_read_unlock();
 
 	ath10k_dbg(ATH10K_DBG_HTT,
 		   "rx skb %p len %u %s%s%s%s%s %srate_idx %u vht_nss %u freq %u band %u\n",
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 3b819c8..c44a2ff 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -177,9 +177,8 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb)
 			break;
 		}
 
-		rcu_assign_pointer(ar->scan_channel, NULL);
-
 		spin_lock_bh(&ar->data_lock);
+		ar->scan_channel = NULL;
 		if (!ar->scan.in_progress) {
 			spin_unlock_bh(&ar->data_lock);
 			ath10k_warn("no scan requested, ignoring\n");
@@ -202,12 +201,15 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb)
 		break;
 	case WMI_SCAN_EVENT_BSS_CHANNEL:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_BSS_CHANNEL\n");
-		rcu_assign_pointer(ar->scan_channel, NULL);
+		spin_lock_bh(&ar->data_lock);
+		ar->scan_channel = NULL;
+		spin_unlock_bh(&ar->data_lock);
 		break;
 	case WMI_SCAN_EVENT_FOREIGN_CHANNEL:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_FOREIGN_CHANNEL\n");
-		rcu_assign_pointer(ar->scan_channel,
-				   ieee80211_get_channel(ar->hw->wiphy, freq));
+		spin_lock_bh(&ar->data_lock);
+		ar->scan_channel = ieee80211_get_channel(ar->hw->wiphy, freq);
+		spin_unlock_bh(&ar->data_lock);
 		break;
 	case WMI_SCAN_EVENT_DEQUEUED:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_DEQUEUED\n");
-- 
1.7.9.5

[ath9k-devel] [PATCH 2/5] ath10k: simplify scan event locking

[Michal Kazior]

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

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index c44a2ff..efa72fc 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -147,16 +147,15 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb)
 		   "scan_id %d vdev_id %d\n",
 		   event_type, reason, freq, req_id, scan_id, vdev_id);
 
+	spin_lock_bh(&ar->data_lock);
+
 	switch (event_type) {
 	case WMI_SCAN_EVENT_STARTED:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_STARTED\n");
-
-		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");
@@ -177,10 +176,8 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb)
 			break;
 		}
 
-		spin_lock_bh(&ar->data_lock);
 		ar->scan_channel = NULL;
 		if (!ar->scan.in_progress) {
-			spin_unlock_bh(&ar->data_lock);
 			ath10k_warn("no scan requested, ignoring\n");
 			break;
 		}
@@ -197,19 +194,14 @@ 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->data_lock);
 		break;
 	case WMI_SCAN_EVENT_BSS_CHANNEL:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_BSS_CHANNEL\n");
-		spin_lock_bh(&ar->data_lock);
 		ar->scan_channel = NULL;
-		spin_unlock_bh(&ar->data_lock);
 		break;
 	case WMI_SCAN_EVENT_FOREIGN_CHANNEL:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_FOREIGN_CHANNEL\n");
-		spin_lock_bh(&ar->data_lock);
 		ar->scan_channel = ieee80211_get_channel(ar->hw->wiphy, freq);
-		spin_unlock_bh(&ar->data_lock);
 		break;
 	case WMI_SCAN_EVENT_DEQUEUED:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_DEQUEUED\n");
@@ -224,6 +216,7 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb)
 		break;
 	}
 
+	spin_unlock_bh(&ar->data_lock);
 	return 0;
 }
 
-- 
1.7.9.5

[ath9k-devel] [PATCH 3/5] ath10k: increase scan timeout

[Michal Kazior]

We can't use the max scan time as the timeout
itself. We must account for various overheads (ie.
command/event processing).

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

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 3a27f35..df57772 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -1481,8 +1481,10 @@ static int ath10k_start_scan(struct ath10k *ar,
 	/* the scan can complete earlier, before we even
 	 * start the timer. in that case the timer handler
 	 * checks ar->scan.in_progress and bails out if its
-	 * false. */
-	mod_timer(&ar->scan.timeout, jiffies + (arg->max_scan_time*HZ)/1000);
+	 * false. Add a 200ms margin to account event/command
+	 * processing. */
+	mod_timer(&ar->scan.timeout, jiffies +
+		  ((arg->max_scan_time+200)*HZ)/1000);
 	return 0;
 }
 
-- 
1.7.9.5

[ath9k-devel] [PATCH 4/5] ath10k: wait for remain on channel chan switch

[Michal Kazior]

We didn't wait for scan channel change event so we
could end up sending frames to a different
channel.

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

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 2be47f3..5ed04af 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -466,6 +466,7 @@ struct ath10k *ath10k_core_create(void *hif_priv, struct device *dev,
 
 	init_completion(&ar->scan.started);
 	init_completion(&ar->scan.completed);
+	init_completion(&ar->scan.on_channel);
 
 	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 1110551..6c681cf 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -303,11 +303,13 @@ struct ath10k {
 	struct {
 		struct completion started;
 		struct completion completed;
+		struct completion on_channel;
 		struct timer_list timeout;
 		bool is_roc;
 		bool in_progress;
 		bool aborting;
 		int vdev_id;
+		int roc_freq;
 	} scan;
 
 	struct {
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index df57772..72ac899 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2294,10 +2294,12 @@ static int ath10k_remain_on_channel(struct ieee80211_hw *hw,
 
 	INIT_COMPLETION(ar->scan.started);
 	INIT_COMPLETION(ar->scan.completed);
+	INIT_COMPLETION(ar->scan.on_channel);
 	ar->scan.in_progress = true;
 	ar->scan.aborting = false;
 	ar->scan.is_roc = true;
 	ar->scan.vdev_id = arvif->vdev_id;
+	ar->scan.roc_freq = chan->center_freq;
 	spin_unlock_bh(&ar->data_lock);
 
 	memset(&arg, 0, sizeof(arg));
@@ -2318,8 +2320,18 @@ static int ath10k_remain_on_channel(struct ieee80211_hw *hw,
 		spin_lock_bh(&ar->data_lock);
 		ar->scan.in_progress = false;
 		spin_unlock_bh(&ar->data_lock);
+		goto exit;
+	}
+
+	ret = wait_for_completion_timeout(&ar->scan.on_channel, 3*HZ);
+	if (ret == 0) {
+		ath10k_warn("could not switch to channel for roc scan\n");
+		ath10k_abort_scan(ar);
+		ret = -ETIMEDOUT;
+		goto exit;
 	}
 
+	ret = 0;
 exit:
 	mutex_unlock(&ar->conf_mutex);
 	return ret;
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index efa72fc..404976d 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -202,6 +202,10 @@ static int ath10k_wmi_event_scan(struct ath10k *ar, struct sk_buff *skb)
 	case WMI_SCAN_EVENT_FOREIGN_CHANNEL:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_FOREIGN_CHANNEL\n");
 		ar->scan_channel = ieee80211_get_channel(ar->hw->wiphy, freq);
+		if (ar->scan.in_progress && ar->scan.is_roc &&
+		    ar->scan.roc_freq == freq) {
+			complete(&ar->scan.on_channel);
+		}
 		break;
 	case WMI_SCAN_EVENT_DEQUEUED:
 		ath10k_dbg(ATH10K_DBG_WMI, "SCAN_EVENT_DEQUEUED\n");
-- 
1.7.9.5

[ath9k-devel] [PATCH 5/5] ath10k: abort scan if start completion never came

[Michal Kazior]

Abort the scan just in case FW has hung
temporarily.

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

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 72ac899..23af4c5 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -1475,8 +1475,10 @@ static int ath10k_start_scan(struct ath10k *ar,
 	ath10k_wmi_flush_tx(ar);
 
 	ret = wait_for_completion_timeout(&ar->scan.started, 1*HZ);
-	if (ret == 0)
+	if (ret == 0) {
+		ath10k_abort_scan(ar);
 		return ret;
+	}
 
 	/* the scan can complete earlier, before we even
 	 * start the timer. in that case the timer handler
-- 
1.7.9.5

[ath9k-devel] [PATCH 3/5] ath10k: increase scan timeout

[Kalle Valo]

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

> We can't use the max scan time as the timeout
> itself. We must account for various overheads (ie.
> command/event processing).
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---
>  drivers/net/wireless/ath/ath10k/mac.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 3a27f35..df57772 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -1481,8 +1481,10 @@ static int ath10k_start_scan(struct ath10k *ar,
>  	/* the scan can complete earlier, before we even
>  	 * start the timer. in that case the timer handler
>  	 * checks ar->scan.in_progress and bails out if its
> -	 * false. */
> -	mod_timer(&ar->scan.timeout, jiffies + (arg->max_scan_time*HZ)/1000);
> +	 * false. Add a 200ms margin to account event/command
> +	 * processing. */
> +	mod_timer(&ar->scan.timeout, jiffies +
> +		  ((arg->max_scan_time+200)*HZ)/1000);

This patch is ok (no need to resend), but this code could use
msecs_to_jiffies().

-- 
Kalle Valo

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

[Kalle Valo]

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

> Michal Kazior (5):
>   ath10k: use data_lock to protect {scan,rx}_channel
>   ath10k: simplify scan event locking
>   ath10k: increase scan timeout
>   ath10k: wait for remain on channel chan switch
>   ath10k: abort scan if start completion never came

Thanks, applied.

-- 
Kalle Valo