Message-Id: <20201028142719.18765-1-kitakar@gmail.com>
Date:   Wed, 28 Oct 2020 23:27:19 +0900
From:   Tsuchiya Yuto <kitakar@...il.com>
To:     Amitkumar Karwar <amitkarwar@...il.com>,
        Ganapathi Bhat <ganapathi.bhat@....com>,
        Xinming Hu <huxinming820@...il.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Maximilian Luz <luzmaximilian@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...el.com>, verdre@...d.nl,
        Tsuchiya Yuto <kitakar@...il.com>
Subject: [RFC PATCH] mwifiex: pcie: use shutdown_sw()/reinit_sw() on suspend/resume

On Microsoft Surface devices (PCIe-88W8897), there are issues with S0ix
achievement and AP scanning after suspend with the current Host Sleep
method.

When using the Host Sleep method, it prevents the platform to reach S0ix
during suspend. Also, sometimes AP scanning won't work, resulting in
non-working wifi after suspend.

To fix such issues, perform shutdown_sw()/reinit_sw() instead of Host
Sleep on suspend/resume.

Signed-off-by: Tsuchiya Yuto <kitakar@...il.com>
---
As a side effect, this patch disables wakeups (means that Wake-On-WLAN
can't be used anymore, if it was working before), and might also reset
some internal states.

Of course it's the best to rather fix Host Sleep itself. But if it's
difficult, I'm afraid we have to go this way.

I reused the contents of suspend()/resume() functions as much as possible,
and removed only the parts that are incompatible or redundant with
shutdown_sw()/reinit_sw().

- Removed wait_for_completion() as redundant
  mwifiex_shutdown_sw() does this.
- Removed flush_workqueue() as incompatible
  Causes kernel crashing.
- Removed mwifiex_enable_wake()/mwifiex_disable_wake()
  as incompatible and redundant because the driver will be shut down
  instead of entering Host Sleep.

I'm worried about why flush_workqueue() causes kernel crash with this
suspend method. Is it OK to just drop it? At least We Microsoft Surface
devices users used this method for about one month and haven't observed
any issues.

Note that suspend() no longer checks if it's already suspended.
With the previous Host Sleep method, the check was done by looking at
adapter->hs_activated in mwifiex_enable_hs() [sta_ioctl.c], but not
MWIFIEX_IS_SUSPENDED. So, what the previous method checked was instead
Host Sleep state, not suspend itself.

Therefore, there is no need to check the suspend state now.
Also removed comment for suspend state check at top of suspend()
accordingly.

 drivers/net/wireless/marvell/mwifiex/pcie.c | 29 +++++++--------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 6a10ff0377a24..3b5c614def2f5 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -293,8 +293,7 @@ static bool mwifiex_pcie_ok_to_access_hw(struct mwifiex_adapter *adapter)
  * registered functions must have drivers with suspend and resume
  * methods. Failing that the kernel simply removes the whole card.
  *
- * If already not suspended, this function allocates and sends a host
- * sleep activate request to the firmware and turns off the traffic.
+ * This function shuts down the adapter.
  */
 static int mwifiex_pcie_suspend(struct device *dev)
 {
@@ -302,31 +301,21 @@ static int mwifiex_pcie_suspend(struct device *dev)
 	struct pcie_service_card *card = dev_get_drvdata(dev);
 
 
-	/* Might still be loading firmware */
-	wait_for_completion(&card->fw_done);
-
 	adapter = card->adapter;
 	if (!adapter) {
 		dev_err(dev, "adapter is not valid\n");
 		return 0;
 	}
 
-	mwifiex_enable_wake(adapter);
-
-	/* Enable the Host Sleep */
-	if (!mwifiex_enable_hs(adapter)) {
+	/* Shut down SW */
+	if (mwifiex_shutdown_sw(adapter)) {
 		mwifiex_dbg(adapter, ERROR,
 			    "cmd: failed to suspend\n");
-		clear_bit(MWIFIEX_IS_HS_ENABLING, &adapter->work_flags);
-		mwifiex_disable_wake(adapter);
 		return -EFAULT;
 	}
 
-	flush_workqueue(adapter->workqueue);
-
 	/* Indicate device suspended */
 	set_bit(MWIFIEX_IS_SUSPENDED, &adapter->work_flags);
-	clear_bit(MWIFIEX_IS_HS_ENABLING, &adapter->work_flags);
 
 	return 0;
 }
@@ -336,13 +325,13 @@ static int mwifiex_pcie_suspend(struct device *dev)
  * registered functions must have drivers with suspend and resume
  * methods. Failing that the kernel simply removes the whole card.
  *
- * If already not resumed, this function turns on the traffic and
- * sends a host sleep cancel request to the firmware.
+ * If already not resumed, this function reinits the adapter.
  */
 static int mwifiex_pcie_resume(struct device *dev)
 {
 	struct mwifiex_adapter *adapter;
 	struct pcie_service_card *card = dev_get_drvdata(dev);
+	int ret;
 
 
 	if (!card->adapter) {
@@ -360,9 +349,11 @@ static int mwifiex_pcie_resume(struct device *dev)
 
 	clear_bit(MWIFIEX_IS_SUSPENDED, &adapter->work_flags);
 
-	mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),
-			  MWIFIEX_ASYNC_CMD);
-	mwifiex_disable_wake(adapter);
+	ret = mwifiex_reinit_sw(adapter);
+	if (ret)
+		dev_err(dev, "reinit failed: %d\n", ret);
+	else
+		mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
 
 	return 0;
 }
-- 
2.29.1

Powered by blists - more mailing lists