* Re: [PATCH] igb: disable the PCI device when resume fails
2026-11-03 10:28 [PATCH] igb: disable the PCI device when resume fails Wentao Liang
@ 2026-11-03 10:34 ` netdev-bot+sinfo
2026-11-07 12:46 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sinfo @ 2026-11-03 10:34 UTC (permalink / raw)
To: Wentao Liang
Cc: Tony Nguyen, Przemek Kitszel, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, intel-wired-lan,
netdev, linux-kernel, stable
Hi!
This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:
- How the issue was discovered, e.g. hit in production, hit during
development, syzbot report, manual code inspection, LLM or static
analysis tool scan.
- Whether the issue was actually triggered, or is only theoretical
(e.g. found by code inspection). If it was triggered please include
the symptoms, like the stack trace or error messages.
- What hardware the change was tested on. For driver fixes please
mention the device (and if relevant firmware version) used for
testing, or say that the change was not tested on real hardware.
Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.
The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] igb: disable the PCI device when resume fails
2026-11-03 10:28 [PATCH] igb: disable the PCI device when resume fails Wentao Liang
2026-11-03 10:34 ` netdev-bot+sinfo
@ 2026-11-07 12:46 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-11-07 12:46 UTC (permalink / raw)
To: vulab
Cc: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, intel-wired-lan, netdev, linux-kernel,
stable
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 0 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] The patch adds pci_disable_device() calls that unbalance the PCI
enable count.
- [High] igb: missing interrupt and hw control cleanup in __igb_resume()
--
igb: disable the PCI device when resume fails
This patch adds pci_disable_device() calls to __igb_resume() for the cases
where igb_init_interrupt_scheme() or __igb_open() fails after
pci_enable_device_mem() has succeeded. The aim is to release the PCI
enable reference that resume takes when the resume fails.
> 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);
> 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);
> +
> return err;
> }
[Severity: High]
When __igb_open() fails here, the new error path disables the PCI
device but leaves the rest of what __igb_resume() set up in place.
Earlier in this function, igb_init_interrupt_scheme(adapter, true)
has enabled MSI-X or MSI on pdev. It has also allocated
adapter->q_vector[] and registered their NAPI instances on
netdev->napi_list. igb_get_hw_control() has then set
E1000_CTRL_EXT_DRV_LOAD.
None of the __igb_open() error paths call
igb_clear_interrupt_scheme(). If __igb_open() fails at err_setup_tx
or err_setup_rx, it also skips igb_release_hw_control(). That happens
when ring allocation fails in igb_setup_all_tx_resources() or
igb_setup_all_rx_resources().
So after this hunk the device is disabled while:
- MSI-X or MSI is still enabled on pdev
- the q_vectors and their NAPI instances are still allocated
- firmware still sees DRV_LOAD and believes the driver owns the
hardware
The runtime PM case is worse. When igb_runtime_resume() returns an
error, rpm_resume() leaves the device in RPM_SUSPENDED. The next
runtime resume then runs __igb_resume() and
igb_init_interrupt_scheme() again on this uncleaned state:
- If MSI-X was in use, pdev->msix_enabled is still set and
__pci_enable_msix_range() hits WARN_ON_ONCE(dev->msix_enabled).
igb then falls back to single vector MSI. That overwrites
adapter->num_q_vectors with 1 and leaks adapter->q_vector[1]
onwards.
- If MSI was in use, the same thing happens through
WARN_ON_ONCE(dev->msi_enabled) in __pci_enable_msi_range().
- igb_alloc_q_vector() then finds the existing q_vector[0] and
memsets it. It calls netif_napi_add_config() on a napi_struct
that is still linked on netdev->napi_list, which corrupts that
list.
If this patch is meant to unwind a failed resume, shouldn't this path
also undo igb_init_interrupt_scheme() and igb_get_hw_control()? For
example, igb_clear_interrupt_scheme() and igb_release_hw_control()
could run before the device is disabled.
__igb_shutdown() and igb_remove() already call both helpers, along
with pci_disable_device(). How do you intend the resume error unwind
to stay consistent with those teardown paths?
[Severity: High]
Can these new calls lead to a double pci_disable_device()?
The commit message says:
The PCI enable count stays elevated and the device remains enabled
even though the driver failed to resume it.
A failed resume doesn't unbind the driver. igb already drops its enable
reference unconditionally on every teardown path:
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);
...
}
igb_io_error_detected() also calls pci_disable_device() before it
requests a slot reset. __igb_shutdown() is reached from igb_suspend(),
igb_runtime_suspend() and igb_shutdown().
Without this patch the count looks balanced:
probe: 1
suspend: 0
resume: 1
failed resume: still 1
remove or next suspend: 0
With this patch, a failed resume drops enable_cnt to 0 while the driver
stays bound. Any of the following then calls pci_disable_device() a
second time:
- rmmod or unbind
- the next system or runtime suspend
- reboot or poweroff
- AER error_detected
pci_device_remove() calls drv->remove() even when pm_runtime_get_sync()
fails, so unbind can also hit this after a failed runtime resume.
In drivers/pci/pci.c:
pci_disable_device() {
dev_WARN_ONCE(&dev->dev, atomic_read(&dev->enable_cnt) <= 0,
"disabling already-disabled device");
if (atomic_dec_return(&dev->enable_cnt) != 0)
return;
...
}
That second call warns, which panics the host when panic_on_warn is set,
and leaves enable_cnt at -1.
>From then on, the count stays wrong:
- The next pci_enable_device_mem() in __igb_resume() moves the count
from -1 to 0. It still enables the device, and __igb_resume() turns
on bus mastering, but pci_is_enabled() now reports false.
- Each later pci_disable_device() moves the count from 0 to -1 and
returns early. do_pci_disable_device() never runs, so
PCI_COMMAND_MASTER is no longer cleared on suspend or unbind.
The existing teardown paths already release this reference. Should the
new pci_disable_device() calls be dropped? If they stay, the other
disable sites would need driver state to make them conditional.
Because of the Cc: stable tag, this change would also be backported to
stable trees.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103102849.1340413-1-vulab%40iscas.ac.cn
^ permalink raw reply [flat|nested] 3+ messages in thread