RE: [PATCH v9 4/6] wifi: rtw88: sdio: track free TX pages and OQT credits for RTL8723BS

[Ping-Ke Shih]

luka.gejak@xxxxxxxxx <luka.gejak@xxxxxxxxx> wrote:

> 
> 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 zero the padding with __skb_pad(), which
> also reallocates a cloned skb instead of writing into a buffer a clone
> still shares.

If the __skb_padd() is necessary for original (generic) part, please
add another patch to fix it. (see below comment)

[...]

> +/*
> + * The free page check, the output queue wait and the accounting after the
> + * transfer have to be one unit, or two writers can both pass the checks and
> + * claim the same pages and output queue entry.
> + */

In this patchset, you add many comments. Please review them across whole
patchset to see if they actually need. Like this one, do you think the code
can't explain itself?

> +static int rtw_sdio_write_port_8723bs(struct rtw_dev *rtwdev,
> +                                     struct sk_buff *skb,
> +                                     enum rtw_tx_queue_type queue)
> +{
> +       struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> +       unsigned int pages;
> +       size_t write_size;
> +       size_t txsize;
> +       u32 txaddr;
> +       int ret;
> +
> +       txaddr = rtw_sdio_get_tx_addr(rtwdev, skb->len, queue);
> +       if (!txaddr)
> +               return -EINVAL;
> +
> +       txsize = round_up(skb->len, 4);
> +       write_size = txsize > RTW_SDIO_BLOCK_SIZE ?
> +                    round_up(txsize, RTW_SDIO_BLOCK_SIZE) : txsize;

As we have sdio_set_block_size(sdio_func, RTW_SDIO_BLOCK_SIZE),
can here use sdio_align_size() to get the writ_size?

> +
> +       if (write_size > skb->len) {
> +               size_t pad_size = write_size - skb->len;

I think write_size must be large or equal to skb->len, right?
Therefore, declare size_t pad_size at top of function, and then

    write_size = ...
    pad_size = write_size - skb->len;

    if (pad_size > 0) {
         ...
         __skb_pad( ... pad_size ...);
    }

I think this is easier to read.

> +
> +               /*
> +                * __skb_pad() must not free the skb on failure: both callers
> +                * still own it, one requeues it and the other frees it.
> +                */
> +               ret = __skb_pad(skb, pad_size, false);

Why doesn't rtw_sdio_write_port_generic() need __skb_pad() as well?
Is there an existing problem by original?

> +               if (ret)
> +                       return ret;
> +       }
> +
> +       guard(mutex)(&rtwsdio->tx_credit_lock);
> +
> +       ret = rtw_sdio_check_free_txpg(rtwdev, queue, txsize);
> +       if (ret)
> +               return ret;
> +
> +       ret = rtw_sdio_8723bs_wait_tx_oqt(rtwdev);
> +       if (ret)
> +               return ret;
> +
> +       ret = rtw_sdio_write_to_port(rtwdev, skb, queue, txaddr, write_size);
> +       if (ret) {
> +               /* nothing was queued, so hand the output queue entry back */
> +               atomic_inc(&rtwsdio->tx_oqt_free);
> +               return ret;
> +       }
> +
> +       pages = DIV_ROUND_UP(txsize, rtwdev->chip->page_size);
> +       rtw_sdio_8723bs_consume_txpg(rtwdev, queue, pages);
> +
> +       return 0;
> +}
> +