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

[Ping-Ke Shih]

Luka Gejak <luka.gejak@xxxxxxxxx> wrote:
> > I'd move this chunk you are adding to a function.
> 
> Done. The conditions now live in rtw_sdio_8723bs_reschedule_tx(), which
> decides whether the work should run again and says why, so the handler
> loop is just:
> 
>         ret = rtw_sdio_process_tx_queue(rtwdev, queue);
>         if (ret > 0)
>                 break;
> 
>         if (rtw_sdio_8723bs_reschedule_tx(rtwdev, work_data, queue, ret))
>                 return;
> 
>         if (skb_queue_empty(&rtwsdio->tx_queue[queue]))
>                 break;

It looks much better. :)

> 
> While rewriting that I found something I had got wrong in v4 and v5, and
> it is worth pointing out because it was not limited to this chip. Both
> versions had an unconditional
> 
>         if (ret < 0)
>                 break;

I didn't get this point. I'll review carefully on this part in v6.

> 
> in that loop. Upstream requeues the frame on a failed transfer and lets
> the loop retry, so that break quietly changed every other SDIO part to
> give up after the first failure. That contradicted the scope claim in my
> own cover letter. It is dropped in v6: the two errors this chip needs to
> retry are handled in the helper before that point, and everything else
> keeps the existing behaviour, so the other parts are untouched again.

Thanks for pointing this. 

If you have another SDIO chip, such as RTL8723CS, please verify your patches
on another real hardware, because it is not possible to catch all flaws by
reviewer (like me). Using real hardware with a simple throughput test is more
important.

Ping-Ke