[PATCH v8 7/8] wifi: ath12k: refactor core start based on hardware group
Harshitha Prem
quic_hprem at quicinc.com
Fri Jun 7 06:49:09 PDT 2024
On 6/6/2024 6:34 PM, Kalle Valo wrote:
> Harshitha Prem <quic_hprem at quicinc.com> writes:
>
>> From: Karthikeyan Periyasamy <quic_periyasa at 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 at quicinc.com>
>> Co-developed-by: Harshitha Prem <quic_hprem at quicinc.com>
>> Signed-off-by: Harshitha Prem <quic_hprem at 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?
>
yeah., more dev flags :( but flags were needed for the race conditions
when multiple devices where involved in a group, some devices would have
completed till pdev create some might not. Some crashes were seen
because hif_irq_disable was called for a device in a group but that
device was not even at the stage of core register. Will check the
possibility to reduce the flag usage but it seemed necessary for
multiple device group clean up.
Thanks,
Harshitha
More information about the ath12k
mailing list