Re: [PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs

[Kalle Valo <kvalo@kernel.org>]

Ping-Ke Shih <pkshih@realtek.com> writes:

>> > +static void rtw89_core_free_tx_wait_work(struct work_struct *work)
>> > +{
>> > +     struct rtw89_tx_wait_info *wait =
>> > +                     container_of(work, struct rtw89_tx_wait_info, work);
>> > +     struct rtw89_dev *rtwdev = wait->rtwdev;
>> > +     int done, ret;
>> > +
>> > +     ret = read_poll_timeout(atomic_read, done, done, 1000, 100000, false,
>> > +                             &wait->wait_done);
>> > +
>> > +     if (ret)
>> > +             rtw89_err(rtwdev, "tx wait timed out, stop polling\n");
>> > +     else
>> > +             kfree(wait);
>> > +}
>> 
>> I admit I didn't try to understand this patch in detail but this
>> function just looks odd to me. Why there's polling able to free
>> something?
>> 
>
> Three works are involved in the "wait/completion".
>
> work 1. remain-on-channel work
>    It trigger TX null data and wait (kmalloc 'wait' object, and wait for completion)
>
> work 2. TX completion by napi_poll
>    It returns TX status (failed or succeed to TX), and complete the 'wait' object,
>    and queue rtw89_core_free_tx_wait_work() to free 'wait' object.
>
>    We queue this by work 2, because time of work 1 is predictable, but
>    it is hard to estimate time of work 2. The read_poll_timeout() is for
>    the work 1 predictable time.
>
> work 3. This work is to do garbage collection of 'wait' object
>    It polls if work 1 is done before doing free 'wait' object.
>
>
> Things are complex because work 1 and 2 are done asynchronously, so one
> of them can't free 'wait' object, or it will causes use-after-free in other
> side.
>
> Use case 1: (work 1 could free 'wait' object)
>     work 1         work 2          work 3
>     wait
>                    completion
>     wait ok
>                                    free 'wait'
>
> Use case 2: (work 2 could free 'wait' object)
>     work 1         work 2          work 3
>     wait
>     wait timeout
>                    completion
>                                    free 'wait'
>
>
> I can add a comment as a hint why we can use a read_poll_timeout() to assist
> in freeing something. 

I would expect that there's polling if you are waiting something from
hardware, or maybe when implementing a spin lock, but not when waiting
for another kernel thread. This just doesn't feel right but I don't have
time to propose a good alternative either, sorry.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches