Re: [PATCH net-next v2 6/6] 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.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_err.c b/drivers/net/ethernet/wangxun/libwx/wx_err.c
> --- a/drivers/net/ethernet/wangxun/libwx/wx_err.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_err.c
[ ... ]
> +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;
> +
> +	netdev = wx->netdev;

If an AER event triggers during ngbe_probe() or txgbe_probe() before
pci_set_drvdata() is called, wx will be NULL here.

Could this lead to a NULL pointer dereference when accessing wx->netdev?

Should the handler validate the wx pointer, or should pci_set_drvdata()
be moved earlier in the probe sequence?

> +	if (!netif_device_present(netdev))
> +		return PCI_ERS_RESULT_DISCONNECT;
> +
> +	rtnl_lock();
> +	netif_device_detach(netdev);
> +
> +	if (netif_running(netdev))
> +		wx->close_suspend(wx);
> +
> +	if (state == pci_channel_io_perm_failure) {
> +		rtnl_unlock();
> +		return PCI_ERS_RESULT_DISCONNECT;
> +	}

Does calling wx->close_suspend(wx) before checking for
pci_channel_io_perm_failure cause illegal MMIO accesses?

If the device is in a permanent failure state, it is physically
disconnected, and MMIO accesses could result in Master Aborts or system
lockups.

Should the check for permanent failure precede the hardware teardown?

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

Should the background service task (wx->service_task) be canceled here
using cancel_work_sync()?

Since WX_STATE_DISABLED is not checked within the service task's
execution path, it appears the task could wake up and attempt to interact
with the device while its MMIO space is disabled.

[ ... ]
> +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_wake_from_d3(pdev, false);
> +
> +		wx->do_reset(wx->netdev, false);

Is it safe to call wx->do_reset() here without holding the rtnl_lock()?

Other paths that reset the hardware (such as wx_reset_subtask()) appear
to acquire rtnl_lock() to serialize against concurrent network
configurations.

Could failing to hold the lock here allow concurrent userspace requests
to access hardware registers while the device is in a reset state?

> +		result = PCI_ERS_RESULT_RECOVERED;
> +	}
> +
> +	pci_aer_clear_nonfatal_status(pdev);
> +
> +	return result;
> +}
[ ... ]
> +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);

By calling ndo_open() directly instead of dev_open(), is there a risk of
bypassing the core network state machine?

If ndo_open() fails, it cleans up its allocations, but the core network
stack's __LINK_STATE_START bit remains set.

If a user subsequently administratively downs the interface, the network
core might unconditionally call ndo_stop(), potentially leading to a
double-free when it attempts to clean up the already-freed resources.

> +		if (err) {
> +			wx_err(wx, "Failed to open netdev after reset\n");
> +			goto out;
> +		}
> +	}
> +	netif_device_attach(netdev);
> +out:
> +	rtnl_unlock();
> +}
[ ... ]
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
[ ... ]
> @@ -900,6 +901,7 @@ static int txgbe_probe(struct pci_dev *pdev,
>  		goto err_remove_phy;
>  
>  	pci_set_drvdata(pdev, wx);
> +	pci_save_state(pdev);
>  
>  	netif_tx_stop_all_queues(netdev);

This isn't a bug introduced by this patch, but does the ordering here
expose the interface to userspace before queue initialization?

Since register_netdev() is called prior to this code, the interface
becomes visible to userspace.

If a management daemon immediately brings the interface UP, invoking
ndo_open() and waking the TX queues, could this netif_tx_stop_all_queues()
erroneously stop the queues of the logically UP interface, resulting in a
silent TX hang?