* [PATCH] wifi: rtl8xxxu: retry firmware download on error @ 2025-01-22 15:31 Soeren Moch 2025-01-23 0:21 ` Ping-Ke Shih 0 siblings, 1 reply; 4+ messages in thread From: Soeren Moch @ 2025-01-22 15:31 UTC (permalink / raw) To: Kalle Valo; +Cc: Soeren Moch, Jes Sorensen, linux-wireless, linux-kernel Occasionally there is an EPROTO error during firmware download. This error is converted to EAGAIN in the download function. But nobody tries again and so device probe fails. Implement download retry to fix this. Signed-off-by: Soeren Moch <smoch@web.de> --- Cc: Jes Sorensen <Jes.Sorensen@gmail.com> Cc: Kalle Valo <kvalo@kernel.org> Cc: linux-wireless@vger.kernel.org Cc: linux-kernel@vger.kernel.org This error was observed (and fix tested) on a tbs2910 board [1] with an embedded RTL8188EU (0bda:8179) device behind a USB hub. [1] arch/arm/boot/dts/nxp/imx/imx6q-tbs2910.dts --- drivers/net/wireless/realtek/rtl8xxxu/core.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/realtek/rtl8xxxu/core.c b/drivers/net/wireless/realtek/rtl8xxxu/core.c index 7891c988dd5f..cd7d904eae62 100644 --- a/drivers/net/wireless/realtek/rtl8xxxu/core.c +++ b/drivers/net/wireless/realtek/rtl8xxxu/core.c @@ -4064,8 +4064,14 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw) */ rtl8xxxu_write16(priv, REG_TRXFF_BNDY + 2, fops->trxff_boundary); - ret = rtl8xxxu_download_firmware(priv); - dev_dbg(dev, "%s: download_firmware %i\n", __func__, ret); + for (int retry = 5; retry ; retry--) { + ret = rtl8xxxu_download_firmware(priv); + dev_dbg(dev, "%s: download_firmware %i\n", __func__, ret); + if (ret != -EAGAIN) + break; + if (retry) + dev_info(dev, "retry firmware download\n"); + } if (ret) goto exit; ret = rtl8xxxu_start_firmware(priv); -- 2.43.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH] wifi: rtl8xxxu: retry firmware download on error 2025-01-22 15:31 [PATCH] wifi: rtl8xxxu: retry firmware download on error Soeren Moch @ 2025-01-23 0:21 ` Ping-Ke Shih 2025-01-23 1:22 ` Soeren Moch 0 siblings, 1 reply; 4+ messages in thread From: Ping-Ke Shih @ 2025-01-23 0:21 UTC (permalink / raw) To: Soeren Moch, Kalle Valo Cc: Jes Sorensen, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org Soeren Moch <smoch@web.de> wrote: > Occasionally there is an EPROTO error during firmware download. > This error is converted to EAGAIN in the download function. > But nobody tries again and so device probe fails. > > Implement download retry to fix this. > > Signed-off-by: Soeren Moch <smoch@web.de> > --- > Cc: Jes Sorensen <Jes.Sorensen@gmail.com> > Cc: Kalle Valo <kvalo@kernel.org> > Cc: linux-wireless@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > > This error was observed (and fix tested) on a tbs2910 board [1] > with an embedded RTL8188EU (0bda:8179) device behind a USB hub. > > [1] arch/arm/boot/dts/nxp/imx/imx6q-tbs2910.dts These can be in formal commit message. > --- > drivers/net/wireless/realtek/rtl8xxxu/core.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/core.c > b/drivers/net/wireless/realtek/rtl8xxxu/core.c > index 7891c988dd5f..cd7d904eae62 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/core.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/core.c > @@ -4064,8 +4064,14 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw) > */ > rtl8xxxu_write16(priv, REG_TRXFF_BNDY + 2, fops->trxff_boundary); > > - ret = rtl8xxxu_download_firmware(priv); > - dev_dbg(dev, "%s: download_firmware %i\n", __func__, ret); > + for (int retry = 5; retry ; retry--) { > + ret = rtl8xxxu_download_firmware(priv); > + dev_dbg(dev, "%s: download_firmware %i\n", __func__, ret); > + if (ret != -EAGAIN) > + break; > + if (retry) > + dev_info(dev, "retry firmware download\n"); It looks like 'if (retry)' is always true and unnecessary. Also, this message isn't so important to user, so dev_dbg() is more suitable, but already printing "%s: download_firmware %i\n" for every retry is enough? Or move it out of loop? > + } > if (ret) > goto exit; > ret = rtl8xxxu_start_firmware(priv); > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] wifi: rtl8xxxu: retry firmware download on error 2025-01-23 0:21 ` Ping-Ke Shih @ 2025-01-23 1:22 ` Soeren Moch 2025-01-23 2:52 ` Ping-Ke Shih 0 siblings, 1 reply; 4+ messages in thread From: Soeren Moch @ 2025-01-23 1:22 UTC (permalink / raw) To: Ping-Ke Shih Cc: Kalle Valo, Jes Sorensen, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org On 23.01.25 01:21, Ping-Ke Shih wrote: > Soeren Moch <smoch@web.de> wrote: >> Occasionally there is an EPROTO error during firmware download. >> This error is converted to EAGAIN in the download function. >> But nobody tries again and so device probe fails. >> >> Implement download retry to fix this. >> >> Signed-off-by: Soeren Moch <smoch@web.de> >> --- >> Cc: Jes Sorensen <Jes.Sorensen@gmail.com> >> Cc: Kalle Valo <kvalo@kernel.org> >> Cc: linux-wireless@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> >> This error was observed (and fix tested) on a tbs2910 board [1] >> with an embedded RTL8188EU (0bda:8179) device behind a USB hub. >> >> [1] arch/arm/boot/dts/nxp/imx/imx6q-tbs2910.dts > These can be in formal commit message. OK, I'm happy to move this to the formal commit message if you think this would be helpful. > >> --- >> drivers/net/wireless/realtek/rtl8xxxu/core.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/core.c >> b/drivers/net/wireless/realtek/rtl8xxxu/core.c >> index 7891c988dd5f..cd7d904eae62 100644 >> --- a/drivers/net/wireless/realtek/rtl8xxxu/core.c >> +++ b/drivers/net/wireless/realtek/rtl8xxxu/core.c >> @@ -4064,8 +4064,14 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw) >> */ >> rtl8xxxu_write16(priv, REG_TRXFF_BNDY + 2, fops->trxff_boundary); >> >> - ret = rtl8xxxu_download_firmware(priv); >> - dev_dbg(dev, "%s: download_firmware %i\n", __func__, ret); >> + for (int retry = 5; retry ; retry--) { >> + ret = rtl8xxxu_download_firmware(priv); >> + dev_dbg(dev, "%s: download_firmware %i\n", __func__, ret); >> + if (ret != -EAGAIN) >> + break; >> + if (retry) >> + dev_info(dev, "retry firmware download\n"); > It looks like 'if (retry)' is always true and unnecessary. Correct. After seeing the patch in my Inbox I already realized that the for loop should be for (int retry = 5; retry >= 0; retry--) This would make clearer to have a maximum of 5 retries (6 attempts in total) and also avoid this message after the last possible retry. > Also, this message > isn't so important to user, so dev_dbg() is more suitable, but already printing > "%s: download_firmware %i\n" for every retry is enough? Or move it out of loop? I think this message is important since there is the "failed to write block" error message before: [ 6.239205] usb 2-1.1: RTL8188EU rev D (TSMC) romver 0, 1T1R, TX queues 2, WiFi=1, BT=0, GPS=0, HI PA=0 [ 6.248793] usb 2-1.1: RTL8188EU MAC: 00:33:0e:00:15:8e [ 6.254218] usb 2-1.1: rtl8xxxu: Loading firmware rtlwifi/rtl8188eufw.bin [ 6.343753] usb 2-1.1: Firmware revision 11.1 (signature 0x88e1) [ 6.374659] usb 2-1.1: rtl8xxxu_writeN: Failed to write block at addr: 1c80 size: 0080 [ 6.382894] usb 2-1.1: retry firmware download [ 7.221505] usbcore: registered new interface driver rtl8xxxu So the retry message after the error makes clear that the error will hopefully be corrected. Since rtl8xxxu_writeN is only used for firmware download, an other option would be to downgrade both the error and the retry message to dev_dbg(). What do you think? Regards, Soeren > >> + } >> if (ret) >> goto exit; >> ret = rtl8xxxu_start_firmware(priv); >> -- >> 2.43.0 >> ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] wifi: rtl8xxxu: retry firmware download on error 2025-01-23 1:22 ` Soeren Moch @ 2025-01-23 2:52 ` Ping-Ke Shih 0 siblings, 0 replies; 4+ messages in thread From: Ping-Ke Shih @ 2025-01-23 2:52 UTC (permalink / raw) To: Soeren Moch Cc: Kalle Valo, Jes Sorensen, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org Soeren Moch <smoch@web.de> wrote: > > Also, this message > > isn't so important to user, so dev_dbg() is more suitable, but already printing > > "%s: download_firmware %i\n" for every retry is enough? Or move it out of loop? > I think this message is important since there is the "failed to write block" > error message before: > > [ 6.239205] usb 2-1.1: RTL8188EU rev D (TSMC) romver 0, 1T1R, TX > queues 2, WiFi=1, BT=0, GPS=0, HI PA=0 > [ 6.248793] usb 2-1.1: RTL8188EU MAC: 00:33:0e:00:15:8e > [ 6.254218] usb 2-1.1: rtl8xxxu: Loading firmware rtlwifi/rtl8188eufw.bin > [ 6.343753] usb 2-1.1: Firmware revision 11.1 (signature 0x88e1) > [ 6.374659] usb 2-1.1: rtl8xxxu_writeN: Failed to write block at > addr: 1c80 size: 0080 > [ 6.382894] usb 2-1.1: retry firmware download > [ 7.221505] usbcore: registered new interface driver rtl8xxxu > > So the retry message after the error makes clear that the error will > hopefully > be corrected. > > Since rtl8xxxu_writeN is only used for firmware download, an other option > would be to downgrade both the error and the retry message to dev_dbg(). > > What do you think? > It looks like rtl8xxxu uses this kind of debug pattern for writing: if (rtl8xxxu_debug & RTL8XXXU_DEBUG_REG_WRITE) dev_info(&udev->dev, "%s(%04x) = 0x%08x\n", __func__, addr, val); Maybe rtl8xxxu_writeN() can use the same pattern. Then, no "rtl8xxxu_writeN: Failed to write block at..." to users. Ping-Ke ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-01-23 2:52 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-22 15:31 [PATCH] wifi: rtl8xxxu: retry firmware download on error Soeren Moch 2025-01-23 0:21 ` Ping-Ke Shih 2025-01-23 1:22 ` Soeren Moch 2025-01-23 2:52 ` Ping-Ke Shih
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox