Ping-Ke Shih <pkshih@xxxxxxxxxxx> 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
