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

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.

[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?

+
+	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?

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?

+
+	/* 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);

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?

+
+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);
+

[ ... ]

[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?

[ ... ]