Received-SPF: pass (google.com: domain of linux-wireless+bounces-8149-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <29f850c5-4f61-466f-9a7a-437b05bc8251@gmail.com>
Date: Tue, 28 May 2024 02:06:23 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v7 12/12] wifi: rtlwifi: Enable the new rtl8192du driver
To: Stefan Lippers-Hollmann <s.l-h@gmx.de>, Ping-Ke Shih <pkshih@realtek.com>
Cc: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
 Larry Finger <Larry.Finger@lwfinger.net>,
 Christian Hewitt <chewitt@libreelec.tv>,
 Johannes Berg <johannes@sipsolutions.net>
References: <8805826b-60b9-4026-9509-7d92c3a43577@gmail.com>
 <5f2da7ee-876a-42fc-8fec-ec5386fa8c26@gmail.com>
 <0002749a3b584bc39fa18b3137153fdf@realtek.com> <20240527112534.4dbcdf75@mir>
Content-Language: en-US
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <20240527112534.4dbcdf75@mir>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 27/05/2024 12:25, Stefan Lippers-Hollmann wrote:
> Hi
> 
> On 2024-05-27, Ping-Ke Shih wrote:
>> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>>> The RTL8192DU is an older Wifi 4 dual band chip. It comes in two
>>> flavours: single MAC single PHY (like most Realtek Wifi 4 USB devices),
>>> and dual MAC dual PHY.
> [...]
>>> Stefan Lippers-Hollmann tested the speed in the 5 GHz band with 40 MHz
>>> channel width: 173/99 megabits/second.
>>
>> Could you please confirm this description is still correct in v7?
>> If so, your Tested-by tag is welcome.
> 
> I held back with a formal Tested-by for this, as rtlwifi needs
> some fixing after the chanctx changes
> (0a44dfc070749514b804ccac0b1fd38718f7daa1, "wifi: mac80211:
> simplify non-chanctx drivers") went in (for v6.9 and up).
> 
> So, yes, rtl8192du v7 itself works fine (as did v6 and previous), but
> the chanctx changes do cause a performance regression, resulting in
> lower (and very inconsistent) throughput and larger packet loss,
> due to the rate control being confused. As Bitterblue found out,
> rtlwifi relies on the previous behaviour imposed by
> 
> ieee80211_hw_config(local, ~0);
> 
> As a consequence of the change to
> 
> ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_LISTEN_INTERVAL |
> 				   IEEE80211_CONF_CHANGE_MONITOR |
> 				   IEEE80211_CONF_CHANGE_PS |
> 				   IEEE80211_CONF_CHANGE_RETRY_LIMITS |
> 				   IEEE80211_CONF_CHANGE_IDLE);
> 
> mac80211 effectively sets a retry limit of 0x4, while rtlwifi
> expects a retry limit of 0x30 (in STA mode or 0x7 in AP mode),
> causing rtlwifi not to settle on a good fallback rate (and thereby
> losing packets).
> 
> Disclaimer: the above issue and its explanation has been discovered
> by Bitterblue, I just did the git bisecting (rtl8192du on top of
> v6.8..v6.9) and performance testing to pinpoint the commit causing
> the regression.
> 
> rtl8192du on top of v6.9+ (with the chanctx changes):
> $ iperf3 -c <remote_ipv6>
> [ ID] Interval           Transfer     Bitrate
> [  5]   0.00-1.00   sec   384 KBytes  3.14 Mbits/sec
> [  5]   1.00-2.00   sec   512 KBytes  4.19 Mbits/sec
> [  5]   2.00-3.00   sec  1.00 MBytes  8.39 Mbits/sec
> [  5]   3.00-4.00   sec   640 KBytes  5.24 Mbits/sec
> [  5]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec
> [  5]   5.00-6.00   sec  0.00 Bytes  0.00 bits/sec
> [  5]   6.00-7.00   sec   256 KBytes  2.10 Mbits/sec
> [  5]   7.00-8.00   sec   640 KBytes  5.25 Mbits/sec
> [  5]   8.00-9.00   sec   512 KBytes  4.19 Mbits/sec
> [  5]   9.00-10.00  sec   256 KBytes  2.10 Mbits/sec
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bitrate
> [  5]   0.00-10.00  sec  4.12 MBytes  3.46 Mbits/sec                  receiver
> 
> $ iperf3 -c <remote_ipv6> --reverse
> [ ID] Interval           Transfer     Bitrate         Retr  Cwnd
> [  5]   0.00-1.00   sec  12.9 MBytes   108 Mbits/sec    0    629 KBytes
> [  5]   1.00-2.00   sec  9.75 MBytes  81.8 Mbits/sec    0   1.13 MBytes
> [  5]   2.00-3.00   sec  11.0 MBytes  92.3 Mbits/sec    0   1.63 MBytes
> [  5]   3.00-4.00   sec  1.38 MBytes  11.5 Mbits/sec    1   1.67 MBytes
> [  5]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec    1   1.39 KBytes
> [  5]   5.00-6.00   sec  0.00 Bytes  0.00 bits/sec    1   1.39 KBytes
> [  5]   6.00-7.00   sec  0.00 Bytes  0.00 bits/sec    0   1.39 KBytes
> [  5]   7.00-8.00   sec  9.88 MBytes  82.8 Mbits/sec    0   2.12 MBytes
> [  5]   8.00-9.00   sec  6.88 MBytes  57.7 Mbits/sec    3   2.32 MBytes
> [  5]   9.00-10.00  sec  0.00 Bytes  0.00 bits/sec    0   2.32 MBytes
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bitrate         Retr
> [  5]   0.00-10.00  sec  51.8 MBytes  43.4 Mbits/sec    6             sender
> 
> $ iperf3 -c <remote_ipv6> --bidir
> [ ID][Role] Interval           Transfer     Bitrate         Retr  Cwnd
> [  5][RX-S]   0.00-1.00   sec  0.00 Bytes  0.00 bits/sec
> [  8][TX-S]   0.00-1.00   sec  1.38 MBytes  11.5 Mbits/sec    0   90.6 KBytes
> [  5][RX-S]   1.00-2.00   sec  0.00 Bytes  0.00 bits/sec
> [  8][TX-S]   1.00-2.00   sec  4.12 MBytes  34.6 Mbits/sec    4   1.39 KBytes
> [  5][RX-S]   2.00-3.00   sec  0.00 Bytes  0.00 bits/sec
> [  8][TX-S]   2.00-3.00   sec  0.00 Bytes  0.00 bits/sec    1   1.39 KBytes
> [  5][RX-S]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec
> [  8][TX-S]   3.00-4.00   sec  0.00 Bytes  0.00 bits/sec    1   1.39 KBytes
> [  5][RX-S]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec
> [  8][TX-S]   4.00-5.00   sec  0.00 Bytes  0.00 bits/sec    0   1.39 KBytes
> [  5][RX-S]   5.00-6.00   sec  0.00 Bytes  0.00 bits/sec
> [  8][TX-S]   5.00-6.00   sec  0.00 Bytes  0.00 bits/sec    2    106 KBytes
> [  5][RX-S]   6.00-7.00   sec  0.00 Bytes  0.00 bits/sec
> [  8][TX-S]   6.00-7.00   sec  0.00 Bytes  0.00 bits/sec    0    106 KBytes
> [  5][RX-S]   7.00-8.00   sec  0.00 Bytes  0.00 bits/sec
> [  8][TX-S]   7.00-8.00   sec  0.00 Bytes  0.00 bits/sec    0    106 KBytes
> [  5][RX-S]   8.00-9.00   sec  0.00 Bytes  0.00 bits/sec
> [  8][TX-S]   8.00-9.00   sec  0.00 Bytes  0.00 bits/sec    0    106 KBytes
> [  5][RX-S]   9.00-10.00  sec  0.00 Bytes  0.00 bits/sec
> [  8][TX-S]   9.00-10.00  sec  1.25 MBytes  10.5 Mbits/sec    2    119 KBytes
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID][Role] Interval           Transfer     Bitrate         Retr
> [  5][RX-S]   0.00-10.01  sec  0.00 Bytes  0.00 bits/sec                  receiver
> [  8][TX-S]   0.00-10.01  sec  6.75 MBytes  5.66 Mbits/sec   10             sender
> 
> Again, rtl8192du v7 itself is 'fine' and could (can) achieve 180/100
> MBit/s before the chanctx changes, so merging v7 might be okay - but
> there needs to be a fix for rtlwifi as a whole (and maybe even other
> drivers) for v6.9+ and the chanctx changes that came with it (not
> every chipset supported by rtlwifi might be affected equally badly,
> e.g. rtl8192cu might not noticeably be, according to Bitterblue's
> testing).
> 
> So with the (performance/ packet loss) caveat above:
> 
> Tested-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>
> 
> I am running rtl8192du v7 on top of v6.9.2 now (and tested it on top
> of wireless-next as of last Friday (2024-05-24)), with the performance
> figures above.
> 
> Regards
> 	Stefan Lippers-Hollmann
> 
> --
> This is the result of my git bisecting:
> 
> $ git bisect good
> 0a44dfc070749514b804ccac0b1fd38718f7daa1 is the first bad commit
> commit 0a44dfc070749514b804ccac0b1fd38718f7daa1
> Author: Johannes Berg <johannes.berg@intel.com>
> Date:   Mon Jan 29 19:34:38 2024 +0100
> 
>     wifi: mac80211: simplify non-chanctx drivers
> 
>     There are still surprisingly many non-chanctx drivers, but in
>     mac80211 that code is a bit awkward. Simplify this by having
>     those drivers assign 'emulated' ops, so that the mac80211 code
>     can be more unified between non-chanctx/chanctx drivers. This
>     cuts the number of places caring about it by about 15, which
>     are scattered across - now they're fewer and no longer in the
>     channel context handling.
> 
>     Link: https://msgid.link/20240129194108.6d0ead50f5cf.I60d093b2fc81ca1853925a4d0ac3a2337d5baa5b@changeid
>     Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> 
>  drivers/net/wireless/admtek/adm8211.c                          |   4 +
>  drivers/net/wireless/ath/ar5523/ar5523.c                       |   4 +
>  drivers/net/wireless/ath/ath5k/mac80211-ops.c                  |   4 +
>  drivers/net/wireless/ath/ath9k/htc_drv_main.c                  |   4 +
>  drivers/net/wireless/ath/ath9k/main.c                          |   4 +
>  drivers/net/wireless/ath/carl9170/main.c                       |   4 +
>  drivers/net/wireless/ath/wcn36xx/main.c                        |   4 +
>  drivers/net/wireless/atmel/at76c50x-usb.c                      |   4 +
>  drivers/net/wireless/broadcom/b43/main.c                       |   4 +
>  drivers/net/wireless/broadcom/b43legacy/main.c                 |   4 +
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/mac80211_if.c |   4 +
>  drivers/net/wireless/intel/iwlegacy/3945-mac.c                 |   4 +
>  drivers/net/wireless/intel/iwlegacy/4965-mac.c                 |   4 +
>  drivers/net/wireless/intel/iwlwifi/dvm/mac80211.c              |   4 +
>  drivers/net/wireless/intersil/p54/main.c                       |   4 +
>  drivers/net/wireless/marvell/libertas_tf/main.c                |   4 +
>  drivers/net/wireless/marvell/mwl8k.c                           |   4 +
>  drivers/net/wireless/mediatek/mt76/mt7603/main.c               |   4 +
>  drivers/net/wireless/mediatek/mt76/mt76x0/pci.c                |   4 +
>  drivers/net/wireless/mediatek/mt76/mt76x0/usb.c                |   4 +
>  drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c           |   4 +
>  drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c           |   4 +
>  drivers/net/wireless/mediatek/mt76/mt792x_core.c               |   7 +-
>  drivers/net/wireless/mediatek/mt76/mt7996/main.c               |   4 +
>  drivers/net/wireless/mediatek/mt7601u/main.c                   |   4 +
>  drivers/net/wireless/purelifi/plfxlc/mac.c                     |   4 +
>  drivers/net/wireless/ralink/rt2x00/rt2400pci.c                 |   4 +
>  drivers/net/wireless/ralink/rt2x00/rt2500pci.c                 |   4 +
>  drivers/net/wireless/ralink/rt2x00/rt2500usb.c                 |   4 +
>  drivers/net/wireless/ralink/rt2x00/rt2800pci.c                 |   4 +
>  drivers/net/wireless/ralink/rt2x00/rt2800soc.c                 |   4 +
>  drivers/net/wireless/ralink/rt2x00/rt2800usb.c                 |   4 +
>  drivers/net/wireless/ralink/rt2x00/rt61pci.c                   |   4 +
>  drivers/net/wireless/ralink/rt2x00/rt73usb.c                   |   4 +
>  drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c             |   4 +
>  drivers/net/wireless/realtek/rtl818x/rtl8187/dev.c             |   4 +
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c          |   4 +
>  drivers/net/wireless/realtek/rtlwifi/core.c                    |   4 +
>  drivers/net/wireless/realtek/rtw88/mac80211.c                  |   4 +
>  drivers/net/wireless/realtek/rtw89/core.c                      |   7 +-
>  drivers/net/wireless/rsi/rsi_91x_mac80211.c                    |   4 +
>  drivers/net/wireless/st/cw1200/main.c                          |   4 +
>  drivers/net/wireless/ti/wl1251/main.c                          |   4 +
>  drivers/net/wireless/virtual/mac80211_hwsim.c                  |   4 +
>  drivers/net/wireless/zydas/zd1211rw/zd_mac.c                   |   4 +
>  drivers/staging/vt6655/device_main.c                           |   4 +
>  drivers/staging/vt6656/main_usb.c                              |   4 +
>  include/net/mac80211.h                                         |  13 ++
>  net/mac80211/cfg.c                                             |  42 +++----
>  net/mac80211/chan.c                                            | 111 +++--------------
>  net/mac80211/ieee80211_i.h                                     |   9 +-
>  net/mac80211/iface.c                                           |   6 +-
>  net/mac80211/main.c                                            | 205 +++++++++++++++++++++++++------
>  net/mac80211/mlme.c                                            |   3 +-
>  net/mac80211/offchannel.c                                      |  21 ++--
>  net/mac80211/scan.c                                            |  18 +--
>  net/mac80211/tx.c                                              |   2 -
>  net/mac80211/util.c                                            |  27 ++--
>  58 files changed, 444 insertions(+), 207 deletions(-)
> 
> $ git bisect log
> git bisect start
> # Status: warte auf guten und schlechten Commit
> # bad: [a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6] Linux 6.9
> git bisect bad a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
> # Status: warte auf gute(n) Commit(s), schlechter Commit bekannt
> # good: [e8f897f4afef0031fe618a8e94127a0934896aba] Linux 6.8
> git bisect good e8f897f4afef0031fe618a8e94127a0934896aba
> # bad: [480e035fc4c714fb5536e64ab9db04fedc89e910] Merge tag 'drm-next-2024-03-13' of https://gitlab.freedesktop.org/drm/kernel
> git bisect bad 480e035fc4c714fb5536e64ab9db04fedc89e910
> # bad: [9187210eee7d87eea37b45ea93454a88681894a4] Merge tag 'net-next-6.9' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next
> git bisect bad 9187210eee7d87eea37b45ea93454a88681894a4
> # bad: [9187210eee7d87eea37b45ea93454a88681894a4] Merge tag 'net-next-6.9' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next
> git bisect bad 9187210eee7d87eea37b45ea93454a88681894a4
> # good: [a01c9fe32378636ae65bec8047b5de3fdb2ba5c8] Merge tag 'nfsd-6.9' of git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux
> git bisect good a01c9fe32378636ae65bec8047b5de3fdb2ba5c8
> # bad: [ca61ba3885274a684c83d8a538eb77b30e38ee92] Merge branch 'rework-genet-mdioclocking'
> git bisect bad ca61ba3885274a684c83d8a538eb77b30e38ee92
> # good: [f42822f22b1c5f72c7e3497d9683f379ab0c5fe4] bnxt_en: Use firmware provided maximum filter counts.
> git bisect good f42822f22b1c5f72c7e3497d9683f379ab0c5fe4
> # bad: [e10cd2ddd89e8b3e61b49247067e79f7debec2f1] wifi: rtw89: load BB parameters to PHY-1
> git bisect bad e10cd2ddd89e8b3e61b49247067e79f7debec2f1
> # good: [2594e4d9e1a2d79bf7bb262974abaf5ef153e371] wifi: iwlwifi: prepare for reading SAR tables from UEFI
> git bisect good 2594e4d9e1a2d79bf7bb262974abaf5ef153e371
> # bad: [719036ae06d4bfdb65139e3947a8404dec298bc5] wifi: cfg80211: move puncturing validation code
> git bisect bad 719036ae06d4bfdb65139e3947a8404dec298bc5
> # good: [1209f487d452ff7e822dec30661fd6b5163fb8cf] wifi: rtl8xxxu: Add TP-Link TL-WN823N V2
> git bisect good 1209f487d452ff7e822dec30661fd6b5163fb8cf
> # good: [4dbd964f33aab6f99891b9610ad4b36cc215be0d] wifi: rtw89: 8922a: add chip_ops::rfk_hw_init
> git bisect good 4dbd964f33aab6f99891b9610ad4b36cc215be0d
> # good: [2fd53eb04c492eb9a2b06f994b36e5cf34ba7541] wifi: mac80211: remove unused MAX_MSG_LEN define
> git bisect good 2fd53eb04c492eb9a2b06f994b36e5cf34ba7541
> # bad: [0a44dfc070749514b804ccac0b1fd38718f7daa1] wifi: mac80211: simplify non-chanctx drivers
> git bisect bad 0a44dfc070749514b804ccac0b1fd38718f7daa1
> # good: [61f0261131c8dc2beeb6b34781a54788221081e9] wifi: mac80211: clean up band switch in duration
> git bisect good 61f0261131c8dc2beeb6b34781a54788221081e9
> # good: [2d9698dd32d086e47b8bff3df4322cc017c17b55] wifi: mac80211: clean up HE 6 GHz and EHT chandef parsing
> git bisect good 2d9698dd32d086e47b8bff3df4322cc017c17b55
> # first bad commit: [0a44dfc070749514b804ccac0b1fd38718f7daa1] wifi: mac80211: simplify non-chanctx drivers

That's all correct.

Johannes suggested that we should set hw->wiphy->retry_{long,short}
before ieee80211_register_hw(). So that would go in
_rtl_init_mac80211(). That has the added benefit of making the
true retry limits visible to userspace ("iw phy").

The problem is that setting hw->wiphy->retry_{long,short} is
not enough. rtlwifi still gets the default retry limits of 4
and 7, because ieee80211_register_hw() doesn't set
hw->conf.long_frame_max_tx_count (and hw->conf.short_frame_max_tx_count).
Johannes suggested moving this code from ieee80211_alloc_hw_nm()
to ieee80211_register_hw():

	local->hw.conf.long_frame_max_tx_count = wiphy->retry_long;
	local->hw.conf.short_frame_max_tx_count = wiphy->retry_short;

I didn't do this yet partly because I don't want to compile
the entire kernel, and partly because I'm not sure how to handle
the different retry limits for AP/IBSS mode and station mode.

Can we change hw->wiphy->retry_{long,short} any time, not just
before ieee80211_register_hw()? If yes, what is even the point
of hw->conf.{short,long}_frame_max_tx_count ? It would be simpler
if we can ignore them and use hw->wiphy->retry_{long,short}
directly.

What do y'all think?