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

[Simon Horman <horms@kernel.org>]

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.

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

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

[ ... ]

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

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

[ ... ]