[PATCH] wifi: rtlwifi: pci: Fix condition to wake the queues

[PATCH] wifi: rtlwifi: pci: Fix condition to wake the queues

[Bitterblue Smith]

The TX queues should be woken when there are at least 4 available
entries, not when there are at most 4.

Tested with RTL8821AE.

Fixes: d0311314d002 ("rtlwifi: rtl8192ee: Fix handling of new style descriptors")
Cc: stable@vger.kernel.org
Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
 drivers/net/wireless/realtek/rtlwifi/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
index 0eafc4d125f9..9614a9e752b9 100644
--- a/drivers/net/wireless/realtek/rtlwifi/pci.c
+++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
@@ -520,7 +520,7 @@ static void _rtl_pci_tx_isr(struct ieee80211_hw *hw, int prio)
 			rtl_tx_ackqueue(hw, skb);
 		}
 
-		if ((ring->entries - skb_queue_len(&ring->queue)) <= 4) {
+		if ((ring->entries - skb_queue_len(&ring->queue)) >= 4) {
 			rtl_dbg(rtlpriv, COMP_ERR, DBG_DMESG,
 				"more desc left, wake skb_queue@%d, ring->idx = %d, skb_queue_len = 0x%x\n",
 				prio, ring->idx,
-- 
2.48.1

RE: [PATCH] wifi: rtlwifi: pci: Fix condition to wake the queues

[Ping-Ke Shih]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> The TX queues should be woken when there are at least 4 available
> entries, not when there are at most 4.
> 
> Tested with RTL8821AE.
> 
> Fixes: d0311314d002 ("rtlwifi: rtl8192ee: Fix handling of new style descriptors")

I agree your change looks more reasonable. Did you really meet the case
and this patch solves it?

By the changes of d0311314d002, I can't really understand why the initial
version and the commit do. Considering the commit "fix" some situations,
I don't know if this patch would break them again. Could you look into
rtlwifi PCI flow deeper and share your ideas? 

Re: [PATCH] wifi: rtlwifi: pci: Fix condition to wake the queues

[Bitterblue Smith]

On 19/02/2025 08:20, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>> The TX queues should be woken when there are at least 4 available
>> entries, not when there are at most 4.
>>
>> Tested with RTL8821AE.
>>
>> Fixes: d0311314d002 ("rtlwifi: rtl8192ee: Fix handling of new style descriptors")
> 
> I agree your change looks more reasonable. Did you really meet the case
> and this patch solves it?
> 
> By the changes of d0311314d002, I can't really understand why the initial
> version and the commit do. Considering the commit "fix" some situations,
> I don't know if this patch would break them again. Could you look into
> rtlwifi PCI flow deeper and share your ideas? 
> 

I saw several problems with rtl8821ae but I can't say if this
patch fixes any of them:

- After a few hours it randomly stops receiving beacons and data
  frames for at least 10 seconds ("AP off, try to reconnect now"
  in dmesg). Then it may or may not be able to reconnect.

- Sometimes the TX speed measured by iperf3 is limited to ~80 Mbps.
  Normally it's over 250.

- Sometimes there are duplicate pings:
      19 packets transmitted, 14 received, +151 duplicates, 26.3158% packet loss, time 18098ms

- Testing the RX speed with iperf3 sometimes makes the mouse cursor
  stutter. (It's usually ~300 Mbps.)

I don't know how to reproduce any of these.

<= seemed like a clear, simple mistake. I'm not sure about it
anymore, so please drop the patch. Maybe I will look into it
again some time.

RE: [PATCH] wifi: rtlwifi: pci: Fix condition to wake the queues

[Ping-Ke Shih]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> On 19/02/2025 08:20, Ping-Ke Shih wrote:
> > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> >> The TX queues should be woken when there are at least 4 available
> >> entries, not when there are at most 4.
> >>
> >> Tested with RTL8821AE.
> >>
> >> Fixes: d0311314d002 ("rtlwifi: rtl8192ee: Fix handling of new style descriptors")
> >
> > I agree your change looks more reasonable. Did you really meet the case
> > and this patch solves it?
> >
> > By the changes of d0311314d002, I can't really understand why the initial
> > version and the commit do. Considering the commit "fix" some situations,
> > I don't know if this patch would break them again. Could you look into
> > rtlwifi PCI flow deeper and share your ideas?
> >
> 
> I saw several problems with rtl8821ae but I can't say if this
> patch fixes any of them:
> 
> - After a few hours it randomly stops receiving beacons and data
>   frames for at least 10 seconds ("AP off, try to reconnect now"
>   in dmesg). Then it may or may not be able to reconnect.

Is interrupt counter increasing in /proc/interrupt?
If not, check if driver ISR (interrupt status register) manually to see
if it received data but not issue interrupt to host properly.
However, if no any of these, it will be WiFi problem, not PCI.

To bisect WiFi problem, we may stop doing rtl_watchdog_wq_callback() to
avoid changing registers dynamically. 

> 
> - Sometimes the TX speed measured by iperf3 is limited to ~80 Mbps.
>   Normally it's over 250.

I suggest to check PHY rate first and MSDU aggregation by sniffer.
Not sure if rtlwifi can be monitor mode, but very sure rtw88 does. 

> 
> - Sometimes there are duplicate pings:
>       19 packets transmitted, 14 received, +151 duplicates, 26.3158% packet loss, time 18098ms

I have never dug this kind of problem, but I suggest to check sniffer
first.  

> 
> - Testing the RX speed with iperf3 sometimes makes the mouse cursor
>   stutter. (It's usually ~300 Mbps.)

If here is Bluetooth mouse, it will be a BT-coexistence problem. If not,
I feel CPU becomes exhausted or huge amount of interrupts at that time.

> 
> I don't know how to reproduce any of these.

If users want/can help to dig problem, please try above suggestions. 

RE: [PATCH] wifi: rtlwifi: pci: Fix condition to wake the queues

[Ping-Ke Shih]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c
> index 0eafc4d125f9..9614a9e752b9 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/pci.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/pci.c
> @@ -520,7 +520,7 @@ static void _rtl_pci_tx_isr(struct ieee80211_hw *hw, int prio)
>                         rtl_tx_ackqueue(hw, skb);
>                 }
> 
> -               if ((ring->entries - skb_queue_len(&ring->queue)) <= 4) {
> +               if ((ring->entries - skb_queue_len(&ring->queue)) >= 4) {
>                         rtl_dbg(rtlpriv, COMP_ERR, DBG_DMESG,
>                                 "more desc left, wake skb_queue@%d, ring->idx = %d, skb_queue_len =
> 0x%x\n",
>                                 prio, ring->idx,

Reading the code about calling ieee80211_stop_queue() and
ieee80211_wake_queue() again. It looks like

In TX path (rtl_pci_tx):

    if (remaining < 2)
        ieee80211_stop_queue();

In TX completion path (_rtl_pci_tx_isr):

    if (remaining <= 4)
        ieee80211_wake_queue();

I guess it uses "<= 4" in TX completion path because origin patch doesn't
want to call ieee80211_wake_queue() redundantly, but looks not so reasonable.
The better way may use ieee80211_queue_stopped() instead.