diff mbox series

[v3,2/2] wifi: ath11k: move update channel list to worker for wait flag

Message ID 20241129070714.226-3-quic_kangyang@quicinc.com
State New
Headers show
Series wifi: ath11k: fix data out of sync for channel list for reg update | expand

Commit Message

Kang Yang Nov. 29, 2024, 7:07 a.m. UTC
From: Wen Gong <quic_wgong@quicinc.com>

When wait flag is set for ath11k_reg_update_chan_list(), it will wait
the completion of 11d/hw scan if 11d/hw scan is running.

With the previous patch "wifi: ath11k: move update channel list from
update reg worker to reg notifier", ath11k_reg_update_chan_list() will
be called when reg_work is running. The global lock rtnl_lock will be
held by reg_work in the meantime. If the wait_for_completion_timeout()
is called due to 11d/hw scan is running, the occupation time of
rtnl_lock will increase. This will increase blocking time for other
threads if they want to use rtnl_lock.

Move update channel list operation in ath11k_reg_update_chan_list() to
a new worker, then the wait of completion of 11d/hw scan will not
happen in reg_work and not increase the occupation time of the rtnl_lock.

Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3

Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
Co-developed-by: Kang Yang <quic_kangyang@quicinc.com>
Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/core.c |  1 +
 drivers/net/wireless/ath/ath11k/core.h |  4 ++
 drivers/net/wireless/ath/ath11k/mac.c  | 15 +++++
 drivers/net/wireless/ath/ath11k/reg.c  | 90 ++++++++++++++++++--------
 drivers/net/wireless/ath/ath11k/reg.h  |  1 +
 drivers/net/wireless/ath/ath11k/wmi.h  |  1 +
 6 files changed, 84 insertions(+), 28 deletions(-)

Comments

Kalle Valo Dec. 12, 2024, 2:07 p.m. UTC | #1
Kang Yang <quic_kangyang@quicinc.com> writes:

> From: Wen Gong <quic_wgong@quicinc.com>
>
> When wait flag is set for ath11k_reg_update_chan_list(), it will wait
> the completion of 11d/hw scan if 11d/hw scan is running.
>
> With the previous patch "wifi: ath11k: move update channel list from
> update reg worker to reg notifier", ath11k_reg_update_chan_list() will
> be called when reg_work is running. The global lock rtnl_lock will be
> held by reg_work in the meantime. If the wait_for_completion_timeout()
> is called due to 11d/hw scan is running, the occupation time of
> rtnl_lock will increase. This will increase blocking time for other
> threads if they want to use rtnl_lock.
>
> Move update channel list operation in ath11k_reg_update_chan_list() to
> a new worker, then the wait of completion of 11d/hw scan will not
> happen in reg_work and not increase the occupation time of the rtnl_lock.
>
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3
>
> Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
> Co-developed-by: Kang Yang <quic_kangyang@quicinc.com>
> Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>

Same here, I think the commit message should be more or less rewritten.

> --- a/drivers/net/wireless/ath/ath11k/core.h
> +++ b/drivers/net/wireless/ath/ath11k/core.h
> @@ -743,6 +743,10 @@ struct ath11k {
>  	struct completion bss_survey_done;
>  
>  	struct work_struct regd_update_work;
> +	struct work_struct channel_update_work;
> +	struct list_head channel_update_queue;
> +	/* protects channel_update_queue data */
> +	spinlock_t channel_update_lock;

Do you really need a new lock? Why not use data_lock?

> @@ -6318,6 +6320,15 @@ static void ath11k_mac_op_stop(struct ieee80211_hw *hw, bool suspend)
>  	}
>  	spin_unlock_bh(&ar->data_lock);
>  
> +	spin_lock_bh(&ar->channel_update_lock);

Empty line here, please.

> +	while ((params = list_first_entry_or_null(&ar->channel_update_queue,
> +						  struct scan_chan_list_params,
> +						  list))) {
> +		list_del(&params->list);
> +		kfree(params);
> +	}

Here also empty line.

> +	spin_unlock_bh(&ar->channel_update_lock);
> +
>  	rcu_assign_pointer(ar->ab->pdevs_active[ar->pdev_idx], NULL);
>  
>  	synchronize_rcu();

[...]

> +void ath11k_regd_update_chan_list_work(struct work_struct *work)
> +{
> +	struct ath11k *ar = container_of(work, struct ath11k,
> +					 channel_update_work);
> +	struct scan_chan_list_params *params;
> +	struct list_head local_update_list;
> +	int left;
> +
> +	INIT_LIST_HEAD(&local_update_list);
> +
> +	spin_lock_bh(&ar->channel_update_lock);
> +	while ((params = list_first_entry_or_null(&ar->channel_update_queue,
> +						  struct scan_chan_list_params,
> +						  list))) {
> +		list_del(&params->list);
> +		list_add_tail(&params->list, &local_update_list);
> +	}
> +	spin_unlock_bh(&ar->channel_update_lock);

What about list_splice_tail_init() or similar?

> +
> +	while ((params = list_first_entry_or_null(&local_update_list,
> +						  struct scan_chan_list_params,
> +						  list))) {
> +		if (ar->state_11d != ATH11K_11D_IDLE) {
> +			left = wait_for_completion_timeout(&ar->completed_11d_scan,
> +							   ATH11K_SCAN_TIMEOUT_HZ);
> +			if (!left) {
> +				ath11k_dbg(ar->ab, ATH11K_DBG_REG,
> +					   "failed to receive 11d scan complete: timed out\n");
> +				ar->state_11d = ATH11K_11D_IDLE;
> +			}

Empty line here.
Kang Yang Dec. 13, 2024, 6:46 a.m. UTC | #2
On 12/12/2024 10:07 PM, Kalle Valo wrote:
> Kang Yang <quic_kangyang@quicinc.com> writes:
> 
>> From: Wen Gong <quic_wgong@quicinc.com>
>>
>> When wait flag is set for ath11k_reg_update_chan_list(), it will wait
>> the completion of 11d/hw scan if 11d/hw scan is running.
>>
>> With the previous patch "wifi: ath11k: move update channel list from
>> update reg worker to reg notifier", ath11k_reg_update_chan_list() will
>> be called when reg_work is running. The global lock rtnl_lock will be
>> held by reg_work in the meantime. If the wait_for_completion_timeout()
>> is called due to 11d/hw scan is running, the occupation time of
>> rtnl_lock will increase. This will increase blocking time for other
>> threads if they want to use rtnl_lock.
>>
>> Move update channel list operation in ath11k_reg_update_chan_list() to
>> a new worker, then the wait of completion of 11d/hw scan will not
>> happen in reg_work and not increase the occupation time of the rtnl_lock.
>>
>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3
>>
>> Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
>> Co-developed-by: Kang Yang <quic_kangyang@quicinc.com>
>> Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
> 
> Same here, I think the commit message should be more or less rewritten.
> 
>> --- a/drivers/net/wireless/ath/ath11k/core.h
>> +++ b/drivers/net/wireless/ath/ath11k/core.h
>> @@ -743,6 +743,10 @@ struct ath11k {
>>   	struct completion bss_survey_done;
>>   
>>   	struct work_struct regd_update_work;
>> +	struct work_struct channel_update_work;
>> +	struct list_head channel_update_queue;
>> +	/* protects channel_update_queue data */
>> +	spinlock_t channel_update_lock;
> 
> Do you really need a new lock? Why not use data_lock?

Seems data_lock is OK, will change in next version.

> 
>> @@ -6318,6 +6320,15 @@ static void ath11k_mac_op_stop(struct ieee80211_hw *hw, bool suspend)
>>   	}
>>   	spin_unlock_bh(&ar->data_lock);
>>   
>> +	spin_lock_bh(&ar->channel_update_lock);
> 
> Empty line here, please.
> 
>> +	while ((params = list_first_entry_or_null(&ar->channel_update_queue,
>> +						  struct scan_chan_list_params,
>> +						  list))) {
>> +		list_del(&params->list);
>> +		kfree(params);
>> +	}
> 
> Here also empty line.
> 
>> +	spin_unlock_bh(&ar->channel_update_lock);
>> +
>>   	rcu_assign_pointer(ar->ab->pdevs_active[ar->pdev_idx], NULL);
>>   
>>   	synchronize_rcu();
> 
> [...]
> 
>> +void ath11k_regd_update_chan_list_work(struct work_struct *work)
>> +{
>> +	struct ath11k *ar = container_of(work, struct ath11k,
>> +					 channel_update_work);
>> +	struct scan_chan_list_params *params;
>> +	struct list_head local_update_list;
>> +	int left;
>> +
>> +	INIT_LIST_HEAD(&local_update_list);
>> +
>> +	spin_lock_bh(&ar->channel_update_lock);
>> +	while ((params = list_first_entry_or_null(&ar->channel_update_queue,
>> +						  struct scan_chan_list_params,
>> +						  list))) {
>> +		list_del(&params->list);
>> +		list_add_tail(&params->list, &local_update_list);
>> +	}
>> +	spin_unlock_bh(&ar->channel_update_lock);
> 
> What about list_splice_tail_init() or similar?

Seems list_splice_tail_init() is better. The time complexity is O(1).👍

> 
>> +
>> +	while ((params = list_first_entry_or_null(&local_update_list,
>> +						  struct scan_chan_list_params,
>> +						  list))) {
>> +		if (ar->state_11d != ATH11K_11D_IDLE) {
>> +			left = wait_for_completion_timeout(&ar->completed_11d_scan,
>> +							   ATH11K_SCAN_TIMEOUT_HZ);
>> +			if (!left) {
>> +				ath11k_dbg(ar->ab, ATH11K_DBG_REG,
>> +					   "failed to receive 11d scan complete: timed out\n");
>> +				ar->state_11d = ATH11K_11D_IDLE;
>> +			}
> 
> Empty line here.
>
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c
index c576bbba52bf..e4db3cdecf29 100644
--- a/drivers/net/wireless/ath/ath11k/core.c
+++ b/drivers/net/wireless/ath/ath11k/core.c
@@ -2056,6 +2056,7 @@  void ath11k_core_halt(struct ath11k *ar)
 	ath11k_mac_scan_finish(ar);
 	ath11k_mac_peer_cleanup_all(ar);
 	cancel_delayed_work_sync(&ar->scan.timeout);
+	cancel_work_sync(&ar->channel_update_work);
 	cancel_work_sync(&ar->regd_update_work);
 	cancel_work_sync(&ab->update_11d_work);
 
diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
index a9dc7fe7765a..b4bcfe0b98d2 100644
--- a/drivers/net/wireless/ath/ath11k/core.h
+++ b/drivers/net/wireless/ath/ath11k/core.h
@@ -743,6 +743,10 @@  struct ath11k {
 	struct completion bss_survey_done;
 
 	struct work_struct regd_update_work;
+	struct work_struct channel_update_work;
+	struct list_head channel_update_queue;
+	/* protects channel_update_queue data */
+	spinlock_t channel_update_lock;
 
 	struct work_struct wmi_mgmt_tx_work;
 	struct sk_buff_head wmi_mgmt_tx_queue;
diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index 31ae9b384a29..958997b51e41 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -6288,6 +6288,7 @@  static void ath11k_mac_op_stop(struct ieee80211_hw *hw, bool suspend)
 {
 	struct ath11k *ar = hw->priv;
 	struct htt_ppdu_stats_info *ppdu_stats, *tmp;
+	struct scan_chan_list_params *params;
 	int ret;
 
 	ath11k_mac_drain_tx(ar);
@@ -6303,6 +6304,7 @@  static void ath11k_mac_op_stop(struct ieee80211_hw *hw, bool suspend)
 	mutex_unlock(&ar->conf_mutex);
 
 	cancel_delayed_work_sync(&ar->scan.timeout);
+	cancel_work_sync(&ar->channel_update_work);
 	cancel_work_sync(&ar->regd_update_work);
 	cancel_work_sync(&ar->ab->update_11d_work);
 
@@ -6318,6 +6320,15 @@  static void ath11k_mac_op_stop(struct ieee80211_hw *hw, bool suspend)
 	}
 	spin_unlock_bh(&ar->data_lock);
 
+	spin_lock_bh(&ar->channel_update_lock);
+	while ((params = list_first_entry_or_null(&ar->channel_update_queue,
+						  struct scan_chan_list_params,
+						  list))) {
+		list_del(&params->list);
+		kfree(params);
+	}
+	spin_unlock_bh(&ar->channel_update_lock);
+
 	rcu_assign_pointer(ar->ab->pdevs_active[ar->pdev_idx], NULL);
 
 	synchronize_rcu();
@@ -10018,6 +10029,7 @@  static const struct wiphy_iftype_ext_capab ath11k_iftypes_ext_capa[] = {
 
 static void __ath11k_mac_unregister(struct ath11k *ar)
 {
+	cancel_work_sync(&ar->channel_update_work);
 	cancel_work_sync(&ar->regd_update_work);
 
 	ieee80211_unregister_hw(ar->hw);
@@ -10417,6 +10429,9 @@  int ath11k_mac_allocate(struct ath11k_base *ab)
 		init_completion(&ar->thermal.wmi_sync);
 
 		INIT_DELAYED_WORK(&ar->scan.timeout, ath11k_scan_timeout_work);
+		INIT_WORK(&ar->channel_update_work, ath11k_regd_update_chan_list_work);
+		INIT_LIST_HEAD(&ar->channel_update_queue);
+		spin_lock_init(&ar->channel_update_lock);
 		INIT_WORK(&ar->regd_update_work, ath11k_regd_update_work);
 
 		INIT_WORK(&ar->wmi_mgmt_tx_work, ath11k_mgmt_over_wmi_tx_work);
diff --git a/drivers/net/wireless/ath/ath11k/reg.c b/drivers/net/wireless/ath/ath11k/reg.c
index cb2cf9b63d18..8df7503997db 100644
--- a/drivers/net/wireless/ath/ath11k/reg.c
+++ b/drivers/net/wireless/ath/ath11k/reg.c
@@ -124,32 +124,7 @@  int ath11k_reg_update_chan_list(struct ath11k *ar, bool wait)
 	struct channel_param *ch;
 	enum nl80211_band band;
 	int num_channels = 0;
-	int i, ret, left;
-
-	if (wait && ar->state_11d != ATH11K_11D_IDLE) {
-		left = wait_for_completion_timeout(&ar->completed_11d_scan,
-						   ATH11K_SCAN_TIMEOUT_HZ);
-		if (!left) {
-			ath11k_dbg(ar->ab, ATH11K_DBG_REG,
-				   "failed to receive 11d scan complete: timed out\n");
-			ar->state_11d = ATH11K_11D_IDLE;
-		}
-		ath11k_dbg(ar->ab, ATH11K_DBG_REG,
-			   "11d scan wait left time %d\n", left);
-	}
-
-	if (wait &&
-	    (ar->scan.state == ATH11K_SCAN_STARTING ||
-	    ar->scan.state == ATH11K_SCAN_RUNNING)) {
-		left = wait_for_completion_timeout(&ar->scan.completed,
-						   ATH11K_SCAN_TIMEOUT_HZ);
-		if (!left)
-			ath11k_dbg(ar->ab, ATH11K_DBG_REG,
-				   "failed to receive hw scan complete: timed out\n");
-
-		ath11k_dbg(ar->ab, ATH11K_DBG_REG,
-			   "hw scan wait left time %d\n", left);
-	}
+	int i, ret = 0;
 
 	if (ar->state == ATH11K_STATE_RESTARTING)
 		return 0;
@@ -231,8 +206,15 @@  int ath11k_reg_update_chan_list(struct ath11k *ar, bool wait)
 		}
 	}
 
-	ret = ath11k_wmi_send_scan_chan_list_cmd(ar, params);
-	kfree(params);
+	if (wait) {
+		spin_lock_bh(&ar->channel_update_lock);
+		list_add_tail(&params->list, &ar->channel_update_queue);
+		spin_unlock_bh(&ar->channel_update_lock);
+		queue_work(ar->ab->workqueue, &ar->channel_update_work);
+	} else {
+		ret = ath11k_wmi_send_scan_chan_list_cmd(ar, params);
+		kfree(params);
+	}
 
 	return ret;
 }
@@ -811,6 +793,58 @@  ath11k_reg_build_regd(struct ath11k_base *ab,
 	return new_regd;
 }
 
+void ath11k_regd_update_chan_list_work(struct work_struct *work)
+{
+	struct ath11k *ar = container_of(work, struct ath11k,
+					 channel_update_work);
+	struct scan_chan_list_params *params;
+	struct list_head local_update_list;
+	int left;
+
+	INIT_LIST_HEAD(&local_update_list);
+
+	spin_lock_bh(&ar->channel_update_lock);
+	while ((params = list_first_entry_or_null(&ar->channel_update_queue,
+						  struct scan_chan_list_params,
+						  list))) {
+		list_del(&params->list);
+		list_add_tail(&params->list, &local_update_list);
+	}
+	spin_unlock_bh(&ar->channel_update_lock);
+
+	while ((params = list_first_entry_or_null(&local_update_list,
+						  struct scan_chan_list_params,
+						  list))) {
+		if (ar->state_11d != ATH11K_11D_IDLE) {
+			left = wait_for_completion_timeout(&ar->completed_11d_scan,
+							   ATH11K_SCAN_TIMEOUT_HZ);
+			if (!left) {
+				ath11k_dbg(ar->ab, ATH11K_DBG_REG,
+					   "failed to receive 11d scan complete: timed out\n");
+				ar->state_11d = ATH11K_11D_IDLE;
+			}
+			ath11k_dbg(ar->ab, ATH11K_DBG_REG,
+				   "reg 11d scan wait left time %d\n", left);
+		}
+
+		if ((ar->scan.state == ATH11K_SCAN_STARTING ||
+		     ar->scan.state == ATH11K_SCAN_RUNNING)) {
+			left = wait_for_completion_timeout(&ar->scan.completed,
+							   ATH11K_SCAN_TIMEOUT_HZ);
+			if (!left)
+				ath11k_dbg(ar->ab, ATH11K_DBG_REG,
+					   "failed to receive hw scan complete: timed out\n");
+
+			ath11k_dbg(ar->ab, ATH11K_DBG_REG,
+				   "reg hw scan wait left time %d\n", left);
+		}
+
+		ath11k_wmi_send_scan_chan_list_cmd(ar, params);
+		list_del(&params->list);
+		kfree(params);
+	}
+}
+
 static bool ath11k_reg_is_world_alpha(char *alpha)
 {
 	if (alpha[0] == '0' && alpha[1] == '0')
diff --git a/drivers/net/wireless/ath/ath11k/reg.h b/drivers/net/wireless/ath/ath11k/reg.h
index 263ea9061948..0d5e10bb5730 100644
--- a/drivers/net/wireless/ath/ath11k/reg.h
+++ b/drivers/net/wireless/ath/ath11k/reg.h
@@ -33,6 +33,7 @@  void ath11k_reg_init(struct ath11k *ar);
 void ath11k_reg_reset_info(struct cur_regulatory_info *reg_info);
 void ath11k_reg_free(struct ath11k_base *ab);
 void ath11k_regd_update_work(struct work_struct *work);
+void ath11k_regd_update_chan_list_work(struct work_struct *work);
 struct ieee80211_regdomain *
 ath11k_reg_build_regd(struct ath11k_base *ab,
 		      struct cur_regulatory_info *reg_info, bool intersect,
diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h
index 8982b909c821..30b4b0c17682 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.h
+++ b/drivers/net/wireless/ath/ath11k/wmi.h
@@ -3817,6 +3817,7 @@  struct wmi_stop_scan_cmd {
 };
 
 struct scan_chan_list_params {
+	struct list_head list;
 	u32 pdev_id;
 	u16 nallchans;
 	struct channel_param ch_param[];