> -----Original Message-----
> From: Kalle Valo <kvalo@xxxxxxxxxx>
> Sent: Wednesday, March 15, 2023 4:40 PM
> To: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> Cc: Bernie Huang <phhuang@xxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs
>
> Ping-Ke Shih <pkshih@xxxxxxxxxxx> writes:
>
> > From: Po-Hao Huang <phhuang@xxxxxxxxxxx>
> >
> > Allocate a per-skb completion to track those skbs we are interested in
> > and wait for them to complete transmission with TX status. To avoid
> > race condition between process and softirq without addtional locking,
> > we use a work to free the tx_wait struct later when waiter is finished
> > referencing it. This must be called in process context and with a
> > timeout value greater than zero since it might sleep.
> >
> > We use another workqueue so works can be processed concurrently and
> > when the PCI device is removed unexpectedly, all pending works can be
> > flushed. This prevents some works that were scheduled but never processed
> > leads to memory leak.
> >
> > Signed-off-by: Po-Hao Huang <phhuang@xxxxxxxxxxx>
> > Signed-off-by: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
>
> [...]
>
> > +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.
Ping-Ke
