RE: [PATCH net-next v10 4/5] net: wangxun: implement soft quiesce for PCIe error recovery

quoted

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index c8a4822d38194..a65312c8078fa 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c

[ ... ]

quoted

@@ -3383,5 +3383,23 @@ void wx_service_timer(struct timer_list *t)
 }
 EXPORT_SYMBOL(wx_service_timer);

+void wx_soft_quiesce(struct wx *wx)
+{
+	if (!netif_running(wx->netdev) ||
+	    test_and_set_bit(WX_STATE_DOWN, wx->state))
+		return;
+
+	pci_clear_master(wx->pdev);
+	netif_tx_stop_all_queues(wx->netdev);
+	netif_carrier_off(wx->netdev);
+	netif_tx_disable(wx->netdev);
+	wx_napi_disable_all(wx);
+	wx_ptp_quiesce(wx);
+
+	clear_bit(WX_FLAG_NEED_PF_RESET, wx->flags);
+	timer_delete_sync(&wx->service_timer);

[Severity: High]
The commit message notes the service task is deliberately not cancelled here
to avoid delaying PCIe error recovery. However, does omitting
cancel_work_sync(&wx->service_task) open a race window with the subtasks
that rely on the WX_STATE_DOWN flag?

quoted

+}
+EXPORT_SYMBOL(wx_soft_quiesce);

quoted

diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index a8773712cff82..a7bde03a98feb 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c

[ ... ]

quoted

@@ -94,6 +94,10 @@ static void txgbe_module_detection_subtask(struct wx *wx)
 {
 	int err;

+	if (test_bit(WX_STATE_DOWN, wx->state) ||
+	    test_bit(WX_STATE_RESETTING, wx->state))
+		return;
+

[Severity: High]
Does this state check adequately protect against concurrent MMIO access
during a PCIe reset?

If the task passes this check, it subsequently sleeps for 200ms:

txgbe_main.c:txgbe_module_detection_subtask() {
    ...
    if (!test_and_clear_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags))
        return;

    /* wait for SFF module ready */
    msleep(200);

    err = txgbe_identify_module(wx);
    ...
}

If wx_soft_quiesce() runs on another CPU during this msleep(), it will set
WX_STATE_DOWN and begin a hardware reset.

When the task wakes up from the sleep, will it call txgbe_identify_module()
and perform MMIO reads (such as rd32(wx, WX_GPIO_EXT)) while the device is
in reset?

Could this time-of-check to time-of-use race lead to Master Aborts or
fatal PCIe errors during AER recovery?

quoted

 	if (!test_and_clear_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags))
 		return;

This task is typically triggered during interface bring-up or optical module
insertion. It has a very low coincidence rate with the timing of pcie errors.
Nevertheless, as a defensive measure, I will add a re-check of WX_STATE_DOWN
immediately after the sleep returns.