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.
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.
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.