RE: [PATCH net-next v10 5/5] net: wangxun: add pcie error handler

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

On Tue, Jul 14, 2026 5:11 PM, Simon Horman 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.
> Full review at: https://sashiko.dev
> ---
> --- Patch [5]: [PATCH net-next v10 5/5] net: wangxun: add pcie error handler ---
> commit 91baa4e77d07ab6772569a7d37ed86cfda625c00
> Author: Jiawen Wu <jiawenwu@trustnetic.com>
> Subject: net: wangxun: add pcie error handler
> 
> This commit adds PCIe error handling support (AER) to the wangxun drivers
> (ngbe and txgbe). It detects hardware errors, attempts to soft quiesce,
> and registers standard PCI error handler callbacks.
> 
> > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_err.c b/drivers/net/ethernet/wangxun/libwx/wx_err.c
> > index ee27f96735dc7..0cb99e456a1c9 100644
> > --- a/drivers/net/ethernet/wangxun/libwx/wx_err.c
> > +++ b/drivers/net/ethernet/wangxun/libwx/wx_err.c
> 
> [ ... ]
> 
> > +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);
> 
> [Severity: High]
> Does this sequence risk corrupting the hw_tc_lock seqlock?
> 
> During AER recovery, wx_io_slot_reset() calls down_suspend(), which bypasses
> wx_ptp_stop(). This leaves the PTP clock registered and potentially in use by
> userspace.
> 
> When wx_io_resume() then calls ndo_open(), this triggers txgbe_up() or
> ngbe_up(), which in turn calls wx_ptp_init(). Since wx_ptp_init()
> unconditionally calls seqlock_init(&wx->hw_tc_lock), won't this corrupt the
> lock if userspace is actively issuing PTP ioctls?

PTP clock was destroyed in wx_ptp_quiesce() before V8 patch set.
The piece of code was removed based on the Sashiko's comment.
I think it should be added back because the order of the calls in wx_soft_quiesce()
has been changed.

> 
> > +		if (err) {
> > +			wx_err(wx, "Failed to open netdev after reset\n");
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> > index 92895f5035118..56d4b63387fd6 100644
> > --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> > +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> 
> [ ... ]
> 
> > @@ -413,6 +428,9 @@ static void ngbe_disable_device(struct wx *wx)
> >
> >  static void ngbe_reset(struct wx *wx)
> >  {
> > +	if (test_bit(WX_FLAG_NEED_PCIE_RECOVERY, wx->flags))
> > +		return;
> > +
> >  	wx_flush_sw_mac_table(wx);
> 
> [Severity: High]
> Is the hardware properly reinitialized here after an AER slot reset?
> 
> In wx_io_slot_reset(), the hardware needs to be restarted from scratch.
> While txgbe_reset() calls txgbe_reset_hw(), ngbe_reset() only flushes the
> software MAC tables and skips calling ngbe_reset_hw().
> 
> Because this patch registers wx_err_handler for the ngbe driver, won't this
> leave the hardware uninitialized and the interface broken after a PCIe slot
> reset?

I think I have answered this question many times and also described ngbe's lack
of functionality in the commit and cover-letter messages.

> 
> [ ... ]
> 
> > @@ -529,12 +548,16 @@ static int ngbe_close(struct net_device *netdev)
> >  {
> >  	struct wx *wx = netdev_priv(netdev);
> >
> > +	if (test_bit(WX_STATE_RES_FREED, wx->state))
> > +		goto out;
> > +
> >  	wx_ptp_stop(wx);
> 
> [Severity: High]
> Does this early return cause a use-after-free for the PTP clock?
> 
> If a PCI error occurs, down_suspend() sets WX_STATE_RES_FREED but
> intentionally skips wx_ptp_stop(). If the interface is subsequently closed or
> unregistered, ngbe_close() will return early here and never call
> wx_ptp_stop().
> 
> Because the PTP clock device is never unregistered, it remains exposed to
> userspace after the driver's memory is freed. Can this lead to a
> use-after-free if userspace interacts with the clock later?
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> > index a7bde03a98feb..d85ee83192e4f 100644
> > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> 
> [ ... ]
> 
> > @@ -530,12 +549,16 @@ static int txgbe_close(struct net_device *netdev)
> >  {
> >  	struct wx *wx = netdev_priv(netdev);
> >
> > +	if (test_bit(WX_STATE_RES_FREED, wx->state))
> > +		goto out;
> > +
> >  	wx_ptp_stop(wx);
> 
> [Severity: High]
> Similar to ngbe_close() above, does this early return skip wx_ptp_stop() and
> lead to a use-after-free if the txgbe device is closed after an AER suspend?
>