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

[Kalle Valo <kvalo@kernel.org>]

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