| Message ID | 20241126171139.2350704-1-kvalo@kernel.org |
|---|---|
| Headers | show |
| Series | wifi: ath12k: MLO support part 4 | expand |
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); > } >
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.
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. >
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