From:   Kalle Valo <kvalo@kernel.org>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     <phhuang@realtek.com>, <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>
Date:   Wed, 15 Mar 2023 10:39:54 +0200
In-Reply-To: <20230310034631.45299-3-pkshih@realtek.com> (Ping-Ke Shih's
        message of "Fri, 10 Mar 2023 11:46:28 +0800")
Message-ID: <87v8j2mmqt.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:

> From: Po-Hao Huang <phhuang@realtek.com>
>
> 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@realtek.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

[...]

> +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?

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

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