| Message ID | 20231206034920.1037449-5-quic_periyasa@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: ath12k: Refactor MAC alloc/destroy/un/register helper functions | expand |
Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: > Currently, the mac80211 hw registration procedure is tightly coupled with > the handling of link/radio (ar). Define a new helper function to separate > the link/radio handling from the mac80211 hw registration procedure for > improved code readability. Also, it can be easy to scale these > functionality to support single/multi link operation in the future. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 > > Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> > --- > drivers/net/wireless/ath/ath12k/core.c | 61 +++++++++++- > drivers/net/wireless/ath/ath12k/mac.c | 132 ++++++++++--------------- > drivers/net/wireless/ath/ath12k/mac.h | 4 +- > 3 files changed, 109 insertions(+), 88 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c > index e10c5f2cd8eb..d1ac00c59b8c 100644 > --- a/drivers/net/wireless/ath/ath12k/core.c > +++ b/drivers/net/wireless/ath/ath12k/core.c > @@ -498,11 +498,62 @@ static void ath12k_core_soc_destroy(struct ath12k_base *ab) > ath12k_qmi_deinit_service(ab); > } > > +static int ath12k_core_mac_register(struct ath12k_base *ab) > +{ > + struct ath12k *ar; > + struct ath12k_pdev *pdev; > + int i; > + int ret; > + > + if (test_bit(ATH12K_FLAG_REGISTERED, &ab->dev_flags)) > + return 0; > + > + /* Initialize channel counters frequency value in hertz */ > + ab->cc_freq_hz = 320000; > + ab->free_vdev_map = (1LL << (ab->num_radios * TARGET_NUM_VDEVS)) - 1; > + > + for (i = 0; i < ab->num_radios; i++) { > + pdev = &ab->pdevs[i]; > + ar = pdev->ar; > + > + ret = ath12k_mac_hw_register(ar); > + if (ret) > + goto err_cleanup; > + } > + > + return 0; > + > +err_cleanup: > + for (i = i - 1; i >= 0; i--) { > + pdev = &ab->pdevs[i]; > + ar = pdev->ar; > + ath12k_mac_hw_unregister(ar); > + } > + > + return ret; > +} Is there a reason why you moved these two functions from mac.c to core.c? This is mac level code anyway, right?
On 1/9/2024 6:55 PM, Kalle Valo wrote: > Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: > >> Currently, the mac80211 hw registration procedure is tightly coupled with >> the handling of link/radio (ar). Define a new helper function to separate >> the link/radio handling from the mac80211 hw registration procedure for >> improved code readability. Also, it can be easy to scale these >> functionality to support single/multi link operation in the future. >> >> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 >> >> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> >> --- >> drivers/net/wireless/ath/ath12k/core.c | 61 +++++++++++- >> drivers/net/wireless/ath/ath12k/mac.c | 132 ++++++++++--------------- >> drivers/net/wireless/ath/ath12k/mac.h | 4 +- >> 3 files changed, 109 insertions(+), 88 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c >> index e10c5f2cd8eb..d1ac00c59b8c 100644 >> --- a/drivers/net/wireless/ath/ath12k/core.c >> +++ b/drivers/net/wireless/ath/ath12k/core.c >> @@ -498,11 +498,62 @@ static void ath12k_core_soc_destroy(struct ath12k_base *ab) >> ath12k_qmi_deinit_service(ab); >> } >> >> +static int ath12k_core_mac_register(struct ath12k_base *ab) >> +{ >> + struct ath12k *ar; >> + struct ath12k_pdev *pdev; >> + int i; >> + int ret; >> + >> + if (test_bit(ATH12K_FLAG_REGISTERED, &ab->dev_flags)) >> + return 0; >> + >> + /* Initialize channel counters frequency value in hertz */ >> + ab->cc_freq_hz = 320000; >> + ab->free_vdev_map = (1LL << (ab->num_radios * TARGET_NUM_VDEVS)) - 1; >> + >> + for (i = 0; i < ab->num_radios; i++) { >> + pdev = &ab->pdevs[i]; >> + ar = pdev->ar; >> + >> + ret = ath12k_mac_hw_register(ar); >> + if (ret) >> + goto err_cleanup; >> + } >> + >> + return 0; >> + >> +err_cleanup: >> + for (i = i - 1; i >= 0; i--) { >> + pdev = &ab->pdevs[i]; >> + ar = pdev->ar; >> + ath12k_mac_hw_unregister(ar); >> + } >> + >> + return ret; >> +} > Is there a reason why you moved these two functions from mac.c to > core.c? This is mac level code anyway, right? This is not fully mac level code, some parts of SoC/chip level code bindup in the mac level. So i segregated the SoC level code like ab related param initialization handling from the mac level procedure. Now, mac/radio should take care only radio level handling procedure. In future for MLO, SoC level can be extend easily. Thanks, Karthikeyan P.
Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: >> Is there a reason why you moved these two functions from mac.c to >> core.c? This is mac level code anyway, right? > > This is not fully mac level code, some parts of SoC/chip level code > bindup in the mac level. So i segregated the SoC level code like ab > related param initialization handling from the mac level procedure. > > Now, mac/radio should take care only radio level handling procedure. > > In future for MLO, SoC level can be extend easily. But is there a concrete reason to have the functions in core.c? In your following patches I couldn't see why to move these functions to core.c, everything seems to be suitable for mac.c. I experimented this in the pending branch and moved the functions to mac.c (tag ath-pending-202401151424): https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=bee89ac2b5755754849deaf7054828e982cc6658 I also fixed your other patchsets to match that and to me it makes more sense to have everything in mac.c. I prefer to make core.c as simple as possible. As you can see I also made changes to the patch titles to make them more understandable: wifi: ath12k: refactor ath12k_mac_register() and ath12k_mac_unregister() Thoughts?
On 1/15/2024 8:57 PM, Kalle Valo wrote: > Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: > >>> Is there a reason why you moved these two functions from mac.c to >>> core.c? This is mac level code anyway, right? >> This is not fully mac level code, some parts of SoC/chip level code >> bindup in the mac level. So i segregated the SoC level code like ab >> related param initialization handling from the mac level procedure. >> >> Now, mac/radio should take care only radio level handling procedure. >> >> In future for MLO, SoC level can be extend easily. > But is there a concrete reason to have the functions in core.c? In your > following patches I couldn't see why to move these functions to core.c, > everything seems to be suitable for mac.c. > > I experimented this in the pending branch and moved the functions to > mac.c (tag ath-pending-202401151424): > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=bee89ac2b5755754849deaf7054828e982cc6658 > > I also fixed your other patchsets to match that and to me it makes more > sense to have everything in mac.c. I prefer to make core.c as simple as possible. > > As you can see I also made changes to the patch titles to make them more > understandable: > > wifi: ath12k: refactor ath12k_mac_register() and ath12k_mac_unregister() > > Thoughts? Looks fine to me. Thanks, Karthikeyan
diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c index e10c5f2cd8eb..d1ac00c59b8c 100644 --- a/drivers/net/wireless/ath/ath12k/core.c +++ b/drivers/net/wireless/ath/ath12k/core.c @@ -498,11 +498,62 @@ static void ath12k_core_soc_destroy(struct ath12k_base *ab) ath12k_qmi_deinit_service(ab); } +static int ath12k_core_mac_register(struct ath12k_base *ab) +{ + struct ath12k *ar; + struct ath12k_pdev *pdev; + int i; + int ret; + + if (test_bit(ATH12K_FLAG_REGISTERED, &ab->dev_flags)) + return 0; + + /* Initialize channel counters frequency value in hertz */ + ab->cc_freq_hz = 320000; + ab->free_vdev_map = (1LL << (ab->num_radios * TARGET_NUM_VDEVS)) - 1; + + for (i = 0; i < ab->num_radios; i++) { + pdev = &ab->pdevs[i]; + ar = pdev->ar; + + ret = ath12k_mac_hw_register(ar); + if (ret) + goto err_cleanup; + } + + return 0; + +err_cleanup: + for (i = i - 1; i >= 0; i--) { + pdev = &ab->pdevs[i]; + ar = pdev->ar; + ath12k_mac_hw_unregister(ar); + } + + return ret; +} + +static void ath12k_core_mac_unregister(struct ath12k_base *ab) +{ + struct ath12k *ar; + struct ath12k_pdev *pdev; + int i; + + for (i = 0; i < ab->num_radios; i++) { + pdev = &ab->pdevs[i]; + ar = pdev->ar; + if (!ar) + continue; + + ath12k_mac_hw_unregister(ar); + } +} + static int ath12k_core_pdev_create(struct ath12k_base *ab) { int ret; - ret = ath12k_mac_register(ab); + ret = ath12k_core_mac_register(ab); if (ret) { ath12k_err(ab, "failed register the radio with mac80211: %d\n", ret); return ret; @@ -511,20 +562,20 @@ static int ath12k_core_pdev_create(struct ath12k_base *ab) ret = ath12k_dp_pdev_alloc(ab); if (ret) { ath12k_err(ab, "failed to attach DP pdev: %d\n", ret); - goto err_mac_unregister; + goto err_core_mac_unregister; } return 0; -err_mac_unregister: - ath12k_mac_unregister(ab); +err_core_mac_unregister: + ath12k_core_mac_unregister(ab); return ret; } static void ath12k_core_pdev_destroy(struct ath12k_base *ab) { - ath12k_mac_unregister(ab); + ath12k_core_mac_unregister(ab); ath12k_hif_irq_disable(ab); ath12k_dp_pdev_free(ab); } diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c index be3ef388e9aa..27f6067fd3fc 100644 --- a/drivers/net/wireless/ath/ath12k/mac.c +++ b/drivers/net/wireless/ath/ath12k/mac.c @@ -7349,7 +7349,17 @@ static const struct wiphy_iftype_ext_capab ath12k_iftypes_ext_capa[] = { }, }; -static void __ath12k_mac_unregister(struct ath12k *ar) +static void ath12k_mac_cleanup_unregister(struct ath12k *ar) +{ + idr_for_each(&ar->txmgmt_idr, ath12k_mac_tx_mgmt_pending_free, ar); + idr_destroy(&ar->txmgmt_idr); + + kfree(ar->mac.sbands[NL80211_BAND_2GHZ].channels); + kfree(ar->mac.sbands[NL80211_BAND_5GHZ].channels); + kfree(ar->mac.sbands[NL80211_BAND_6GHZ].channels); +} + +void ath12k_mac_hw_unregister(struct ath12k *ar) { struct ieee80211_hw *hw = ar->hw; struct wiphy *wiphy = hw->wiphy; @@ -7358,12 +7368,7 @@ static void __ath12k_mac_unregister(struct ath12k *ar) ieee80211_unregister_hw(hw); - idr_for_each(&ar->txmgmt_idr, ath12k_mac_tx_mgmt_pending_free, ar); - idr_destroy(&ar->txmgmt_idr); - - kfree(ar->mac.sbands[NL80211_BAND_2GHZ].channels); - kfree(ar->mac.sbands[NL80211_BAND_5GHZ].channels); - kfree(ar->mac.sbands[NL80211_BAND_6GHZ].channels); + ath12k_mac_cleanup_unregister(ar); kfree(wiphy->iface_combinations[0].limits); kfree(wiphy->iface_combinations); @@ -7371,28 +7376,41 @@ static void __ath12k_mac_unregister(struct ath12k *ar) SET_IEEE80211_DEV(hw, NULL); } -void ath12k_mac_unregister(struct ath12k_base *ab) +static int ath12k_mac_setup_register(struct ath12k *ar, + u32 *ht_cap, + struct ieee80211_supported_band *bands[]) { - struct ath12k *ar; - struct ath12k_pdev *pdev; - int i; + struct ath12k_pdev_cap *cap = &ar->pdev->cap; + int ret; - for (i = 0; i < ab->num_radios; i++) { - pdev = &ab->pdevs[i]; - ar = pdev->ar; - if (!ar) - continue; + init_waitqueue_head(&ar->txmgmt_empty_waitq); + idr_init(&ar->txmgmt_idr); + spin_lock_init(&ar->txmgmt_idr_lock); - __ath12k_mac_unregister(ar); - } + ath12k_pdev_caps_update(ar); + + ret = ath12k_mac_setup_channels_rates(ar, + cap->supported_bands, + bands); + if (ret) + return ret; + + ath12k_mac_setup_ht_vht_cap(ar, cap, ht_cap); + ath12k_mac_setup_sband_iftype_data(ar, cap); + + ar->max_num_stations = TARGET_NUM_STATIONS; + ar->max_num_peers = TARGET_NUM_PEERS_PDEV; + + return 0; } -static int __ath12k_mac_register(struct ath12k *ar) +int ath12k_mac_hw_register(struct ath12k *ar) { struct ath12k_base *ab = ar->ab; struct ieee80211_hw *hw = ar->hw; struct wiphy *wiphy = hw->wiphy; - struct ath12k_pdev_cap *cap = &ar->pdev->cap; + struct ath12k_pdev *pdev = ar->pdev; + struct ath12k_pdev_cap *cap = &pdev->cap; static const u32 cipher_suites[] = { WLAN_CIPHER_SUITE_TKIP, WLAN_CIPHER_SUITE_CCMP, @@ -7407,25 +7425,24 @@ static int __ath12k_mac_register(struct ath12k *ar) int ret; u32 ht_cap = 0; - ath12k_pdev_caps_update(ar); - - SET_IEEE80211_PERM_ADDR(hw, ar->mac_addr); - - SET_IEEE80211_DEV(hw, ab->dev); + if (ab->pdevs_macaddr_valid) { + ether_addr_copy(ar->mac_addr, pdev->mac_addr); + } else { + ether_addr_copy(ar->mac_addr, ab->mac_addr); + ar->mac_addr[4] += ar->pdev_idx; + } - ret = ath12k_mac_setup_channels_rates(ar, - cap->supported_bands, - hw->wiphy->bands); + ret = ath12k_mac_setup_register(ar, &ht_cap, hw->wiphy->bands); if (ret) - goto err; + goto out; - ath12k_mac_setup_ht_vht_cap(ar, cap, &ht_cap); - ath12k_mac_setup_sband_iftype_data(ar, cap); + SET_IEEE80211_PERM_ADDR(hw, ar->mac_addr); + SET_IEEE80211_DEV(hw, ab->dev); ret = ath12k_mac_setup_iface_combinations(ar); if (ret) { ath12k_err(ar->ab, "failed to setup interface combinations: %d\n", ret); - goto err_free_channels; + goto err_setup_unregister; } wiphy->available_antennas_rx = cap->rx_chain_mask; @@ -7484,9 +7501,6 @@ static int __ath12k_mac_register(struct ath12k *ar) wiphy->features |= NL80211_FEATURE_AP_MODE_CHAN_WIDTH_CHANGE | NL80211_FEATURE_AP_SCAN; - ar->max_num_stations = TARGET_NUM_STATIONS; - ar->max_num_peers = TARGET_NUM_PEERS_PDEV; - wiphy->max_ap_assoc_sta = ar->max_num_stations; hw->queues = ATH12K_HW_MAX_QUEUES; @@ -7553,58 +7567,14 @@ static int __ath12k_mac_register(struct ath12k *ar) kfree(wiphy->iface_combinations[0].limits); kfree(wiphy->iface_combinations); -err_free_channels: +err_setup_unregister: kfree(ar->mac.sbands[NL80211_BAND_2GHZ].channels); kfree(ar->mac.sbands[NL80211_BAND_5GHZ].channels); kfree(ar->mac.sbands[NL80211_BAND_6GHZ].channels); -err: SET_IEEE80211_DEV(hw, NULL); - return ret; -} - -int ath12k_mac_register(struct ath12k_base *ab) -{ - struct ath12k *ar; - struct ath12k_pdev *pdev; - int i; - int ret; - - if (test_bit(ATH12K_FLAG_REGISTERED, &ab->dev_flags)) - return 0; - - for (i = 0; i < ab->num_radios; i++) { - pdev = &ab->pdevs[i]; - ar = pdev->ar; - if (ab->pdevs_macaddr_valid) { - ether_addr_copy(ar->mac_addr, pdev->mac_addr); - } else { - ether_addr_copy(ar->mac_addr, ab->mac_addr); - ar->mac_addr[4] += i; - } - - ret = __ath12k_mac_register(ar); - if (ret) - goto err_cleanup; - - init_waitqueue_head(&ar->txmgmt_empty_waitq); - idr_init(&ar->txmgmt_idr); - spin_lock_init(&ar->txmgmt_idr_lock); - } - - /* Initialize channel counters frequency value in hertz */ - ab->cc_freq_hz = 320000; - ab->free_vdev_map = (1LL << (ab->num_radios * TARGET_NUM_VDEVS)) - 1; - - return 0; - -err_cleanup: - for (i = i - 1; i >= 0; i--) { - pdev = &ab->pdevs[i]; - ar = pdev->ar; - __ath12k_mac_unregister(ar); - } +out: return ret; } diff --git a/drivers/net/wireless/ath/ath12k/mac.h b/drivers/net/wireless/ath/ath12k/mac.h index fb4c0662581b..c30ebda77b0a 100644 --- a/drivers/net/wireless/ath/ath12k/mac.h +++ b/drivers/net/wireless/ath/ath12k/mac.h @@ -49,8 +49,8 @@ enum ath12k_supported_bw { extern const struct htt_rx_ring_tlv_filter ath12k_mac_mon_status_filter_default; void ath12k_mac_hw_destroy(struct ath12k_base *ab); -void ath12k_mac_unregister(struct ath12k_base *ab); -int ath12k_mac_register(struct ath12k_base *ab); +void ath12k_mac_hw_unregister(struct ath12k *ar); +int ath12k_mac_hw_register(struct ath12k *ar); int ath12k_mac_hw_allocate(struct ath12k_base *ab); int ath12k_mac_hw_ratecode_to_legacy_rate(u8 hw_rc, u8 preamble, u8 *rateidx, u16 *rate);
Currently, the mac80211 hw registration procedure is tightly coupled with the handling of link/radio (ar). Define a new helper function to separate the link/radio handling from the mac80211 hw registration procedure for improved code readability. Also, it can be easy to scale these functionality to support single/multi link operation in the future. Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> --- drivers/net/wireless/ath/ath12k/core.c | 61 +++++++++++- drivers/net/wireless/ath/ath12k/mac.c | 132 ++++++++++--------------- drivers/net/wireless/ath/ath12k/mac.h | 4 +- 3 files changed, 109 insertions(+), 88 deletions(-)