linux-kernel - Re: [PATCH] wifi: rtl8xxxu: retry firmware download on error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0fb6f633-7ce0-4229-a2fc-df65aeeefd83@web.de>
Date: Thu, 23 Jan 2025 02:22:01 +0100
From: Soeren Moch <smoch@....de>
To: Ping-Ke Shih <pkshih@...ltek.com>
Cc: Kalle Valo <kvalo@...nel.org>, Jes Sorensen <Jes.Sorensen@...il.com>,
 "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] wifi: rtl8xxxu: retry firmware download on error

On 23.01.25 01:21, Ping-Ke Shih wrote:
> Soeren Moch <smoch@....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@....de>
>> ---
>> Cc: Jes Sorensen <Jes.Sorensen@...il.com>
>> Cc: Kalle Valo <kvalo@...nel.org>
>> Cc: linux-wireless@...r.kernel.org
>> Cc: linux-kernel@...r.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
>>