Re: [net-next,v4,10/10] net: wangxun: implement pci_error_handlers ops

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: wangxun: implement pci_error_handlers ops

This commit adds PCIe AER error handling support by implementing
pci_error_handlers operations. The implementation includes error
detection, slot reset, and resume callbacks with device state tracking.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_err.c b/drivers/net/ethernet/wangxun/libwx/wx_err.c
> index bb77bdce69d2..86b318647a3c 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_err.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_err.c

[ ... ]

> +static pci_ers_result_t wx_io_slot_reset(struct pci_dev *pdev)
> +{
> +	struct wx *wx = pci_get_drvdata(pdev);
> +	pci_ers_result_t result;
> +
> +	if (pci_enable_device_mem(pdev)) {
> +		wx_err(wx, "Cannot re-enable PCI device after reset.\n");
> +		result = PCI_ERS_RESULT_DISCONNECT;
> +	} else {
> +		/* make all bar access done before reset. */
> +		smp_mb__before_atomic();
> +		clear_bit(WX_STATE_DISABLED, wx->state);
> +		pci_set_master(pdev);
> +		pci_restore_state(pdev);
> +		pci_save_state(pdev);
> +		pci_wake_from_d3(pdev, false);
> +
> +		wx->do_reset(wx->netdev, false);
> +		result = PCI_ERS_RESULT_RECOVERED;
> +	}
> +
> +	pci_aer_clear_nonfatal_status(pdev);
> +
> +	return result;
> +}

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index 5a21478a592b..1cd73b424d7f 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> @@ -3418,7 +3418,8 @@ static void wx_dev_shutdown(struct pci_dev *pdev, bool *enable_wake)
>  	*enable_wake = !!wufc;
>  	wx_control_hw(wx, false);
>
> -	pci_disable_device(pdev);
> +	if (!test_and_set_bit(WX_STATE_DISABLED, wx->state))
> +		pci_disable_device(pdev);
>  }
>
>  int wx_suspend(struct pci_dev *pdev, pm_message_t state)

Does this code correctly handle the WX_STATE_DISABLED bit across suspend/resume
cycles?

The AER recovery path clears this bit in wx_io_slot_reset():

wx_err.c:wx_io_slot_reset() {
	...
	clear_bit(WX_STATE_DISABLED, wx->state);
	...
}

But the normal PM resume path doesn't clear it. Looking at wx_resume() in
wx_lib.c (lines 3436-3464), after calling pci_enable_device_mem()
successfully, the WX_STATE_DISABLED bit remains set from the earlier
suspend.

This means after a suspend/resume cycle:
1. wx_suspend() calls wx_dev_shutdown() which sets WX_STATE_DISABLED
2. wx_resume() calls pci_enable_device_mem() but doesn't clear the bit
3. A subsequent wx_dev_shutdown() (from second suspend or shutdown) will see
   the bit already set and skip pci_disable_device(), leaving the device
   enabled

The same stale bit affects ngbe_remove() and txgbe_remove(), which would skip
pci_disable_device() if the device was previously suspended and resumed.

Should wx_resume() clear WX_STATE_DISABLED after successfully enabling the
device, matching the pattern in wx_io_slot_reset()?