RE: [PATCH v5 6/6] wifi: rtw88: sdio: add TX back-pressure and retry on page starvation

[Ping-Ke Shih]

luka.gejak@xxxxxxxxx <luka.gejak@xxxxxxxxx> wrote:
> +
> +static void rtw_sdio_reschedule_tx_work(struct rtw_dev *rtwdev,
> +                                       struct rtw_sdio_work_data *work_data,
> +                                       unsigned long delay)
> +{
> +       struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> +
> +       queue_delayed_work(rtwsdio->txwq, &work_data->work, delay);
>  }

Actually, I didn't request this wrapper by v4. (Also I don't prefer a
simple wrapper like this that hides kernel API).

(See below)

> 
>  static void rtw_sdio_tx_handler(struct work_struct *work)
>  {
>         struct rtw_sdio_work_data *work_data =
> -               container_of(work, struct rtw_sdio_work_data, work);
> +               container_of(to_delayed_work(work), struct rtw_sdio_work_data,
> +                            work);
>         struct rtw_sdio *rtwsdio;
>         struct rtw_dev *rtwdev;
> -       int limit, queue;
> +       int limit, queue, ret;
> +       bool rtl8723bs;
> 
>         rtwdev = work_data->rtwdev;
>         rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> +       rtl8723bs = rtw_is_8723bs(rtwdev);
> 
>         if (!rtw_fw_feature_check(&rtwdev->fw, FW_FEATURE_TX_WAKE))
>                 rtw_sdio_deep_ps_leave(rtwdev);
> 
>         for (queue = RTK_MAX_TX_QUEUE_NUM - 1; queue >= 0; queue--) {
>                 for (limit = 0; limit < 1000; limit++) {
> -                       rtw_sdio_process_tx_queue(rtwdev, queue);
> +                       ret = rtw_sdio_process_tx_queue(rtwdev, queue);
> +                       if (ret > 0)
> +                               break;
> +
> +                       if (ret < 0) {
> +                               /*
> +                                * A page or output queue shortage and a failed
> +                                * skb expansion are both transient, and the
> +                                * frame is still queued, so come back for it.
> +                                * That matters once the queue can be stopped:
> +                                * a stopped queue is handed no further frames,
> +                                * so nothing else would kick this work item and
> +                                * the queue would stay stopped for good. The
> +                                * remaining errors cannot succeed on a retry
> +                                * and each log where they happen.
> +                                */
> +                               if (rtl8723bs &&
> +                                   (ret == -EBUSY || ret == -ENOMEM)) {
> +                                       rtw_sdio_reschedule_tx_work(rtwdev, work_data,
> +                                                                   RTW_SDIO_TX_RETRY_DELAY);
> +                                       return;
> +                               }
> +                               break;
> +                       }
> +
> +                       /*
> +                        * Restart from the highest priority queue after every
> +                        * management frame so the join sequence is not held up
> +                        * behind a data backlog.
> +                        */
> +                       if (rtl8723bs && queue == RTW_TX_QUEUE_MGMT) {
> +                               rtw_sdio_reschedule_tx_work(rtwdev, work_data, 0);
> +                               return;
> +                       }

I'd move this chunk you are adding to a function. I think it is just to
reschedule TX work for RTL8723BS for certain conditions, but explaining 
a lot of things (conditions) in common flow makes people hard to read
the flow. 

> 
>                         if (skb_queue_empty(&rtwsdio->tx_queue[queue]))
>                                 break;