RE: [PATCH v4 5/7] wifi: rtw88: sdio: track free TX pages and OQT credits for RTL8723BS

[email protected] <[email protected]> wrote:
> From: Luka Gejak <[email protected]>
> 
> The RTL8723BS reports free TX page counts that the generic 8051 path
> reads back from the chip on every transfer, which is both slow over SDIO
> and unreliable on this part: the register frequently reads back zero
> while pages are in fact available. It also gates transmission on a free
> count in the SDIO output queue, REG_SDIO_OQT_FREE_PG, which rtw88 does

It seems like I asked you what OQT is... Output Queue Track?
Please mention it in commit message. I'd be easier to reviewers. 

> not track at all. That count is how many more descriptors the SDIO
> engine can accept, and the chip discards writes that arrive with none
> left.
> 
> Mirror the vendor driver and keep the per-queue and public page counts
> in software, seeded at start and resynchronised from the chip only when
> the cached counts say there is not enough room. Wait for a free output
> queue entry before writing, and account for the pages consumed after a
> successful transfer.
> 
> Transfers also have to be padded up to the SDIO block size for this
> chip rather than using the generic alignment, so size the write
> separately from the frame and trim the skb back afterwards.
> 
> Measured on RTL8723BS hardware against an iperf3 server one hop behind
> the AP, with the wlan0 byte counters as ground truth. On the generic
> path the association completes but no data passes at all: TCP and UDP
> both measure 0 bit/s in either direction. With this patch TCP is
> 25.3 Mbit/s up and 37.3 Mbit/s down, and UDP is 25.0 Mbit/s up at 0%
> loss.
> 
> Signed-off-by: Luka Gejak <[email protected]>
> ---
>  drivers/net/wireless/realtek/rtw88/sdio.c | 245 ++++++++++++++++++++--
>  drivers/net/wireless/realtek/rtw88/sdio.h |  10 +
>  2 files changed, 243 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index 5b40d74b16ee..493eda559607 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -20,6 +20,7 @@
>  #include "tx.h"
> 
>  #define RTW_SDIO_INDIRECT_RW_RETRIES                   50
> +#define RTW_SDIO_OQT_TIMEOUT_MS                                1000

nit: The unit can be '_MS' only if the delay in the loop is
'usleep_range(1000, 2000);' (1ms). 

With _MS, it is easier to understand, but any opinion to improve it?

> 
>  static bool rtw_sdio_is_bus_addr(u32 addr)
>  {
> @@ -548,12 +549,122 @@ static int rtw_sdio_read_port(struct rtw_dev *rtwdev, u8 *buf, size_t count)
>         return ret;
>  }
> 
> +/*
> + * The cached free page counters are a fast path hint only. They are written
> + * from the single threaded TX work and, for H2C and reserved page writes,
> + * from process context, so they are atomic_t; whenever they claim there is
> + * not enough room they are resynchronised from the chip before the caller
> + * gives up, which also absorbs a lost update.
> + */
> +static void rtw_sdio_8723bs_store_free_txpg(struct rtw_dev *rtwdev,
> +                                           u32 free_txpg)
> +{

nit: It looks like you can combine callers of rtw_sdio_8723bs_store_free_txpg()
into single function. Maybe, just rtw_sdio_8723bs_sync_free_txpg(), and add
a return value for rtw_sdio_8723bs_init_free_txpg().

How about you?

> +       struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> +
> +       atomic_set(&rtwsdio->free_pg_high,
> +                  u32_get_bits(free_txpg, BIT_FREE_TXPG_HIGH));
> +       atomic_set(&rtwsdio->free_pg_normal,
> +                  u32_get_bits(free_txpg, BIT_FREE_TXPG_NORMAL));
> +       atomic_set(&rtwsdio->free_pg_low,
> +                  u32_get_bits(free_txpg, BIT_FREE_TXPG_LOW));
> +       atomic_set(&rtwsdio->free_pg_pub,
> +                  u32_get_bits(free_txpg, BIT_FREE_TXPG_PUB));
> +}
> +

[...]

> @@ -749,8 +939,39 @@ static int rtw_sdio_setup(struct rtw_dev *rtwdev)
>         return 0;
>  }
> 
> +static void rtw_sdio_8723bs_check_rqpn(struct rtw_dev *rtwdev)
> +{
> +       const struct rtw_chip_info *chip = rtwdev->chip;
> +       struct rtw_fifo_conf *fifo = &rtwdev->fifo;
> +       const struct rtw_page_table *pg_tbl;
> +       u16 reserved_num;
> +       u32 free_txpg;
> +       u16 pubq_num;
> +
> +       free_txpg = rtw_read32(rtwdev, REG_SDIO_FREE_TXPG);
> +       if (free_txpg || !fifo->acq_pg_num)

It will return directly if free_txpg is not zero... what does it mean?

> +               return;
> +
> +       pg_tbl = &chip->page_table[0];
> +       reserved_num = pg_tbl->hq_num + pg_tbl->lq_num + pg_tbl->nq_num +
> +                      pg_tbl->exq_num + pg_tbl->gapq_num;
> +       if (fifo->acq_pg_num <= reserved_num)

If it falls into this case, can it still work? If not, should it return
an error?

> +               return;
> +
> +       pubq_num = fifo->acq_pg_num - reserved_num;
> +       rtw_write32(rtwdev, REG_RQPN_NPQ,
> +                   BIT_RQPN_NE(pg_tbl->nq_num, pg_tbl->exq_num));
> +       rtw_write32(rtwdev, REG_RQPN,
> +                   BIT_RQPN_HLP(pg_tbl->hq_num, pg_tbl->lq_num, pubq_num));
> +}
> +
>  static int rtw_sdio_start(struct rtw_dev *rtwdev)
>  {
> +       if (rtw_is_8723bs(rtwdev)) {
> +               rtw_sdio_8723bs_check_rqpn(rtwdev);
> +               rtw_sdio_8723bs_init_free_txpg(rtwdev);
> +       }
> +
>         rtw_sdio_enable_rx_aggregation(rtwdev);
>         rtw_sdio_enable_interrupt(rtwdev);
> 

[...]