Received-SPF: pass (google.com: domain of linux-wireless+bounces-4221-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <60d9970b-b397-4c3d-b3cb-9d90866acebf@quicinc.com>
Date: Thu, 29 Feb 2024 10:30:04 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 3/3] wifi: ath11k: support hibernation
Content-Language: en-US
To: Jeff Johnson <quic_jjohnson@quicinc.com>, <ath11k@lists.infradead.org>,
        <manivannan.sadhasivam@linaro.org>
CC: <linux-wireless@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <mhi@lists.linux.dev>, <davem@davemloft.net>, <edumazet@google.com>,
        <kuba@kernel.org>, <pabeni@redhat.com>, <netdev@vger.kernel.org>
References: <20240228022243.17762-1-quic_bqiang@quicinc.com>
 <20240228022243.17762-4-quic_bqiang@quicinc.com>
 <ae3ec744-2157-4a8c-aa1b-38a22dc18042@quicinc.com>
From: Baochen Qiang <quic_bqiang@quicinc.com>
In-Reply-To: <ae3ec744-2157-4a8c-aa1b-38a22dc18042@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2/28/2024 11:31 PM, Jeff Johnson wrote:
> On 2/27/2024 6:22 PM, Baochen Qiang wrote:
>> Now that all infrastructure is in place and ath11k is fixed to handle all the
>> corner cases, power down the ath11k firmware during suspend and power it back
>> up during resume. This fixes the problem when using hibernation with ath11k PCI
>> devices.
>>
>> For suspend, two conditions needs to be satisfied:
>>          1. since MHI channel unprepare would be done in late suspend stage,
>>             ath11k needs to get all QMI-dependent things done before that stage.
>>          2. and because unprepare MHI channels requires a working MHI stack,
>>             ath11k is not allowed to call mhi_power_down() until that finishes.
>> So the original suspend callback is separated into two parts: the first part
>> handles all QMI-dependent things in suspend callback; while the second part
>> powers down MHI in suspend_late callback. This is valid because kernel calls
>> ath11k's suspend callback before all suspend_late callbacks, making the first
>> condition happy. And because MHI devices are children of ath11k device
>> (ab->dev), kernel guarantees that ath11k's suspend_late callback is called
>> after QRTR's suspend_late callback, this satisfies the second condition.
>>
>> Above analysis also applies to resume process. so the original resume
>> callback is separated into two parts: the first part powers up MHI stack
>> in resume_early callback, this guarantees MHI stack is working when
>> QRTR tries to prepare MHI channels (kernel calls QRTR's resume_early callback
>> after ath11k's resume_early callback, due to the child-father relationship);
>> the second part waits for the completion of restart, which won't fail now
>> since MHI channels are ready for use by QMI.
>>
>> Another notable change is in power down path, we tell mhi_power_down() to not
>> to destroy MHI devices, making it possible for QRTR to help unprepare/prepare
>> MHI channels, and finally get us rid of the probe-defer issue when resume.
>>
>> Also change related code due to interface changes.
>>
>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
>>
>> Tested-by: Takashi Iwai <tiwai@suse.de>
>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
>> ---
>>   drivers/net/wireless/ath/ath11k/ahb.c  |   6 +-
>>   drivers/net/wireless/ath/ath11k/core.c | 105 +++++++++++++++++--------
>>   drivers/net/wireless/ath/ath11k/core.h |   6 +-
>>   drivers/net/wireless/ath/ath11k/hif.h  |  14 +++-
>>   drivers/net/wireless/ath/ath11k/mhi.c  |  12 ++-
>>   drivers/net/wireless/ath/ath11k/mhi.h  |   5 +-
>>   drivers/net/wireless/ath/ath11k/pci.c  |  44 +++++++++--
>>   drivers/net/wireless/ath/ath11k/qmi.c  |   2 +-
>>   8 files changed, 142 insertions(+), 52 deletions(-)
> ...snip...
>> +int ath11k_core_resume_early(struct ath11k_base *ab)
>> +{
>> +	int ret;
>> +	struct ath11k_pdev *pdev;
>> +	struct ath11k *ar;
>> +
>> +	if (!ab->hw_params.supports_suspend)
>> +		return -EOPNOTSUPP;
>> +
>> +	/* so far signle_pdev_only chips have supports_suspend as true
> 
> nit: s/signle/single/
> 
>> +	 * and only the first pdev is valid.
>> +	 */
>> +	pdev = ath11k_core_get_single_pdev(ab);
>> +	ar = pdev->ar;
>> +	if (!ar || ar->state != ATH11K_STATE_OFF)
>> +		return 0;
>> +
>> +	reinit_completion(&ab->restart_completed);
>> +	ret = ath11k_hif_power_up(ab);
>> +	if (ret)
>> +		ath11k_warn(ab, "failed to power up hif during resume: %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(ath11k_core_resume_early);
>>   
>>   int ath11k_core_resume(struct ath11k_base *ab)
>>   {
>>   	int ret;
>>   	struct ath11k_pdev *pdev;
>>   	struct ath11k *ar;
>> +	long time_left;
>>   
>>   	if (!ab->hw_params.supports_suspend)
>>   		return -EOPNOTSUPP;
>> @@ -940,29 +990,19 @@ int ath11k_core_resume(struct ath11k_base *ab)
>>   	if (!ar || ar->state != ATH11K_STATE_OFF)
>>   		return 0;
>>   
>> -	ret = ath11k_hif_resume(ab);
>> -	if (ret) {
>> -		ath11k_warn(ab, "failed to resume hif during resume: %d\n", ret);
>> -		return ret;
>> +	time_left = wait_for_completion_timeout(&ab->restart_completed,
>> +						ATH11K_RESET_TIMEOUT_HZ);
>> +	if (time_left == 0) {
>> +		ath11k_warn(ab, "timeout while waiting for restart complete");
>> +		return -ETIMEDOUT;
>>   	}
>>   
>> -	ath11k_hif_ce_irq_enable(ab);
>> -	ath11k_hif_irq_enable(ab);
> 
> these are disabled in suspend_late()
> do you need to enable in resume_early()?
> or are they expected to be enabled via ath11k_wow_op_resume()?
> 
> and if that is the case, why isn't the disable in
> ath11k_wow_op_suspend() sufficient? can the disables in suspend_late()
> be removed?
There are two user cases here:
1. if WoWLAN is enabled, IRQ enable/disable only happens in 
ath11k_wow_op_suspend()/resume(), ath11k_core_suspend()/late_suspend() 
and ath11k_core_resume()/early_resume() do nothing but return directly 
due to below check:
		if (!ar || ar->state != ATH11K_STATE_OFF)
			return 0;
so this is symmetric and no issues here.

2. if WoWLAN is disabled, ath11k_wow_op_suspend()/resume() won't get 
called, see the check on 'local->wowlan' in __ieee80211_suspend(). Then 
IRQ is disabled in ath11k_core_suspend_late(). The reason why IRQ is not 
enabled in ath11k_core_resume()/early_resume() is because in this case 
we power down/up firmware, and during power up we go the reset path 
where IRQ would be enabled back in below calls:
	CE irqs: ath11k_core_qmi_firmware_ready() -> ath11k_core_start() -> 
ath11k_hif_start() -> ath11k_pci_start() -> ath11k_pcic_start() -> 
ath11k_pcic_ce_irqs_enable()
	DP irqs: ath11k_core_qmi_firmware_ready() -> ath11k_hif_irq_enable()
> 
> just concerned about the lack of symmetry here
Yes, also noticed it but didn't figure out a better way.

> 
> /jeff