RE: [PATCH net-next v10 4/5] net: wangxun: implement soft quiesce for PCIe error recovery

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

> > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > index c8a4822d38194..a65312c8078fa 100644
> > --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> [ ... ]
> > @@ -3383,5 +3383,23 @@ void wx_service_timer(struct timer_list *t)
> >  }
> >  EXPORT_SYMBOL(wx_service_timer);
> >
> > +void wx_soft_quiesce(struct wx *wx)
> > +{
> > +	if (!netif_running(wx->netdev) ||
> > +	    test_and_set_bit(WX_STATE_DOWN, wx->state))
> > +		return;
> > +
> > +	pci_clear_master(wx->pdev);
> > +	netif_tx_stop_all_queues(wx->netdev);
> > +	netif_carrier_off(wx->netdev);
> > +	netif_tx_disable(wx->netdev);
> > +	wx_napi_disable_all(wx);
> > +	wx_ptp_quiesce(wx);
> > +
> > +	clear_bit(WX_FLAG_NEED_PF_RESET, wx->flags);
> > +	timer_delete_sync(&wx->service_timer);
> 
> [Severity: High]
> The commit message notes the service task is deliberately not cancelled here
> to avoid delaying PCIe error recovery. However, does omitting
> cancel_work_sync(&wx->service_task) open a race window with the subtasks
> that rely on the WX_STATE_DOWN flag?
> 
> > +}
> > +EXPORT_SYMBOL(wx_soft_quiesce);
> 
> > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> > index a8773712cff82..a7bde03a98feb 100644
> > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> [ ... ]
> > @@ -94,6 +94,10 @@ static void txgbe_module_detection_subtask(struct wx *wx)
> >  {
> >  	int err;
> >
> > +	if (test_bit(WX_STATE_DOWN, wx->state) ||
> > +	    test_bit(WX_STATE_RESETTING, wx->state))
> > +		return;
> > +
> 
> [Severity: High]
> Does this state check adequately protect against concurrent MMIO access
> during a PCIe reset?
> 
> If the task passes this check, it subsequently sleeps for 200ms:
> 
> txgbe_main.c:txgbe_module_detection_subtask() {
>     ...
>     if (!test_and_clear_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags))
>         return;
> 
>     /* wait for SFF module ready */
>     msleep(200);
> 
>     err = txgbe_identify_module(wx);
>     ...
> }
> 
> If wx_soft_quiesce() runs on another CPU during this msleep(), it will set
> WX_STATE_DOWN and begin a hardware reset.
> 
> When the task wakes up from the sleep, will it call txgbe_identify_module()
> and perform MMIO reads (such as rd32(wx, WX_GPIO_EXT)) while the device is
> in reset?
> 
> Could this time-of-check to time-of-use race lead to Master Aborts or
> fatal PCIe errors during AER recovery?
> 
> >  	if (!test_and_clear_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags))
> >  		return;
> 

This task is typically triggered during interface bring-up or optical module
insertion. It has a very low coincidence rate with the timing of pcie errors.
Nevertheless, as a defensive measure, I will add a re-check of WX_STATE_DOWN
immediately after the sleep returns.