Ping-Ke Shih pkshih@realtek.com wrote:
Fedor Pchelkin pchelkin@ispras.ru wrote:
On Thu, 18. Sep 05:47, Ping-Ke Shih wrote:
Fedor Pchelkin pchelkin@ispras.ru wrote:
@@ -1094,22 +1094,13 @@ int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff
*sk
int qsel, unsigned int timeout){ struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
struct rtw89_tx_wait_info *wait;
struct rtw89_tx_wait_info *wait = wiphy_dereference(rtwdev->hw->wiphy,skb_data->wait);Can't we just pass 'wait' by function argument?
Yep.
unsigned long time_left; int ret = 0; lockdep_assert_wiphy(rtwdev->hw->wiphy);
wait = kzalloc(sizeof(*wait), GFP_KERNEL);if (!wait) {rtw89_core_tx_kick_off(rtwdev, qsel);return 0;}init_completion(&wait->completion);wait->skb = skb;rcu_assign_pointer(skb_data->wait, wait);Here, original code prepares completion before TX kick off. How it could be a problem? Do I miss something?
That's a good question and it made me rethink the cause of the race scenario. I didn't initially take TX kick off into consideration when it actually matters.
Do it mean that you pictured the racing scenario in commit message by code review instead of a real case you met?
The thing is: there might have been another thread initiating TX kick off for the same queue in parallel. But no such thread exists because a taken wiphy lock generally protects from such situations. rtw89_core_txq_schedule() worker looks like a good candidate but it doesn't operate on the needed management queues.
Last night I also thought if another thread works in parallel. Maybe rtw89_ops_tx() could be?
So I may conclude this patch doesn't fix any real bug though I'd prefer to keep it (with description rewritten of course) because it helps to avoid potential issues in future.
Agree.
Forgot to say. Could you mention this racing scenario was found by core review and your perspective in commit message?