Re: [PATCH net-next v6 11/11] net: wangxun: implement pci_error_handlers ops

[Jakub Kicinski <kuba@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.
---
net: wangxun: implement pci_error_handlers ops

Support AER driver to handle the PCIe errors for Wangxun devices.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_err.c b/drivers/net/ethernet/wangxun/libwx/wx_err.c
> index bb77bdce69d22..86b318647a3ca 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_err.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_err.c
[ ... ]
> @@ -3,11 +3,113 @@
>  
>  #include <linux/netdevice.h>
>  #include <linux/pci.h>
> +#include <linux/aer.h>
>  
>  #include "wx_type.h"
>  #include "wx_lib.h"
>  #include "wx_err.h"
[ ... ]
> +static pci_ers_result_t wx_io_slot_reset(struct pci_dev *pdev)
> +{
> +	struct wx *wx = pci_get_drvdata(pdev);
> +	pci_ers_result_t result;
> +
> +	if (pci_enable_device_mem(pdev)) {
> +		wx_err(wx, "Cannot re-enable PCI device after reset.\n");
> +		result = PCI_ERS_RESULT_DISCONNECT;
> +	} else {
> +		/* make all bar access done before reset. */
> +		smp_mb__before_atomic();
> +		clear_bit(WX_STATE_DISABLED, wx->state);
> +		pci_set_master(pdev);
> +		pci_restore_state(pdev);
> +		pci_save_state(pdev);
> +		pci_wake_from_d3(pdev, false);
> +
> +		wx->do_reset(wx->netdev, false);

Is it possible that the ngbe driver ignores this hardware reset request?

If ngbe_do_reset() only performs an action when reinit is true, it appears it
would be a no-op when called with false from this AER recovery path. Will
this leave the ngbe MAC hardware uninitialized after the PCI slot reset and
break the device when traffic resumes?

> +		result = PCI_ERS_RESULT_RECOVERED;
> +	}
> +
> +	pci_aer_clear_nonfatal_status(pdev);
> +
> +	return result;
> +}
> +
> +/**
> + * 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;
> +
> +	netdev = wx->netdev;
> +	rtnl_lock();
> +	if (netif_running(netdev))
> +		netdev->netdev_ops->ndo_open(netdev);

Does this code need to check the return value of ndo_open()?

If ndo_open() fails due to memory allocation or IRQ request failures, the
resources will remain unallocated. Because the failure is ignored and
netif_device_attach() is called unconditionally below, will the networking
stack invoke the transmit handler and dereference the unallocated rings?

> +
> +	netif_device_attach(netdev);
> +	rtnl_unlock();
> +}