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: <e08051130a124fcfab8d42e6893c581a@realtek.com>
Date: Thu, 23 Jan 2025 00:21:08 +0000
From: Ping-Ke Shih <pkshih@...ltek.com>
To: Soeren Moch <smoch@....de>, Kalle Valo <kvalo@...nel.org>
CC: 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

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. 

> ---
>  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
>