Re: [PATCH] igb: disable the PCI device when resume fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] igb: double pci_disable_device() after __igb_resume() failure
- [High] igb: missing interrupt and hw control cleanup in __igb_resume()
--

commit 7cf84b374723aebb6dc086448341821f57e54043
Author: Wentao Liang <vulab@iscas.ac.cn>

igb: disable the PCI device when resume fails

This commit calls pci_disable_device() in __igb_resume() when
igb_init_interrupt_scheme() or __igb_open() fails after
pci_enable_device_mem() has succeeded.

> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index d4a897a8c82c6..853bc99a10113 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -9621,6 +9621,7 @@ static int __igb_resume(struct device *dev, bool rpm)
>  
>  	if (igb_init_interrupt_scheme(adapter, true)) {
>  		dev_err(&pdev->dev, "Unable to allocate memory for queues\n");
> +		pci_disable_device(pdev);

[Severity: High]
Can calling pci_disable_device(pdev) on failure in __igb_resume() lead to a
double disable and pdev->enable_cnt underflow later?

During suspend, __igb_shutdown() calls pci_disable_device(pdev), which drops
pdev->enable_cnt from 1 to 0. When __igb_resume() runs,
pci_enable_device_mem(pdev) increments pdev->enable_cnt back from 0 to 1.

If igb_init_interrupt_scheme() or __igb_open() fails, calling
pci_disable_device(pdev) here drops pdev->enable_cnt back to 0 while the
driver remains bound to the device.

Because igb does not track whether the PCI device is enabled, a subsequent
call to igb_remove(), __igb_shutdown() (via suspend or shutdown), or
igb_io_error_detected() will access MMIO registers on the disabled device and
unconditionally call pci_disable_device(pdev) again:

drivers/net/ethernet/intel/igb/igb_main.c:igb_remove() {
	...
	free_netdev(netdev);

	pci_disable_device(pdev);
}

drivers/net/ethernet/intel/igb/igb_main.c:__igb_shutdown() {
	...
	igb_release_hw_control(adapter);

	pci_disable_device(pdev);

	return 0;
}

Wouldn't that second pci_disable_device(pdev) call trigger the
"disabling already-disabled device" dev_WARN_ONCE() in pci_disable_device()
and underflow pdev->enable_cnt to -1?

>  		return -ENOMEM;
>  	}
>  
> @@ -9647,6 +9648,9 @@ static int __igb_resume(struct device *dev, bool rpm)
>  	if (!rpm)
>  		rtnl_unlock();
>  
> +	if (err)
> +		pci_disable_device(pdev);

[Severity: High]
When __igb_open() fails here in __igb_resume(), does this error path leave
the interrupt scheme and firmware hardware control active while disabling the
PCI device?

Earlier in __igb_resume(), the driver sets up MSI-X/MSI interrupts, allocates
adapter->q_vector[] and NAPI instances, and acquires hardware control from
firmware:

drivers/net/ethernet/intel/igb/igb_main.c:__igb_resume() {
	...
	if (igb_init_interrupt_scheme(adapter, true)) {
		...
	}

	igb_reset(adapter);

	igb_get_hw_control(adapter);
	...
	if (!err && netif_running(netdev))
		err = __igb_open(netdev, true);
}

If __igb_open() fails (for example, at err_setup_tx or err_setup_rx when ring
allocation fails), __igb_open() does not call igb_clear_interrupt_scheme() or
igb_release_hw_control():

drivers/net/ethernet/intel/igb/igb_main.c:__igb_open() {
	...
err_req_irq:
	igb_release_hw_control(adapter);
	igb_power_down_link(adapter);
	igb_free_all_rx_resources(adapter);
err_setup_rx:
	igb_free_all_tx_resources(adapter);
err_setup_tx:
	igb_reset(adapter);
	if (!resuming)
		pm_runtime_put(&pdev->dev);

	return err;
}

If this failure happens during igb_runtime_resume(), the device remains in
RPM_SUSPENDED, and the next runtime resume attempt will call
igb_init_interrupt_scheme(adapter, true) again while pdev->msix_enabled or
pdev->msi_enabled is still set and adapter->q_vector[] is still populated.
Wouldn't that hit WARN_ON_ONCE() in __pci_enable_msix_range() or
__pci_enable_msi_range(), leak q_vectors if MSI-X falls back to MSI, and
corrupt netdev->napi_list when igb_alloc_q_vector() runs memset() and
netif_napi_add_config() on already-registered NAPI structures? Should this
error path also unwind igb_get_hw_control() and igb_init_interrupt_scheme()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103102849.1340413-1-vulab@iscas.ac.cn?part=1