Hi Kalle,
On Fri, 24 Sept 2021 at 10:36, Kalle Valo <kvalo@xxxxxxxxxxxxxx> wrote:
>
> Loic Poulain <loic.poulain@xxxxxxxxxx> writes:
>
> > Hi Kalle,
> >
> > On Thu, 16 Sept 2021 at 10:00, Kalle Valo <kvalo@xxxxxxxxxxxxxx> wrote:
> >>
> >> Hi Loic and Mani,
> >>
> >> I hate to be the bearer of bad news again :)
> >>
> >> I noticed already a while ago that commit 020d3b26c07a ("bus: mhi: Early
> >> MHI resume failure in non M3 state"), introduced in v5.13-rc1, broke
> >> ath11k resume on my NUC x86 testbox using QCA6390. Interestingly enough
> >> Dell XPS 13 9310 laptop (with QCA6390 as well) does not have this
> >> problem, I only see the problem on the NUC. I do not know what's causing
> >> this difference.
> >
> > I suppose the NUC is current PCI-Express power during suspend while
> > the laptop maintains PCIe/M2 power.
>
> Sorry, I'm not able to parse that sentence. Can you elaborate more?
Ouch, yes, I wanted to say that the NUC does not maintain the power of
PCI express during suspend (leading to PCI D3cold state), whereas the
laptop maintains the power of the M2 card... well, not sure now I see
your logs.
>
> >> At the moment I'm running my tests with commit 020d3b26c07a reverted and
> >> everything works without problems. Is there a simple way to fix this? Or
> >> maybe we should just revert the commit? Commit log and kernel logs from
> >> a failing case below.
> >
> > Do you have log of success case?
>
> A log from a successful case in the end of email, using v5.15-rc1 plus
> revert of commit 020d3b26c07abe27.
>
> > To me, the device loses power, that is why MHI resuming is failing.
> > Normally the device should be properly recovered/reinitialized. Before
> > that patch the power loss was simply not detected (or handled at
> > higher stack level).
>
> Currently in ath11k we always keep the firmware running when in suspend,
> this is a workaround due to problems between mac80211 and MHI stack.
> IIRC the problem was something related MHI creating struct device during
> resume or something like that.
Could you give a try with the attached patch? It should solve your
issue without breaking modem support.
Regards,
Loic
From 697cf90155d78225d967836dcdb2291468c3e60d Mon Sep 17 00:00:00 2001
From: Loic Poulain <loic.poulain@linaro.org>
Date: Fri, 24 Sep 2021 11:14:03 +0200
Subject: [PATCH] bus: mhi: Add support for forced resume
For whatever reason, some devices like ath11k are not in M3 state when
resuming, but still functional. The mhi_pm_resume should then not fail
in that case, and let the higher level device specific stack continue
resuming process.
Add a new parameter to mhi_pm_resume, to try resuming, whatever the
current MHI state is. This fixes a regression with non functional
ath11k WiFi after suspend/resume cycle on some machines.
Fixes: 020d3b26c07a ("bus: mhi: Early MHI resume failure in non M3 state")
Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
drivers/bus/mhi/core/pm.c | 10 +++++++---
drivers/bus/mhi/pci_generic.c | 2 +-
drivers/net/wireless/ath/ath11k/mhi.c | 2 +-
include/linux/mhi.h | 3 ++-
4 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index bbf6cd0..ef0e698 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -881,7 +881,7 @@ int mhi_pm_suspend(struct mhi_controller *mhi_cntrl)
}
EXPORT_SYMBOL_GPL(mhi_pm_suspend);
-int mhi_pm_resume(struct mhi_controller *mhi_cntrl)
+int mhi_pm_resume(struct mhi_controller *mhi_cntrl, bool force)
{
struct mhi_chan *itr, *tmp;
struct device *dev = &mhi_cntrl->mhi_dev->dev;
@@ -898,8 +898,12 @@ int mhi_pm_resume(struct mhi_controller *mhi_cntrl)
if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state))
return -EIO;
- if (mhi_get_mhi_state(mhi_cntrl) != MHI_STATE_M3)
- return -EINVAL;
+ if (mhi_get_mhi_state(mhi_cntrl) != MHI_STATE_M3) {
+ dev_warn(dev, "Resuming from non M3 state (%s)\n",
+ TO_MHI_STATE_STR(mhi_get_mhi_state(mhi_cntrl)));
+ if (!force)
+ return -EINVAL;
+ }
/* Notify clients about exiting LPM */
list_for_each_entry_safe(itr, tmp, &mhi_cntrl->lpm_chans, node) {
diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index b33b9d7..dba4b7d 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -924,7 +924,7 @@ static int __maybe_unused mhi_pci_runtime_resume(struct device *dev)
return 0; /* Nothing to do at MHI level */
/* Exit M3, transition to M0 state */
- err = mhi_pm_resume(mhi_cntrl);
+ err = mhi_pm_resume(mhi_cntrl, false);
if (err) {
dev_err(&pdev->dev, "failed to resume device: %d\n", err);
goto err_recovery;
diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
index 75cc2d8..f913439 100644
--- a/drivers/net/wireless/ath/ath11k/mhi.c
+++ b/drivers/net/wireless/ath/ath11k/mhi.c
@@ -532,7 +532,7 @@ static int ath11k_mhi_set_state(struct ath11k_pci *ab_pci,
ret = mhi_pm_suspend(ab_pci->mhi_ctrl);
break;
case ATH11K_MHI_RESUME:
- ret = mhi_pm_resume(ab_pci->mhi_ctrl);
+ ret = mhi_pm_resume(ab_pci->mhi_ctrl, true);
break;
case ATH11K_MHI_TRIGGER_RDDM:
ret = mhi_force_rddm_mode(ab_pci->mhi_ctrl);
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index c493a80..f73b270 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -658,8 +658,9 @@ int mhi_pm_suspend(struct mhi_controller *mhi_cntrl);
/**
* mhi_pm_resume - Resume MHI from suspended state
* @mhi_cntrl: MHI controller
+ * @force: Force resuming to M0, whatever the current state
*/
-int mhi_pm_resume(struct mhi_controller *mhi_cntrl);
+int mhi_pm_resume(struct mhi_controller *mhi_cntrl, bool force);
/**
* mhi_download_rddm_image - Download ramdump image from device for
--
2.7.4
