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@xxxxxxx>
Signed-off-by: Kalle Valo <quic_kvalo@xxxxxxxxxxx>
Signed-off-by: Baochen Qiang <quic_bqiang@xxxxxxxxxxx>
---
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
