[PATCH 0/4] wifi: ath12k: Refactor MAC alloc/destroy/un/register helper functions

[PATCH 0/4] wifi: ath12k: Refactor MAC alloc/destroy/un/register helper functions

[Karthikeyan Periyasamy]

Currently, the driver does not support single/multi link operation in 11be
mode. Also, each link/radio gets registered as a separate wiphy through
mac80211. In order to support multi link operation (MLO), the current
separate wiphy registration approach brings a lot of complexity in cfg80211
and mac80211, such as synchronization across the multiple wiphy/hw.
Determining the compatibility of the hw across multiple wiphy/hw is another
challenge for userspace. To reduce these complexities in
userspace/cfg80211/mac80211, need to move from the multi wiphy model to a
single wiphy model. To support the single wiphy registration, we have to
decouple the wiphy/mac80211 hw data from the link/radio (ar) structure. So
refactor the MAC helper functions.

			Current Multi wiphy Model

+---------------+            +---------------+            +-------------+
|  Mac80211 hw  |            | Mac80211 hw   |            |Mac80211 hw  |
|  private data |            | private data  |            |private data |
|               |            |               |            |             |
|               |            |               |            |             |
|               |            |               |            |             |
|   ar (2GHz)   |            |   ar (5GHz)   |            |  ar (6GHz)  |
|               |            |               |            |             |
|               |            |               |            |             |
|               |            |               |            |             |
+---------------+            +---------------+            +-------------+




			  Single wiphy Model

                           +--------------+
                           | Mac80211 hw  |
                           | private data |
                           |              |
                           |ath12k hw (ah)|
                           | +----------+ |
                           | |ar (2GHz) | |
                           | +----------+ |
                           | |          | |
                           | |ar (5GHz) | |
                           | +----------+ |
                           | |          | |
                           | |ar (6GHz) | |
                           | |          | |
                           | +----------+ |
                           +--------------+

Karthikeyan Periyasamy (4):
  wifi: ath12k: Refactor the DP pdev pre alloc call sequence
  wifi: ath12k: Refactor the MAC allocation and destroy
  wifi: ath12k: Refactor MAC setup channel helper function
  wifi: ath12k: Refactor MAC un/register helper function

 drivers/net/wireless/ath/ath12k/core.c |  96 +++++++++--
 drivers/net/wireless/ath/ath12k/mac.c  | 216 +++++++++++--------------
 drivers/net/wireless/ath/ath12k/mac.h  |   8 +-
 3 files changed, 184 insertions(+), 136 deletions(-)


base-commit: 22d737065b8c4fbb29a3a818adcf88004ea7d5bb
-- 
2.34.1

[PATCH 1/4] wifi: ath12k: Refactor the DP pdev pre alloc call sequence

[Karthikeyan Periyasamy]

Currently, the data path pdev pre alloc and mac allocate are called
separately from the core start procedure. The data path pdev pre alloc
can be called from the mac allocate procedure itself since initialization
related to pdev happens in the mac allocate procedure. So move the caller
of DP pdev pre alloc from the core start procedure to the mac allocate
procedure. This change helps in the future to easily decouple the mac
allocate procedure from core start handling in order to support MLO in
multi chip.

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 | 2 --
 drivers/net/wireless/ath/ath12k/mac.c  | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
index 6c01b282fcd3..6f634b57dde8 100644
--- a/drivers/net/wireless/ath/ath12k/core.c
+++ b/drivers/net/wireless/ath/ath12k/core.c
@@ -592,8 +592,6 @@ static int ath12k_core_start(struct ath12k_base *ab,
 
 	ath12k_dp_cc_config(ab);
 
-	ath12k_dp_pdev_pre_alloc(ab);
-
 	ret = ath12k_dp_rx_pdev_reo_setup(ab);
 	if (ret) {
 		ath12k_err(ab, "failed to initialize reo destination rings: %d\n", ret);
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 88cec54c6c2e..49d56f5d8896 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -7667,6 +7667,8 @@ int ath12k_mac_allocate(struct ath12k_base *ab)
 		clear_bit(ATH12K_FLAG_MONITOR_ENABLED, &ar->monitor_flags);
 	}
 
+	ath12k_dp_pdev_pre_alloc(ab);
+
 	return 0;
 
 err_free_mac:
-- 
2.34.1

Re: [PATCH 1/4] wifi: ath12k: Refactor the DP pdev pre alloc call sequence

[Jeff Johnson]

On 12/5/2023 7:49 PM, Karthikeyan Periyasamy wrote:
> Currently, the data path pdev pre alloc and mac allocate are called
> separately from the core start procedure. The data path pdev pre alloc
> can be called from the mac allocate procedure itself since initialization
> related to pdev happens in the mac allocate procedure. So move the caller
> of DP pdev pre alloc from the core start procedure to the mac allocate
> procedure. This change helps in the future to easily decouple the mac
> allocate procedure from core start handling in order to support MLO in
> multi chip.
> 
> 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>

Re: [PATCH 1/4] wifi: ath12k: Refactor the DP pdev pre alloc call sequence

[Kalle Valo]

Karthikeyan Periyasamy <quic_periyasa@quicinc.com> wrote:

> Currently, the data path pdev pre alloc and mac allocate are called
> separately from the core start procedure. The data path pdev pre alloc
> can be called from the mac allocate procedure itself since initialization
> related to pdev happens in the mac allocate procedure. So move the caller
> of DP pdev pre alloc from the core start procedure to the mac allocate
> procedure. This change helps in the future to easily decouple the mac
> allocate procedure from core start handling in order to support MLO in
> multi chip.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

4 patches applied to ath-next branch of ath.git, thanks.

eaf9f17b861b wifi: ath12k: relocate ath12k_dp_pdev_pre_alloc() call
8a742a79f90e wifi: ath12k: refactor ath12k_mac_allocate() and ath12k_mac_destroy()
d2b7a6e5fa1c wifi: ath12k: refactor ath12k_mac_setup_channels_rates()
d786c9f5fe34 wifi: ath12k: refactor ath12k_mac_register() and ath12k_mac_unregister()

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20231206034920.1037449-2-quic_periyasa@quicinc.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH 2/4] wifi: ath12k: Refactor the MAC allocation and destroy

[Karthikeyan Periyasamy]

Currently, the MAC allocation and destroy helper functions are tightly
coupled with the link/radio (ar) structure. In the future, to support
single/Multi link operations, need to refactor these helper functions
across the core and mac sub modules, so that it can be easy to scale
these functions to support single/Multi link operations.

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 | 33 ++++++++--
 drivers/net/wireless/ath/ath12k/mac.c  | 83 ++++++++++++++------------
 drivers/net/wireless/ath/ath12k/mac.h  |  4 +-
 3 files changed, 73 insertions(+), 47 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
index 6f634b57dde8..e10c5f2cd8eb 100644
--- a/drivers/net/wireless/ath/ath12k/core.c
+++ b/drivers/net/wireless/ath/ath12k/core.c
@@ -529,6 +529,27 @@ static void ath12k_core_pdev_destroy(struct ath12k_base *ab)
 	ath12k_dp_pdev_free(ab);
 }
 
+static void ath12k_core_mac_destroy(struct ath12k_base *ab)
+{
+	ath12k_mac_hw_destroy(ab);
+}
+
+static int ath12k_core_mac_allocate(struct ath12k_base *ab)
+{
+	int ret;
+
+	if (test_bit(ATH12K_FLAG_REGISTERED, &ab->dev_flags))
+		return 0;
+
+	ret = ath12k_mac_hw_allocate(ab);
+	if (ret)
+		return ret;
+
+	ath12k_dp_pdev_pre_alloc(ab);
+
+	return 0;
+}
+
 static int ath12k_core_start(struct ath12k_base *ab,
 			     enum ath12k_firmware_mode mode)
 {
@@ -583,7 +604,7 @@ static int ath12k_core_start(struct ath12k_base *ab,
 		goto err_hif_stop;
 	}
 
-	ret = ath12k_mac_allocate(ab);
+	ret = ath12k_core_mac_allocate(ab);
 	if (ret) {
 		ath12k_err(ab, "failed to create new hw device with mac80211 :%d\n",
 			   ret);
@@ -595,7 +616,7 @@ static int ath12k_core_start(struct ath12k_base *ab,
 	ret = ath12k_dp_rx_pdev_reo_setup(ab);
 	if (ret) {
 		ath12k_err(ab, "failed to initialize reo destination rings: %d\n", ret);
-		goto err_mac_destroy;
+		goto err_core_mac_destroy;
 	}
 
 	ret = ath12k_wmi_cmd_init(ab);
@@ -631,8 +652,8 @@ static int ath12k_core_start(struct ath12k_base *ab,
 
 err_reo_cleanup:
 	ath12k_dp_rx_pdev_reo_cleanup(ab);
-err_mac_destroy:
-	ath12k_mac_destroy(ab);
+err_core_mac_destroy:
+	ath12k_core_mac_destroy(ab);
 err_hif_stop:
 	ath12k_hif_stop(ab);
 err_wmi_detach:
@@ -707,7 +728,7 @@ int ath12k_core_qmi_firmware_ready(struct ath12k_base *ab)
 	ath12k_core_pdev_destroy(ab);
 err_core_stop:
 	ath12k_core_stop(ab);
-	ath12k_mac_destroy(ab);
+	ath12k_core_mac_destroy(ab);
 err_dp_free:
 	ath12k_dp_free(ab);
 	mutex_unlock(&ab->core_lock);
@@ -1003,7 +1024,7 @@ void ath12k_core_deinit(struct ath12k_base *ab)
 	mutex_unlock(&ab->core_lock);
 
 	ath12k_hif_power_down(ab);
-	ath12k_mac_destroy(ab);
+	ath12k_core_mac_destroy(ab);
 	ath12k_core_soc_destroy(ab);
 }
 
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 49d56f5d8896..a110119ad701 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -7607,7 +7607,47 @@ int ath12k_mac_register(struct ath12k_base *ab)
 	return ret;
 }
 
-int ath12k_mac_allocate(struct ath12k_base *ab)
+static void ath12k_mac_setup(struct ath12k *ar)
+{
+	struct ath12k_base *ab = ar->ab;
+	struct ath12k_pdev *pdev = ar->pdev;
+	u8 pdev_idx = ar->pdev_idx;
+
+	ar->lmac_id = ath12k_hw_get_mac_from_pdev_id(ab->hw_params, pdev_idx);
+
+	ar->wmi = &ab->wmi_ab.wmi[pdev_idx];
+	/* FIXME: wmi[0] is already initialized during attach,
+	 * Should we do this again?
+	 */
+	ath12k_wmi_pdev_attach(ab, pdev_idx);
+
+	ar->cfg_tx_chainmask = pdev->cap.tx_chain_mask;
+	ar->cfg_rx_chainmask = pdev->cap.rx_chain_mask;
+	ar->num_tx_chains = hweight32(pdev->cap.tx_chain_mask);
+	ar->num_rx_chains = hweight32(pdev->cap.rx_chain_mask);
+
+	spin_lock_init(&ar->data_lock);
+	INIT_LIST_HEAD(&ar->arvifs);
+	INIT_LIST_HEAD(&ar->ppdu_stats_info);
+	mutex_init(&ar->conf_mutex);
+	init_completion(&ar->vdev_setup_done);
+	init_completion(&ar->vdev_delete_done);
+	init_completion(&ar->peer_assoc_done);
+	init_completion(&ar->peer_delete_done);
+	init_completion(&ar->install_key_done);
+	init_completion(&ar->bss_survey_done);
+	init_completion(&ar->scan.started);
+	init_completion(&ar->scan.completed);
+
+	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);
+	skb_queue_head_init(&ar->wmi_mgmt_tx_queue);
+	clear_bit(ATH12K_FLAG_MONITOR_ENABLED, &ar->monitor_flags);
+}
+
+int ath12k_mac_hw_allocate(struct ath12k_base *ab)
 {
 	struct ieee80211_hw *hw;
 	struct ath12k *ar;
@@ -7615,9 +7655,6 @@ int ath12k_mac_allocate(struct ath12k_base *ab)
 	int ret;
 	int i;
 
-	if (test_bit(ATH12K_FLAG_REGISTERED, &ab->dev_flags))
-		return 0;
-
 	for (i = 0; i < ab->num_radios; i++) {
 		pdev = &ab->pdevs[i];
 		hw = ieee80211_alloc_hw(sizeof(struct ath12k), &ath12k_ops);
@@ -7632,52 +7669,20 @@ int ath12k_mac_allocate(struct ath12k_base *ab)
 		ar->ab = ab;
 		ar->pdev = pdev;
 		ar->pdev_idx = i;
-		ar->lmac_id = ath12k_hw_get_mac_from_pdev_id(ab->hw_params, i);
-
-		ar->wmi = &ab->wmi_ab.wmi[i];
-		/* FIXME: wmi[0] is already initialized during attach,
-		 * Should we do this again?
-		 */
-		ath12k_wmi_pdev_attach(ab, i);
-
-		ar->cfg_tx_chainmask = pdev->cap.tx_chain_mask;
-		ar->cfg_rx_chainmask = pdev->cap.rx_chain_mask;
-		ar->num_tx_chains = hweight32(pdev->cap.tx_chain_mask);
-		ar->num_rx_chains = hweight32(pdev->cap.rx_chain_mask);
-
 		pdev->ar = ar;
-		spin_lock_init(&ar->data_lock);
-		INIT_LIST_HEAD(&ar->arvifs);
-		INIT_LIST_HEAD(&ar->ppdu_stats_info);
-		mutex_init(&ar->conf_mutex);
-		init_completion(&ar->vdev_setup_done);
-		init_completion(&ar->vdev_delete_done);
-		init_completion(&ar->peer_assoc_done);
-		init_completion(&ar->peer_delete_done);
-		init_completion(&ar->install_key_done);
-		init_completion(&ar->bss_survey_done);
-		init_completion(&ar->scan.started);
-		init_completion(&ar->scan.completed);
 
-		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);
-		skb_queue_head_init(&ar->wmi_mgmt_tx_queue);
-		clear_bit(ATH12K_FLAG_MONITOR_ENABLED, &ar->monitor_flags);
+		ath12k_mac_setup(ar);
 	}
 
-	ath12k_dp_pdev_pre_alloc(ab);
-
 	return 0;
 
 err_free_mac:
-	ath12k_mac_destroy(ab);
+	ath12k_mac_hw_destroy(ab);
 
 	return ret;
 }
 
-void ath12k_mac_destroy(struct ath12k_base *ab)
+void ath12k_mac_hw_destroy(struct ath12k_base *ab)
 {
 	struct ath12k *ar;
 	struct ath12k_pdev *pdev;
diff --git a/drivers/net/wireless/ath/ath12k/mac.h b/drivers/net/wireless/ath/ath12k/mac.h
index 7c63bb628adc..fb4c0662581b 100644
--- a/drivers/net/wireless/ath/ath12k/mac.h
+++ b/drivers/net/wireless/ath/ath12k/mac.h
@@ -48,10 +48,10 @@ enum ath12k_supported_bw {
 
 extern const struct htt_rx_ring_tlv_filter ath12k_mac_mon_status_filter_default;
 
-void ath12k_mac_destroy(struct ath12k_base *ab);
+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);
-int ath12k_mac_allocate(struct ath12k_base *ab);
+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);
 u8 ath12k_mac_bitrate_to_idx(const struct ieee80211_supported_band *sband,
-- 
2.34.1

Re: [PATCH 2/4] wifi: ath12k: Refactor the MAC allocation and destroy

[Jeff Johnson]

On 12/5/2023 7:49 PM, Karthikeyan Periyasamy wrote:
> Currently, the MAC allocation and destroy helper functions are tightly
> coupled with the link/radio (ar) structure. In the future, to support
> single/Multi link operations, need to refactor these helper functions
> across the core and mac sub modules, so that it can be easy to scale
> these functions to support single/Multi link operations.
> 
> 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>

[PATCH 3/4] wifi: ath12k: Refactor MAC setup channel helper function

[Karthikeyan Periyasamy]

Currently, the MAC setup helper function is accessing the mac80211 hw
data. In the future, to support single/multi link operation, need to
decouple the mac80211 hw data from this helper function so that it can be
easy to scale these functions to support single/multi link operations. So
remove the mac80211 hw access from the ath12k_mac_setup_channels_rates()
helper function.

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/mac.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index a110119ad701..be3ef388e9aa 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -7158,9 +7158,9 @@ static u32 ath12k_get_phy_id(struct ath12k *ar, u32 band)
 }
 
 static int ath12k_mac_setup_channels_rates(struct ath12k *ar,
-					   u32 supported_bands)
+					   u32 supported_bands,
+					   struct ieee80211_supported_band *bands[])
 {
-	struct ieee80211_hw *hw = ar->hw;
 	struct ieee80211_supported_band *band;
 	struct ath12k_wmi_hal_reg_capabilities_ext_arg *reg_cap;
 	void *channels;
@@ -7186,7 +7186,7 @@ static int ath12k_mac_setup_channels_rates(struct ath12k *ar,
 		band->channels = channels;
 		band->n_bitrates = ath12k_g_rates_size;
 		band->bitrates = ath12k_g_rates;
-		hw->wiphy->bands[NL80211_BAND_2GHZ] = band;
+		bands[NL80211_BAND_2GHZ] = band;
 
 		if (ar->ab->hw_params->single_pdev_only) {
 			phy_id = ath12k_get_phy_id(ar, WMI_HOST_WLAN_2G_CAP);
@@ -7213,7 +7213,7 @@ static int ath12k_mac_setup_channels_rates(struct ath12k *ar,
 			band->channels = channels;
 			band->n_bitrates = ath12k_a_rates_size;
 			band->bitrates = ath12k_a_rates;
-			hw->wiphy->bands[NL80211_BAND_6GHZ] = band;
+			bands[NL80211_BAND_6GHZ] = band;
 			ath12k_mac_update_ch_list(ar, band,
 						  reg_cap->low_5ghz_chan,
 						  reg_cap->high_5ghz_chan);
@@ -7235,7 +7235,7 @@ static int ath12k_mac_setup_channels_rates(struct ath12k *ar,
 			band->channels = channels;
 			band->n_bitrates = ath12k_a_rates_size;
 			band->bitrates = ath12k_a_rates;
-			hw->wiphy->bands[NL80211_BAND_5GHZ] = band;
+			bands[NL80211_BAND_5GHZ] = band;
 
 			if (ar->ab->hw_params->single_pdev_only) {
 				phy_id = ath12k_get_phy_id(ar, WMI_HOST_WLAN_5G_CAP);
@@ -7414,7 +7414,8 @@ static int __ath12k_mac_register(struct ath12k *ar)
 	SET_IEEE80211_DEV(hw, ab->dev);
 
 	ret = ath12k_mac_setup_channels_rates(ar,
-					      cap->supported_bands);
+					      cap->supported_bands,
+					      hw->wiphy->bands);
 	if (ret)
 		goto err;
 
-- 
2.34.1

Re: [PATCH 3/4] wifi: ath12k: Refactor MAC setup channel helper function

[Jeff Johnson]

On 12/5/2023 7:49 PM, Karthikeyan Periyasamy wrote:
> Currently, the MAC setup helper function is accessing the mac80211 hw
> data. In the future, to support single/multi link operation, need to
> decouple the mac80211 hw data from this helper function so that it can be
> easy to scale these functions to support single/multi link operations. So
> remove the mac80211 hw access from the ath12k_mac_setup_channels_rates()
> helper function.
> 
> 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>

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

[Karthikeyan Periyasamy]

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;
+}
+
+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);
-- 
2.34.1

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

[Jeff Johnson]

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>

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

[Kalle Valo]

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?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Karthikeyan Periyasamy]

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.

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

[Kalle Valo]

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?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Karthikeyan Periyasamy]

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