Received-SPF: pass (google.com: domain of linux-wireless+bounces-8610-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Kalle Valo <kvalo@kernel.org>
To: Harshitha Prem <quic_hprem@quicinc.com>
Cc: ath12k@lists.infradead.org,  linux-wireless@vger.kernel.org,
  Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
Subject: Re: [PATCH v8 7/8] wifi: ath12k: refactor core start based on
 hardware group
References: <20240531180411.1149605-1-quic_hprem@quicinc.com>
	<20240531180411.1149605-8-quic_hprem@quicinc.com>
Date: Thu, 06 Jun 2024 16:04:05 +0300
In-Reply-To: <20240531180411.1149605-8-quic_hprem@quicinc.com> (Harshitha
	Prem's message of "Fri, 31 May 2024 23:34:10 +0530")
Message-ID: <87plsuql2y.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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?

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

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