[PATCH 00/10] wifi: ath12k: MLO support part 4

[PATCH 00/10] wifi: ath12k: MLO support part 4

[Kalle Valo]

From: Kalle Valo <quic_kvalo@quicinc.com>

Here mac.c refactoring continues but there's also few fixes included. Please review.

Aditya Kumar Singh (1):
  wifi: ath12k: ath12k_bss_assoc(): MLO support

Kalle Valo (3):
  wifi: ath12k: convert struct ath12k::wmi_mgmt_tx_work to struct
    wiphy_work
  wifi: ath12k: ath12k_mac_op_set_key(): fix uninitialized symbol 'ret'
  wifi: ath12k: ath12k_mac_op_sta_rc_update(): use mac80211 provided
    link id

Rameshkumar Sundaram (2):
  wifi: ath12k: ath12k_mac_station_add(): fix potential rx_stats leak
  wifi: ath12k: defer vdev creation for MLO

Sriram R (4):
  wifi: ath12k: ath12k_mac_op_tx(): MLO support
  wifi: ath12k: ath12k_mac_op_flush(): MLO support
  wifi: ath12k: ath12k_mac_op_ampdu_action(): MLO support
  wifi: ath12k: do not return invalid link id for scan link

 drivers/net/wireless/ath/ath12k/core.c  |   6 +
 drivers/net/wireless/ath/ath12k/core.h  |   6 +-
 drivers/net/wireless/ath/ath12k/dp_rx.c |  36 +-
 drivers/net/wireless/ath/ath12k/dp_rx.h |   6 +-
 drivers/net/wireless/ath/ath12k/mac.c   | 436 ++++++++++++++++++------
 drivers/net/wireless/ath/ath12k/mac.h   |   6 +
 6 files changed, 371 insertions(+), 125 deletions(-)


base-commit: 175616a7658cd5d53389d1f9c1ce22debd4595a5
-- 
2.39.5

[PATCH 01/10] wifi: ath12k: convert struct ath12k::wmi_mgmt_tx_work to struct wiphy_work

[Kalle Valo]

From: Kalle Valo <quic_kvalo@quicinc.com>

To simplify locking for the next patches convert struct
ath12k::wmi_mgmt_tx_work to use wiphy_work. After this
ath12k_mgmt_over_wmi_tx_work() is called with wiphy_lock() taken. In
ath12k_core_suspend() we need to take wiphy_lock() because
ath12k_mac_wait_tx_complete() requires it.

Also add lockdep_assert_wiphy() to document when wiphy_lock() is held.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/core.c |  6 ++++++
 drivers/net/wireless/ath/ath12k/core.h |  2 +-
 drivers/net/wireless/ath/ath12k/mac.c  | 20 ++++++++++++++++----
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
index c57322221e1d..263a7c789122 100644
--- a/drivers/net/wireless/ath/ath12k/core.c
+++ b/drivers/net/wireless/ath/ath12k/core.c
@@ -79,11 +79,17 @@ int ath12k_core_suspend(struct ath12k_base *ab)
 		ar = ab->pdevs[i].ar;
 		if (!ar)
 			continue;
+
+		wiphy_lock(ath12k_ar_to_hw(ar)->wiphy);
+
 		ret = ath12k_mac_wait_tx_complete(ar);
 		if (ret) {
+			wiphy_unlock(ath12k_ar_to_hw(ar)->wiphy);
 			ath12k_warn(ab, "failed to wait tx complete: %d\n", ret);
 			return ret;
 		}
+
+		wiphy_unlock(ath12k_ar_to_hw(ar)->wiphy);
 	}
 
 	/* PM framework skips suspend_late/resume_early callbacks
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index c1d5e93b679a..5be977008319 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -679,7 +679,7 @@ struct ath12k {
 
 	struct work_struct regd_update_work;
 
-	struct work_struct wmi_mgmt_tx_work;
+	struct wiphy_work wmi_mgmt_tx_work;
 	struct sk_buff_head wmi_mgmt_tx_queue;
 
 	struct ath12k_wow wow;
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 60702bf07141..a6fe998c177e 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -6726,6 +6726,8 @@ static void ath12k_mgmt_over_wmi_tx_drop(struct ath12k *ar, struct sk_buff *skb)
 {
 	int num_mgmt;
 
+	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
+
 	ieee80211_free_txskb(ath12k_ar_to_hw(ar), skb);
 
 	num_mgmt = atomic_dec_if_positive(&ar->num_pending_mgmt_tx);
@@ -6787,6 +6789,8 @@ static int ath12k_mac_mgmt_tx_wmi(struct ath12k *ar, struct ath12k_link_vif *arv
 	int buf_id;
 	int ret;
 
+	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
+
 	ATH12K_SKB_CB(skb)->ar = ar;
 	spin_lock_bh(&ar->txmgmt_idr_lock);
 	buf_id = idr_alloc(&ar->txmgmt_idr, skb, 0,
@@ -6841,7 +6845,7 @@ static void ath12k_mgmt_over_wmi_tx_purge(struct ath12k *ar)
 		ath12k_mgmt_over_wmi_tx_drop(ar, skb);
 }
 
-static void ath12k_mgmt_over_wmi_tx_work(struct work_struct *work)
+static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work *work)
 {
 	struct ath12k *ar = container_of(work, struct ath12k, wmi_mgmt_tx_work);
 	struct ath12k_skb_cb *skb_cb;
@@ -6850,6 +6854,8 @@ static void ath12k_mgmt_over_wmi_tx_work(struct work_struct *work)
 	struct sk_buff *skb;
 	int ret;
 
+	lockdep_assert_wiphy(wiphy);
+
 	while ((skb = skb_dequeue(&ar->wmi_mgmt_tx_queue)) != NULL) {
 		skb_cb = ATH12K_SKB_CB(skb);
 		if (!skb_cb->vif) {
@@ -6904,7 +6910,7 @@ static int ath12k_mac_mgmt_tx(struct ath12k *ar, struct sk_buff *skb,
 
 	skb_queue_tail(q, skb);
 	atomic_inc(&ar->num_pending_mgmt_tx);
-	ieee80211_queue_work(ath12k_ar_to_hw(ar), &ar->wmi_mgmt_tx_work);
+	wiphy_work_queue(ath12k_ar_to_hw(ar)->wiphy, &ar->wmi_mgmt_tx_work);
 
 	return 0;
 }
@@ -6981,10 +6987,12 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
 
 void ath12k_mac_drain_tx(struct ath12k *ar)
 {
+	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
+
 	/* make sure rcu-protected mac80211 tx path itself is drained */
 	synchronize_net();
 
-	cancel_work_sync(&ar->wmi_mgmt_tx_work);
+	wiphy_work_cancel(ath12k_ar_to_hw(ar)->wiphy, &ar->wmi_mgmt_tx_work);
 	ath12k_mgmt_over_wmi_tx_purge(ar);
 }
 
@@ -7101,6 +7109,8 @@ static void ath12k_drain_tx(struct ath12k_hw *ah)
 	struct ath12k *ar;
 	int i;
 
+	lockdep_assert_wiphy(ah->hw->wiphy);
+
 	for_each_ar(ah, ar, i)
 		ath12k_mac_drain_tx(ar);
 }
@@ -9134,6 +9144,8 @@ static int ath12k_mac_flush(struct ath12k *ar)
 
 int ath12k_mac_wait_tx_complete(struct ath12k *ar)
 {
+	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
+
 	ath12k_mac_drain_tx(ar);
 	return ath12k_mac_flush(ar);
 }
@@ -10604,7 +10616,7 @@ static void ath12k_mac_setup(struct ath12k *ar)
 	INIT_DELAYED_WORK(&ar->scan.timeout, ath12k_scan_timeout_work);
 	INIT_WORK(&ar->regd_update_work, ath12k_regd_update_work);
 
-	INIT_WORK(&ar->wmi_mgmt_tx_work, ath12k_mgmt_over_wmi_tx_work);
+	wiphy_work_init(&ar->wmi_mgmt_tx_work, ath12k_mgmt_over_wmi_tx_work);
 	skb_queue_head_init(&ar->wmi_mgmt_tx_queue);
 }
 
-- 
2.39.5

Re: [PATCH 01/10] wifi: ath12k: convert struct ath12k::wmi_mgmt_tx_work to struct wiphy_work

[Baochen Qiang]

On 11/27/2024 1:11 AM, Kalle Valo wrote:
> From: Kalle Valo <quic_kvalo@quicinc.com>
> 
> To simplify locking for the next patches convert struct
> ath12k::wmi_mgmt_tx_work to use wiphy_work. After this
> ath12k_mgmt_over_wmi_tx_work() is called with wiphy_lock() taken. In
> ath12k_core_suspend() we need to take wiphy_lock() because
> ath12k_mac_wait_tx_complete() requires it.
> 
> Also add lockdep_assert_wiphy() to document when wiphy_lock() is held.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> 
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath12k/core.c |  6 ++++++
>  drivers/net/wireless/ath/ath12k/core.h |  2 +-
>  drivers/net/wireless/ath/ath12k/mac.c  | 20 ++++++++++++++++----
>  3 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
> index c57322221e1d..263a7c789122 100644
> --- a/drivers/net/wireless/ath/ath12k/core.c
> +++ b/drivers/net/wireless/ath/ath12k/core.c
> @@ -79,11 +79,17 @@ int ath12k_core_suspend(struct ath12k_base *ab)
>  		ar = ab->pdevs[i].ar;
>  		if (!ar)
>  			continue;
> +
> +		wiphy_lock(ath12k_ar_to_hw(ar)->wiphy);
> +
>  		ret = ath12k_mac_wait_tx_complete(ar);
>  		if (ret) {
> +			wiphy_unlock(ath12k_ar_to_hw(ar)->wiphy);
>  			ath12k_warn(ab, "failed to wait tx complete: %d\n", ret);
>  			return ret;
>  		}
> +
> +		wiphy_unlock(ath12k_ar_to_hw(ar)->wiphy);
>  	}
>  
>  	/* PM framework skips suspend_late/resume_early callbacks
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index c1d5e93b679a..5be977008319 100644
> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -679,7 +679,7 @@ struct ath12k {
>  
>  	struct work_struct regd_update_work;
>  
> -	struct work_struct wmi_mgmt_tx_work;
> +	struct wiphy_work wmi_mgmt_tx_work;
>  	struct sk_buff_head wmi_mgmt_tx_queue;
>  
>  	struct ath12k_wow wow;
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index 60702bf07141..a6fe998c177e 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -6726,6 +6726,8 @@ static void ath12k_mgmt_over_wmi_tx_drop(struct ath12k *ar, struct sk_buff *skb)
>  {
>  	int num_mgmt;
>  
> +	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
why would we need wiphy lock protect here? I don;t see anything in this function need it.

> +
>  	ieee80211_free_txskb(ath12k_ar_to_hw(ar), skb);
>  
>  	num_mgmt = atomic_dec_if_positive(&ar->num_pending_mgmt_tx);
> @@ -6787,6 +6789,8 @@ static int ath12k_mac_mgmt_tx_wmi(struct ath12k *ar, struct ath12k_link_vif *arv
>  	int buf_id;
>  	int ret;
>  
> +	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
and here the same question as above. I know this function is only called from
ath12k_mgmt_over_wmi_tx_work() which is under wiphy lock protection. But the function
itself doesn't need to assert it if the function does not need its protection.

> +
>  	ATH12K_SKB_CB(skb)->ar = ar;
>  	spin_lock_bh(&ar->txmgmt_idr_lock);
>  	buf_id = idr_alloc(&ar->txmgmt_idr, skb, 0,
> @@ -6841,7 +6845,7 @@ static void ath12k_mgmt_over_wmi_tx_purge(struct ath12k *ar)
>  		ath12k_mgmt_over_wmi_tx_drop(ar, skb);
>  }
>  
> -static void ath12k_mgmt_over_wmi_tx_work(struct work_struct *work)
> +static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work *work)
>  {
>  	struct ath12k *ar = container_of(work, struct ath12k, wmi_mgmt_tx_work);
>  	struct ath12k_skb_cb *skb_cb;
> @@ -6850,6 +6854,8 @@ static void ath12k_mgmt_over_wmi_tx_work(struct work_struct *work)
>  	struct sk_buff *skb;
>  	int ret;
>  
> +	lockdep_assert_wiphy(wiphy);
we are definitely under wiphy lock protection since this is a wiphy_work item, hence no
need to assert it explicitly. see also

ieee80211_sta_monitor_work()
ieee80211_beacon_connection_loss_work()
ieee80211_csa_connection_drop_work()
ieee80211_teardown_ttlm_work()

> +
>  	while ((skb = skb_dequeue(&ar->wmi_mgmt_tx_queue)) != NULL) {
>  		skb_cb = ATH12K_SKB_CB(skb);
>  		if (!skb_cb->vif) {
> @@ -6904,7 +6910,7 @@ static int ath12k_mac_mgmt_tx(struct ath12k *ar, struct sk_buff *skb,
>  
>  	skb_queue_tail(q, skb);
>  	atomic_inc(&ar->num_pending_mgmt_tx);
> -	ieee80211_queue_work(ath12k_ar_to_hw(ar), &ar->wmi_mgmt_tx_work);
> +	wiphy_work_queue(ath12k_ar_to_hw(ar)->wiphy, &ar->wmi_mgmt_tx_work);
>  
>  	return 0;
>  }
> @@ -6981,10 +6987,12 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>  
>  void ath12k_mac_drain_tx(struct ath12k *ar)
>  {
> +	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
> +
>  	/* make sure rcu-protected mac80211 tx path itself is drained */
>  	synchronize_net();
>  
> -	cancel_work_sync(&ar->wmi_mgmt_tx_work);
> +	wiphy_work_cancel(ath12k_ar_to_hw(ar)->wiphy, &ar->wmi_mgmt_tx_work);
>  	ath12k_mgmt_over_wmi_tx_purge(ar);
>  }
>  
> @@ -7101,6 +7109,8 @@ static void ath12k_drain_tx(struct ath12k_hw *ah)
>  	struct ath12k *ar;
>  	int i;
>  
> +	lockdep_assert_wiphy(ah->hw->wiphy);
> +
>  	for_each_ar(ah, ar, i)
>  		ath12k_mac_drain_tx(ar);
>  }
> @@ -9134,6 +9144,8 @@ static int ath12k_mac_flush(struct ath12k *ar)
>  
>  int ath12k_mac_wait_tx_complete(struct ath12k *ar)
>  {
> +	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
> +
>  	ath12k_mac_drain_tx(ar);
>  	return ath12k_mac_flush(ar);
>  }
> @@ -10604,7 +10616,7 @@ static void ath12k_mac_setup(struct ath12k *ar)
>  	INIT_DELAYED_WORK(&ar->scan.timeout, ath12k_scan_timeout_work);
>  	INIT_WORK(&ar->regd_update_work, ath12k_regd_update_work);
>  
> -	INIT_WORK(&ar->wmi_mgmt_tx_work, ath12k_mgmt_over_wmi_tx_work);
> +	wiphy_work_init(&ar->wmi_mgmt_tx_work, ath12k_mgmt_over_wmi_tx_work);
>  	skb_queue_head_init(&ar->wmi_mgmt_tx_queue);
>  }
>  

Re: [PATCH 01/10] wifi: ath12k: convert struct ath12k::wmi_mgmt_tx_work to struct wiphy_work

[Kalle Valo]

Baochen Qiang <quic_bqiang@quicinc.com> writes:

> On 11/27/2024 1:11 AM, Kalle Valo wrote:
>> From: Kalle Valo <quic_kvalo@quicinc.com>
>> 
>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> @@ -6726,6 +6726,8 @@ static void ath12k_mgmt_over_wmi_tx_drop(struct ath12k *ar, struct sk_buff *skb)
>>  {
>>  	int num_mgmt;
>>  
>> +	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>
> why would we need wiphy lock protect here? I don;t see anything in this function need it.
>
>> +
>>  	ieee80211_free_txskb(ath12k_ar_to_hw(ar), skb);
>>  
>>  	num_mgmt = atomic_dec_if_positive(&ar->num_pending_mgmt_tx);
>> @@ -6787,6 +6789,8 @@ static int ath12k_mac_mgmt_tx_wmi(struct ath12k *ar, struct ath12k_link_vif *arv
>>  	int buf_id;
>>  	int ret;
>>  
>> +	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>
> and here the same question as above. I know this function is only called from
> ath12k_mgmt_over_wmi_tx_work() which is under wiphy lock protection. But the function
> itself doesn't need to assert it if the function does not need its protection.
>
>> +
>>  	ATH12K_SKB_CB(skb)->ar = ar;
>>  	spin_lock_bh(&ar->txmgmt_idr_lock);
>>  	buf_id = idr_alloc(&ar->txmgmt_idr, skb, 0,
>> @@ -6841,7 +6845,7 @@ static void ath12k_mgmt_over_wmi_tx_purge(struct ath12k *ar)
>>  		ath12k_mgmt_over_wmi_tx_drop(ar, skb);
>>  }
>>  
>> -static void ath12k_mgmt_over_wmi_tx_work(struct work_struct *work)
>> +static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work *work)
>>  {
>>  	struct ath12k *ar = container_of(work, struct ath12k, wmi_mgmt_tx_work);
>>  	struct ath12k_skb_cb *skb_cb;
>> @@ -6850,6 +6854,8 @@ static void ath12k_mgmt_over_wmi_tx_work(struct work_struct *work)
>>  	struct sk_buff *skb;
>>  	int ret;
>>  
>> +	lockdep_assert_wiphy(wiphy);
>
> we are definitely under wiphy lock protection since this is a wiphy_work item, hence no
> need to assert it explicitly. see also
>
> ieee80211_sta_monitor_work()
> ieee80211_beacon_connection_loss_work()
> ieee80211_csa_connection_drop_work()
> ieee80211_teardown_ttlm_work()

I have deliberately added all these lockdep_assert_wiphy() calls to
document which functions are called with wiphy_lock() held, otherwise
doing any locking analysis is much harder. My plan is that once MLO
support has landed to ath-next my plan is to document ath12k locking
design properly in the code. I think at that point we can also discuss
how we should use lockdep_assert_wiphy() in ath12k and should we drop
the extra calls.

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

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

Re: [PATCH 01/10] wifi: ath12k: convert struct ath12k::wmi_mgmt_tx_work to struct wiphy_work

[Baochen Qiang]

On 11/28/2024 8:08 PM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang@quicinc.com> writes:
> 
>> On 11/27/2024 1:11 AM, Kalle Valo wrote:
>>> From: Kalle Valo <quic_kvalo@quicinc.com>
>>>
>>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>>> @@ -6726,6 +6726,8 @@ static void ath12k_mgmt_over_wmi_tx_drop(struct ath12k *ar, struct sk_buff *skb)
>>>  {
>>>  	int num_mgmt;
>>>  
>>> +	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>>
>> why would we need wiphy lock protect here? I don;t see anything in this function need it.
>>
>>> +
>>>  	ieee80211_free_txskb(ath12k_ar_to_hw(ar), skb);
>>>  
>>>  	num_mgmt = atomic_dec_if_positive(&ar->num_pending_mgmt_tx);
>>> @@ -6787,6 +6789,8 @@ static int ath12k_mac_mgmt_tx_wmi(struct ath12k *ar, struct ath12k_link_vif *arv
>>>  	int buf_id;
>>>  	int ret;
>>>  
>>> +	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
>>
>> and here the same question as above. I know this function is only called from
>> ath12k_mgmt_over_wmi_tx_work() which is under wiphy lock protection. But the function
>> itself doesn't need to assert it if the function does not need its protection.
>>
>>> +
>>>  	ATH12K_SKB_CB(skb)->ar = ar;
>>>  	spin_lock_bh(&ar->txmgmt_idr_lock);
>>>  	buf_id = idr_alloc(&ar->txmgmt_idr, skb, 0,
>>> @@ -6841,7 +6845,7 @@ static void ath12k_mgmt_over_wmi_tx_purge(struct ath12k *ar)
>>>  		ath12k_mgmt_over_wmi_tx_drop(ar, skb);
>>>  }
>>>  
>>> -static void ath12k_mgmt_over_wmi_tx_work(struct work_struct *work)
>>> +static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work *work)
>>>  {
>>>  	struct ath12k *ar = container_of(work, struct ath12k, wmi_mgmt_tx_work);
>>>  	struct ath12k_skb_cb *skb_cb;
>>> @@ -6850,6 +6854,8 @@ static void ath12k_mgmt_over_wmi_tx_work(struct work_struct *work)
>>>  	struct sk_buff *skb;
>>>  	int ret;
>>>  
>>> +	lockdep_assert_wiphy(wiphy);
>>
>> we are definitely under wiphy lock protection since this is a wiphy_work item, hence no
>> need to assert it explicitly. see also
>>
>> ieee80211_sta_monitor_work()
>> ieee80211_beacon_connection_loss_work()
>> ieee80211_csa_connection_drop_work()
>> ieee80211_teardown_ttlm_work()
> 
> I have deliberately added all these lockdep_assert_wiphy() calls to
> document which functions are called with wiphy_lock() held, otherwise
> doing any locking analysis is much harder. My plan is that once MLO
> support has landed to ath-next my plan is to document ath12k locking
> design properly in the code. I think at that point we can also discuss
> how we should use lockdep_assert_wiphy() in ath12k and should we drop
> the extra calls.
Ah, good to know. thanks for sharing the plan.

> 

Re: [PATCH 01/10] wifi: ath12k: convert struct ath12k::wmi_mgmt_tx_work to struct wiphy_work

[Kalle Valo]

Kalle Valo <kvalo@kernel.org> wrote:

> To simplify locking for the next patches convert struct
> ath12k::wmi_mgmt_tx_work to use wiphy_work. After this
> ath12k_mgmt_over_wmi_tx_work() is called with wiphy_lock() taken. In
> ath12k_core_suspend() we need to take wiphy_lock() because
> ath12k_mac_wait_tx_complete() requires it.
> 
> Also add lockdep_assert_wiphy() to document when wiphy_lock() is held.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> 
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

10 patches applied to ath-next branch of ath.git, thanks.

56dcbf0b5207 wifi: ath12k: convert struct ath12k::wmi_mgmt_tx_work to struct wiphy_work
648a121bafa3 wifi: ath12k: ath12k_mac_op_tx(): MLO support
2197feb0249d wifi: ath12k: ath12k_mac_op_flush(): MLO support
5419ef950da4 wifi: ath12k: ath12k_mac_op_ampdu_action(): MLO support
85edf16384d1 wifi: ath12k: ath12k_mac_station_add(): fix potential rx_stats leak
90570ba4610b wifi: ath12k: do not return invalid link id for scan link
1833a2ce5d7d wifi: ath12k: ath12k_bss_assoc(): MLO support
aa80f12f3bed wifi: ath12k: defer vdev creation for MLO
ad969bc9ee73 wifi: ath12k: ath12k_mac_op_set_key(): fix uninitialized symbol 'ret'
8c2143702d07 wifi: ath12k: ath12k_mac_op_sta_rc_update(): use mac80211 provided link id

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20241126171139.2350704-2-kvalo@kernel.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
https://docs.kernel.org/process/submitting-patches.html

[PATCH 02/10] wifi: ath12k: ath12k_mac_op_tx(): MLO support

[Kalle Valo]

From: Sriram R <quic_srirrama@quicinc.com>

For a frame transmission for an ML vif, mac80211 mentions transmit link id in
the tx control info. Use it to convert the RA/TA to the corresponding link sta
and link vif address before enqueueing the frame for transmission.

For 802.3 data frames, always enqueue the frame on the primary (assoc) link id.
Firmware does the link selection, builds 802.11 header and therefore the address
translation too.

Also ensure right link vif is used for WMI based management transmission and
add comments to document when RCU read lock is held.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Sriram R <quic_srirrama@quicinc.com>
Co-developed-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/core.h |   1 +
 drivers/net/wireless/ath/ath12k/mac.c  | 141 ++++++++++++++++++++++++-
 2 files changed, 138 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 5be977008319..e246e3d3c162 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -122,6 +122,7 @@ struct ath12k_skb_cb {
 	dma_addr_t paddr_ext_desc;
 	u32 cipher;
 	u8 flags;
+	u8 link_id;
 };
 
 struct ath12k_skb_rxcb {
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index a6fe998c177e..97a5f26cc577 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -6848,6 +6848,7 @@ static void ath12k_mgmt_over_wmi_tx_purge(struct ath12k *ar)
 static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work *work)
 {
 	struct ath12k *ar = container_of(work, struct ath12k, wmi_mgmt_tx_work);
+	struct ath12k_hw *ah = ar->ah;
 	struct ath12k_skb_cb *skb_cb;
 	struct ath12k_vif *ahvif;
 	struct ath12k_link_vif *arvif;
@@ -6865,7 +6866,15 @@ static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work
 		}
 
 		ahvif = ath12k_vif_to_ahvif(skb_cb->vif);
-		arvif = &ahvif->deflink;
+		if (!(ahvif->links_map & BIT(skb_cb->link_id))) {
+			ath12k_warn(ar->ab,
+				    "invalid linkid %u in mgmt over wmi tx with linkmap 0x%X\n",
+				    skb_cb->link_id, ahvif->links_map);
+			ath12k_mgmt_over_wmi_tx_drop(ar, skb);
+			continue;
+		}
+
+		arvif = wiphy_dereference(ah->hw->wiphy, ahvif->link[skb_cb->link_id]);
 		if (ar->allocated_vdev_map & (1LL << arvif->vdev_id)) {
 			ret = ath12k_mac_mgmt_tx_wmi(ar, arvif, skb);
 			if (ret) {
@@ -6875,9 +6884,10 @@ static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work
 			}
 		} else {
 			ath12k_warn(ar->ab,
-				    "dropping mgmt frame for vdev %d, is_started %d\n",
+				    "dropping mgmt frame for vdev %d link_id %u, is_started %d\n",
 				    arvif->vdev_id,
-				    arvif->is_started);
+				    arvif->is_started,
+				    skb_cb->link_id);
 			ath12k_mgmt_over_wmi_tx_drop(ar, skb);
 		}
 	}
@@ -6936,6 +6946,105 @@ static void ath12k_mac_add_p2p_noa_ie(struct ath12k *ar,
 	spin_unlock_bh(&ar->data_lock);
 }
 
+/* Note: called under rcu_read_lock() */
+static u8 ath12k_mac_get_tx_link(struct ieee80211_sta *sta, struct ieee80211_vif *vif,
+				 u8 link, struct sk_buff *skb, u32 info_flags)
+{
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
+	struct ieee80211_link_sta *link_sta;
+	struct ieee80211_bss_conf *bss_conf;
+	struct ath12k_sta *ahsta;
+
+	/* Use the link id passed or the default vif link */
+	if (!sta) {
+		if (link != IEEE80211_LINK_UNSPECIFIED)
+			return link;
+
+		return ahvif->deflink.link_id;
+	}
+
+	ahsta = ath12k_sta_to_ahsta(sta);
+
+	/* Below translation ensures we pass proper A2 & A3 for non ML clients.
+	 * Also it assumes for now support only for MLO AP in this path
+	 */
+	if (!sta->mlo) {
+		link = ahsta->deflink.link_id;
+
+		if (info_flags & IEEE80211_TX_CTL_HW_80211_ENCAP)
+			return link;
+
+		bss_conf = rcu_dereference(vif->link_conf[link]);
+		if (bss_conf) {
+			ether_addr_copy(hdr->addr2, bss_conf->addr);
+			if (!ieee80211_has_tods(hdr->frame_control) &&
+			    !ieee80211_has_fromds(hdr->frame_control))
+				ether_addr_copy(hdr->addr3, bss_conf->addr);
+		}
+
+		return link;
+	}
+
+	/* enqueue eth enacap & data frames on primary link, FW does link
+	 * selection and address translation.
+	 */
+	if (info_flags & IEEE80211_TX_CTL_HW_80211_ENCAP ||
+	    ieee80211_is_data(hdr->frame_control))
+		return ahsta->assoc_link_id;
+
+	/* 802.11 frame cases */
+	if (link == IEEE80211_LINK_UNSPECIFIED)
+		link = ahsta->deflink.link_id;
+
+	if (!ieee80211_is_mgmt(hdr->frame_control))
+		return link;
+
+	/* Perform address conversion for ML STA Tx */
+	bss_conf = rcu_dereference(vif->link_conf[link]);
+	link_sta = rcu_dereference(sta->link[link]);
+
+	if (bss_conf && link_sta) {
+		ether_addr_copy(hdr->addr1, link_sta->addr);
+		ether_addr_copy(hdr->addr2, bss_conf->addr);
+
+		if (vif->type == NL80211_IFTYPE_STATION && bss_conf->bssid)
+			ether_addr_copy(hdr->addr3, bss_conf->bssid);
+		else if (vif->type == NL80211_IFTYPE_AP)
+			ether_addr_copy(hdr->addr3, bss_conf->addr);
+
+		return link;
+	}
+
+	if (bss_conf) {
+		/* In certain cases where a ML sta associated and added subset of
+		 * links on which the ML AP is active, but now sends some frame
+		 * (ex. Probe request) on a different link which is active in our
+		 * MLD but was not added during previous association, we can
+		 * still honor the Tx to that ML STA via the requested link.
+		 * The control would reach here in such case only when that link
+		 * address is same as the MLD address or in worst case clients
+		 * used MLD address at TA wrongly which would have helped
+		 * identify the ML sta object and pass it here.
+		 * If the link address of that STA is different from MLD address,
+		 * then the sta object would be NULL and control won't reach
+		 * here but return at the start of the function itself with !sta
+		 * check. Also this would not need any translation at hdr->addr1
+		 * from MLD to link address since the RA is the MLD address
+		 * (same as that link address ideally) already.
+		 */
+		ether_addr_copy(hdr->addr2, bss_conf->addr);
+
+		if (vif->type == NL80211_IFTYPE_STATION && bss_conf->bssid)
+			ether_addr_copy(hdr->addr3, bss_conf->bssid);
+		else if (vif->type == NL80211_IFTYPE_AP)
+			ether_addr_copy(hdr->addr3, bss_conf->addr);
+	}
+
+	return link;
+}
+
+/* Note: called under rcu_read_lock() */
 static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
 			     struct ieee80211_tx_control *control,
 			     struct sk_buff *skb)
@@ -6945,13 +7054,16 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
 	struct ieee80211_vif *vif = info->control.vif;
 	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
 	struct ath12k_link_vif *arvif = &ahvif->deflink;
-	struct ath12k *ar = arvif->ar;
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
 	struct ieee80211_key_conf *key = info->control.hw_key;
+	struct ieee80211_sta *sta = control->sta;
 	u32 info_flags = info->flags;
+	struct ath12k *ar;
 	bool is_prb_rsp;
+	u8 link_id;
 	int ret;
 
+	link_id = u32_get_bits(info->control.flags, IEEE80211_TX_CTRL_MLO_LINK);
 	memset(skb_cb, 0, sizeof(*skb_cb));
 	skb_cb->vif = vif;
 
@@ -6960,6 +7072,27 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
 		skb_cb->flags |= ATH12K_SKB_CIPHER_SET;
 	}
 
+	/* handle only for MLO case, use deflink for non MLO case */
+	if (vif->valid_links) {
+		link_id = ath12k_mac_get_tx_link(sta, vif, link_id, skb, info_flags);
+		if (link_id >= IEEE80211_MLD_MAX_NUM_LINKS) {
+			ieee80211_free_txskb(hw, skb);
+			return;
+		}
+	} else {
+		link_id = 0;
+	}
+
+	arvif = rcu_dereference(ahvif->link[link_id]);
+	if (!arvif || !arvif->ar) {
+		ath12k_warn(ahvif->ah, "failed to find arvif link id %u for frame transmission",
+			    link_id);
+		ieee80211_free_txskb(hw, skb);
+		return;
+	}
+
+	ar = arvif->ar;
+	skb_cb->link_id = link_id;
 	is_prb_rsp = ieee80211_is_probe_resp(hdr->frame_control);
 
 	if (info_flags & IEEE80211_TX_CTL_HW_80211_ENCAP) {
-- 
2.39.5

Re: [PATCH 02/10] wifi: ath12k: ath12k_mac_op_tx(): MLO support

[Baochen Qiang]

On 11/27/2024 1:11 AM, Kalle Valo wrote:
> From: Sriram R <quic_srirrama@quicinc.com>
> 
> For a frame transmission for an ML vif, mac80211 mentions transmit link id in
> the tx control info. Use it to convert the RA/TA to the corresponding link sta
> and link vif address before enqueueing the frame for transmission.
> 
> For 802.3 data frames, always enqueue the frame on the primary (assoc) link id.
> Firmware does the link selection, builds 802.11 header and therefore the address
> translation too.
> 
> Also ensure right link vif is used for WMI based management transmission and
> add comments to document when RCU read lock is held.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> 
> Signed-off-by: Sriram R <quic_srirrama@quicinc.com>
> Co-developed-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
> Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath12k/core.h |   1 +
>  drivers/net/wireless/ath/ath12k/mac.c  | 141 ++++++++++++++++++++++++-
>  2 files changed, 138 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index 5be977008319..e246e3d3c162 100644
> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -122,6 +122,7 @@ struct ath12k_skb_cb {
>  	dma_addr_t paddr_ext_desc;
>  	u32 cipher;
>  	u8 flags;
> +	u8 link_id;
>  };
>  
>  struct ath12k_skb_rxcb {
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index a6fe998c177e..97a5f26cc577 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -6848,6 +6848,7 @@ static void ath12k_mgmt_over_wmi_tx_purge(struct ath12k *ar)
>  static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work *work)
>  {
>  	struct ath12k *ar = container_of(work, struct ath12k, wmi_mgmt_tx_work);
> +	struct ath12k_hw *ah = ar->ah;
>  	struct ath12k_skb_cb *skb_cb;
>  	struct ath12k_vif *ahvif;
>  	struct ath12k_link_vif *arvif;
> @@ -6865,7 +6866,15 @@ static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work
>  		}
>  
>  		ahvif = ath12k_vif_to_ahvif(skb_cb->vif);
> -		arvif = &ahvif->deflink;
> +		if (!(ahvif->links_map & BIT(skb_cb->link_id))) {
> +			ath12k_warn(ar->ab,
> +				    "invalid linkid %u in mgmt over wmi tx with linkmap 0x%X\n",
s/0x%X/0x%x/ ?

> +				    skb_cb->link_id, ahvif->links_map);
> +			ath12k_mgmt_over_wmi_tx_drop(ar, skb);
> +			continue;
> +		}
> +
> +		arvif = wiphy_dereference(ah->hw->wiphy, ahvif->link[skb_cb->link_id]);
>  		if (ar->allocated_vdev_map & (1LL << arvif->vdev_id)) {
>  			ret = ath12k_mac_mgmt_tx_wmi(ar, arvif, skb);
>  			if (ret) {
> @@ -6875,9 +6884,10 @@ static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work
>  			}
>  		} else {
>  			ath12k_warn(ar->ab,
> -				    "dropping mgmt frame for vdev %d, is_started %d\n",
> +				    "dropping mgmt frame for vdev %d link_id %u, is_started %d\n",
>  				    arvif->vdev_id,
> -				    arvif->is_started);
> +				    arvif->is_started,
> +				    skb_cb->link_id);
swap 'arvif->is_started' and 'skb_cb->link_id'.

>  			ath12k_mgmt_over_wmi_tx_drop(ar, skb);
>  		}
>  	}
> @@ -6936,6 +6946,105 @@ static void ath12k_mac_add_p2p_noa_ie(struct ath12k *ar,
>  	spin_unlock_bh(&ar->data_lock);
>  }
>  
> +/* Note: called under rcu_read_lock() */
> +static u8 ath12k_mac_get_tx_link(struct ieee80211_sta *sta, struct ieee80211_vif *vif,
> +				 u8 link, struct sk_buff *skb, u32 info_flags)
> +{
> +	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> +	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
> +	struct ieee80211_link_sta *link_sta;
> +	struct ieee80211_bss_conf *bss_conf;
> +	struct ath12k_sta *ahsta;
better to assert RCU read lock here?

> +
> +	/* Use the link id passed or the default vif link */
> +	if (!sta) {
> +		if (link != IEEE80211_LINK_UNSPECIFIED)
> +			return link;
> +
> +		return ahvif->deflink.link_id;
> +	}
> +
> +	ahsta = ath12k_sta_to_ahsta(sta);
> +
> +	/* Below translation ensures we pass proper A2 & A3 for non ML clients.
> +	 * Also it assumes for now support only for MLO AP in this path
> +	 */
> +	if (!sta->mlo) {
> +		link = ahsta->deflink.link_id;
> +
> +		if (info_flags & IEEE80211_TX_CTL_HW_80211_ENCAP)
> +			return link;
> +
> +		bss_conf = rcu_dereference(vif->link_conf[link]);
> +		if (bss_conf) {
> +			ether_addr_copy(hdr->addr2, bss_conf->addr);
> +			if (!ieee80211_has_tods(hdr->frame_control) &&
> +			    !ieee80211_has_fromds(hdr->frame_control))
> +				ether_addr_copy(hdr->addr3, bss_conf->addr);
> +		}
> +
> +		return link;
> +	}
> +
> +	/* enqueue eth enacap & data frames on primary link, FW does link
> +	 * selection and address translation.
> +	 */
> +	if (info_flags & IEEE80211_TX_CTL_HW_80211_ENCAP ||
> +	    ieee80211_is_data(hdr->frame_control))
> +		return ahsta->assoc_link_id;
> +
> +	/* 802.11 frame cases */
> +	if (link == IEEE80211_LINK_UNSPECIFIED)
> +		link = ahsta->deflink.link_id;
> +
> +	if (!ieee80211_is_mgmt(hdr->frame_control))
> +		return link;
> +
> +	/* Perform address conversion for ML STA Tx */
> +	bss_conf = rcu_dereference(vif->link_conf[link]);
> +	link_sta = rcu_dereference(sta->link[link]);
> +
> +	if (bss_conf && link_sta) {
> +		ether_addr_copy(hdr->addr1, link_sta->addr);
> +		ether_addr_copy(hdr->addr2, bss_conf->addr);
> +
> +		if (vif->type == NL80211_IFTYPE_STATION && bss_conf->bssid)
> +			ether_addr_copy(hdr->addr3, bss_conf->bssid);
> +		else if (vif->type == NL80211_IFTYPE_AP)
> +			ether_addr_copy(hdr->addr3, bss_conf->addr);
> +
> +		return link;
> +	}
> +
> +	if (bss_conf) {
> +		/* In certain cases where a ML sta associated and added subset of
> +		 * links on which the ML AP is active, but now sends some frame
> +		 * (ex. Probe request) on a different link which is active in our
> +		 * MLD but was not added during previous association, we can
> +		 * still honor the Tx to that ML STA via the requested link.
> +		 * The control would reach here in such case only when that link
> +		 * address is same as the MLD address or in worst case clients
> +		 * used MLD address at TA wrongly which would have helped
> +		 * identify the ML sta object and pass it here.
> +		 * If the link address of that STA is different from MLD address,
> +		 * then the sta object would be NULL and control won't reach
> +		 * here but return at the start of the function itself with !sta
> +		 * check. Also this would not need any translation at hdr->addr1
> +		 * from MLD to link address since the RA is the MLD address
> +		 * (same as that link address ideally) already.
> +		 */
> +		ether_addr_copy(hdr->addr2, bss_conf->addr);
> +
> +		if (vif->type == NL80211_IFTYPE_STATION && bss_conf->bssid)
> +			ether_addr_copy(hdr->addr3, bss_conf->bssid);
> +		else if (vif->type == NL80211_IFTYPE_AP)
> +			ether_addr_copy(hdr->addr3, bss_conf->addr);
> +	}
> +
> +	return link;
> +}
> +
> +/* Note: called under rcu_read_lock() */
>  static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>  			     struct ieee80211_tx_control *control,
>  			     struct sk_buff *skb)
> @@ -6945,13 +7054,16 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>  	struct ieee80211_vif *vif = info->control.vif;
>  	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
>  	struct ath12k_link_vif *arvif = &ahvif->deflink;
> -	struct ath12k *ar = arvif->ar;
>  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>  	struct ieee80211_key_conf *key = info->control.hw_key;
> +	struct ieee80211_sta *sta = control->sta;
>  	u32 info_flags = info->flags;
> +	struct ath12k *ar;
>  	bool is_prb_rsp;
> +	u8 link_id;
>  	int ret;
>  
better to assert RCU read lock here?

> +	link_id = u32_get_bits(info->control.flags, IEEE80211_TX_CTRL_MLO_LINK);
>  	memset(skb_cb, 0, sizeof(*skb_cb));
>  	skb_cb->vif = vif;
>  
> @@ -6960,6 +7072,27 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>  		skb_cb->flags |= ATH12K_SKB_CIPHER_SET;
>  	}
>  
> +	/* handle only for MLO case, use deflink for non MLO case */
> +	if (vif->valid_links) {
better to use ieee80211_vif_is_mld() helper?

> +		link_id = ath12k_mac_get_tx_link(sta, vif, link_id, skb, info_flags);
> +		if (link_id >= IEEE80211_MLD_MAX_NUM_LINKS) {
> +			ieee80211_free_txskb(hw, skb);
> +			return;
> +		}
> +	} else {
> +		link_id = 0;
> +	}
> +
> +	arvif = rcu_dereference(ahvif->link[link_id]);
> +	if (!arvif || !arvif->ar) {
> +		ath12k_warn(ahvif->ah, "failed to find arvif link id %u for frame transmission",
> +			    link_id);
> +		ieee80211_free_txskb(hw, skb);
> +		return;
> +	}
> +
> +	ar = arvif->ar;
> +	skb_cb->link_id = link_id;
>  	is_prb_rsp = ieee80211_is_probe_resp(hdr->frame_control);
>  
>  	if (info_flags & IEEE80211_TX_CTL_HW_80211_ENCAP) {

Re: [PATCH 02/10] wifi: ath12k: ath12k_mac_op_tx(): MLO support

[Kalle Valo]

Baochen Qiang <quic_bqiang@quicinc.com> writes:

> On 11/27/2024 1:11 AM, Kalle Valo wrote:
>> From: Sriram R <quic_srirrama@quicinc.com>
>> 
>> @@ -6848,6 +6848,7 @@ static void ath12k_mgmt_over_wmi_tx_purge(struct ath12k *ar)
>>  static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work *work)
>>  {
>>  	struct ath12k *ar = container_of(work, struct ath12k, wmi_mgmt_tx_work);
>> +	struct ath12k_hw *ah = ar->ah;
>>  	struct ath12k_skb_cb *skb_cb;
>>  	struct ath12k_vif *ahvif;
>>  	struct ath12k_link_vif *arvif;
>> @@ -6865,7 +6866,15 @@ static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work
>>  		}
>>  
>>  		ahvif = ath12k_vif_to_ahvif(skb_cb->vif);
>> -		arvif = &ahvif->deflink;
>> +		if (!(ahvif->links_map & BIT(skb_cb->link_id))) {
>> +			ath12k_warn(ar->ab,
>> +				    "invalid linkid %u in mgmt over wmi tx with linkmap 0x%X\n",
>
> s/0x%X/0x%x/ ?

Fixed.

>
>> +				    skb_cb->link_id, ahvif->links_map);
>> +			ath12k_mgmt_over_wmi_tx_drop(ar, skb);
>> +			continue;
>> +		}
>> +
>> +		arvif = wiphy_dereference(ah->hw->wiphy, ahvif->link[skb_cb->link_id]);
>>  		if (ar->allocated_vdev_map & (1LL << arvif->vdev_id)) {
>>  			ret = ath12k_mac_mgmt_tx_wmi(ar, arvif, skb);
>>  			if (ret) {
>> @@ -6875,9 +6884,10 @@ static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work
>>  			}
>>  		} else {
>>  			ath12k_warn(ar->ab,
>> -				    "dropping mgmt frame for vdev %d, is_started %d\n",
>> +				    "dropping mgmt frame for vdev %d link_id %u, is_started %d\n",
>>  				    arvif->vdev_id,
>> -				    arvif->is_started);
>> +				    arvif->is_started,
>> +				    skb_cb->link_id);
>
> swap 'arvif->is_started' and 'skb_cb->link_id'.

Good catch! Fixed as well.

>> +/* Note: called under rcu_read_lock() */
>> +static u8 ath12k_mac_get_tx_link(struct ieee80211_sta *sta, struct ieee80211_vif *vif,
>> +				 u8 link, struct sk_buff *skb, u32 info_flags)
>> +{
>> +	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>> +	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
>> +	struct ieee80211_link_sta *link_sta;
>> +	struct ieee80211_bss_conf *bss_conf;
>> +	struct ath12k_sta *ahsta;
>
> better to assert RCU read lock here?

You mean something like WARN_ON(!rcu_read_lock_held())? I'm not really a
fan of that, I think it's better that we discuss this also once we
document locking design properly.

>> +/* Note: called under rcu_read_lock() */
>>  static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>>  			     struct ieee80211_tx_control *control,
>>  			     struct sk_buff *skb)
>> @@ -6945,13 +7054,16 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>>  	struct ieee80211_vif *vif = info->control.vif;
>>  	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
>>  	struct ath12k_link_vif *arvif = &ahvif->deflink;
>> -	struct ath12k *ar = arvif->ar;
>>  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>>  	struct ieee80211_key_conf *key = info->control.hw_key;
>> +	struct ieee80211_sta *sta = control->sta;
>>  	u32 info_flags = info->flags;
>> +	struct ath12k *ar;
>>  	bool is_prb_rsp;
>> +	u8 link_id;
>>  	int ret;
>>  
> better to assert RCU read lock here?

Same comment here as above.

>
>> +	link_id = u32_get_bits(info->control.flags, IEEE80211_TX_CTRL_MLO_LINK);
>>  	memset(skb_cb, 0, sizeof(*skb_cb));
>>  	skb_cb->vif = vif;
>>  
>> @@ -6960,6 +7072,27 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>>  		skb_cb->flags |= ATH12K_SKB_CIPHER_SET;
>>  	}
>>  
>> +	/* handle only for MLO case, use deflink for non MLO case */
>> +	if (vif->valid_links) {
>
> better to use ieee80211_vif_is_mld() helper?

Indeed, fixed.

I did all the changes directly in the pending branch:

https://git.kernel.org/pub/scm/linux/kernel/git/ath/ath.git/commit/?h=pending&id=57ee27f3b3aa13c63978f03ce544c2f4210a9cd7

BTW when you reply please include an empty line between the quote and
your reply, this improves readability.

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

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

Re: [PATCH 02/10] wifi: ath12k: ath12k_mac_op_tx(): MLO support

[Baochen Qiang]

On 11/28/2024 8:32 PM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang@quicinc.com> writes:
> 
>> On 11/27/2024 1:11 AM, Kalle Valo wrote:
>>> From: Sriram R <quic_srirrama@quicinc.com>
>>>
>>> @@ -6848,6 +6848,7 @@ static void ath12k_mgmt_over_wmi_tx_purge(struct ath12k *ar)
>>>  static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work *work)
>>>  {
>>>  	struct ath12k *ar = container_of(work, struct ath12k, wmi_mgmt_tx_work);
>>> +	struct ath12k_hw *ah = ar->ah;
>>>  	struct ath12k_skb_cb *skb_cb;
>>>  	struct ath12k_vif *ahvif;
>>>  	struct ath12k_link_vif *arvif;
>>> @@ -6865,7 +6866,15 @@ static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work
>>>  		}
>>>  
>>>  		ahvif = ath12k_vif_to_ahvif(skb_cb->vif);
>>> -		arvif = &ahvif->deflink;
>>> +		if (!(ahvif->links_map & BIT(skb_cb->link_id))) {
>>> +			ath12k_warn(ar->ab,
>>> +				    "invalid linkid %u in mgmt over wmi tx with linkmap 0x%X\n",
>>
>> s/0x%X/0x%x/ ?
> 
> Fixed.
> 
>>
>>> +				    skb_cb->link_id, ahvif->links_map);
>>> +			ath12k_mgmt_over_wmi_tx_drop(ar, skb);
>>> +			continue;
>>> +		}
>>> +
>>> +		arvif = wiphy_dereference(ah->hw->wiphy, ahvif->link[skb_cb->link_id]);
>>>  		if (ar->allocated_vdev_map & (1LL << arvif->vdev_id)) {
>>>  			ret = ath12k_mac_mgmt_tx_wmi(ar, arvif, skb);
>>>  			if (ret) {
>>> @@ -6875,9 +6884,10 @@ static void ath12k_mgmt_over_wmi_tx_work(struct wiphy *wiphy, struct wiphy_work
>>>  			}
>>>  		} else {
>>>  			ath12k_warn(ar->ab,
>>> -				    "dropping mgmt frame for vdev %d, is_started %d\n",
>>> +				    "dropping mgmt frame for vdev %d link_id %u, is_started %d\n",
>>>  				    arvif->vdev_id,
>>> -				    arvif->is_started);
>>> +				    arvif->is_started,
>>> +				    skb_cb->link_id);
>>
>> swap 'arvif->is_started' and 'skb_cb->link_id'.
> 
> Good catch! Fixed as well.
> 
>>> +/* Note: called under rcu_read_lock() */
>>> +static u8 ath12k_mac_get_tx_link(struct ieee80211_sta *sta, struct ieee80211_vif *vif,
>>> +				 u8 link, struct sk_buff *skb, u32 info_flags)
>>> +{
>>> +	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>>> +	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
>>> +	struct ieee80211_link_sta *link_sta;
>>> +	struct ieee80211_bss_conf *bss_conf;
>>> +	struct ath12k_sta *ahsta;
>>
>> better to assert RCU read lock here?
> 
> You mean something like WARN_ON(!rcu_read_lock_held())? I'm not really a
> fan of that, I think it's better that we discuss this also once we
> document locking design properly.
> 
>>> +/* Note: called under rcu_read_lock() */
>>>  static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>>>  			     struct ieee80211_tx_control *control,
>>>  			     struct sk_buff *skb)
>>> @@ -6945,13 +7054,16 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>>>  	struct ieee80211_vif *vif = info->control.vif;
>>>  	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
>>>  	struct ath12k_link_vif *arvif = &ahvif->deflink;
>>> -	struct ath12k *ar = arvif->ar;
>>>  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>>>  	struct ieee80211_key_conf *key = info->control.hw_key;
>>> +	struct ieee80211_sta *sta = control->sta;
>>>  	u32 info_flags = info->flags;
>>> +	struct ath12k *ar;
>>>  	bool is_prb_rsp;
>>> +	u8 link_id;
>>>  	int ret;
>>>  
>> better to assert RCU read lock here?
> 
> Same comment here as above.
> 
>>
>>> +	link_id = u32_get_bits(info->control.flags, IEEE80211_TX_CTRL_MLO_LINK);
>>>  	memset(skb_cb, 0, sizeof(*skb_cb));
>>>  	skb_cb->vif = vif;
>>>  
>>> @@ -6960,6 +7072,27 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
>>>  		skb_cb->flags |= ATH12K_SKB_CIPHER_SET;
>>>  	}
>>>  
>>> +	/* handle only for MLO case, use deflink for non MLO case */
>>> +	if (vif->valid_links) {
>>
>> better to use ieee80211_vif_is_mld() helper?
> 
> Indeed, fixed.
> 
> I did all the changes directly in the pending branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ath/ath.git/commit/?h=pending&id=57ee27f3b3aa13c63978f03ce544c2f4210a9cd7

LGTM

> 
> BTW when you reply please include an empty line between the quote and
> your reply, this improves readability.

sure, will keep in mind.

> 

[PATCH 03/10] wifi: ath12k: ath12k_mac_op_flush(): MLO support

[Kalle Valo]

From: Sriram R <quic_srirrama@quicinc.com>

Currently when tx flush is requested for an vif only packets corresponding to
deflink are flushed, with MLO multiple link arvif could be affiliated to the ML
vif and packets corresponding to all of them should be flushed.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Sriram R <quic_srirrama@quicinc.com>
Co-developed-by: Maharaja Kennadyrajan <quic_mkenna@quicinc.com>
Signed-off-by: Maharaja Kennadyrajan <quic_mkenna@quicinc.com>
Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/mac.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 97a5f26cc577..d1c94eb8145a 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -9287,7 +9287,11 @@ static void ath12k_mac_op_flush(struct ieee80211_hw *hw, struct ieee80211_vif *v
 				u32 queues, bool drop)
 {
 	struct ath12k_hw *ah = ath12k_hw_to_ah(hw);
+	struct ath12k_link_vif *arvif;
+	struct ath12k_vif *ahvif;
+	unsigned long links;
 	struct ath12k *ar;
+	u8 link_id;
 	int i;
 
 	lockdep_assert_wiphy(hw->wiphy);
@@ -9302,12 +9306,18 @@ static void ath12k_mac_op_flush(struct ieee80211_hw *hw, struct ieee80211_vif *v
 		return;
 	}
 
-	ar = ath12k_get_ar_by_vif(hw, vif);
+	for_each_ar(ah, ar, i)
+		wiphy_work_flush(hw->wiphy, &ar->wmi_mgmt_tx_work);
 
-	if (!ar)
-		return;
+	ahvif = ath12k_vif_to_ahvif(vif);
+	links = ahvif->links_map;
+	for_each_set_bit(link_id, &links, IEEE80211_MLD_MAX_NUM_LINKS) {
+		arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]);
+		if (!(arvif && arvif->ar))
+			continue;
 
-	ath12k_mac_flush(ar);
+		ath12k_mac_flush(arvif->ar);
+	}
 }
 
 static int
-- 
2.39.5

[PATCH 04/10] wifi: ath12k: ath12k_mac_op_ampdu_action(): MLO support

[Kalle Valo]

From: Sriram R <quic_srirrama@quicinc.com>

Apply tid queue setup based on all link stations on receiving ampdu action
params for an ML Station.

Modify ath12k_get_ar_by_vif() to fetch ar based on link arvif inside ahvif.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Sriram R <quic_srirrama@quicinc.com>
Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/dp_rx.c | 36 +++++++++---
 drivers/net/wireless/ath/ath12k/dp_rx.h |  6 +-
 drivers/net/wireless/ath/ath12k/mac.c   | 76 ++++++++++++++-----------
 3 files changed, 76 insertions(+), 42 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
index 70680f2124e5..b24d1de4aabb 100644
--- a/drivers/net/wireless/ath/ath12k/dp_rx.c
+++ b/drivers/net/wireless/ath/ath12k/dp_rx.c
@@ -1065,15 +1065,25 @@ int ath12k_dp_rx_peer_tid_setup(struct ath12k *ar, const u8 *peer_mac, int vdev_
 }
 
 int ath12k_dp_rx_ampdu_start(struct ath12k *ar,
-			     struct ieee80211_ampdu_params *params)
+			     struct ieee80211_ampdu_params *params,
+			     u8 link_id)
 {
 	struct ath12k_base *ab = ar->ab;
 	struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(params->sta);
-	struct ath12k_link_sta *arsta = &ahsta->deflink;
-	int vdev_id = arsta->arvif->vdev_id;
+	struct ath12k_link_sta *arsta;
+	int vdev_id;
 	int ret;
 
-	ret = ath12k_dp_rx_peer_tid_setup(ar, params->sta->addr, vdev_id,
+	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
+
+	arsta = wiphy_dereference(ath12k_ar_to_hw(ar)->wiphy,
+				  ahsta->link[link_id]);
+	if (!arsta)
+		return -ENOLINK;
+
+	vdev_id = arsta->arvif->vdev_id;
+
+	ret = ath12k_dp_rx_peer_tid_setup(ar, arsta->addr, vdev_id,
 					  params->tid, params->buf_size,
 					  params->ssn, arsta->ahsta->pn_type);
 	if (ret)
@@ -1083,19 +1093,29 @@ int ath12k_dp_rx_ampdu_start(struct ath12k *ar,
 }
 
 int ath12k_dp_rx_ampdu_stop(struct ath12k *ar,
-			    struct ieee80211_ampdu_params *params)
+			    struct ieee80211_ampdu_params *params,
+			    u8 link_id)
 {
 	struct ath12k_base *ab = ar->ab;
 	struct ath12k_peer *peer;
 	struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(params->sta);
-	struct ath12k_link_sta *arsta = &ahsta->deflink;
-	int vdev_id = arsta->arvif->vdev_id;
+	struct ath12k_link_sta *arsta;
+	int vdev_id;
 	bool active;
 	int ret;
 
+	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
+
+	arsta = wiphy_dereference(ath12k_ar_to_hw(ar)->wiphy,
+				  ahsta->link[link_id]);
+	if (!arsta)
+		return -ENOLINK;
+
+	vdev_id = arsta->arvif->vdev_id;
+
 	spin_lock_bh(&ab->base_lock);
 
-	peer = ath12k_peer_find(ab, vdev_id, params->sta->addr);
+	peer = ath12k_peer_find(ab, vdev_id, arsta->addr);
 	if (!peer) {
 		spin_unlock_bh(&ab->base_lock);
 		ath12k_warn(ab, "failed to find the peer to stop rx aggregation\n");
diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.h b/drivers/net/wireless/ath/ath12k/dp_rx.h
index bfd4f814553e..1ce82088c954 100644
--- a/drivers/net/wireless/ath/ath12k/dp_rx.h
+++ b/drivers/net/wireless/ath/ath12k/dp_rx.h
@@ -85,9 +85,11 @@ static inline u32 ath12k_he_gi_to_nl80211_he_gi(u8 sgi)
 }
 
 int ath12k_dp_rx_ampdu_start(struct ath12k *ar,
-			     struct ieee80211_ampdu_params *params);
+			     struct ieee80211_ampdu_params *params,
+			     u8 link_id);
 int ath12k_dp_rx_ampdu_stop(struct ath12k *ar,
-			    struct ieee80211_ampdu_params *params);
+			    struct ieee80211_ampdu_params *params,
+			    u8 link_id);
 int ath12k_dp_rx_peer_pn_replay_config(struct ath12k_link_vif *arvif,
 				       const u8 *peer_addr,
 				       enum set_key_cmd key_cmd,
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index d1c94eb8145a..428415237831 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -725,11 +725,14 @@ static struct ath12k *ath12k_get_ar_by_ctx(struct ieee80211_hw *hw,
 }
 
 static struct ath12k *ath12k_get_ar_by_vif(struct ieee80211_hw *hw,
-					   struct ieee80211_vif *vif)
+					   struct ieee80211_vif *vif,
+					   u8 link_id)
 {
 	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
-	struct ath12k_link_vif *arvif = &ahvif->deflink;
 	struct ath12k_hw *ah = ath12k_hw_to_ah(hw);
+	struct ath12k_link_vif *arvif;
+
+	lockdep_assert_wiphy(hw->wiphy);
 
 	/* If there is one pdev within ah, then we return
 	 * ar directly.
@@ -737,7 +740,11 @@ static struct ath12k *ath12k_get_ar_by_vif(struct ieee80211_hw *hw,
 	if (ah->num_radio == 1)
 		return ah->radio;
 
-	if (arvif->is_created)
+	if (!(ahvif->links_map & BIT(link_id)))
+		return NULL;
+
+	arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]);
+	if (arvif && arvif->is_created)
 		return arvif->ar;
 
 	return NULL;
@@ -5667,6 +5674,7 @@ static void ath12k_mac_op_sta_rc_update(struct ieee80211_hw *hw,
 	struct ath12k *ar;
 	struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta);
 	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
+	struct ath12k_hw *ah = ath12k_hw_to_ah(hw);
 	struct ath12k_link_sta *arsta;
 	struct ath12k_link_vif *arvif;
 	struct ath12k_peer *peer;
@@ -5676,20 +5684,17 @@ static void ath12k_mac_op_sta_rc_update(struct ieee80211_hw *hw,
 	 */
 	u8 link_id = ATH12K_DEFAULT_LINK_ID;
 
-	ar = ath12k_get_ar_by_vif(hw, vif);
-	if (!ar) {
-		WARN_ON_ONCE(1);
-		return;
-	}
-
 	rcu_read_lock();
 	arvif = rcu_dereference(ahvif->link[link_id]);
 	if (!arvif) {
-		ath12k_warn(ar->ab, "mac sta rc update failed to fetch link vif on link id %u for peer %pM\n",
-			    link_id, sta->addr);
+		ath12k_hw_warn(ah, "mac sta rc update failed to fetch link vif on link id %u for peer %pM\n",
+			       link_id, sta->addr);
 		rcu_read_unlock();
 		return;
 	}
+
+	ar = arvif->ar;
+
 	arsta = rcu_dereference(ahsta->link[link_id]);
 	if (!arsta) {
 		rcu_read_unlock();
@@ -8288,20 +8293,26 @@ static int ath12k_mac_op_set_antenna(struct ieee80211_hw *hw, u32 tx_ant, u32 rx
 	return ret;
 }
 
-static int ath12k_mac_ampdu_action(struct ath12k_link_vif *arvif,
-				   struct ieee80211_ampdu_params *params)
+static int ath12k_mac_ampdu_action(struct ieee80211_hw *hw,
+				   struct ieee80211_vif *vif,
+				   struct ieee80211_ampdu_params *params,
+				   u8 link_id)
 {
-	struct ath12k *ar = arvif->ar;
+	struct ath12k *ar;
 	int ret = -EINVAL;
 
-	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
+	lockdep_assert_wiphy(hw->wiphy);
+
+	ar = ath12k_get_ar_by_vif(hw, vif, link_id);
+	if (!ar)
+		return -EINVAL;
 
 	switch (params->action) {
 	case IEEE80211_AMPDU_RX_START:
-		ret = ath12k_dp_rx_ampdu_start(ar, params);
+		ret = ath12k_dp_rx_ampdu_start(ar, params, link_id);
 		break;
 	case IEEE80211_AMPDU_RX_STOP:
-		ret = ath12k_dp_rx_ampdu_stop(ar, params);
+		ret = ath12k_dp_rx_ampdu_stop(ar, params, link_id);
 		break;
 	case IEEE80211_AMPDU_TX_START:
 	case IEEE80211_AMPDU_TX_STOP_CONT:
@@ -8315,6 +8326,10 @@ static int ath12k_mac_ampdu_action(struct ath12k_link_vif *arvif,
 		break;
 	}
 
+	if (ret)
+		ath12k_warn(ar->ab, "unable to perform ampdu action %d for vif %pM link %u ret %d\n",
+			    params->action, vif->addr, link_id, ret);
+
 	return ret;
 }
 
@@ -8322,27 +8337,24 @@ static int ath12k_mac_op_ampdu_action(struct ieee80211_hw *hw,
 				      struct ieee80211_vif *vif,
 				      struct ieee80211_ampdu_params *params)
 {
-	struct ath12k_hw *ah = ath12k_hw_to_ah(hw);
-	struct ath12k *ar;
-	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
-	struct ath12k_link_vif *arvif;
+	struct ieee80211_sta *sta = params->sta;
+	struct ath12k_sta *ahsta = ath12k_sta_to_ahsta(sta);
+	unsigned long links_map = ahsta->links_map;
 	int ret = -EINVAL;
+	u8 link_id;
 
 	lockdep_assert_wiphy(hw->wiphy);
 
-	ar = ath12k_get_ar_by_vif(hw, vif);
-	if (!ar)
-		return -EINVAL;
+	if (WARN_ON(!links_map))
+		return ret;
 
-	ar = ath12k_ah_to_ar(ah, 0);
-	arvif = &ahvif->deflink;
+	for_each_set_bit(link_id, &links_map, IEEE80211_MLD_MAX_NUM_LINKS) {
+		ret = ath12k_mac_ampdu_action(hw, vif, params, link_id);
+		if (ret)
+			return ret;
+	}
 
-	ret = ath12k_mac_ampdu_action(arvif, params);
-	if (ret)
-		ath12k_warn(ar->ab, "pdev idx %d unable to perform ampdu action %d ret %d\n",
-			    ar->pdev_idx, params->action, ret);
-
-	return ret;
+	return 0;
 }
 
 static int ath12k_mac_op_add_chanctx(struct ieee80211_hw *hw,
-- 
2.39.5

[PATCH 05/10] wifi: ath12k: ath12k_mac_station_add(): fix potential rx_stats leak

[Kalle Valo]

From: Rameshkumar Sundaram <quic_ramess@quicinc.com>

If peer creation fails ar->rx_stats needs to be freed in error handling.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/mac.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 428415237831..8287c2e6b765 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -5308,6 +5308,8 @@ static int ath12k_mac_station_add(struct ath12k *ar,
 
 free_peer:
 	ath12k_peer_delete(ar, arvif->vdev_id, arsta->addr);
+	kfree(arsta->rx_stats);
+	arsta->rx_stats = NULL;
 dec_num_station:
 	ath12k_mac_dec_num_stations(arvif, arsta);
 exit:
-- 
2.39.5

[PATCH 06/10] wifi: ath12k: do not return invalid link id for scan link

[Kalle Valo]

From: Sriram R <quic_srirrama@quicinc.com>

When a scan request is received, driver selects a link id for which the arvif
can be mapped. Same link is also used for getting the link conf address.
Currently, we return 0 as link id for a non ML vif, which is correct since that
is the default link id. Also when any of the link vif is active and the scan
request is for a channel in the active link we return its link id. But, when we
don't hit both of the above cases (i.e not a ML vif or no active link vif for
the channel is present) we currently return 0 as the link id.

Bu the problemis that this might not work out always, eg., when only one link
(eg. linkid = 1) is added to vif, then we won't find any link conf for link id
0 in the vif resulting in scan failure. During AP bringup, such scan failure
causes bringup issues.  Hence avoid sending link id 0 as default. Rather use a
default link for scan and default link address for the same. This scan vdev
will either be deleted if another scan is requested on same vif or when AP is
broughtup on same link or during interface cleanup.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Sriram R <quic_srirrama@quicinc.com>
Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/core.h |  3 +-
 drivers/net/wireless/ath/ath12k/mac.c  | 65 +++++++++++++++++++-------
 drivers/net/wireless/ath/ath12k/mac.h  |  6 +++
 3 files changed, 56 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index e246e3d3c162..f4a710d49584 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -322,10 +322,11 @@ struct ath12k_vif {
 	bool ps;
 
 	struct ath12k_link_vif deflink;
-	struct ath12k_link_vif __rcu *link[IEEE80211_MLD_MAX_NUM_LINKS];
+	struct ath12k_link_vif __rcu *link[ATH12K_NUM_MAX_LINKS];
 	struct ath12k_vif_cache *cache[IEEE80211_MLD_MAX_NUM_LINKS];
 	/* indicates bitmap of link vif created in FW */
 	u16 links_map;
+	u8 last_scan_link;
 
 	/* Must be last - ends in a flexible-array member.
 	 *
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 8287c2e6b765..85cfbe1e4b9f 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -3792,6 +3792,9 @@ static void ath12k_ahvif_put_link_key_cache(struct ath12k_vif_cache *cache)
 
 static void ath12k_ahvif_put_link_cache(struct ath12k_vif *ahvif, u8 link_id)
 {
+	if (link_id >= IEEE80211_MLD_MAX_NUM_LINKS)
+		return;
+
 	ath12k_ahvif_put_link_key_cache(ahvif->cache[link_id]);
 	kfree(ahvif->cache[link_id]);
 	ahvif->cache[link_id] = NULL;
@@ -3852,9 +3855,9 @@ static struct ath12k_link_vif *ath12k_mac_assign_link_vif(struct ath12k_hw *ah,
 		arvif = &ahvif->deflink;
 	} else {
 		/* If this is the first link arvif being created for an ML VIF
-		 * use the preallocated deflink memory
+		 * use the preallocated deflink memory except for scan arvifs
 		 */
-		if (!ahvif->links_map) {
+		if (!ahvif->links_map && link_id != ATH12K_DEFAULT_SCAN_LINK) {
 			arvif = &ahvif->deflink;
 		} else {
 			arvif = (struct ath12k_link_vif *)
@@ -4154,10 +4157,10 @@ ath12k_mac_find_link_id_by_ar(struct ath12k_vif *ahvif, struct ath12k *ar)
 			return link_id;
 	}
 
-	/* input ar is not assigned to any of the links, use link id
-	 * 0 for scan vdev creation.
+	/* input ar is not assigned to any of the links of ML VIF, use scan
+	 * link (15) for scan vdev creation.
 	 */
-	return 0;
+	return ATH12K_DEFAULT_SCAN_LINK;
 }
 
 static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw,
@@ -4188,7 +4191,7 @@ static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw,
 
 	/* check if any of the links of ML VIF is already started on
 	 * radio(ar) correpsondig to given scan frequency and use it,
-	 * if not use deflink(link 0) for scan purpose.
+	 * if not use scan link (link 15) for scan purpose.
 	 */
 	link_id = ath12k_mac_find_link_id_by_ar(ahvif, ar);
 	arvif = ath12k_mac_assign_link_vif(ah, vif, link_id);
@@ -4298,6 +4301,13 @@ static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw,
 		spin_unlock_bh(&ar->data_lock);
 	}
 
+	/* As per cfg80211/mac80211 scan design, it allows only one
+	 * scan at a time. Hence last_scan link id is used for
+	 * tracking the link id on which the scan is been done on
+	 * this vif.
+	 */
+	ahvif->last_scan_link = arvif->link_id;
+
 	/* Add a margin to account for event/command processing */
 	ieee80211_queue_delayed_work(ath12k_ar_to_hw(ar), &ar->scan.timeout,
 				     msecs_to_jiffies(arg->max_scan_time +
@@ -4317,14 +4327,14 @@ static void ath12k_mac_op_cancel_hw_scan(struct ieee80211_hw *hw,
 					 struct ieee80211_vif *vif)
 {
 	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
+	u16 link_id = ahvif->last_scan_link;
 	struct ath12k_link_vif *arvif;
 	struct ath12k *ar;
 
 	lockdep_assert_wiphy(hw->wiphy);
 
-	arvif = &ahvif->deflink;
-
-	if (!arvif->is_created)
+	arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]);
+	if (!arvif || arvif->is_created)
 		return;
 
 	ar = arvif->ar;
@@ -7688,10 +7698,19 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)
 	u16 nss;
 	int i;
 	int ret, vdev_id;
+	u8 link_id;
 
 	lockdep_assert_wiphy(hw->wiphy);
 
-	link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[arvif->link_id]);
+	/* If no link is active and scan vdev is requested
+	 * use a default link conf for scan address purpose.
+	 */
+	if (arvif->link_id == ATH12K_DEFAULT_SCAN_LINK && vif->valid_links)
+		link_id = ffs(vif->valid_links) - 1;
+	else
+		link_id = arvif->link_id;
+
+	link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[link_id]);
 	if (!link_conf) {
 		ath12k_warn(ar->ab, "unable to access bss link conf in vdev create for vif %pM link %u\n",
 			    vif->addr, arvif->link_id);
@@ -7971,7 +7990,9 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
 						    struct ath12k_link_vif *arvif,
 						    struct ieee80211_chanctx_conf *ctx)
 {
-	struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
+	struct ath12k_vif *ahvif = arvif->ahvif;
+	struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif);
+	struct ath12k_link_vif *scan_arvif;
 	struct ath12k_hw *ah = hw->priv;
 	struct ath12k *ar;
 	struct ath12k_base *ab;
@@ -7990,6 +8011,19 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
 	if (!ar)
 		return NULL;
 
+	/* cleanup the scan vdev if we are done scan on that ar
+	 * and now we want to create for actual usage.
+	 */
+	if (vif->valid_links) {
+		scan_arvif = wiphy_dereference(hw->wiphy,
+					       ahvif->link[ATH12K_DEFAULT_SCAN_LINK]);
+		if (scan_arvif && scan_arvif->ar == ar) {
+			ar->scan.vdev_id = -1;
+			ath12k_mac_remove_link_interface(hw, scan_arvif);
+			ath12k_mac_unassign_link_vif(scan_arvif);
+		}
+	}
+
 	if (arvif->ar) {
 		/* This is not expected really */
 		if (WARN_ON(!arvif->is_created)) {
@@ -8194,7 +8228,7 @@ static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw,
 
 	lockdep_assert_wiphy(hw->wiphy);
 
-	for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) {
+	for (link_id = 0; link_id < ATH12K_NUM_MAX_LINKS; link_id++) {
 		/* if we cached some config but never received assign chanctx,
 		 * free the allocated cache.
 		 */
@@ -9042,11 +9076,8 @@ ath12k_mac_op_assign_vif_chanctx(struct ieee80211_hw *hw,
 		return -ENOMEM;
 	}
 
-	if (!arvif->is_started) {
-		ar = ath12k_mac_assign_vif_to_vdev(hw, arvif, ctx);
-		if (!ar)
-			return -EINVAL;
-	} else {
+	ar = ath12k_mac_assign_vif_to_vdev(hw, arvif, ctx);
+	if (!ar) {
 		ath12k_warn(arvif->ar->ab, "failed to assign chanctx for vif %pM link id %u link vif is already started",
 			    vif->addr, link_id);
 		return -EINVAL;
diff --git a/drivers/net/wireless/ath/ath12k/mac.h b/drivers/net/wireless/ath/ath12k/mac.h
index c13630ee479a..abdc9a6c0740 100644
--- a/drivers/net/wireless/ath/ath12k/mac.h
+++ b/drivers/net/wireless/ath/ath12k/mac.h
@@ -44,6 +44,12 @@ struct ath12k_generic_iter {
 #define ATH12K_DEFAULT_LINK_ID	0
 #define ATH12K_INVALID_LINK_ID	255
 
+/* Default link after the IEEE802.11 defined Max link id limit
+ * for driver usage purpose.
+ */
+#define ATH12K_DEFAULT_SCAN_LINK	IEEE80211_MLD_MAX_NUM_LINKS
+#define ATH12K_NUM_MAX_LINKS		(IEEE80211_MLD_MAX_NUM_LINKS + 1)
+
 enum ath12k_supported_bw {
 	ATH12K_BW_20    = 0,
 	ATH12K_BW_40    = 1,
-- 
2.39.5

Re: [PATCH 06/10] wifi: ath12k: do not return invalid link id for scan link

[Baochen Qiang]

On 11/27/2024 1:11 AM, Kalle Valo wrote:
> From: Sriram R <quic_srirrama@quicinc.com>
> 
> When a scan request is received, driver selects a link id for which the arvif
> can be mapped. Same link is also used for getting the link conf address.
> Currently, we return 0 as link id for a non ML vif, which is correct since that
> is the default link id. Also when any of the link vif is active and the scan
> request is for a channel in the active link we return its link id. But, when we
> don't hit both of the above cases (i.e not a ML vif or no active link vif for
> the channel is present) we currently return 0 as the link id.
> 
> Bu the problemis that this might not work out always, eg., when only one link
> (eg. linkid = 1) is added to vif, then we won't find any link conf for link id
> 0 in the vif resulting in scan failure. During AP bringup, such scan failure
> causes bringup issues.  Hence avoid sending link id 0 as default. Rather use a
> default link for scan and default link address for the same. This scan vdev
> will either be deleted if another scan is requested on same vif or when AP is
> broughtup on same link or during interface cleanup.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> 
> Signed-off-by: Sriram R <quic_srirrama@quicinc.com>
> Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath12k/core.h |  3 +-
>  drivers/net/wireless/ath/ath12k/mac.c  | 65 +++++++++++++++++++-------
>  drivers/net/wireless/ath/ath12k/mac.h  |  6 +++
>  3 files changed, 56 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index e246e3d3c162..f4a710d49584 100644
> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -322,10 +322,11 @@ struct ath12k_vif {
>  	bool ps;
>  
>  	struct ath12k_link_vif deflink;
> -	struct ath12k_link_vif __rcu *link[IEEE80211_MLD_MAX_NUM_LINKS];
> +	struct ath12k_link_vif __rcu *link[ATH12K_NUM_MAX_LINKS];
>  	struct ath12k_vif_cache *cache[IEEE80211_MLD_MAX_NUM_LINKS];
>  	/* indicates bitmap of link vif created in FW */
>  	u16 links_map;
> +	u8 last_scan_link;
>  
>  	/* Must be last - ends in a flexible-array member.
>  	 *
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index 8287c2e6b765..85cfbe1e4b9f 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -3792,6 +3792,9 @@ static void ath12k_ahvif_put_link_key_cache(struct ath12k_vif_cache *cache)
>  
>  static void ath12k_ahvif_put_link_cache(struct ath12k_vif *ahvif, u8 link_id)
>  {
> +	if (link_id >= IEEE80211_MLD_MAX_NUM_LINKS)
> +		return;
> +
>  	ath12k_ahvif_put_link_key_cache(ahvif->cache[link_id]);
>  	kfree(ahvif->cache[link_id]);
>  	ahvif->cache[link_id] = NULL;
> @@ -3852,9 +3855,9 @@ static struct ath12k_link_vif *ath12k_mac_assign_link_vif(struct ath12k_hw *ah,
>  		arvif = &ahvif->deflink;
>  	} else {
>  		/* If this is the first link arvif being created for an ML VIF
> -		 * use the preallocated deflink memory
> +		 * use the preallocated deflink memory except for scan arvifs
>  		 */
> -		if (!ahvif->links_map) {
> +		if (!ahvif->links_map && link_id != ATH12K_DEFAULT_SCAN_LINK) {
>  			arvif = &ahvif->deflink;
>  		} else {
>  			arvif = (struct ath12k_link_vif *)
> @@ -4154,10 +4157,10 @@ ath12k_mac_find_link_id_by_ar(struct ath12k_vif *ahvif, struct ath12k *ar)
>  			return link_id;
>  	}
>  
> -	/* input ar is not assigned to any of the links, use link id
> -	 * 0 for scan vdev creation.
> +	/* input ar is not assigned to any of the links of ML VIF, use scan
> +	 * link (15) for scan vdev creation.
>  	 */
> -	return 0;
> +	return ATH12K_DEFAULT_SCAN_LINK;
>  }
>  
>  static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw,
> @@ -4188,7 +4191,7 @@ static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw,
>  
>  	/* check if any of the links of ML VIF is already started on
>  	 * radio(ar) correpsondig to given scan frequency and use it,
> -	 * if not use deflink(link 0) for scan purpose.
> +	 * if not use scan link (link 15) for scan purpose.
>  	 */
>  	link_id = ath12k_mac_find_link_id_by_ar(ahvif, ar);
>  	arvif = ath12k_mac_assign_link_vif(ah, vif, link_id);
> @@ -4298,6 +4301,13 @@ static int ath12k_mac_op_hw_scan(struct ieee80211_hw *hw,
>  		spin_unlock_bh(&ar->data_lock);
>  	}
>  
> +	/* As per cfg80211/mac80211 scan design, it allows only one
> +	 * scan at a time. Hence last_scan link id is used for
> +	 * tracking the link id on which the scan is been done on
> +	 * this vif.
> +	 */
> +	ahvif->last_scan_link = arvif->link_id;
> +
>  	/* Add a margin to account for event/command processing */
>  	ieee80211_queue_delayed_work(ath12k_ar_to_hw(ar), &ar->scan.timeout,
>  				     msecs_to_jiffies(arg->max_scan_time +
> @@ -4317,14 +4327,14 @@ static void ath12k_mac_op_cancel_hw_scan(struct ieee80211_hw *hw,
>  					 struct ieee80211_vif *vif)
>  {
>  	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
> +	u16 link_id = ahvif->last_scan_link;
>  	struct ath12k_link_vif *arvif;
>  	struct ath12k *ar;
>  
>  	lockdep_assert_wiphy(hw->wiphy);
>  
> -	arvif = &ahvif->deflink;
> -
> -	if (!arvif->is_created)
> +	arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]);
> +	if (!arvif || arvif->is_created)
s/arvif->is_created/!arvif->is_created/ ?

>  		return;
>  
>  	ar = arvif->ar;
> @@ -7688,10 +7698,19 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)
>  	u16 nss;
>  	int i;
>  	int ret, vdev_id;
> +	u8 link_id;
>  
>  	lockdep_assert_wiphy(hw->wiphy);
>  
> -	link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[arvif->link_id]);
> +	/* If no link is active and scan vdev is requested
> +	 * use a default link conf for scan address purpose.
> +	 */
> +	if (arvif->link_id == ATH12K_DEFAULT_SCAN_LINK && vif->valid_links)
> +		link_id = ffs(vif->valid_links) - 1;
> +	else
> +		link_id = arvif->link_id;
> +
> +	link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[link_id]);
>  	if (!link_conf) {
>  		ath12k_warn(ar->ab, "unable to access bss link conf in vdev create for vif %pM link %u\n",
>  			    vif->addr, arvif->link_id);
> @@ -7971,7 +7990,9 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
>  						    struct ath12k_link_vif *arvif,
>  						    struct ieee80211_chanctx_conf *ctx)
>  {
> -	struct ieee80211_vif *vif = ath12k_ahvif_to_vif(arvif->ahvif);
> +	struct ath12k_vif *ahvif = arvif->ahvif;
> +	struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif);
> +	struct ath12k_link_vif *scan_arvif;
>  	struct ath12k_hw *ah = hw->priv;
>  	struct ath12k *ar;
>  	struct ath12k_base *ab;
> @@ -7990,6 +8011,19 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
>  	if (!ar)
>  		return NULL;
>  
> +	/* cleanup the scan vdev if we are done scan on that ar
> +	 * and now we want to create for actual usage.
> +	 */
> +	if (vif->valid_links) {
better to use ieee80211_vif_is_mld()?

> +		scan_arvif = wiphy_dereference(hw->wiphy,
> +					       ahvif->link[ATH12K_DEFAULT_SCAN_LINK]);
> +		if (scan_arvif && scan_arvif->ar == ar) {
> +			ar->scan.vdev_id = -1;
> +			ath12k_mac_remove_link_interface(hw, scan_arvif);
> +			ath12k_mac_unassign_link_vif(scan_arvif);
> +		}
> +	}
> +
>  	if (arvif->ar) {
>  		/* This is not expected really */
>  		if (WARN_ON(!arvif->is_created)) {
> @@ -8194,7 +8228,7 @@ static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw,
>  
>  	lockdep_assert_wiphy(hw->wiphy);
>  
> -	for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) {
> +	for (link_id = 0; link_id < ATH12K_NUM_MAX_LINKS; link_id++) {
>  		/* if we cached some config but never received assign chanctx,
>  		 * free the allocated cache.
>  		 */
> @@ -9042,11 +9076,8 @@ ath12k_mac_op_assign_vif_chanctx(struct ieee80211_hw *hw,
>  		return -ENOMEM;
>  	}
>  
> -	if (!arvif->is_started) {
> -		ar = ath12k_mac_assign_vif_to_vdev(hw, arvif, ctx);
> -		if (!ar)
> -			return -EINVAL;
> -	} else {
> +	ar = ath12k_mac_assign_vif_to_vdev(hw, arvif, ctx);
> +	if (!ar) {
>  		ath12k_warn(arvif->ar->ab, "failed to assign chanctx for vif %pM link id %u link vif is already started",
>  			    vif->addr, link_id);
>  		return -EINVAL;
> diff --git a/drivers/net/wireless/ath/ath12k/mac.h b/drivers/net/wireless/ath/ath12k/mac.h
> index c13630ee479a..abdc9a6c0740 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.h
> +++ b/drivers/net/wireless/ath/ath12k/mac.h
> @@ -44,6 +44,12 @@ struct ath12k_generic_iter {
>  #define ATH12K_DEFAULT_LINK_ID	0
>  #define ATH12K_INVALID_LINK_ID	255
>  
> +/* Default link after the IEEE802.11 defined Max link id limit
> + * for driver usage purpose.
> + */
> +#define ATH12K_DEFAULT_SCAN_LINK	IEEE80211_MLD_MAX_NUM_LINKS
> +#define ATH12K_NUM_MAX_LINKS		(IEEE80211_MLD_MAX_NUM_LINKS + 1)
> +
>  enum ath12k_supported_bw {
>  	ATH12K_BW_20    = 0,
>  	ATH12K_BW_40    = 1,

Re: [PATCH 06/10] wifi: ath12k: do not return invalid link id for scan link

[Kalle Valo]

Baochen Qiang <quic_bqiang@quicinc.com> writes:

> On 11/27/2024 1:11 AM, Kalle Valo wrote:
>> From: Sriram R <quic_srirrama@quicinc.com>
>> 
>> @@ -4317,14 +4327,14 @@ static void ath12k_mac_op_cancel_hw_scan(struct ieee80211_hw *hw,
>>  					 struct ieee80211_vif *vif)
>>  {
>>  	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
>> +	u16 link_id = ahvif->last_scan_link;
>>  	struct ath12k_link_vif *arvif;
>>  	struct ath12k *ar;
>>  
>>  	lockdep_assert_wiphy(hw->wiphy);
>>  
>> -	arvif = &ahvif->deflink;
>> -
>> -	if (!arvif->is_created)
>> +	arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]);
>> +	if (!arvif || arvif->is_created)
>
> s/arvif->is_created/!arvif->is_created/ ?

Another good catch! Fixed now.

>> @@ -7990,6 +8011,19 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
>>  	if (!ar)
>>  		return NULL;
>>  
>> +	/* cleanup the scan vdev if we are done scan on that ar
>> +	 * and now we want to create for actual usage.
>> +	 */
>> +	if (vif->valid_links) {
>
> better to use ieee80211_vif_is_mld()?

Yup, fixed in the pending branch:

https://git.kernel.org/pub/scm/linux/kernel/git/ath/ath.git/commit/?h=pending&id=54504518cb26fef3dbaf16457cde91a9fd7e9c3d

Thanks for the detailed review, very much appreciated.

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

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

Re: [PATCH 06/10] wifi: ath12k: do not return invalid link id for scan link

[Baochen Qiang]

On 11/28/2024 8:34 PM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang@quicinc.com> writes:
> 
>> On 11/27/2024 1:11 AM, Kalle Valo wrote:
>>> From: Sriram R <quic_srirrama@quicinc.com>
>>>
>>> @@ -4317,14 +4327,14 @@ static void ath12k_mac_op_cancel_hw_scan(struct ieee80211_hw *hw,
>>>  					 struct ieee80211_vif *vif)
>>>  {
>>>  	struct ath12k_vif *ahvif = ath12k_vif_to_ahvif(vif);
>>> +	u16 link_id = ahvif->last_scan_link;
>>>  	struct ath12k_link_vif *arvif;
>>>  	struct ath12k *ar;
>>>  
>>>  	lockdep_assert_wiphy(hw->wiphy);
>>>  
>>> -	arvif = &ahvif->deflink;
>>> -
>>> -	if (!arvif->is_created)
>>> +	arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]);
>>> +	if (!arvif || arvif->is_created)
>>
>> s/arvif->is_created/!arvif->is_created/ ?
> 
> Another good catch! Fixed now.
> 
>>> @@ -7990,6 +8011,19 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
>>>  	if (!ar)
>>>  		return NULL;
>>>  
>>> +	/* cleanup the scan vdev if we are done scan on that ar
>>> +	 * and now we want to create for actual usage.
>>> +	 */
>>> +	if (vif->valid_links) {
>>
>> better to use ieee80211_vif_is_mld()?
> 
> Yup, fixed in the pending branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ath/ath.git/commit/?h=pending&id=54504518cb26fef3dbaf16457cde91a9fd7e9c3d

LGTM

> 
> Thanks for the detailed review, very much appreciated.
> 

RE: [PATCH 06/10] wifi: ath12k: do not return invalid link id for scan link

[Ping-Ke Shih]

Kalle Valo <kvalo@kernel.org> wrote:
> @@ -7688,10 +7698,19 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)
>         u16 nss;
>         int i;
>         int ret, vdev_id;
> +       u8 link_id;
> 
>         lockdep_assert_wiphy(hw->wiphy);
> 
> -       link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[arvif->link_id]);
> +       /* If no link is active and scan vdev is requested
> +        * use a default link conf for scan address purpose.
> +        */
> +       if (arvif->link_id == ATH12K_DEFAULT_SCAN_LINK && vif->valid_links)
> +               link_id = ffs(vif->valid_links) - 1;

link_id = __ffs(vif->valid_links);

since it checked vif->valid_links against 0 first.


> +       else
> +               link_id = arvif->link_id;
> +
> +       link_conf = wiphy_dereference(hw->wiphy, vif->link_conf[link_id]);
>         if (!link_conf) {
>                 ath12k_warn(ar->ab, "unable to access bss link conf in vdev create for vif %pM link %u\n",
>                             vif->addr, arvif->link_id);

[PATCH 07/10] wifi: ath12k: ath12k_bss_assoc(): MLO support

[Kalle Valo]

From: Aditya Kumar Singh <quic_adisi@quicinc.com>

Currently, the ath12k_bss_assoc() function handles only deflink station
connections. To support multi-link station connections, make the necessary
changes to retrieve the required information from the link-level members.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/mac.c | 28 +++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 85cfbe1e4b9f..364e9c6adc73 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -3133,7 +3133,9 @@ static void ath12k_bss_assoc(struct ath12k *ar,
 	struct ath12k_vif *ahvif = arvif->ahvif;
 	struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif);
 	struct ath12k_wmi_vdev_up_params params = {};
-	struct ath12k_wmi_peer_assoc_arg peer_arg;
+	struct ath12k_wmi_peer_assoc_arg peer_arg = {};
+	struct ieee80211_link_sta *link_sta;
+	u8 link_id = bss_conf->link_id;
 	struct ath12k_link_sta *arsta;
 	struct ieee80211_sta *ap_sta;
 	struct ath12k_sta *ahsta;
@@ -3143,27 +3145,38 @@ static void ath12k_bss_assoc(struct ath12k *ar,
 
 	lockdep_assert_wiphy(ath12k_ar_to_hw(ar)->wiphy);
 
-	ath12k_dbg(ar->ab, ATH12K_DBG_MAC, "mac vdev %i assoc bssid %pM aid %d\n",
-		   arvif->vdev_id, arvif->bssid, ahvif->aid);
+	ath12k_dbg(ar->ab, ATH12K_DBG_MAC,
+		   "mac vdev %i link id %u assoc bssid %pM aid %d\n",
+		   arvif->vdev_id, link_id, arvif->bssid, ahvif->aid);
 
 	rcu_read_lock();
 
-	ap_sta = ieee80211_find_sta(vif, bss_conf->bssid);
+	/* During ML connection, cfg.ap_addr has the MLD address. For
+	 * non-ML connection, it has the BSSID.
+	 */
+	ap_sta = ieee80211_find_sta(vif, vif->cfg.ap_addr);
 	if (!ap_sta) {
 		ath12k_warn(ar->ab, "failed to find station entry for bss %pM vdev %i\n",
-			    bss_conf->bssid, arvif->vdev_id);
+			    vif->cfg.ap_addr, arvif->vdev_id);
 		rcu_read_unlock();
 		return;
 	}
 
 	ahsta = ath12k_sta_to_ahsta(ap_sta);
-	arsta = &ahsta->deflink;
 
+	arsta = wiphy_dereference(ath12k_ar_to_hw(ar)->wiphy,
+				  ahsta->link[link_id]);
 	if (WARN_ON(!arsta)) {
 		rcu_read_unlock();
 		return;
 	}
 
+	link_sta = ath12k_mac_get_link_sta(arsta);
+	if (WARN_ON(!link_sta)) {
+		rcu_read_unlock();
+		return;
+	}
+
 	ath12k_peer_assoc_prepare(ar, arvif, arsta, &peer_arg, false);
 
 	rcu_read_unlock();
@@ -3182,8 +3195,7 @@ static void ath12k_bss_assoc(struct ath12k *ar,
 	}
 
 	ret = ath12k_setup_peer_smps(ar, arvif, bss_conf->bssid,
-				     &ap_sta->deflink.ht_cap,
-				     &ap_sta->deflink.he_6ghz_capa);
+				     &link_sta->ht_cap, &link_sta->he_6ghz_capa);
 	if (ret) {
 		ath12k_warn(ar->ab, "failed to setup peer SMPS for vdev %d: %d\n",
 			    arvif->vdev_id, ret);
-- 
2.39.5

[PATCH 08/10] wifi: ath12k: defer vdev creation for MLO

[Kalle Valo]

From: Rameshkumar Sundaram <quic_ramess@quicinc.com>

Currently for single radio devices (ah->num_radio == 1)
ath12k_mac_op_add_interface() creates vdev and later hw scan and
assign_vif_chanctx uses the same. For MLO, vdev create request should carry ML
address which will not be known during ath12k_mac_op_add_interface() as vif
will be marked as ML only after links are added to it.

If hw scan is requested, the vdev will be deleted post hw scan and subsequent
assign_vif_chanctx call will create new vdev with ML address. But in certain
cases assign_vif_chanctx could be called without any prior hw scan request and
reusing the previously created vdev causes a non-ML vdev to be used for an ML
vif and firmware operates the vdev in non-ML mode.

Fix this by deferring vdev creation for interface until hw scan or
assign_vif_chanctx request is received from mac80211.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/mac.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 364e9c6adc73..f85661534d9e 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -8131,14 +8131,9 @@ static int ath12k_mac_op_add_interface(struct ieee80211_hw *hw,
 		vif->hw_queue[i] = ATH12K_HW_DEFAULT_QUEUE;
 
 	vif->driver_flags |= IEEE80211_VIF_SUPPORTS_UAPSD;
-	/* For non-ml vifs, vif->addr is the actual vdev address but for
-	 * ML vif link(link BSSID) address is the vdev address and it can be a
-	 * different one from vif->addr (i.e ML address).
-	 * Defer vdev creation until assign_chanctx or hw_scan is initiated as driver
+	/* Defer vdev creation until assign_chanctx or hw_scan is initiated as driver
 	 * will not know if this interface is an ML vif at this point.
 	 */
-	ath12k_mac_assign_vif_to_vdev(hw, arvif, NULL);
-
 	return 0;
 }
 
-- 
2.39.5

[PATCH 09/10] wifi: ath12k: ath12k_mac_op_set_key(): fix uninitialized symbol 'ret'

[Kalle Valo]

From: Kalle Valo <quic_kvalo@quicinc.com>

Dan reported that in some cases the ret variable could be uninitialized. Fix
that by removing the out label entirely and returning zero explicitly on
succesful cases.

Also remove the unnecessary else branches to follow more the style used in
ath12k and now it's easier to see the error handling.

No functional changes.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/7e7afd00-ad84-4744-8d94-416bab7e7dd9@stanley.mountain/
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/mac.c | 59 +++++++++++++++------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index f85661534d9e..f6c3128a675a 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -4657,6 +4657,7 @@ static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
 
 	if (sta) {
 		ahsta = ath12k_sta_to_ahsta(sta);
+
 		/* For an ML STA Pairwise key is same for all associated link Stations,
 		 * hence do set key for all link STAs which are active.
 		 */
@@ -4679,41 +4680,47 @@ static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
 				if (ret)
 					break;
 			}
-		} else {
-			arsta = &ahsta->deflink;
-			arvif = arsta->arvif;
-			if (WARN_ON(!arvif)) {
-				ret = -EINVAL;
-				goto out;
-			}
 
-			ret = ath12k_mac_set_key(arvif->ar, cmd, arvif, arsta, key);
-		}
-	} else {
-		if (key->link_id >= 0 && key->link_id < IEEE80211_MLD_MAX_NUM_LINKS) {
-			link_id = key->link_id;
-			arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]);
-		} else {
-			link_id = 0;
-			arvif = &ahvif->deflink;
+			return 0;
 		}
 
-		if (!arvif || !arvif->is_created) {
-			cache = ath12k_ahvif_get_link_cache(ahvif, link_id);
-			if (!cache)
-				return -ENOSPC;
-
-			ret = ath12k_mac_update_key_cache(cache, cmd, sta, key);
+		arsta = &ahsta->deflink;
+		arvif = arsta->arvif;
+		if (WARN_ON(!arvif))
+			return -EINVAL;
 
+		ret = ath12k_mac_set_key(arvif->ar, cmd, arvif, arsta, key);
+		if (ret)
 			return ret;
-		}
 
-		ret = ath12k_mac_set_key(arvif->ar, cmd, arvif, NULL, key);
+		return 0;
 	}
 
-out:
+	if (key->link_id >= 0 && key->link_id < IEEE80211_MLD_MAX_NUM_LINKS) {
+		link_id = key->link_id;
+		arvif = wiphy_dereference(hw->wiphy, ahvif->link[link_id]);
+	} else {
+		link_id = 0;
+		arvif = &ahvif->deflink;
+	}
 
-	return ret;
+	if (!arvif || !arvif->is_created) {
+		cache = ath12k_ahvif_get_link_cache(ahvif, link_id);
+		if (!cache)
+			return -ENOSPC;
+
+		ret = ath12k_mac_update_key_cache(cache, cmd, sta, key);
+		if (ret)
+			return ret;
+
+		return 0;
+	}
+
+	ret = ath12k_mac_set_key(arvif->ar, cmd, arvif, NULL, key);
+	if (ret)
+		return ret;
+
+	return 0;
 }
 
 static int
-- 
2.39.5

[PATCH 10/10] wifi: ath12k: ath12k_mac_op_sta_rc_update(): use mac80211 provided link id

[Kalle Valo]

From: Kalle Valo <quic_kvalo@quicinc.com>

There's a todo comment to use mac80211 provided link id. As mac80211 now
provides it use it in ath12k and remove the comment.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/mac.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index f6c3128a675a..164c974fb5f4 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -5696,10 +5696,10 @@ static int ath12k_mac_op_sta_set_txpwr(struct ieee80211_hw *hw,
 	return ret;
 }
 
-static void ath12k_mac_op_sta_rc_update(struct ieee80211_hw *hw,
-					struct ieee80211_vif *vif,
-					struct ieee80211_link_sta *link_sta,
-					u32 changed)
+static void ath12k_mac_op_link_sta_rc_update(struct ieee80211_hw *hw,
+					     struct ieee80211_vif *vif,
+					     struct ieee80211_link_sta *link_sta,
+					     u32 changed)
 {
 	struct ieee80211_sta *sta = link_sta->sta;
 	struct ath12k *ar;
@@ -5710,27 +5710,23 @@ static void ath12k_mac_op_sta_rc_update(struct ieee80211_hw *hw,
 	struct ath12k_link_vif *arvif;
 	struct ath12k_peer *peer;
 	u32 bw, smps;
-	/* TODO: use proper link id once link sta specific rc update support is
-	 * available in mac80211.
-	 */
-	u8 link_id = ATH12K_DEFAULT_LINK_ID;
 
 	rcu_read_lock();
-	arvif = rcu_dereference(ahvif->link[link_id]);
+	arvif = rcu_dereference(ahvif->link[link_sta->link_id]);
 	if (!arvif) {
 		ath12k_hw_warn(ah, "mac sta rc update failed to fetch link vif on link id %u for peer %pM\n",
-			       link_id, sta->addr);
+			       link_sta->link_id, sta->addr);
 		rcu_read_unlock();
 		return;
 	}
 
 	ar = arvif->ar;
 
-	arsta = rcu_dereference(ahsta->link[link_id]);
+	arsta = rcu_dereference(ahsta->link[link_sta->link_id]);
 	if (!arsta) {
 		rcu_read_unlock();
 		ath12k_warn(ar->ab, "mac sta rc update failed to fetch link sta on link id %u for peer %pM\n",
-			    link_id, sta->addr);
+			    link_sta->link_id, sta->addr);
 		return;
 	}
 	spin_lock_bh(&ar->ab->base_lock);
@@ -10165,7 +10161,7 @@ static const struct ieee80211_ops ath12k_ops = {
 	.set_rekey_data	                = ath12k_mac_op_set_rekey_data,
 	.sta_state                      = ath12k_mac_op_sta_state,
 	.sta_set_txpwr			= ath12k_mac_op_sta_set_txpwr,
-	.link_sta_rc_update		= ath12k_mac_op_sta_rc_update,
+	.link_sta_rc_update		= ath12k_mac_op_link_sta_rc_update,
 	.conf_tx                        = ath12k_mac_op_conf_tx,
 	.set_antenna			= ath12k_mac_op_set_antenna,
 	.get_antenna			= ath12k_mac_op_get_antenna,
-- 
2.39.5