Received-SPF: pass (google.com: domain of linux-wireless+bounces-6298-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <ae78bdad-6f6b-8d8e-c324-7dc990895e9f@quicinc.com>
Date: Mon, 15 Apr 2024 13:38:48 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH 01/10] wifi: ath12k: rearrange IRQ enable/disable in reset
 path
Content-Language: en-US
To: Sidhanta Sahu <quic_sidhanta@quicinc.com>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>
References: <20240412060620.27519-1-quic_bqiang@quicinc.com>
 <20240412060620.27519-2-quic_bqiang@quicinc.com>
 <8122e56b-1e57-4501-8180-24a65247465f@quicinc.com>
From: Baochen Qiang <quic_bqiang@quicinc.com>
In-Reply-To: <8122e56b-1e57-4501-8180-24a65247465f@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


On 4/13/2024 2:24 AM, Sidhanta Sahu wrote:
>
>
> On 4/11/2024 11:06 PM, Baochen Qiang wrote:
>> For non-WoW suspend/resume, ath12k host powers down whole hardware
>> when suspend and powers up it when resume, the code path it goes
>> through is very like the ath12k reset logic.
>>
>> In order to reuse that logic, rearrange IRQ handling in the reset
>> path.
>>
>> Tested-on: WCN7850 hw2.0 PCI 
>> WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>>
>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
>> ---
>>   drivers/net/wireless/ath/ath12k/core.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/core.c 
>> b/drivers/net/wireless/ath/ath12k/core.c
>> index 3c522a4b3e9b..28663cf4db30 100644
>> --- a/drivers/net/wireless/ath/ath12k/core.c
>> +++ b/drivers/net/wireless/ath/ath12k/core.c
>> @@ -879,9 +879,8 @@ static int 
>> ath12k_core_reconfigure_on_crash(struct ath12k_base *ab)
>>       int ret;
>>         mutex_lock(&ab->core_lock);
>> -    ath12k_hif_irq_disable(ab);
>>       ath12k_dp_pdev_free(ab);
>> -    ath12k_hif_stop(ab);
>> +    ath12k_ce_cleanup_pipes(ab);
>>       ath12k_wmi_detach(ab);
>>       ath12k_dp_rx_pdev_reo_cleanup(ab);
>>       mutex_unlock(&ab->core_lock);
>> @@ -1136,6 +1135,9 @@ static void ath12k_core_reset(struct 
>> work_struct *work)
>>       time_left = wait_for_completion_timeout(&ab->recovery_start,
>>                           ATH12K_RECOVER_START_TIMEOUT_HZ);
>>   +    ath12k_hif_irq_disable(ab);
>> +    ath12k_hif_ce_irq_disable(ab);
>> +
>
> This core reset is called due to a fatal error. In case of an unlikely 
> scenario, when there are consecutive fatal errors notification before 
> the first fatal error is handled, then we will be disabling same irqs 
> again. Shall we have a check here to see if ab's already in reset 
> using ab->is_reset.
There is similar logic using ab->reset_count in ath12k_core_reset().
>
>>       ath12k_hif_power_down(ab);
>>       ath12k_qmi_free_resource(ab);
>>       ath12k_hif_power_up(ab);