| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <60d9970b-b397-4c3d-b3cb-9d90866acebf@quicinc.com>
Date: Thu, 29 Feb 2024 10:30:04 +0800
From: Baochen Qiang <quic_bqiang@...cinc.com>
To: Jeff Johnson <quic_jjohnson@...cinc.com>, <ath11k@...ts.infradead.org>,
<manivannan.sadhasivam@...aro.org>
CC: <linux-wireless@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<mhi@...ts.linux.dev>, <davem@...emloft.net>, <edumazet@...gle.com>,
<kuba@...nel.org>, <pabeni@...hat.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH v4 3/3] wifi: ath11k: support hibernation
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@...e.de>
>> Signed-off-by: Kalle Valo <quic_kvalo@...cinc.com>
>> Signed-off-by: Baochen Qiang <quic_bqiang@...cinc.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
Powered by blists - more mailing lists