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

[email protected] <[email protected]> wrote:

[...]

>  static int rtw_sdio_write_port(struct rtw_dev *rtwdev, struct sk_buff *skb,
>                                enum rtw_tx_queue_type queue)
>  {
>         struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> +       bool rtl8723bs = rtw_is_8723bs(rtwdev);
> +       unsigned int pages;
> +       size_t write_size;
>         bool bus_claim;
>         size_t txsize;
>         u32 txaddr;
> @@ -645,31 +837,73 @@ static int rtw_sdio_write_port(struct rtw_dev *rtwdev, struct sk_buff *skb,
>         if (!txaddr)
>                 return -EINVAL;
> 
> -       txsize = sdio_align_size(rtwsdio->sdio_func, skb->len);
> +       if (rtl8723bs) {
> +               txsize = round_up(skb->len, 4);
> +               write_size = txsize > RTW_SDIO_BLOCK_SIZE ?
> +                            round_up(txsize, RTW_SDIO_BLOCK_SIZE) : txsize;
> +
> +               /*
> +                * __skb_pad() zeroes the padding without moving skb->len and
> +                * reallocates when the skb is cloned or short on tailroom,
> +                * so the padding can never land in a buffer a clone still
> +                * shares. It must not free the skb on failure: both callers
> +                * still own it, one requeues it and the other frees it.
> +                */

I think you only need to note 'not free the skb on failure'. 

> +               if (write_size > skb->len) {

And move comment here to note __skb_pad().

By the way, we can have a local variable 'pad_size = write_size > skb->len'.
Then,

    if (pad_size) {
        ret = __skb_pad( ..., pad_size, ...);
        ...
    }

> +                       ret = __skb_pad(skb, write_size - skb->len, false);
> +                       if (ret)
> +                               return ret;
> +               }
> +       } else {
> +               txsize = sdio_align_size(rtwsdio->sdio_func, skb->len);
> +               write_size = txsize;
> +       }
> +
> +       /*
> +        * The free page check, the output queue wait and the accounting
> +        * after the transfer must not interleave with another writer: the
> +        * TX worker and the H2C path run concurrently, and two writers that
> +        * both pass the checks can claim the same pages and output queue
> +        * entry, after which the chip silently discards whichever transfer
> +        * arrives second.
> +        */

Not sure if the comment along declaration of tx_credit_lock is enough?
If so, maybe we don't need this comment. 

> +       if (rtl8723bs)
> +               mutex_lock(&rtwsdio->tx_credit_lock);

guard(mutex)(&rtwsdio->tx_credit_lock);

I think you can add lockdep_assert_held() to the places the locks (mutex)
must be held, and run test if somewhere throw warning (must not). 

> 
>         ret = rtw_sdio_check_free_txpg(rtwdev, queue, txsize);
>         if (ret)
> -               return ret;
> +               goto out_unlock;
> +
> +       if (rtl8723bs) {
> +               ret = rtw_sdio_8723bs_wait_tx_oqt(rtwdev);
> +               if (ret)
> +                       goto out_unlock;
> +       }
> 
>         if (!IS_ALIGNED((unsigned long)skb->data, RTW_SDIO_DATA_PTR_ALIGN))
>                 rtw_warn(rtwdev, "Got unaligned SKB in %s() for queue %u\n",
>                          __func__, queue);
> 
>         bus_claim = rtw_sdio_bus_claim_needed(rtwsdio);
> -
>         if (bus_claim)
>                 sdio_claim_host(rtwsdio->sdio_func);
> -
> -       ret = sdio_memcpy_toio(rtwsdio->sdio_func, txaddr, skb->data, txsize);
> -
> +       ret = sdio_memcpy_toio(rtwsdio->sdio_func, txaddr, skb->data,
> +                              write_size);
>         if (bus_claim)
>                 sdio_release_host(rtwsdio->sdio_func);
> 
> -       if (ret)
> +       if (ret) {
>                 rtw_warn(rtwdev,
>                          "Failed to write %zu byte(s) to SDIO port 0x%08x",
> -                        txsize, txaddr);
> +                        write_size, txaddr);
> +       } else if (rtl8723bs) {
> +               pages = DIV_ROUND_UP(txsize, rtwdev->chip->page_size);
> +               rtw_sdio_8723bs_consume_txpg(rtwdev, queue, pages);
> +       }
> 
> +out_unlock:
> +       if (rtl8723bs)
> +               mutex_unlock(&rtwsdio->tx_credit_lock);
>         return ret;
>  }
> 

[...]