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

[Simon Horman <horms@kernel.org>]

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 [4]: [PATCH net-next v10 4/5] net: wangxun: implement soft quiesce for PCIe error recovery ---
commit 1b8a0e5e722ffb89c8fe24492e80dca44f3410f2
Author: Jiawen Wu <jiawenwu@trustnetic.com>

net: wangxun: implement soft quiesce for PCIe error recovery

This commit introduces a soft quiesce path for PCIe error recovery. It adds
WX_STATE_DOWN and WX_STATE_RESETTING state checks to subtasks to block
execution, explicitly avoiding a cancellation of the service task to speed
up recovery.

> 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;