On 11/28/2024 8:08 PM, Kalle Valo wrote:
> 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.
Ah, good to know. thanks for sharing the plan.
>
