wifi: mwifiex: added extra delay for firmware ready.

[PATCH] wifi: mwifiex: added extra delay for firmware ready.

Posted by David Lin 2 years ago

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>
---
 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);
+
 	return ret;
 }
 

base-commit: 783004b6dbda2cfe9a552a4cc9c1d168a2068f6c
-- 
2.25.1

Re: [PATCH] wifi: mwifiex: added extra delay for firmware ready.

Posted by Brian Norris 2 years ago

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.

Do you have any idea why? Is it specific to this chip and/or firmware?
I'm hesitant to add magic sleeps to everything, just because you have
one buggy chip/firmware.

If it's a known issue with a single chip, it seems like you should add a
flag to struct mwifiex_sdio_device / mwifiex_sdio_sd8978.

Brian

> Workaround by adding 100ms delay at checking FW status.
> 
> Signed-off-by: David Lin <yu-hao.lin@nxp.com>

RE: [EXT] Re: [PATCH] wifi: mwifiex: added extra delay for firmware ready.

Posted by David Lin 2 years ago

> From: Brian Norris <briannorris@chromium.org>
> Sent: Wednesday, December 6, 2023 4:27 AM
> To: David Lin <yu-hao.lin@nxp.com>
> Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org;
> kvalo@kernel.org; francesco@dolcini.it; Pete Hsieh
> <tsung-hsien.hsieh@nxp.com>
> Subject: [EXT] Re: [PATCH] wifi: mwifiex: added extra delay for firmware ready.
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> 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.
> 
> Do you have any idea why? Is it specific to this chip and/or firmware?
> I'm hesitant to add magic sleeps to everything, just because you have one
> buggy chip/firmware.
> 
> If it's a known issue with a single chip, it seems like you should add a flag to
> struct mwifiex_sdio_device / mwifiex_sdio_sd8978.
> 

This issue is reported by customer
We confirmed it's specific to this chip and 100ms is sufficient and relatively safe/easy then change FW. 

Will add flag to struct mwifiex_sdio_device / mwifiex_sdio_sd8978

> Brian
> 
> > Workaround by adding 100ms delay at checking FW status.
> >
> > Signed-off-by: David Lin <yu-hao.lin@nxp.com>

Re: [PATCH] wifi: mwifiex: added extra delay for firmware ready.

Posted by Francesco Dolcini 2 years ago

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

RE: [EXT] Re: [PATCH] wifi: mwifiex: added extra delay for firmware ready.

Posted by David Lin 2 years ago

> From: Francesco Dolcini <francesco@dolcini.it>
> Sent: Wednesday, December 6, 2023 4:01 AM
> 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; Pete
> Hsieh <tsung-hsien.hsieh@nxp.com>
> Subject: [EXT] Re: [PATCH] wifi: mwifiex: added extra delay for firmware ready.
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> 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.
> 

Thanks. I will fix it in patch v2.

> 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@...
> 

Thanks. I will add "cc: stable" in patch v2.

> > ---
> >  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 */
> 

O.K.

> 
> Francesco