Baochen Qiang <quic_bqiang@xxxxxxxxxxx> writes:
> On 11/27/2024 1:11 AM, Kalle Valo wrote:
>> From: Kalle Valo <quic_kvalo@xxxxxxxxxxx>
>>
>> --- 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
