RE: [EXT] Re: [PATCH v3] wifi: mwifiex: add extra delay for firmware ready

[David Lin]

> From: Marcel Ziswiler <marcel@xxxxxxxxxxxx>
> Sent: Thursday, December 14, 2023 6:44 PM
> To: David Lin <yu-hao.lin@xxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; briannorris@xxxxxxxxxxxx;
> kvalo@xxxxxxxxxx; francesco@xxxxxxxxxx; Pete Hsieh
> <tsung-hsien.hsieh@xxxxxxx>; stable@xxxxxxxxxxxxxxx
> Subject: [EXT] Re: [PATCH v3] wifi: mwifiex: add 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 Sat, 2023-12-09 at 07:40 +0800, David Lin wrote:
> > For SDIO IW416, due to a bug, FW may return ready before complete full
> > initialization.
> 
> BTW: What makes you think this issue is exclusive to the IW416?
> 
> We have also seen this in the past both on our Verdin iMX8M Mini
> (SDIO/SDIO) and Verdin iMX8M Plus (SDIO/UART) with 88W8997.
> 
> good case:
> 
> [    6.496541] mwifiex_sdio mmc0:0001:1: info: FW download over, size
> 594556 bytes
> ...
> [    7.272436] mwifiex_sdio mmc0:0001:1: WLAN FW is active
> [    7.314958] mwifiex_sdio mmc0:0001:1: Unknown api_id: 5
> [    7.347647] mwifiex_sdio mmc0:0001:1: info: MWIFIEX VERSION: mwifiex
> 1.0 (16.92.21.p55)
> [    7.355977] mwifiex_sdio mmc0:0001:1: driver_version = mwifiex 1.0
> (16.92.21.p55)
> 
> bad case:
> 
> [    8.720216] mwifiex_sdio mmc0:0001:1: info: FW download over, size
> 594556 bytes
> ...
> [   24.976699] mwifiex_sdio mmc0:0001:1: FW failed to be active in time
> [   24.983098] mwifiex_sdio mmc0:0001:1: info: _mwifiex_fw_dpc:
> unregister device
> 

From the log, it is not related to the issue fixed by this patch. This patch fixes command timeout for the first command sent to firmware.

Error log:

[   20.192096] mwifiex_sdio mmc1:0001:1: mwifiex_cmd_timeout_func: Timeout cmd id = 0xa9, act = 0x0

Command ID indicates this it the first command sent to firmware.



> > 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>
> > Cc: stable@xxxxxxxxxxxxxxx
> Tested-by: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx> # Verdin AM62
> (IW416)
> 
> > ---
> >
> > v3:
> >    - v2 was a not finished patch that was send to the LKML by mistake
> >    - changed check condition for extra delay with clear comments.
> >    - added flag to struct mwifiex_sdio_device / mwifiex_sdio_sd8978 to
> >      enable extra delay only for IW416.
> > ---
> >  drivers/net/wireless/marvell/mwifiex/sdio.c | 19 +++++++++++++++++++
> > drivers/net/wireless/marvell/mwifiex/sdio.h |  2 ++
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > index 6462a0ffe698..ef3e68d1059c 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > @@ -331,6 +331,7 @@ static const struct mwifiex_sdio_device
> mwifiex_sdio_sd8786 = {
> >       .can_dump_fw = false,
> >       .can_auto_tdls = false,
> >       .can_ext_scan = false,
> > +     .fw_ready_extra_delay = false,
> >  };
> >
> >  static const struct mwifiex_sdio_device mwifiex_sdio_sd8787 = { @@
> > -346,6 +347,7 @@ static const struct mwifiex_sdio_device
> mwifiex_sdio_sd8787 = {
> >       .can_dump_fw = false,
> >       .can_auto_tdls = false,
> >       .can_ext_scan = true,
> > +     .fw_ready_extra_delay = false,
> >  };
> >
> >  static const struct mwifiex_sdio_device mwifiex_sdio_sd8797 = { @@
> > -361,6 +363,7 @@ static const struct mwifiex_sdio_device
> mwifiex_sdio_sd8797 = {
> >       .can_dump_fw = false,
> >       .can_auto_tdls = false,
> >       .can_ext_scan = true,
> > +     .fw_ready_extra_delay = false,
> >  };
> >
> >  static const struct mwifiex_sdio_device mwifiex_sdio_sd8897 = { @@
> > -376,6 +379,7 @@ static const struct mwifiex_sdio_device
> mwifiex_sdio_sd8897 = {
> >       .can_dump_fw = true,
> >       .can_auto_tdls = false,
> >       .can_ext_scan = true,
> > +     .fw_ready_extra_delay = false,
> >  };
> >
> >  static const struct mwifiex_sdio_device mwifiex_sdio_sd8977 = { @@
> > -392,6 +396,7 @@ static const struct mwifiex_sdio_device
> mwifiex_sdio_sd8977 = {
> >       .fw_dump_enh = true,
> >       .can_auto_tdls = false,
> >       .can_ext_scan = true,
> > +     .fw_ready_extra_delay = false,
> >  };
> >
> >  static const struct mwifiex_sdio_device mwifiex_sdio_sd8978 = { @@
> > -408,6 +413,7 @@ static const struct mwifiex_sdio_device
> mwifiex_sdio_sd8978 = {
> >       .fw_dump_enh = true,
> >       .can_auto_tdls = false,
> >       .can_ext_scan = true,
> > +     .fw_ready_extra_delay = true,
> >  };
> >
> >  static const struct mwifiex_sdio_device mwifiex_sdio_sd8997 = { @@
> > -425,6 +431,7 @@ static const struct mwifiex_sdio_device
> mwifiex_sdio_sd8997 = {
> >       .fw_dump_enh = true,
> >       .can_auto_tdls = false,
> >       .can_ext_scan = true,
> > +     .fw_ready_extra_delay = false,
> >  };
> >
> >  static const struct mwifiex_sdio_device mwifiex_sdio_sd8887 = { @@
> > -440,6 +447,7 @@ static const struct mwifiex_sdio_device
> mwifiex_sdio_sd8887 = {
> >       .can_dump_fw = false,
> >       .can_auto_tdls = true,
> >       .can_ext_scan = true,
> > +     .fw_ready_extra_delay = false,
> >  };
> >
> >  static const struct mwifiex_sdio_device mwifiex_sdio_sd8987 = { @@
> > -456,6 +464,7 @@ static const struct mwifiex_sdio_device
> mwifiex_sdio_sd8987 = {
> >       .fw_dump_enh = true,
> >       .can_auto_tdls = true,
> >       .can_ext_scan = true,
> > +     .fw_ready_extra_delay = false,
> >  };
> >
> >  static const struct mwifiex_sdio_device mwifiex_sdio_sd8801 = { @@
> > -471,6 +480,7 @@ static const struct mwifiex_sdio_device
> mwifiex_sdio_sd8801 = {
> >       .can_dump_fw = false,
> >       .can_auto_tdls = false,
> >       .can_ext_scan = true,
> > +     .fw_ready_extra_delay = false,
> >  };
> >
> >  static struct memory_type_mapping generic_mem_type_map[] = { @@
> > -563,6 +573,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct
> sdio_device_id *id)
> >               card->fw_dump_enh = data->fw_dump_enh;
> >               card->can_auto_tdls = data->can_auto_tdls;
> >               card->can_ext_scan = data->can_ext_scan;
> > +             card->fw_ready_extra_delay =
> data->fw_ready_extra_delay;
> >               INIT_WORK(&card->work, mwifiex_sdio_work);
> >       }
> >
> > @@ -766,6 +777,7 @@ mwifiex_sdio_read_fw_status(struct
> mwifiex_adapter
> > *adapter, u16 *dat)  static int mwifiex_check_fw_status(struct
> mwifiex_adapter *adapter,
> >                                  u32 poll_num)  {
> > +     struct sdio_mmc_card *card = adapter->card;
> >       int ret = 0;
> >       u16 firmware_stat;
> >       u32 tries;
> > @@ -783,6 +795,13 @@ static int mwifiex_check_fw_status(struct
> mwifiex_adapter *adapter,
> >               ret = -1;
> >       }
> >
> > +     if (card->fw_ready_extra_delay &&
> > +         firmware_stat == FIRMWARE_READY_SDIO)
> > +             /* firmware might pretend to be ready, when it's not.
> > +              * Wait a little bit more as a workaround.
> > +              */
> > +             msleep(100);
> > +
> >       return ret;
> >  }
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h
> > b/drivers/net/wireless/marvell/mwifiex/sdio.h
> > index b86a9263a6a8..cb63ad55d675 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/sdio.h
> > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
> > @@ -255,6 +255,7 @@ struct sdio_mmc_card {
> >       bool fw_dump_enh;
> >       bool can_auto_tdls;
> >       bool can_ext_scan;
> > +     bool fw_ready_extra_delay;
> >
> >       struct mwifiex_sdio_mpa_tx mpa_tx;
> >       struct mwifiex_sdio_mpa_rx mpa_rx; @@ -278,6 +279,7 @@ struct
> > mwifiex_sdio_device {
> >       bool fw_dump_enh;
> >       bool can_auto_tdls;
> >       bool can_ext_scan;
> > +     bool fw_ready_extra_delay;
> >  };
> >
> >  /*
> >
> > base-commit: 783004b6dbda2cfe9a552a4cc9c1d168a2068f6c