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

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

[Wen Gong]

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

Fixes: f45cb6b29cd3 ("wifi: ath11k: avoid deadlock during regulatory update in ath11k_regd_update()")

Wen Gong (2):
  wifi: ath11k: move update channel list from update reg worker to reg
    notifier
  wifi: ath11k: move udpate 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  |  13 ++++
 drivers/net/wireless/ath/ath11k/reg.c  | 100 ++++++++++++++++---------
 drivers/net/wireless/ath/ath11k/reg.h  |   1 +
 drivers/net/wireless/ath/ath11k/wmi.h  |   1 +
 6 files changed, 86 insertions(+), 34 deletions(-)


base-commit: 5aa9ff08c0b71be15a607341fa5344546e19ceb2
-- 
2.31.1

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

[Wen Gong]

Currently ath11k call regulatory_set_wiphy_regd() in ath11k_regd_update()
to notify the reg domain change to cfg80211, and then ath11k update the
channel list to firmware by ath11k_reg_update_chan_list() immediately in
ath11k_regd_update(), they are running in two threads, it leads the channel
list data out of sync caused by muti-threads without synchronization.
cfg80211 calculate the flags of ieee80211_channel asynchronously in its
reg_work, it is calculated by the callstack below and ath11k get the
flags of ieee80211_channel in another thread without synchronization
for the channel list data.

callstack:
handle_channel_custom()
handle_band_custom()
reg_process_self_managed_hint()
reg_process_self_managed_hints()
reg_todo()

ath11k_reg_update_chan_list() need to use the flags calculated by the
reg_work, they are two thread and have no synchronization for the flags,
then ath11k_reg_update_chan_list() maybe get the wrong flags value before
the calculation finished.

The correct flow is ath11k_reg_update_chan_list() should be called after
the flags finished calculation by reg_work. reg_call_notifier() with
initiator type NL80211_REGDOM_SET_BY_DRIVER will be called by
reg_process_self_managed_hint() after the calculation finished.

So change to update channel list in ath11k_reg_notifier() for initiator
type NL80211_REGDOM_SET_BY_DRIVER, then the calculation of flags of
ieee80211_channel has already finished and ath11k_reg_update_chan_list()
will use the correct value of flags.

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>
---
 drivers/net/wireless/ath/ath11k/reg.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/reg.c b/drivers/net/wireless/ath/ath11k/reg.c
index 6fae4e61ede7..f50740219202 100644
--- a/drivers/net/wireless/ath/ath11k/reg.c
+++ b/drivers/net/wireless/ath/ath11k/reg.c
@@ -55,6 +55,17 @@ 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) &&
+	    (ar->state == ATH11K_STATE_ON)) {
+		ath11k_dbg(ar->ab, ATH11K_DBG_REG,
+			   "dynamically updated by driver\n");
+		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
@@ -294,12 +305,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);
@@ -743,6 +748,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.31.1

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

[Wen Gong]

When wait flag is set for ath11k_reg_update_chan_list(), it maybe wait
the completion of 11d/hw scan if 11d/hw scan are running, and now after
the previous patch "wifi: ath11k: move update channel list from update
reg worker to reg notifier", ath11k_reg_update_chan_list() is called by
ath11k_reg_notifier() which is running in the reg_work of cfg80211, the
reg_work is running with rtnl_lock() which is a global lock, if the wait
of completion of 11d/hw scan happened in ath11k_reg_update_chan_list(),
it will increase the time of occupy the rtnl_lock by reg_work, and then
increase the wait time of the rtnl_lock for other threads.

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

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>
---
 drivers/net/wireless/ath/ath11k/core.c |  1 +
 drivers/net/wireless/ath/ath11k/core.h |  4 ++
 drivers/net/wireless/ath/ath11k/mac.c  | 13 ++++
 drivers/net/wireless/ath/ath11k/reg.c  | 82 +++++++++++++++++---------
 drivers/net/wireless/ath/ath11k/reg.h  |  1 +
 drivers/net/wireless/ath/ath11k/wmi.h  |  1 +
 6 files changed, 74 insertions(+), 28 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c
index 75fdbe4ef83a..650e6744923c 100644
--- a/drivers/net/wireless/ath/ath11k/core.c
+++ b/drivers/net/wireless/ath/ath11k/core.c
@@ -1702,6 +1702,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 0830276e5028..97712814e4cf 100644
--- a/drivers/net/wireless/ath/ath11k/core.h
+++ b/drivers/net/wireless/ath/ath11k/core.h
@@ -694,6 +694,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 110a38cce0a7..1a0c597e03bc 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -5962,6 +5962,7 @@ static void ath11k_mac_op_stop(struct ieee80211_hw *hw)
 {
 	struct ath11k *ar = hw->priv;
 	struct htt_ppdu_stats_info *ppdu_stats, *tmp;
+	struct scan_chan_list_params *params, *tmp_ch;
 	int ret;
 
 	ath11k_mac_drain_tx(ar);
@@ -5977,6 +5978,7 @@ static void ath11k_mac_op_stop(struct ieee80211_hw *hw)
 	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);
 
@@ -5992,6 +5994,13 @@ static void ath11k_mac_op_stop(struct ieee80211_hw *hw)
 	}
 	spin_unlock_bh(&ar->data_lock);
 
+	spin_lock_bh(&ar->channel_update_lock);
+	list_for_each_entry_safe(params, tmp_ch, &ar->channel_update_queue, 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();
@@ -8924,6 +8933,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);
@@ -9315,6 +9325,9 @@ int ath11k_mac_allocate(struct ath11k_base *ab)
 
 		INIT_DELAYED_WORK(&ar->scan.timeout, ath11k_scan_timeout_work);
 		INIT_WORK(&ar->regd_update_work, ath11k_regd_update_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->wmi_mgmt_tx_work, ath11k_mgmt_over_wmi_tx_work);
 		skb_queue_head_init(&ar->wmi_mgmt_tx_queue);
diff --git a/drivers/net/wireless/ath/ath11k/reg.c b/drivers/net/wireless/ath/ath11k/reg.c
index f50740219202..79d074455196 100644
--- a/drivers/net/wireless/ath/ath11k/reg.c
+++ b/drivers/net/wireless/ath/ath11k/reg.c
@@ -123,32 +123,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,
-			   "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,
-			   "reg hw scan wait left time %d\n", left);
-	}
+	int i, ret = 0;
 
 	if (ar->state == ATH11K_STATE_RESTARTING)
 		return 0;
@@ -230,8 +205,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;
 }
@@ -728,6 +710,50 @@ 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, *tmp;
+	int left;
+
+	spin_lock_bh(&ar->channel_update_lock);
+
+	list_for_each_entry_safe(params, tmp, &ar->channel_update_queue, list) {
+		list_del(&params->list);
+		spin_unlock_bh(&ar->channel_update_lock);
+
+		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);
+		kfree(params);
+		spin_lock_bh(&ar->channel_update_lock);
+	}
+	spin_unlock_bh(&ar->channel_update_lock);
+}
+
 void ath11k_regd_update_work(struct work_struct *work)
 {
 	struct ath11k *ar = container_of(work, struct ath11k,
diff --git a/drivers/net/wireless/ath/ath11k/reg.h b/drivers/net/wireless/ath/ath11k/reg.h
index 2f284f26378d..13751fab06c4 100644
--- a/drivers/net/wireless/ath/ath11k/reg.h
+++ b/drivers/net/wireless/ath/ath11k/reg.h
@@ -28,6 +28,7 @@ enum ath11k_dfs_region {
 void ath11k_reg_init(struct ath11k *ar);
 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 2273d885f7fa..be324be080be 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.h
+++ b/drivers/net/wireless/ath/ath11k/wmi.h
@@ -3685,6 +3685,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.31.1

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

[Wen Gong]

On 3/29/2023 5:12 PM, Wen Gong wrote:
> 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 for 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.
>
> Fixes: f45cb6b29cd3 ("wifi: ath11k: avoid deadlock during regulatory update in ath11k_regd_update()")
>
> Wen Gong (2):
>    wifi: ath11k: move update channel list from update reg worker to reg
>      notifier
>    wifi: ath11k: move udpate 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  |  13 ++++
>   drivers/net/wireless/ath/ath11k/reg.c  | 100 ++++++++++++++++---------
>   drivers/net/wireless/ath/ath11k/reg.h  |   1 +
>   drivers/net/wireless/ath/ath11k/wmi.h  |   1 +
>   6 files changed, 86 insertions(+), 34 deletions(-)
>
>
> base-commit: 5aa9ff08c0b71be15a607341fa5344546e19ceb2

Hi Kalle,

Will you plan to merge it or will you give comments?

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

[Wen Gong]

Hi Kalle,

Will you plan to merge it or will you give comments since it has 
deferred/pending here for 4 month ? 😁

It is to fix firmware crash and reg domain used wrong in firmware/hardware.

On 7/10/2023 11:03 AM, Wen Gong wrote:
> On 3/29/2023 5:12 PM, Wen Gong wrote:
>> 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 for 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.
>>
>> Fixes: f45cb6b29cd3 ("wifi: ath11k: avoid deadlock during regulatory 
>> update in ath11k_regd_update()")
>>
>> Wen Gong (2):
>>    wifi: ath11k: move update channel list from update reg worker to reg
>>      notifier
>>    wifi: ath11k: move udpate 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  |  13 ++++
>>   drivers/net/wireless/ath/ath11k/reg.c  | 100 ++++++++++++++++---------
>>   drivers/net/wireless/ath/ath11k/reg.h  |   1 +
>>   drivers/net/wireless/ath/ath11k/wmi.h  |   1 +
>>   6 files changed, 86 insertions(+), 34 deletions(-)
>>
>>
>> base-commit: 5aa9ff08c0b71be15a607341fa5344546e19ceb2
>
> Hi Kalle,
>
> Will you plan to merge it or will you give comments?
>

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

[Wen Gong]

Hi Kalle,

May I change the subject to "wifi: ath11k: fix firmware crash and wrong 
reg domain used in firmware and hardware" and sent new version again?

Or will you merge this version?

On 8/1/2023 1:32 PM, Wen Gong wrote:
> Hi Kalle,
>
> Will you plan to merge it or will you give comments since it has 
> deferred/pending here for 4 month ? 😁
>
> It is to fix firmware crash and reg domain used wrong in 
> firmware/hardware.
>
> On 7/10/2023 11:03 AM, Wen Gong wrote:
>> On 3/29/2023 5:12 PM, Wen Gong wrote:
>>> 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 for 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.
>>>
>>> Fixes: f45cb6b29cd3 ("wifi: ath11k: avoid deadlock during regulatory 
>>> update in ath11k_regd_update()")
>>>
>>> Wen Gong (2):
>>>    wifi: ath11k: move update channel list from update reg worker to reg
>>>      notifier
>>>    wifi: ath11k: move udpate 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  |  13 ++++
>>>   drivers/net/wireless/ath/ath11k/reg.c  | 100 
>>> ++++++++++++++++---------
>>>   drivers/net/wireless/ath/ath11k/reg.h  |   1 +
>>>   drivers/net/wireless/ath/ath11k/wmi.h  |   1 +
>>>   6 files changed, 86 insertions(+), 34 deletions(-)
>>>
>>>
>>> base-commit: 5aa9ff08c0b71be15a607341fa5344546e19ceb2
>>
>> Hi Kalle,
>>
>> Will you plan to merge it or will you give comments?
>>