Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Kalle Valo <kvalo@kernel.org>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     Bernie Huang <phhuang@realtek.com>,
        "linux-wireless\@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs
References: <20230310034631.45299-1-pkshih@realtek.com>
        <20230310034631.45299-3-pkshih@realtek.com>
        <87v8j2mmqt.fsf@kernel.org>
        <360e6dd64e3645c68742fc4c603b3c2b@realtek.com>
Date:   Mon, 03 Apr 2023 13:32:54 +0300
In-Reply-To: <360e6dd64e3645c68742fc4c603b3c2b@realtek.com> (Ping-Ke Shih's
        message of "Wed, 15 Mar 2023 12:09:26 +0000")
Message-ID: <875yadb6i1.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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