RE: [PATCH net-next v4 4/5] net: wangxun: introduce soft quiesce callbacks for AER recovery

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

On Tue, Jun 2, 2026 7:16 PM, Larysa Zaremba wrote:
> On Mon, Jun 01, 2026 at 03:22:20PM +0800, Jiawen Wu wrote:
> > Introduce device-specific soft quiesce callbacks for ngbe and txgbe to
> > provide a lightweight shutdown path during PCI error recovery. It avoids
> > MMIO-dependent operations in PCI error status, for the later
> > implementation of PCIe error callback function in libwx.
> >
> > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> > ---
> >  drivers/net/ethernet/wangxun/libwx/wx_type.h  |  1 +
> >  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 23 +++++++++++++++++++
> >  .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 22 ++++++++++++++++++
> >  3 files changed, 46 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> > index a8b4e84787f4..1b25a52188f7 100644
> > --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> > +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> > @@ -1409,6 +1409,7 @@ struct wx {
> >  	void (*configure_fdir)(struct wx *wx);
> >  	int (*setup_tc)(struct net_device *netdev, u8 tc);
> >  	void (*do_reset)(struct net_device *netdev, bool reinit);
> > +	void (*soft_quiesce)(struct wx *wx);
> >  	int (*ptp_setup_sdp)(struct wx *wx);
> >  	void (*set_num_queues)(struct wx *wx);
> >
> > diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> > index 3dc8342dd3a7..2484d3177034 100644
> > --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> > +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> > @@ -47,6 +47,28 @@ static const struct pci_device_id ngbe_pci_tbl[] = {
> >  	{ }
> >  };
> >
> > +static void ngbe_soft_quiesce(struct wx *wx)
> > +{
> > +	if (test_and_set_bit(WX_STATE_DOWN, wx->state))
> > +		return;
> 
> Careful about reusing the same flags for different reinit processes.

I think WX_STATE_DOWN should be used here.
The soft_quiesce calls back only in pcie error handler when netif is running.
It is desired to implement all software quiesce on the device close path.

> 
> > +
> > +	wx_ptp_stop(wx);
> > +	phylink_stop(wx->phylink);
> > +	pci_clear_master(wx->pdev);
> > +	wx_napi_disable_all(wx);
> 
> Sashiko says:
> 
>  "If NAPI is not disabled yet, the NAPI poll routine wx_clean_tx_irq() can run
>   concurrently. If WX_STATE_PTP_TX_IN_PROGRESS is set, wx_clean_tx_irq() might
>   call ptp_schedule_worker(), leading to a dereference of the ptp_clock
>   object just as it's being freed."
> 
> I am not sure, if such race actually happens, but stopping napi before any kind
> of deinit is not a terrible idea.

WX_STATE_PTP_TX_IN_PROGRESS is cleared in wx_ptp_stop().
But I'll change wx_ptp_stop() to wx_ptp_quiesce() that drop all MMIO operations.

> 
> > +
> > +	clear_bit(WX_FLAG_NEED_PF_RESET, wx->flags);
> 
> Same here, what if a caller needs a full reset?

PCIe error handler has a higher priority and it will achieve a more complete
reset. In fact, these two resets are not expected to be executed at the same
time.

> 
> > +	timer_delete_sync(&wx->service_timer);
> 
> Sashiko says:
> 
>  "The timer is stopped, but the corresponding cancel_work_sync(&wx->service_task)
>   is missing."
> 
> And it does seem to me that this is the case here. I see that you have explained
> this in another thread, but you proposal does not address the case, when
> ngbe_service_task() starts, interface is not down, and after that we have a PCI
> error, and its handling interferes with service task. While this is unlikely,
> synchronization should be more robust.

I plan to check WX_STATE_DOWN and WX_STATE_RESETTING at the entry of every work
item. Looks like a rough block, but is there a better way? :(

> 
> > +
> > +	wx_clean_all_tx_rings(wx);
> > +	wx_clean_all_rx_rings(wx);
> > +
> > +	wx_free_irq(wx);
> > +	wx_free_isb_resources(wx);
> > +	wx_free_resources(wx);
> > +	phylink_disconnect_phy(wx->phylink);
> > +}
> > +
> >  /**
> >   *  ngbe_init_type_code - Initialize the shared code
> >   *  @wx: pointer to hardware structure
> > @@ -135,6 +157,7 @@ static int ngbe_sw_init(struct wx *wx)
> >  	wx->mbx.size = WX_VXMAILBOX_SIZE;
> >  	wx->setup_tc = ngbe_setup_tc;
> >  	wx->do_reset = ngbe_do_reset;
> > +	wx->soft_quiesce = ngbe_soft_quiesce;
> >  	set_bit(0, &wx->fwd_bitmask);
> >
> >  	return 0;
> > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> > index b37c9ed57cf7..816aba4a9099 100644
> > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> > @@ -296,6 +296,27 @@ void txgbe_up(struct wx *wx)
> >  	txgbe_up_complete(wx);
> >  }
> >
> > +static void txgbe_soft_quiesce(struct wx *wx)
> > +{
> > +	if (test_and_set_bit(WX_STATE_DOWN, wx->state))
> > +		return;
> > +
> > +	wx_ptp_stop(wx);
> > +	phylink_stop(wx->phylink);
> > +	pci_clear_master(wx->pdev);
> > +	wx_napi_disable_all(wx);
> > +
> > +	clear_bit(WX_FLAG_NEED_PF_RESET, wx->flags);
> > +	timer_delete_sync(&wx->service_timer);
> > +
> > +	wx_clean_all_tx_rings(wx);
> > +	wx_clean_all_rx_rings(wx);
> > +
> > +	wx_free_irq(wx);
> > +	txgbe_free_misc_irq(wx->priv);
> > +	wx_free_resources(wx);
> > +}
> > +
> >  /**
> >   *  txgbe_init_type_code - Initialize the shared code
> >   *  @wx: pointer to hardware structure
> > @@ -412,6 +433,7 @@ static int txgbe_sw_init(struct wx *wx)
> >
> >  	wx->setup_tc = txgbe_setup_tc;
> >  	wx->do_reset = txgbe_do_reset;
> > +	wx->soft_quiesce = txgbe_soft_quiesce;
> >  	set_bit(0, &wx->fwd_bitmask);
> >
> >  	switch (wx->mac.type) {
> > --
> > 2.51.0
> >
>