Re: [PATCH net-next v6 09/11] net: wangxun: add Tx timeout process
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-03-31 00:55:48
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- net: wangxun: add Tx timeout process Implement .ndo_tx_timeout to handle Tx side timeout event. When Tx timeout event occur, it will triger driver into reset process. The WX_HANG_CHECK_ARMED bit is set to indicate a potential hang. It will be cleared if a pause frame is received to remove false hang detection due to 802.3 frames, which is implemented in later commit.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_err.c b/drivers/net/ethernet/wangxun/libwx/wx_err.c new file mode 100644 index 0000000000000..bb77bdce69d22 --- /dev/null +++ b/drivers/net/ethernet/wangxun/libwx/wx_err.c@@ -0,0 +1,126 @@
[ ... ]
+static void wx_reset_subtask(struct wx *wx)
+{
+ if (!test_bit(WX_FLAG_NEED_PF_RESET, wx->flags))
+ return;
+
+ if (!netif_running(wx->netdev) ||
+ test_bit(WX_STATE_RESETTING, wx->state))
+ return;
+
+ rtnl_lock();
+
+ wx_warn(wx, "Reset adapter.\n");
+
+ if (test_bit(WX_FLAG_NEED_PF_RESET, wx->flags)) {
+ if (wx->do_reset)
+ wx->do_reset(wx->netdev, true);Can this race with dev_close()? If the interface is brought down concurrently, dev_close() acquires rtnl_lock(), clears the running state, and frees resources. When wx_reset_subtask() later acquires the lock, it does not re-check netif_running(). Could this result in calling do_reset() on a closed interface, leading to hardware state inconsistencies or use-after-free crashes?
+ clear_bit(WX_FLAG_NEED_PF_RESET, wx->flags);
Could this unconditionally clear WX_FLAG_NEED_PF_RESET even if another context sets it during the do_reset() execution? Should this use test_and_clear_bit() before the reset operation to avoid losing concurrent reset requests?
+ } + + rtnl_unlock(); +}
[ ... ]
+void wx_tx_timeout(struct net_device *netdev, unsigned int txqueue)
+{
+ struct wx *wx = netdev_priv(netdev);
+ u32 head, tail;
+ int i;
+
+ for (i = 0; i < wx->num_tx_queues; i++) {
+ struct wx_ring *tx_ring = wx->tx_ring[i];
+
+ if (test_bit(WX_TX_DETECT_HANG, tx_ring->state) &&
+ wx_check_tx_hang(tx_ring))Does calling wx_check_tx_hang() from the dev_watchdog softirq context cause data races with the NAPI polling context (wx_clean_tx_irq)? wx_check_tx_hang() modifies ring state bits (WX_HANG_CHECK_ARMED) and updates the 64-bit statistic tx_done_old. Can this lead to torn reads or writes on 32-bit platforms and silent corruption of the hang detection state?
+ wx_warn(wx, "Real tx hang detected on queue %d\n", i); +
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c index baffcadb22690..b8db772c37c5c 100644 --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -370,6 +391,7 @@ static void ngbe_disable_device(struct wx *wx) wx_napi_disable_all(wx); netif_tx_stop_all_queues(netdev); netif_tx_disable(netdev); + timer_delete_sync(&wx->service_timer);
Does this code leave the service task running? timer_delete_sync() is called, but cancel_work_sync(&wx->service_task) is missing. If the work is already queued, could wx_check_tx_hang_subtask() run after the transmission and reception rings are freed, resulting in a use-after-free when accessing wx->tx_ring[i]->state?
if (wx->gpio_ctrl) ngbe_sfp_modules_txrx_powerctl(wx, false); wx_irq_disable(wx);
[ ... ]
quoted hunk ↗ jump to hunk
@@ -816,6 +845,9 @@ static void ngbe_remove(struct pci_dev *pdev) struct wx *wx = pci_get_drvdata(pdev); struct net_device *netdev; + timer_delete_sync(&wx->service_timer); + cancel_work_sync(&wx->service_task); + netdev = wx->netdev; wx_disable_sriov(wx); unregister_netdev(netdev);
Does this teardown order result in a use-after-free? cancel_work_sync() is called before unregister_netdev(). Since the network device is still active, could a concurrent timeout or interrupt re-queue the service task before unregister_netdev() finishes? This might allow the re-queued work item to execute and access the wx structure after it has been freed by devres.