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

[Kalle Valo <kvalo@kernel.org>]

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