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

[Simon Horman <horms@kernel.org>]

On Thu, Jul 16, 2026 at 02:55:30PM +0800, Jiawen Wu wrote:
> 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.

Thanks.

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

Sorry about that, I missed that this is already covered.

...