Received-SPF: pass (google.com: domain of linux-wireless+bounces-4204-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <ae3ec744-2157-4a8c-aa1b-38a22dc18042@quicinc.com>
Date: Wed, 28 Feb 2024 07:31:20 -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: Baochen Qiang <quic_bqiang@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>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20240228022243.17762-4-quic_bqiang@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

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?

just concerned about the lack of symmetry here

/jeff