diff mbox series

[4/4] wifi: ath12k: Refactor MAC un/register helper function

Message ID 20231206034920.1037449-5-quic_periyasa@quicinc.com (mailing list archive)
State Accepted
Commit d786c9f5fe3472ac102160c113a7bba8ec79a6c6
Delegated to: Kalle Valo
Headers show
Series wifi: ath12k: Refactor MAC alloc/destroy/un/register helper functions | expand

Commit Message

Karthikeyan Periyasamy Dec. 6, 2023, 3:49 a.m. UTC
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(-)

Comments

Jeff Johnson Dec. 8, 2023, 12:23 a.m. UTC | #1
On 12/5/2023 7:49 PM, Karthikeyan Periyasamy wrote:
> 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>
Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Kalle Valo Jan. 9, 2024, 1:25 p.m. UTC | #2
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?
Karthikeyan Periyasamy Jan. 9, 2024, 1:41 p.m. UTC | #3
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.
Kalle Valo Jan. 15, 2024, 3:27 p.m. UTC | #4
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?
Karthikeyan Periyasamy Jan. 16, 2024, 4:49 a.m. UTC | #5
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 mbox series

Patch

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);