Received-SPF: pass (google.com: domain of linux-wireless+bounces-437-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 5 Dec 2023 21:00:37 +0100
From: Francesco Dolcini <francesco@dolcini.it>
To: David Lin <yu-hao.lin@nxp.com>
Cc: linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org,
	briannorris@chromium.org, kvalo@kernel.org, francesco@dolcini.it,
	tsung-hsien.hsieh@nxp.com
Subject: Re: [PATCH] wifi: mwifiex: added extra delay for firmware ready.
Message-ID: <ZW+BZRHDta7vKZgs@francesco-nb.int.toradex.com>
References: <20231128082544.613179-1-yu-hao.lin@nxp.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231128082544.613179-1-yu-hao.lin@nxp.com>

Hello David,

thanks for your patch. Planning to run some test on this over the next
days and we'll provide some actual feedback if this really solves the
issue we are affected by.

Just a couple of nitpicky comments on the actual patch.


On the commit message you should use imperative mood, e.g.

`wifi: mwifiex: add extra delay for firmware ready`

with no period at the end of the line.

On Tue, Nov 28, 2023 at 04:25:44PM +0800, David Lin wrote:
> For SDIO IW416, in a corner case FW may return ready before complete full
> initialization.
> Command timeout may occur at driver load after reboot.
> Workaround by adding 100ms delay at checking FW status.
> 
> Signed-off-by: David Lin <yu-hao.lin@nxp.com>

Add

Cc: stable@...

> ---
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 6462a0ffe698..744e9403430a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -783,6 +783,9 @@ static int mwifiex_check_fw_status(struct mwifiex_adapter *adapter,
>  		ret = -1;
>  	}
>  
> +	if (!ret)
> +		msleep(100);
> +

you could just add the delay after

  if (firmware_stat == FIRMWARE_READY_SDIO) {

this would be more read-able to me. Adding also a short comment like

  /* Firmware might pretend to be ready, when it's not.
   * Wait a little bit more as a workaround */


Francesco