[PATCH v3 0/2] wifi: ath11k: fix data out of sync for channel list for reg update

[PATCH v3 0/2] wifi: ath11k: fix data out of sync for channel list for reg update

[Kang Yang]

Currently there are two threads to updating/fetch data of channel
list, and there are no synchronization for the data, it leads data out
of sync for channel list when doing reg update.

So change the call flow to make sure the fetch data running after the
update data finished, then data of channel list become synchronization.

Note: This patch-set is an old patch-set in public review written by
Wen Gong. Just continue sending it for him.
Link: https://lore.kernel.org/linux-wireless/20230329091235.19500-1-quic_wgong@quicinc.com/

v3: 
    1. rebase on tag ath-202411251703.
    2. add KASAN BUG report in patch #1.
v2:
    1. rewrite commit message for patch #1 and #2.
    2. use a local list without the spinlock held for patch #2.

Wen Gong (2):
  wifi: ath11k: move update channel list from update reg worker to reg
    notifier
  wifi: ath11k: move update channel list to worker for wait flag

 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  | 110 +++++++++++++++++--------
 drivers/net/wireless/ath/ath11k/reg.h  |   1 +
 drivers/net/wireless/ath/ath11k/wmi.h  |   1 +
 6 files changed, 98 insertions(+), 34 deletions(-)


base-commit: 175616a7658cd5d53389d1f9c1ce22debd4595a5
-- 
2.34.1

[PATCH v3 1/2] wifi: ath11k: move update channel list from update reg worker to reg notifier

[Kang Yang]

From: Wen Gong <quic_wgong@quicinc.com>

Currently ath11k call regulatory_set_wiphy_regd() in ath11k_regd_update()
to notify the reg domain change to cfg80211 and update channel list by
reg_work, then ath11k immediately update channel list to firmware by
ath11k_reg_update_chan_list().

callstack:
ath11k_regd_update
->regulatory_set_wiphy_regd
           -> schedule_work(&reg_work)
-> ath11k_reg_update_chan_list

They are running in two threads, it leads the channel list data out of
sync caused by muti-threads without synchronization. At this time,
ath11k may update wrong channel list to firmware because the reg_work
still running or even hasn't started yet. In this case, if the
ath11k_reg_update_chan_list accesses an improperly updated channel list
before reg_work is completed, it may result in out of bounds write
errors, as shown in the KASAN report:

BUG: KASAN: slab-out-of-bounds in ath11k_reg_update_chan_list
Call Trace:
    ath11k_reg_update_chan_list+0xbfe/0xfe0 [ath11k]
    kfree+0x109/0x3a0
    ath11k_regd_update+0x1cf/0x350 [ath11k]
    ath11k_regd_update_work+0x14/0x20 [ath11k]
    process_one_work+0xe35/0x14c0

The correct flow is after reg_work update the channel list according to
new reg domain, ath11k call ath11k_reg_update_chan_list() and update the
new channel list to firmware.

reg_call_notifier()(finally it will call ath11k_reg_notifier()) will be
called to by reg_work to notify ath11k when it finishes the channel
list update. So at this time, call ath11k_reg_update_chan_list() in
reg_call_notifier() with initiator type NL80211_REGDOM_SET_BY_DRIVER.
Then ath11k_reg_update_chan_list() will use the correct channel list.

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

Fixes: f45cb6b29cd3 ("wifi: ath11k: avoid deadlock during regulatory update in ath11k_regd_update()")
Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/reg.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/reg.c b/drivers/net/wireless/ath/ath11k/reg.c
index b0f289784dd3..cb2cf9b63d18 100644
--- a/drivers/net/wireless/ath/ath11k/reg.c
+++ b/drivers/net/wireless/ath/ath11k/reg.c
@@ -55,6 +55,19 @@ ath11k_reg_notifier(struct wiphy *wiphy, struct regulatory_request *request)
 	ath11k_dbg(ar->ab, ATH11K_DBG_REG,
 		   "Regulatory Notification received for %s\n", wiphy_name(wiphy));
 
+	if (request->initiator == NL80211_REGDOM_SET_BY_DRIVER) {
+		ath11k_dbg(ar->ab, ATH11K_DBG_REG,
+			   "driver initiated regd update\n");
+		if (ar->state != ATH11K_STATE_ON)
+			return;
+
+		ret = ath11k_reg_update_chan_list(ar, true);
+		if (ret)
+			ath11k_warn(ar->ab, "failed to update channel list: %d\n", ret);
+
+		return;
+	}
+
 	/* Currently supporting only General User Hints. Cell base user
 	 * hints to be handled later.
 	 * Hints from other sources like Core, Beacons are not expected for
@@ -293,12 +306,6 @@ int ath11k_regd_update(struct ath11k *ar)
 	if (ret)
 		goto err;
 
-	if (ar->state == ATH11K_STATE_ON) {
-		ret = ath11k_reg_update_chan_list(ar, true);
-		if (ret)
-			goto err;
-	}
-
 	return 0;
 err:
 	ath11k_warn(ab, "failed to perform regd update : %d\n", ret);
@@ -977,6 +984,7 @@ void ath11k_regd_update_work(struct work_struct *work)
 void ath11k_reg_init(struct ath11k *ar)
 {
 	ar->hw->wiphy->regulatory_flags = REGULATORY_WIPHY_SELF_MANAGED;
+	ar->hw->wiphy->flags |= WIPHY_FLAG_NOTIFY_REGDOM_BY_DRIVER;
 	ar->hw->wiphy->reg_notifier = ath11k_reg_notifier;
 }
 
-- 
2.34.1

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

[Kang Yang]

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

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[];
-- 
2.34.1

Re: [PATCH v3 1/2] wifi: ath11k: move update channel list from update reg worker to reg notifier

[Kalle Valo]

Kang Yang <quic_kangyang@quicinc.com> writes:

> From: Wen Gong <quic_wgong@quicinc.com>
>
> Currently ath11k call regulatory_set_wiphy_regd() in ath11k_regd_update()
> to notify the reg domain change to cfg80211 and update channel list by
> reg_work, then ath11k immediately update channel list to firmware by
> ath11k_reg_update_chan_list().
>
> callstack:
> ath11k_regd_update
> ->regulatory_set_wiphy_regd
>            -> schedule_work(&reg_work)
> -> ath11k_reg_update_chan_list
>
> They are running in two threads, it leads the channel list data out of
> sync caused by muti-threads without synchronization. At this time,
> ath11k may update wrong channel list to firmware because the reg_work
> still running or even hasn't started yet. In this case, if the
> ath11k_reg_update_chan_list accesses an improperly updated channel list
> before reg_work is completed, it may result in out of bounds write
> errors, as shown in the KASAN report:
>
> BUG: KASAN: slab-out-of-bounds in ath11k_reg_update_chan_list
> Call Trace:
>     ath11k_reg_update_chan_list+0xbfe/0xfe0 [ath11k]
>     kfree+0x109/0x3a0
>     ath11k_regd_update+0x1cf/0x350 [ath11k]
>     ath11k_regd_update_work+0x14/0x20 [ath11k]
>     process_one_work+0xe35/0x14c0
>
> The correct flow is after reg_work update the channel list according to
> new reg domain, ath11k call ath11k_reg_update_chan_list() and update the
> new channel list to firmware.
>
> reg_call_notifier()(finally it will call ath11k_reg_notifier()) will be
> called to by reg_work to notify ath11k when it finishes the channel
> list update. So at this time, call ath11k_reg_update_chan_list() in
> reg_call_notifier() with initiator type NL80211_REGDOM_SET_BY_DRIVER.
> Then ath11k_reg_update_chan_list() will use the correct channel list.
>
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3
>
> Fixes: f45cb6b29cd3 ("wifi: ath11k: avoid deadlock during regulatory update in ath11k_regd_update()")
> Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
> Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>

The commit message would need significant work to make it more
understandable, I feel that it's just explaining call flows. But clearly
describing the problem and the design how it's solved would be a lot
more helpful.

Jeff had good guidance how to write a good commit message but I don't
have a link at hand right now.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Kang Yang]

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