Re: [PATCH v8 7/8] wifi: ath12k: refactor core start based on hardware group

Kalle Valo
2024-06-06T13:04:16Z

Harshitha Prem <quic_hprem@quicinc.com> writes:

> From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
>
> Currently, mac allocate/register and core_pdev_create are initiated
> immediately when QMI firmware ready event is received for a particular
> device.
>
> With hardware device group abstraction, QMI firmware ready event can be
> received simultaneously for different devices in the group and so, it
> should not be registered immediately rather it has to be deferred until
> all devices in the group has received QMI firmware ready.
>
> To handle this, refactor the code of core start to move the following
> apis inside a wrapper ath12k_core_hw_group_start()
> * ath12k_mac_allocate()
> * ath12k_core_pdev_create()
> * ath12k_core_rfkill_config()
> * ath12k_mac_register()
> * ath12k_hif_irq_enable()
>
> similarly, move the corresponding destroy/unregister/disable apis
> inside wrapper ath12k_core_hw_group_stop()
>
> Add the device flags to indicate pdev created and IRQ enabled which would
> be helpful for device clean up during failure cases.
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4
>
> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
> Co-developed-by: Harshitha Prem <quic_hprem@quicinc.com>
> Signed-off-by: Harshitha Prem <quic_hprem@quicinc.com>
> ---
> drivers/net/wireless/ath/ath12k/core.c | 210 +++++++++++++++++++------
> drivers/net/wireless/ath/ath12k/core.h | 32 ++++
> 2 files changed, 191 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
> index ebe31cbb6435..90c70dbfc50a 100644
> --- a/drivers/net/wireless/ath/ath12k/core.c
> +++ b/drivers/net/wireless/ath/ath12k/core.c
> @@ -563,6 +563,9 @@ u32 ath12k_core_get_max_num_tids(struct ath12k_base *ab)
>
> static void ath12k_core_stop(struct ath12k_base *ab)
> {
> + clear_bit(ATH12K_FLAG_CORE_STARTED, &ab->dev_flags);
> + ath12k_dec_num_core_started(ab);
> +
> if (!test_bit(ATH12K_FLAG_CRASH_FLUSH, &ab->dev_flags))
> ath12k_qmi_firmware_stop(ab);
>
> @@ -689,11 +692,15 @@ static int ath12k_core_pdev_create(struct ath12k_base *ab)
> return ret;
> }
>
> + set_bit(ATH12K_FLAG_PDEV_CREATED, &ab->dev_flags);
> +
> return 0;
> }
>
> static void ath12k_core_pdev_destroy(struct ath12k_base *ab)
> {
> + clear_bit(ATH12K_FLAG_PDEV_CREATED, &ab->dev_flags);
> +
> ath12k_dp_pdev_free(ab);
> }
>
> @@ -702,6 +709,8 @@ static int ath12k_core_start(struct ath12k_base *ab,
> {
> int ret;
>
> + lockdep_assert_held(&ab->core_lock);
> +
> ret = ath12k_wmi_attach(ab);
> if (ret) {
> ath12k_err(ab, "failed to attach wmi: %d\n", ret);
> @@ -795,6 +804,12 @@ static int ath12k_core_start(struct ath12k_base *ab,
> /* ACPI is optional so continue in case of an error */
> ath12k_dbg(ab, ATH12K_DBG_BOOT, "acpi failed: %d\n", ret);
>
> + if (!test_bit(ATH12K_FLAG_CORE_STARTED, &ab->dev_flags)) {
> + /* Indicate the core start in the appropriate group */
> + ath12k_inc_num_core_started(ab);
> + set_bit(ATH12K_FLAG_CORE_STARTED, &ab->dev_flags);
> + }
> +
> return 0;
>
> err_reo_cleanup:
> @@ -806,6 +821,108 @@ static int ath12k_core_start(struct ath12k_base *ab,
> return ret;
> }
>
> +static void ath12k_core_device_cleanup(struct ath12k_base *ab)
> +{
> + mutex_lock(&ab->core_lock);
> +
> + if (test_and_clear_bit(ATH12K_FLAG_CORE_HIF_IRQ_ENABLED, &ab->dev_flags))
> + ath12k_hif_irq_disable(ab);
> +
> + if (test_bit(ATH12K_FLAG_PDEV_CREATED, &ab->dev_flags))
> + ath12k_core_pdev_destroy(ab);
> +
> + if (test_bit(ATH12K_FLAG_REGISTERED, &ab->dev_flags)) {
> + ath12k_mac_unregister(ab);
> + ath12k_mac_destroy(ab);
> + }
> +
> + mutex_unlock(&ab->core_lock);
> +}

This patch is just abusing flags and because of that we have spaghetti
code. I have been disliking use of enum ath12k_dev_flags before but this
is just looks too much. I am wondering do we need to cleanup the ath12k
architecture first, reduce the usage of flags and then revisit this
patchset?

--