Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <8c56efa5420eb4211b1af789ef63931d3504d8e1.camel@gmail.com>
Subject: Re: [RFC PATCH] mwifiex: pcie: use shutdown_sw()/reinit_sw() on
 suspend/resume
From:   Tsuchiya Yuto <kitakar@gmail.com>
To:     Amitkumar Karwar <amitkarwar@gmail.com>,
        Ganapathi Bhat <ganapathi.bhat@nxp.com>,
        Xinming Hu <huxinming820@gmail.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>
Cc:     linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Maximilian Luz <luzmaximilian@gmail.com>,
        Andy Shevchenko <andriy.shevchenko@intel.com>, verdre@v0yd.nl
Date:   Sat, 31 Oct 2020 00:27:36 +0900
In-Reply-To: <20201028142719.18765-1-kitakar@gmail.com>
References: <20201028142719.18765-1-kitakar@gmail.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 2020-10-28 at 23:27 +0900, Tsuchiya Yuto wrote:
> 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@gmail.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.

This patch depends on the following mwifiex_shutdown_sw() fix I sent
separately.

[PATCH 1/2] mwifiex: fix mwifiex_shutdown_sw() causing sw reset failure
https://lore.kernel.org/linux-wireless/20201028142110.18144-2-kitakar@gmail.com/

>  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;
>  }