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 <[email protected]> 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/[email protected]?part=1
