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

[David Lin]

> From: Francesco Dolcini <francesco@xxxxxxxxxx>
> Sent: Wednesday, December 6, 2023 4:01 AM
> To: David Lin <yu-hao.lin@xxxxxxx>
> Cc: linux-wireless@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> briannorris@xxxxxxxxxxxx; kvalo@xxxxxxxxxx; francesco@xxxxxxxxxx; Pete
> Hsieh <tsung-hsien.hsieh@xxxxxxx>
> 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@xxxxxxx>
> 
> 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