Re: [EXT] Re: [PATCH v3] wifi: mwifiex: add extra delay for firmware ready
From: Marcel Ziswiler
Date: Thu Dec 14 2023 - 08:41:40 EST
On Thu, 2023-12-14 at 11:46 +0000, David Lin wrote:
> > 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.
Okay, any idea about that issue?
> 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.
Okay, but also that we have seen on 88W8997 at least on Verdin iMX8M Plus before. Looking at such logs more
closely, however, this seems after the firmware crashed and Linux tried to re-start it:
[ 4252.250242] mmc0: card 0001 removed
[ 4252.364265] mmc0: new ultra high speed SDR104 SDIO card at address 0001
[ 4252.374466] Bluetooth: vendor=0x2df, device=0x9142, class=255, fn=2
[ 4252.374475] mwifiex_sdio mmc0:0001:1: WLAN FW already running! Skip FW dnld
[ 4252.387774] mwifiex_sdio mmc0:0001:1: WLAN FW is active
[ 4252.394716] btmrvl_sdio mmc0:0001:2: sdio device tree data not available
[ 4257.569620] Bluetooth: module_cfg_cmd(f1) failed
[ 4262.433941] mwifiex_sdio mmc0:0001:1: mwifiex_cmd_timeout_func: Timeout cmd id = 0xa9, act = 0x0
Basically, once the firmware crashed there is no way ever to recover...
Any ideas?
> > > 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