RE: [PATCH net-next v2 6/6] net: wangxun: implement pci_error_handlers ops

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Sun, May 3, 2026 10:16 AM, Jakub Kicinski 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.
> ---
> 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?

I'll validate the wx pointer first.

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

I'll fix it.

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

Service task is canceled in the device close path, and no event triggers it
when netif is not running.

> 
> [ ... ]
> > +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.

I think there is no need to acquire rtnl_lock().

> 
> 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 ndo_open() fails, the hardware state must have been wrong beyond
recovery. At this point, all further actions are invalid.

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

I'll consider this in another patch.
Thanks.