Re: [PATCH rtw-next 4/5] wifi: rtw88: pci: Add functions to support older chips

["Luka Gejak" <luka.gejak@linux.dev>]

Hi Bitterblue,

Thanks for working on this. I went through pci_old.c and have a few
questions.


October 24, 2026 at 1:02 AM, "Bitterblue Smith" <rtl8821cerfe2@gmail.com> wrote:

> +++ b/drivers/net/wireless/realtek/rtw88/pci_old.c

[...]

> +static void rtw_pci_old_reset_buf_desc(struct rtw_dev *rtwdev)
> +{

[...]

> +     rtw_write32(rtwdev, REG_RX_DESA,
> +                 rtwpci->rx_rings[RTW_RX_QUEUE_MPDU].r.dma);
> +
> +     rtw_write8(rtwdev, RTK_PCI_CTRL + 3, 0x77);
> +}

If I read this right, this only runs on setup, and rtw_pci_stop() still
goes to the new rtw_pci_reset_buf_desc() when the device stops:

rtw_pci_stop()
  rtw_pci_dma_release()
    rtw_pci_reset_trx_ring()
      rtw_pci_reset_buf_desc()

Some of those offsets seem to be different registers on these chips,
for example 0x340 is REG_RX_DESA and 0x338 is REG_BKQ_DESA in
pci_old.h. Since rtw88xxa_power_off() calls rtw_hci_stop() before it
clears REG_CR, I think RX would still be running when they get written.
Would it make sense to send rtw_pci_dma_release() through pci_gen too?

Also, after stop the descriptors keep their OWN bit and buffer address,
and only rp and wp are reset. If a frame was still queued at that point,
could the hardware pick it up again after the next start? Would clearing
them here help?

[...]

> +     /*
> +      * Because the time taked by the I/O in __pci_get_hw_tx_ring_rp is a
> +      * bit dynamic, it's hard to define a reasonable fixed total timeout to
> +      * use read_poll_timeout* helper. Instead, we can ensure a reasonable
> +      * polling times, so we just use for loop with udelay here.
> +      */

This comment looks copied, I don't see any I/O in this loop.

[...]

> +     if (queue == RTW_TX_QUEUE_BCN)
> +             rtw_pci_release_rsvd_page(rtwpci, ring);
> +     else if (le32_get_bits(tx_desc[wp].w0_9.w0, RTW_TX_DESC_W0_OWN))
> +             return -ENOSPC;

I'm wondering if checking only OWN is enough here. OWN is cleared when
the hardware is done, but the skb stays in ring->queue until
rtw_pci_old_tx_isr() runs. rtw_pci_tx_write() only stops the mac80211
queue of the current skb, and the MGMT ring can get frames from more
than one queue (BARs use the AC of their TID), so I think the ring could
fill up completely. Then wp == rp, and the next frame would overwrite a
descriptor whose skb isn't freed yet. Would the avail_desc() check from
rtw_pci_tx_write_data() make sense here too?

[...]

> +             if (dma != tx_data->dma)
> +                     rtw_err(rtwdev,
> +                             "tx_data dma %pad != tx_desc dma %pad\n",
> +                             &tx_data->dma, &dma);
> +
> +             dma_unmap_single(&rtwpci->pdev->dev, dma, skb->len,
> +                              DMA_TO_DEVICE);

If that can happen, dma here would belong to the next skb, so it would
be unmapped too early and this skb's mapping would leak. Maybe use
tx_data->dma like rtw_pci_tx_isr() does?

[...]

> +             if (ring->queue_stopped &&
> +                 avail_desc(ring->r.wp, ring->r.rp, ring->r.len) <= 4) {

rtw_pci_tx_isr() uses > 4 here, so this seems to be the other way
around. Is that on purpose? I guess it came from rtlwifi.

[...]

> +     /*
> +      * TODO: is it correct? it seems to work, but should we count
> +      * the number of occupied buffers for rtw_pci_napi_poll?
> +      */
> +     return 0;

About the TODO, I'm not sure returning 0 is enough. rtw_pci_napi_poll()
uses this to pick up frames that are already in the ring when interrupts
are enabled again. rtw_pci_old_rx_napi() doesn't count C2H or bad
frames, so it could return 0 while there's still data in the ring, which
would then wait for the next interrupt. Would checking OWN on the
descriptor at ring->r.rp be enough?

[...]

> +             if (le32_get_bits(rx_desc[cur_rp].w0_5.w0, RTW_RX_DESC_W0_OWN))
> +                     break;

Does this need a dma_rmb() after the OWN check? As far as I understand,
a weakly ordered CPU like arm64 could read the rest of the descriptor
and the buffer before it sees OWN cleared, and dma_sync_single_for_cpu()
doesn't add a barrier when the device is DMA coherent. It looks like the
example in Documentation/memory-barriers.txt. The new design gets this
from the readl() in rtw_pci_get_hw_rx_ring_nr().

Also, I think the wmb() calls could be dma_wmb().

[...]

> +             new_len = pkt_stat.pkt_len + pkt_offset;
> +             new = dev_alloc_skb(new_len);
> +             if (WARN_ONCE(!new, "rx routine starvation\n"))
> +                     goto next_rp;
> +
> +             skb_put_data(new, skb->data, new_len);

rtw_pci_rx_napi() got a length check in 6e76e9ed273d ("wifi: rtw88: fix
OOB read from firmware RX descriptor exceeding DMA buffer"), should it
be added here too? rtw_rx_query_rx_desc() allows pkt_len up to 11454, so
with 32 bytes of drv_info and a shift of 3 new_len could reach 11489,
while the buffer is 11478 bytes.

[...]

> +             wmb();
> +             le32p_replace_bits(&rx_desc[cur_rp].w0_5.w0, 1,
> +                                RTW_RX_DESC_W0_OWN);
> +
> +             if (cur_rp == ring->r.len - 1)
> +                     le32p_replace_bits(&rx_desc[cur_rp].w0_5.w0, 1,
> +                                        RTW_RX_DESC_W0_EOR);

Is it safe to set EOR after OWN? le32p_replace_bits() reads and writes
back the whole w0, so if the hardware fills this descriptor in between,
I think the old value with OWN set could end up on top of its status,
and RX would wait there until the hardware wraps around. Setting EOR
before the wmb(), together with PKT_LEN, might avoid that.

[...]

> +             pcie_capability_set_word(pdev, PCI_EXP_LNKCTL,
> +                     PCI_EXP_LNKCTL_CCC | PCI_EXP_LNKCTL_ASPMC);

checkpatch --strict complains about the alignment here.

[...]

> +++ b/drivers/net/wireless/realtek/rtw88/pci_old.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Is GPL-2.0 only intended for this header? The rest of rtw88, pci_old.c
included, is GPL-2.0 OR BSD-3-Clause.

[...]

> +++ b/drivers/net/wireless/realtek/rtw88/reg.h
> @@ -11,6 +11,7 @@
>  #define BIT_FEN_PPLL         BIT(7)
>  #define BIT_FEN_PCIEA                BIT(6)
>  #define BIT_FEN_DIO_PCIE     BIT(5)
> +#define BIT_FEN_PCIED                BIT(8)

Maybe put BIT(8) above BIT_FEN_PPLL, these seem to be sorted high to low.

Best regards,
Luka Gejak