RE: [PATCH net-next v11 5/5] net: wangxun: add pcie error handler

On Thu, Jul 23, 2026 5:17 PM, Simon Horman wrote:

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260716073822.24356-2-jiawenwu@trustnetic.com?part=5
---
Subject: net: wangxun: add pcie error handler

This patch wires up pci_error_handlers callbacks (error_detected,
slot_reset, resume) for the ngbe and txgbe drivers, adds a
WX_FLAG_NEED_PCIE_RECOVERY flag plus a down_suspend op, and routes
ndo_tx_timeout through a check that inspects config space and defers
to the AER path when the device looks disconnected.

quoted

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_err.c b/drivers/net/ethernet/wangxun/libwx/wx_err.c
index ee27f96735dc..0cb99e456a1c 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_err.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_err.c

@@ -4,11 +4,120 @@

 #include <linux/netdevice.h>
 #include <linux/pci.h>
+#include <linux/aer.h>

 #include "wx_type.h"
 #include "wx_lib.h"
 #include "wx_err.h"

+/**
+ * wx_io_error_detected - called when PCI error is detected
+ * @pdev: Pointer to PCI device
+ * @state: The current pci connection state
+ *
+ * Return: pci_ers_result_t.
+ *
+ * This function is called after a PCI bus error affecting
+ * this device has been detected.
+ */
+static pci_ers_result_t wx_io_error_detected(struct pci_dev *pdev,
+					     pci_channel_state_t state)
+{
+	struct wx *wx = pci_get_drvdata(pdev);
+	struct net_device *netdev;
+
+	if (!wx)
+		return PCI_ERS_RESULT_DISCONNECT;
+
+	netdev = wx->netdev;
+	if (!netif_device_present(netdev))
+		return PCI_ERS_RESULT_DISCONNECT;
+
+	if (state == pci_channel_io_perm_failure)
+		return PCI_ERS_RESULT_DISCONNECT;

[High]
Should the pci_channel_io_perm_failure branch perform teardown before
returning?  In this branch the device is guaranteed gone forever, yet no
netif_device_detach(), wx_soft_quiesce(), or pci_disable_device() runs.

The service_timer keeps firing every couple of seconds and
wx_check_hang_subtask()/wx_update_stats() will keep doing MMIO reads
against the disconnected device.  On arm64 an MMIO to a disconnected
device raises a synchronous external abort.  TX queues are also still up
and will keep taking skbs through ndo_start_xmit until remove() runs.

Compare with ixgbe_io_error_detected() in
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c, which performs
rtnl_lock/netif_device_detach/ixgbe_close_suspend before the
perm_failure check.  Would applying the same ordering here work?

quoted

+
+	rtnl_lock();
+	netif_device_detach(netdev);
+	set_bit(WX_FLAG_NEED_PCIE_RECOVERY, wx->flags);
+	wx_soft_quiesce(wx);
+
+	if (!test_and_set_bit(WX_STATE_DISABLED, wx->state))
+		pci_disable_device(pdev);
+	rtnl_unlock();
+
+	/* Request a slot reset. */
+	return PCI_ERS_RESULT_NEED_RESET;
+}
+
+/**
+ * wx_io_slot_reset - called after the pci bus has been reset.
+ * @pdev: Pointer to PCI device
+ *
+ * Return: pci_ers_result_t.
+ *
+ * Restart the card from scratch, as if from a cold-boot.
+ */
+static pci_ers_result_t wx_io_slot_reset(struct pci_dev *pdev)
+{
+	struct wx *wx = pci_get_drvdata(pdev);
+
+	if (pci_enable_device_mem(pdev)) {
+		wx_err(wx, "Cannot re-enable PCI device after reset.\n");
+		return PCI_ERS_RESULT_DISCONNECT;
+	}

[Medium, Low]
When pci_enable_device_mem() fails here, does wx_io_slot_reset() need to
unwind the state set up by wx_io_error_detected()?  WX_STATE_DISABLED,
WX_STATE_DOWN (via wx_soft_quiesce), and WX_FLAG_NEED_PCIE_RECOVERY all
remain latched because the clear_bit() calls further down are never
reached.

If remove() runs later, ngbe_close()/txgbe_close() reach
ngbe_down()/txgbe_down() and call ngbe_disable_device()/
txgbe_disable_device(), which do:

    if (test_and_set_bit(WX_STATE_DOWN, wx->state))
        return;

so the queue/ring disable MMIO sequence is skipped.  Similarly
ngbe_reset()/txgbe_reset() early-return on WX_FLAG_NEED_PCIE_RECOVERY,
so wx_flush_sw_mac_table() and related MAC cleanup is skipped.  Is that
intended?

If pci_enable_device_mem() fails here, it indicates that the PCIe recovery has
completely failed and the device's MMIO space is inaccessible. The only
solution is a system reboot. We don't need to unwind the state for a dead
device.

Separately, wx_err(wx, ...) here expands to
dev_err(&(wx)->pdev->dev, ...) and dereferences wx unconditionally,
while wx_io_error_detected() explicitly guards with
if (!wx) return PCI_ERS_RESULT_DISCONNECT.  Should the three callbacks
be consistent — either the guard in error_detected is dead code, or
slot_reset and resume are missing an intended check?

quoted

+
+	/* make all memory operations done before clearing the flag */
+	smp_mb__before_atomic();
+	clear_bit(WX_STATE_DISABLED, wx->state);
+	clear_bit(WX_FLAG_NEED_PCIE_RECOVERY, wx->flags);
+	pci_set_master(pdev);
+	pci_restore_state(pdev);
+	pci_wake_from_d3(pdev, false);
+
+	rtnl_lock();
+	if (netif_running(wx->netdev) && wx->down_suspend)
+		wx->down_suspend(wx);
+	if (wx->do_reset)
+		wx->do_reset(wx->netdev, false);
+	rtnl_unlock();
+
+	return PCI_ERS_RESULT_RECOVERED;
+}
+
+/**
+ * wx_io_resume - called when traffic can start flowing again.
+ * @pdev: Pointer to PCI device
+ *
+ * This callback is called when the error recovery driver tells us that
+ * its OK to resume normal operation.
+ */
+static void wx_io_resume(struct pci_dev *pdev)
+{
+	struct wx *wx = pci_get_drvdata(pdev);
+	struct net_device *netdev;
+	int err;
+
+	netdev = wx->netdev;
+	rtnl_lock();
+	if (netif_running(netdev)) {
+		err = netdev->netdev_ops->ndo_open(netdev);
+		if (err) {
+			wx_err(wx, "Failed to open netdev after reset\n");
+			goto out;
+		}
+	}
+	netif_device_attach(netdev);
+out:
+	rtnl_unlock();
+}

[High]
When ndo_open() fails in wx_io_resume(), the "out" path drops the rtnl
and returns without calling netif_device_attach(), without clearing
WX_STATE_DOWN, and without clearing WX_STATE_RES_FREED.

Because wx_io_error_detected() only cleared __LINK_STATE_PRESENT via
netif_device_detach(), __LINK_STATE_START is still set, so
netif_running() continues to return true.  WX_STATE_DOWN and
WX_STATE_RES_FREED (set earlier in the AER cycle by wx_soft_quiesce()
and down_suspend()) are only cleared inside ngbe_up_complete() /
txgbe_up_complete(), which are only reached if ndo_open() runs to
completion.

If open fails partway (wx_setup_resources, request_irq,
phylink_connect_phy, ...), the netdev is left detached but "running"
with WX_STATE_DOWN and WX_STATE_RES_FREED latched.  A subsequent
userspace "ip link set eth0 down" then hits the new short-circuit
early in ngbe_close()/txgbe_close():

    if (test_bit(WX_STATE_RES_FREED, wx->state))
        goto out;
    ...
    out:
        wx_control_hw(wx, false);

If ndo_open() fails during the resume phase, it indicates that the hardware or
its underlying resources could not be successfully restored after the PCIe
reset. At this point, the device is considered broken and unusable.

Therefore, intentionally leaving WX_STATE_DOWN and WX_STATE_RES_FREED latched
(and skipping netif_device_attach()) is the desired behavior. If we allowed
subsequent operations like 'ip link set eth0 down' or remove() to execute normal
hardware shutdown routines (such as queue/ring disable MMIOs), we would risk
triggering secondary errors or timeouts against a device that failed to
initialize properly. Short-circuiting these hardware accesses via the latched
flags is the safest approach.

Since ->resume returns void, there is no way to inform the AER core.
Is there a recovery path other than unbind/rebind if ndo_open() fails
here?

There is no alternative recovery path within the driver. The device must
either wait for another AER error interrupt/reset cycle to attempt recovery
again, or be manually removed/unbound by the user.

quoted

+
+const struct pci_error_handlers wx_err_handler = {
+	.error_detected = wx_io_error_detected,
+	.slot_reset = wx_io_slot_reset,
+	.resume = wx_io_resume,
+};
+EXPORT_SYMBOL(wx_err_handler);
+

[ ... ]

quoted

@@ -139,6 +251,33 @@ void wx_check_hang_subtask(struct wx *wx)
 }
 EXPORT_SYMBOL(wx_check_hang_subtask);

+static bool wx_check_pcie_error(struct wx *wx)
+{
+	u16 vid, pci_cmd;
+
+	pci_read_config_word(wx->pdev, PCI_VENDOR_ID, &vid);
+	pci_read_config_word(wx->pdev, PCI_COMMAND, &pci_cmd);
+
+	/* PCIe link loss or memory space can't access */
+	if (vid == U16_MAX || !(pci_cmd & PCI_COMMAND_MEMORY))
+		return true;
+
+	return false;
+}
+
+static void wx_tx_timeout_recovery(struct wx *wx)
+{
+	/*
+	 * When a PCIe hardware error occurs, the driver should initiate a PCIe
+	 * recovery mechanism. However, this recovery flow relies on the AER
+	 * driver for current kernel policy. Therefore, a self-contained
+	 * recovery mechanism is not implemented yet.
+	 */
+	set_bit(WX_FLAG_NEED_PCIE_RECOVERY, wx->flags);
+	wx_err(wx, "PCIe error detected during tx timeout\n");
+	queue_work(wx->reset_wq, &wx->reset_task);
+}

[Medium]
What happens on a platform where AER never fires (AER disabled at boot,
firmware masks the event, or CONFIG_PCIEAER=n)?

wx_tx_timeout_recovery() only sets WX_FLAG_NEED_PCIE_RECOVERY and
queues reset_task.  WX_FLAG_NEED_PCIE_RECOVERY is only cleared in
wx_io_slot_reset(), so without AER the flag latches.

wx_reset_task() then does:

    rtnl_lock();
    if (test_bit(WX_FLAG_NEED_PCIE_RECOVERY, wx->flags))
        wx_soft_quiesce(wx);           /* sets WX_STATE_DOWN */
    if (test_bit(WX_STATE_DOWN, wx->state) ||
        test_bit(WX_STATE_RESETTING, wx->state))
        goto out;                      /* always taken now */
    wx_pf_reset_subtask(wx);           /* checks a different flag */

and ngbe_reset()/txgbe_reset() early-return on
WX_FLAG_NEED_PCIE_RECOVERY, so any future reset is a silent no-op.  Is
there an intended fallback recovery when AER is unavailable, or is the
interface expected to stay unusable until reboot?  The comment
acknowledges "a self-contained recovery mechanism is not implemented
yet" — should this be gated on IS_ENABLED(CONFIG_PCIEAER) or similar?

Stay unusable until reboot.

quoted

+
 static void wx_tx_timeout_reset(struct wx *wx)
 {
 	if (test_bit(WX_STATE_DOWN, wx->state))

@@ -153,7 +292,10 @@ void wx_tx_timeout(struct net_device *netdev, unsigned int __always_unused txque
 {
 	struct wx *wx = netdev_priv(netdev);

-	wx_tx_timeout_reset(wx);
+	if (wx_check_pcie_error(wx))
+		wx_tx_timeout_recovery(wx);
+	else
+		wx_tx_timeout_reset(wx);
 }
 EXPORT_SYMBOL(wx_tx_timeout);

[ ... ]