RE: [PATCH rtw-next 1/2] wifi: rtw88: pci: wake the TX queues when the rings are reset

[Ping-Ke Shih]

Abdurrahman Karadag <abdurrahmankaradag19@xxxxxxxxx> wrote:
> For a queue stopped by the PCI TX ring-full path, ring->queue_stopped
> is normally cleared and the stop reason released only from the
> completion loop in rtw_pci_tx_isr(). When the rings are reset the
> pending descriptors are dropped and their skbs are freed directly by
> rtw_pci_free_tx_ring_skbs(), so that loop never runs for them. The flag
> and the stop reason both survive the reset, and because the ring is now
> empty no completion will ever arrive to clear them. Any queue stopped
> that way stays stopped.
> 
> This makes ieee80211_restart_hw() unable to recover a device that
> stopped a queue before the restart, which is the opposite of what the
> recovery is for.
> 
> Reproduced on an RTL8821CE by pausing TX in hardware, which freezes the
> read index while the driver keeps submitting, the same shape the chip
> shows when it wedges on its own:
> 
>     # echo "522 f 1" > /sys/kernel/debug/ieee80211/phy0/rtw88/write_reg
>     # ... push traffic until the ring fills ...
>     BE 0x3a8: 0x0081007f, avail_desc() 1, BE queue stop reason 0x1
> 
>     # (call rtw_fw_recovery() from a debug build)
>     firmware crash, start reset and recover
>     ieee80211 phy1: Hardware restart was requested
>     wlan0: associated
> 
>     REG_TXPAUSE 0x00, BE 0x3a8: 0x00000000, ring empty
>     BE queue stop reason 0x1, 100% packet loss, no recovery in 90 s
> 
> The station reassociated twice during those 90 s, so the link was fine;
> only the queue was still stopped. With this patch the same sequence
> clears the stop reason and traffic returns within 2 s.

I feel there are too much detail in commit message. Just describe why
this patch is necessary, and how it fix the problem.

> 
> Record the queue mappings this path stops and release them both from
> the completion loop and when the reset empties the ring. It has to be a
> set rather than one value: the stop runs after every submission that
> leaves fewer than two descriptors, rtw_pci_tx_write_data() still
> accepts a frame while one is left, and rtw_tx_queue_mapping() places
> management frames on the MGMT ring and multicast on HI0 whatever their
> skb queue mapping is, so one ring can stop two different queues before
> it is emptied. Keeping only the last one would leave the other stopped
> for good.
> 
> Recording the mappings also limits the wake to the queues this
> ring-full path actually stopped, instead of waking every mac80211
> queue.
> 
> Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")

This is to assist with recovery, right? So at the initial moment,
we didn't support recovery yet. No this fixes then.

> Signed-off-by: Abdurrahman Karadag <abdurrahmankaradag19@xxxxxxxxx>
> ---

[...]

>  static void rtw_pci_reset_trx_ring(struct rtw_dev *rtwdev)
>  {
> +       struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> +       struct rtw_pci_tx_ring *ring;
> +       enum rtw_tx_queue_type queue;
> +
>         rtw_pci_reset_buf_desc(rtwdev);
> +
> +       /*
> +        * The rings are empty again, so nothing is left whose completion
> +        * could reach the wake in rtw_pci_tx_isr(). Release the queues this
> +        * path stopped - the stop reasons it set are cleared nowhere else,
> +        * and over an empty ring no completion will ever arrive to clear
> +        * them.
> +        */
> +       for (queue = 0; queue < RTK_MAX_TX_QUEUE_NUM; queue++) {
> +               ring = &rtwpci->tx_rings[queue];
> +
> +               if (!ring->queue_stopped)
> +                       continue;
> +
> +               rtw_pci_wake_stopped_queues(rtwdev, ring);
> +       }

Is it enough just a ieee80211_wake_queues()? No other changes.

>  }
> 
>  static void rtw_pci_enable_interrupt(struct rtw_dev *rtwdev,