| Message ID | 20231208234029.2197-1-yu-hao.lin@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] wifi: mwifiex: add extra delay for firmware ready | expand |
Hello David, thanks for your patch. On Sat, Dec 09, 2023 at 07:40:29AM +0800, David Lin wrote: > For SDIO IW416, due to a bug, 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> > Cc: stable@vger.kernel.org Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> Francesco
On Sat, Dec 09, 2023 at 07:40:29AM +0800, David Lin wrote: > For SDIO IW416, due to a bug, 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> > Cc: stable@vger.kernel.org Acked-by: Brian Norris <briannorris@chromium.org>
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 > 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> > Cc: stable@vger.kernel.org Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> # 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
> From: Marcel Ziswiler <marcel@ziswiler.com> > Sent: Thursday, December 14, 2023 6:44 PM > To: David Lin <yu-hao.lin@nxp.com>; linux-wireless@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; briannorris@chromium.org; > kvalo@kernel.org; francesco@dolcini.it; Pete Hsieh > <tsung-hsien.hsieh@nxp.com>; stable@vger.kernel.org > 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@nxp.com> > > Cc: stable@vger.kernel.org > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> # 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
On Thu, 2023-12-14 at 11:46 +0000, David Lin wrote: > > From: Marcel Ziswiler <marcel@ziswiler.com> > > Sent: Thursday, December 14, 2023 6:44 PM > > To: David Lin <yu-hao.lin@nxp.com>; linux-wireless@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org; briannorris@chromium.org; > > kvalo@kernel.org; francesco@dolcini.it; Pete Hsieh > > <tsung-hsien.hsieh@nxp.com>; stable@vger.kernel.org > > 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@nxp.com> > > > Cc: stable@vger.kernel.org > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> # 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
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > Sent: Thursday, December 14, 2023 9:35 PM > To: linux-wireless@vger.kernel.org; David Lin <yu-hao.lin@nxp.com> > Cc: kvalo@kernel.org; francesco@dolcini.it; Pete Hsieh > <tsung-hsien.hsieh@nxp.com>; stable@vger.kernel.org; > linux-kernel@vger.kernel.org; briannorris@chromium.org > Subject: Re: [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 Thu, 2023-12-14 at 11:46 +0000, David Lin wrote: > > > From: Marcel Ziswiler <marcel@ziswiler.com> > > > Sent: Thursday, December 14, 2023 6:44 PM > > > To: David Lin <yu-hao.lin@nxp.com>; linux-wireless@vger.kernel.org > > > Cc: linux-kernel@vger.kernel.org; briannorris@chromium.org; > > > kvalo@kernel.org; francesco@dolcini.it; Pete Hsieh > > > <tsung-hsien.hsieh@nxp.com>; stable@vger.kernel.org > > > 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? > From the log, it looks like firmware can't be ready in time. Maybe you can try to enlarge wait time to see if more time is needed for firmware to be ready. > > 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? > Card is removed then reinserted. "WLAN FW already running! Skip FW dnld" means driver skips firmware download under this case. It is hard to say if firmware is still all right. > > > > 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> > > > > Cc: stable@vger.kernel.org > > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> # 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
David Lin <yu-hao.lin@nxp.com> wrote: > For SDIO IW416, due to a bug, 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> > Cc: stable@vger.kernel.org > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> > Acked-by: Brian Norris <briannorris@chromium.org> > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> # Verdin AM62 (IW416) Patch applied to wireless-next.git, thanks. 1c5d463c0770 wifi: mwifiex: add extra delay for firmware ready
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; }; /*
For SDIO IW416, due to a bug, 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> Cc: stable@vger.kernel.org --- 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(+) base-commit: 783004b6dbda2cfe9a552a4cc9c1d168a2068f6c