[PATCH] wifi: ath11k: use work queue to process beacon tx event

[PATCH] wifi: ath11k: use work queue to process beacon tx event

[Kang Yang]

Commit 3a415daa3e8b ("wifi: ath11k: add P2P IE in beacon template")
from Feb 28, 2024 (linux-next), leads to the following Smatch static
checker warning:

drivers/net/wireless/ath/ath11k/wmi.c:1742 ath11k_wmi_p2p_go_bcn_ie()
warn: sleeping in atomic context

The reason is that ath11k_bcn_tx_status_event() will directly call might
sleep function ath11k_wmi_cmd_send() during RCU read-side critical
sections. The call trace is like:

ath11k_bcn_tx_status_event()
-> rcu_read_lock()
-> ath11k_mac_bcn_tx_event()
	-> ath11k_mac_setup_bcn_tmpl()
	……
		-> ath11k_wmi_bcn_tmpl()
			-> ath11k_wmi_cmd_send()
-> rcu_read_unlock()

Commit 886433a98425 ("ath11k: add support for BSS color change") added the
ath11k_mac_bcn_tx_event(), commit 01e782c89108 ("ath11k: fix warning
of RCU usage for ath11k_mac_get_arvif_by_vdev_id()") added the RCU lock
to avoid warning but also introduced this BUG.

Use work queue to avoid directly calling ath11k_mac_bcn_tx_event()
during RCU critical sections. No need to worry the deletion of vif.
Because cancel_work_sync() will drop the work if it doesn't start or
block vif deletion until the running work is done.

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

Fixes: 3a415daa3e8b ("wifi: ath11k: add P2P IE in beacon template")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/2d277abd-5e7b-4da0-80e0-52bd96337f6e@moroto.mountain/
Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/core.h |  1 +
 drivers/net/wireless/ath/ath11k/mac.c  | 12 ++++++++++++
 drivers/net/wireless/ath/ath11k/wmi.c  |  4 +++-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
index df24f0e409af..7122176dd91e 100644
--- a/drivers/net/wireless/ath/ath11k/core.h
+++ b/drivers/net/wireless/ath/ath11k/core.h
@@ -399,6 +399,7 @@ struct ath11k_vif {
 	u8 bssid[ETH_ALEN];
 	struct cfg80211_bitrate_mask bitrate_mask;
 	struct delayed_work connection_loss_work;
+	struct work_struct bcn_tx_work;
 	int num_legacy_stations;
 	int rtscts_prot_mode;
 	int txpower;
diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index 779009774567..7ed606aaf1a6 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -6599,6 +6599,16 @@ static int ath11k_mac_vdev_delete(struct ath11k *ar, struct ath11k_vif *arvif)
 	return ret;
 }
 
+static void ath11k_mac_bcn_tx_work(struct work_struct *work)
+{
+	struct ath11k_vif *arvif = container_of(work, struct ath11k_vif,
+						bcn_tx_work);
+
+	mutex_lock(&arvif->ar->conf_mutex);
+	ath11k_mac_bcn_tx_event(arvif);
+	mutex_unlock(&arvif->ar->conf_mutex);
+}
+
 static int ath11k_mac_op_add_interface(struct ieee80211_hw *hw,
 				       struct ieee80211_vif *vif)
 {
@@ -6637,6 +6647,7 @@ static int ath11k_mac_op_add_interface(struct ieee80211_hw *hw,
 	arvif->vif = vif;
 
 	INIT_LIST_HEAD(&arvif->list);
+	INIT_WORK(&arvif->bcn_tx_work, ath11k_mac_bcn_tx_work);
 	INIT_DELAYED_WORK(&arvif->connection_loss_work,
 			  ath11k_mac_vif_sta_connection_loss_work);
 
@@ -6879,6 +6890,7 @@ static void ath11k_mac_op_remove_interface(struct ieee80211_hw *hw,
 	int i;
 
 	cancel_delayed_work_sync(&arvif->connection_loss_work);
+	cancel_work_sync(&arvif->bcn_tx_work);
 
 	mutex_lock(&ar->conf_mutex);
 
diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c
index 38f175dd1557..2662092ee00a 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.c
+++ b/drivers/net/wireless/ath/ath11k/wmi.c
@@ -7404,7 +7404,9 @@ static void ath11k_bcn_tx_status_event(struct ath11k_base *ab, struct sk_buff *s
 		rcu_read_unlock();
 		return;
 	}
-	ath11k_mac_bcn_tx_event(arvif);
+
+	queue_work(ab->workqueue, &arvif->bcn_tx_work);
+
 	rcu_read_unlock();
 }
 

base-commit: cac9bfd02678adbcca9a7dce770609b9f7434d37
-- 
2.34.1

Re: [PATCH] wifi: ath11k: use work queue to process beacon tx event

[Jeff Johnson]

On 6/25/2024 10:35 PM, Kang Yang wrote:
> Commit 3a415daa3e8b ("wifi: ath11k: add P2P IE in beacon template")
> from Feb 28, 2024 (linux-next), leads to the following Smatch static
> checker warning:
> 
> drivers/net/wireless/ath/ath11k/wmi.c:1742 ath11k_wmi_p2p_go_bcn_ie()
> warn: sleeping in atomic context
> 
> The reason is that ath11k_bcn_tx_status_event() will directly call might
> sleep function ath11k_wmi_cmd_send() during RCU read-side critical
> sections. The call trace is like:
> 
> ath11k_bcn_tx_status_event()
> -> rcu_read_lock()
> -> ath11k_mac_bcn_tx_event()
> 	-> ath11k_mac_setup_bcn_tmpl()
> 	……
> 		-> ath11k_wmi_bcn_tmpl()
> 			-> ath11k_wmi_cmd_send()
> -> rcu_read_unlock()
> 
> Commit 886433a98425 ("ath11k: add support for BSS color change") added the
> ath11k_mac_bcn_tx_event(), commit 01e782c89108 ("ath11k: fix warning
> of RCU usage for ath11k_mac_get_arvif_by_vdev_id()") added the RCU lock
> to avoid warning but also introduced this BUG.
> 
> Use work queue to avoid directly calling ath11k_mac_bcn_tx_event()
> during RCU critical sections. No need to worry the deletion of vif.

comment nit:
s/worry the deletion of vif. Because/worry about the deletion of vif because/

Kalle can apply this to pending

> Because cancel_work_sync() will drop the work if it doesn't start or
> block vif deletion until the running work is done.
> 
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
> 
> Fixes: 3a415daa3e8b ("wifi: ath11k: add P2P IE in beacon template")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/2d277abd-5e7b-4da0-80e0-52bd96337f6e@moroto.mountain/
> Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Re: [PATCH] wifi: ath11k: use work queue to process beacon tx event

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 6/25/2024 10:35 PM, Kang Yang wrote:
>
>> Commit 3a415daa3e8b ("wifi: ath11k: add P2P IE in beacon template")
>> from Feb 28, 2024 (linux-next), leads to the following Smatch static
>> checker warning:
>> 
>> drivers/net/wireless/ath/ath11k/wmi.c:1742 ath11k_wmi_p2p_go_bcn_ie()
>> warn: sleeping in atomic context
>> 
>> The reason is that ath11k_bcn_tx_status_event() will directly call might
>> sleep function ath11k_wmi_cmd_send() during RCU read-side critical
>> sections. The call trace is like:
>> 
>> ath11k_bcn_tx_status_event()
>> -> rcu_read_lock()
>> -> ath11k_mac_bcn_tx_event()
>> 	-> ath11k_mac_setup_bcn_tmpl()
>> 	……
>> 		-> ath11k_wmi_bcn_tmpl()
>> 			-> ath11k_wmi_cmd_send()
>> -> rcu_read_unlock()
>> 
>> Commit 886433a98425 ("ath11k: add support for BSS color change") added the
>> ath11k_mac_bcn_tx_event(), commit 01e782c89108 ("ath11k: fix warning
>> of RCU usage for ath11k_mac_get_arvif_by_vdev_id()") added the RCU lock
>> to avoid warning but also introduced this BUG.
>> 
>> Use work queue to avoid directly calling ath11k_mac_bcn_tx_event()
>> during RCU critical sections. No need to worry the deletion of vif.
>
> comment nit:
> s/worry the deletion of vif. Because/worry about the deletion of vif because/
>
> Kalle can apply this to pending

Yup, fixed.

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

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

Re: [PATCH] wifi: ath11k: use work queue to process beacon tx event

[Kalle Valo]

Kang Yang <quic_kangyang@quicinc.com> wrote:

> Commit 3a415daa3e8b ("wifi: ath11k: add P2P IE in beacon template")
> from Feb 28, 2024 (linux-next), leads to the following Smatch static
> checker warning:
> 
> drivers/net/wireless/ath/ath11k/wmi.c:1742 ath11k_wmi_p2p_go_bcn_ie()
> warn: sleeping in atomic context
> 
> The reason is that ath11k_bcn_tx_status_event() will directly call might
> sleep function ath11k_wmi_cmd_send() during RCU read-side critical
> sections. The call trace is like:
> 
> ath11k_bcn_tx_status_event()
> -> rcu_read_lock()
> -> ath11k_mac_bcn_tx_event()
>         -> ath11k_mac_setup_bcn_tmpl()
>         ……
>                 -> ath11k_wmi_bcn_tmpl()
>                         -> ath11k_wmi_cmd_send()
> -> rcu_read_unlock()
> 
> Commit 886433a98425 ("ath11k: add support for BSS color change") added the
> ath11k_mac_bcn_tx_event(), commit 01e782c89108 ("ath11k: fix warning
> of RCU usage for ath11k_mac_get_arvif_by_vdev_id()") added the RCU lock
> to avoid warning but also introduced this BUG.
> 
> Use work queue to avoid directly calling ath11k_mac_bcn_tx_event()
> during RCU critical sections. No need to worry about the deletion of vif
> because cancel_work_sync() will drop the work if it doesn't start or
> block vif deletion until the running work is done.
> 
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
> 
> Fixes: 3a415daa3e8b ("wifi: ath11k: add P2P IE in beacon template")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/2d277abd-5e7b-4da0-80e0-52bd96337f6e@moroto.mountain/
> Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

177b49dbf9c1 wifi: ath11k: use work queue to process beacon tx event

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240626053543.1946-1-quic_kangyang@quicinc.com/

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