Received-SPF: pass (google.com: domain of linux-wireless+bounces-1936-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <303651da-7ab7-4e49-8331-f541318b851a@gmail.com>
Date: Mon, 15 Jan 2024 17:29:06 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: rtlwifi: Speed up firmware loading for USB
Content-Language: en-US
To: Ping-Ke Shih <pkshih@realtek.com>,
 "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
References: <0d262acd-4f94-41c2-8d15-83486aeb976b@gmail.com>
 <7d8e8424b768418ea1d2a38ba874c400@realtek.com>
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <7d8e8424b768418ea1d2a38ba874c400@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 15/01/2024 02:50, Ping-Ke Shih wrote:
> 
> 
>> -----Original Message-----
>> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> Sent: Saturday, January 13, 2024 3:51 AM
>> To: linux-wireless@vger.kernel.org
>> Cc: Ping-Ke Shih <pkshih@realtek.com>; Larry Finger <Larry.Finger@lwfinger.net>
>> Subject: [PATCH] wifi: rtlwifi: Speed up firmware loading for USB
>>
>> Currently it takes almost 6 seconds to upload the firmware for RTL8192CU
>> (and 11 seconds for RTL8192DU). That's because the firmware is uploaded
>> one byte at a time.
>>
>> Also, after plugging the device, the firmware gets uploaded three times
>> before a connection to the AP is established.
>>
>> Maybe this is fine for most users, but when testing changes to the
>> driver it's really annoying to wait so long.
>>
>> Speed up the firmware upload by writing chunks of 64 bytes at a time.
>> This way it takes about 110 ms for RTL8192CU (and about 210 ms for
>> RTL8192DU).
>>
>> PCI devices could upload it in chunks of 4 bytes, but I don't have any
>> to test and commit 89d32c9071aa ("rtlwifi: Download firmware as bytes
>> rather than as dwords") decided otherwise anyway.
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>>  drivers/net/wireless/realtek/rtlwifi/efuse.c  | 65 +++++++++++++++++--
>>  drivers/net/wireless/realtek/rtlwifi/efuse.h  |  4 +-
>>  .../wireless/realtek/rtlwifi/rtl8192cu/sw.c   |  6 +-
>>  drivers/net/wireless/realtek/rtlwifi/usb.c    |  9 +++
>>  drivers/net/wireless/realtek/rtlwifi/wifi.h   |  8 +++
>>  5 files changed, 82 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtlwifi/efuse.c
>> b/drivers/net/wireless/realtek/rtlwifi/efuse.c
>> index 2e945554ed6d..870a276299f5 100644
>> --- a/drivers/net/wireless/realtek/rtlwifi/efuse.c
>> +++ b/drivers/net/wireless/realtek/rtlwifi/efuse.c
>> @@ -1287,18 +1287,73 @@ int rtl_get_hwinfo(struct ieee80211_hw *hw, struct rtl_priv *rtlpriv,
>>  }
>>  EXPORT_SYMBOL_GPL(rtl_get_hwinfo);
>>
>> -void rtl_fw_block_write(struct ieee80211_hw *hw, const u8 *buffer, u32 size)
>> +static void _rtl_fw_block_write_usb(struct ieee80211_hw *hw, u8 *buffer, u32 size)
>> +{
>> +       struct rtl_priv *rtlpriv = rtl_priv(hw);
>> +       u32 blockcount, blockcount8, blockcount4;
>> +       u32 remain8 = 0, remain4 = 0, remain = 0;
>> +       const u32 blocksize = 64;
>> +       const u32 blocksize8 = 8;
>> +       const u32 blocksize4 = 4;
>> +       u32 i, offset;
>> +
>> +       blockcount = size / blocksize;
>> +       remain8 = size % blocksize;
>> +       for (i = 0; i < blockcount; i++) {
>> +               offset = i * blocksize;
>> +               rtl_write_chunk(rtlpriv,
>> +                               START_ADDRESS + offset,
>> +                               blocksize, buffer + offset);
>> +       }
>> +
>> +       if (remain8) {
>> +               offset = blockcount * blocksize;
>> +               blockcount8 = remain8 / blocksize8;
>> +               remain4 = remain8 % blocksize8;
>> +
>> +               for (i = 0; i < blockcount8; i++)
>> +                       rtl_write_chunk(rtlpriv,
>> +                                       START_ADDRESS + offset + i * blocksize8,
>> +                                       blocksize8,
>> +                                       buffer + offset + i * blocksize8);
>> +       }
>> +
>> +       if (remain4) {
>> +               offset += blockcount8 * blocksize8;
>> +               blockcount4 = remain4 / blocksize4;
>> +               remain = remain8 % blocksize4;
>> +
>> +               for (i = 0; i < blockcount4; i++)
>> +                       rtl_write_dword(rtlpriv,
>> +                                       START_ADDRESS + offset + i * blocksize4,
>> +                                       cpu_to_le32(*(u32 *)(buffer + offset + i)));
> 
> Here should be le32_to_cpu(). 
> 

Right. But now I realise that rtl_write_dword is called at most once here,
so there is not much point using it.

>> +       }
>> +
>> +       if (remain) {
>> +               offset += blockcount4 * blocksize4;
>> +
>> +               for (i = 0; i < remain; i++)
>> +                       rtl_write_byte(rtlpriv, START_ADDRESS + offset + i,
>> +                                      *(buffer + offset + i));
>> +       }
>> +}
> 
> I think we can increase 'start' and 'buffer' addresses after writing, so 
> arithmetic can be simple. And, combine 64/8/4/1 block writing into single
> loop. Pseudo code like
> 
> static void _rtl_fw_block_write_usb(struct ieee80211_hw *hw, u8 *buffer, u32 size)
> {
> 	u32 start = START_ADDRESS;
> 	u32 n;
> 
> 	while (size > 0) {
> 		if (size >= 64) {
> 			n = 64;
> 			rtl_write_chunk(rtlpriv, start, 64, buffer);
> 		} else if (size >= 8) {
> 			n = 8;
> 			rtl_write_chunk(rtlpriv, start, 8, buffer);
> 		} else if (size >= 4) {
> 			n = 4;
> 			rtl_write_dword(rtlpriv, start, le32_to_cpu(*(u32 *)buffer));
> 		} else {
> 			n = 1;
> 			rtl_write_byte(rtlpriv, start, *buffer);
> 		}
> 
> 		start += n;
> 		buffer += n;
> 		size -= n;
> 	}
> }
> 

Why didn't I think of that? (Because I didn't think much, just copied
the code from the RTL8192DU driver and cleaned it a bit.)

>> +
>> +void rtl_fw_block_write(struct ieee80211_hw *hw, u8 *buffer, u32 size)
>>  {
>>         struct rtl_priv *rtlpriv = rtl_priv(hw);
>> -       u8 *pu4byteptr = (u8 *)buffer;
>>         u32 i;
>>
>> -       for (i = 0; i < size; i++)
>> -               rtl_write_byte(rtlpriv, (START_ADDRESS + i), *(pu4byteptr + i));
>> +       if (rtlpriv->rtlhal.interface == INTF_PCI) {
>> +               for (i = 0; i < size; i++)
>> +                       rtl_write_byte(rtlpriv, (START_ADDRESS + i),
>> +                                      *(buffer + i));
>> +       } else if (rtlpriv->rtlhal.interface == INTF_USB) {
>> +               _rtl_fw_block_write_usb(hw, buffer, size);
>> +       }
>>  }
>>  EXPORT_SYMBOL_GPL(rtl_fw_block_write);
>>
>> -void rtl_fw_page_write(struct ieee80211_hw *hw, u32 page, const u8 *buffer,
>> +void rtl_fw_page_write(struct ieee80211_hw *hw, u32 page, u8 *buffer,
>>                        u32 size)
>>  {
>>         struct rtl_priv *rtlpriv = rtl_priv(hw);
>> diff --git a/drivers/net/wireless/realtek/rtlwifi/efuse.h
>> b/drivers/net/wireless/realtek/rtlwifi/efuse.h
>> index 1ec59f439382..4821625ad1e5 100644
>> --- a/drivers/net/wireless/realtek/rtlwifi/efuse.h
>> +++ b/drivers/net/wireless/realtek/rtlwifi/efuse.h
>> @@ -91,8 +91,8 @@ void efuse_power_switch(struct ieee80211_hw *hw, u8 write, u8 pwrstate);
>>  int rtl_get_hwinfo(struct ieee80211_hw *hw, struct rtl_priv *rtlpriv,
>>                    int max_size, u8 *hwinfo, int *params);
>>  void rtl_fill_dummy(u8 *pfwbuf, u32 *pfwlen);
>> -void rtl_fw_page_write(struct ieee80211_hw *hw, u32 page, const u8 *buffer,
>> +void rtl_fw_page_write(struct ieee80211_hw *hw, u32 page, u8 *buffer,
>>                        u32 size);
>> -void rtl_fw_block_write(struct ieee80211_hw *hw, const u8 *buffer, u32 size);
>> +void rtl_fw_block_write(struct ieee80211_hw *hw, u8 *buffer, u32 size);
>>  void rtl_efuse_ops_init(struct ieee80211_hw *hw);
>>  #endif
>> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/sw.c
>> b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/sw.c
>> index 20b4aac69642..9f4cf09090d6 100644
>> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/sw.c
>> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/sw.c
>> @@ -40,7 +40,7 @@ static int rtl92cu_init_sw_vars(struct ieee80211_hw *hw)
>>         rtlpriv->dm.thermalvalue = 0;
>>
>>         /* for firmware buf */
>> -       rtlpriv->rtlhal.pfirmware = vzalloc(0x4000);
>> +       rtlpriv->rtlhal.pfirmware = kmalloc(0x4000, GFP_KERNEL);
> 
> Why should we use kmalloc instead? I don't see any description about this in
> commit message. 
> 

That's because usb_control_msg() can't use memory allocated by vmalloc:

Jan 09 19:39:29 ideapad2 kernel: xhci_hcd 0000:03:00.3: rejecting DMA map of vmalloc memory

I will mention this in the commit message.

>>         if (!rtlpriv->rtlhal.pfirmware) {
>>                 pr_err("Can't alloc buffer for fw\n");
>>                 return 1;
>> @@ -61,7 +61,7 @@ static int rtl92cu_init_sw_vars(struct ieee80211_hw *hw)
>>                                       fw_name, rtlpriv->io.dev,
>>                                       GFP_KERNEL, hw, rtl_fw_cb);
>>         if (err) {
>> -               vfree(rtlpriv->rtlhal.pfirmware);
>> +               kfree(rtlpriv->rtlhal.pfirmware);
>>                 rtlpriv->rtlhal.pfirmware = NULL;
>>         }
>>         return err;
>> @@ -72,7 +72,7 @@ static void rtl92cu_deinit_sw_vars(struct ieee80211_hw *hw)
>>         struct rtl_priv *rtlpriv = rtl_priv(hw);
>>
>>         if (rtlpriv->rtlhal.pfirmware) {
>> -               vfree(rtlpriv->rtlhal.pfirmware);
>> +               kfree(rtlpriv->rtlhal.pfirmware);
>>                 rtlpriv->rtlhal.pfirmware = NULL;
>>         }
>>  }
> 
> [...]
> 
>