On Tue, Jun 24, 2025 at 09:24:07AM -0500, Bjorn Helgaas wrote:
> On Mon, Jun 23, 2025 at 07:08:20PM +0200, Lukas Wunner wrote:
> > pcie_portdrv_probe() and pcie_portdrv_remove() both call
> > pci_bridge_d3_possible() to determine whether to use runtime power
> > management. The underlying assumption is that pci_bridge_d3_possible()
> > always returns the same value because otherwise a runtime PM reference
> > imbalance occurs.
> >
> > That assumption falls apart if the device is inaccessible on ->remove()
> > due to hot-unplug: pci_bridge_d3_possible() calls pciehp_is_native(),
> > which accesses Config Space to determine whether the device is Hot-Plug
> > Capable. An inaccessible device returns "all ones", which is converted
> > to "all zeroes" by pcie_capability_read_dword(). Hence the device no
> > longer seems Hot-Plug Capable on ->remove() even though it was on
> > ->probe().
>
> This is pretty subtle; thanks for chasing it down.
>
> It doesn't look like anything in pci_bridge_d3_possible() should
> change over the life of the device, although acpi_pci_bridge_d3() is
> non-trivial.
>
> Should we consider calling pci_bridge_d3_possible() only once and
> caching the result? We already call it in pci_pm_init() and save the
> result in dev->bridge_d3. That member can be changed by
> pci_bridge_d3_update(), but we could add another copy that we never
> update after pci_pm_init().
If we did that, I think we'd still want to have a WARN_ON() like this in
pcie_portdrv_remove():
+ WARN_ON(dev->bridge_d3_orig != pci_bridge_d3_possible(dev));
+
+ if (dev->bridge_d3_orig) {
- if (pci_bridge_d3_possible(dev)) {
Because without the WARN_ON(), such bugs would fly under the radar.
However currently we get the WARN_ON() for free because of the runtime PM
refcount underflow.
So caching the original return value of pci_bridge_d3_possible(dev)
wouldn't be a net positive.
Also note that the bug isn't catastrophic: The struct device is about
to be free()'d anyway because it's been hot-removed. It's just the
annoying warning message that we want to get rid of.
But maybe we should amend the kernel-doc of pci_bridge_d3_possible()
to clearly state that the return value must be constant across the
entire lifetime of the device. For me that's obvious because I was
involved when the code was originally conceived, but I realized upon
seeing Mario's attempts to solve this that it may not be obvious at all
for anyone else.
Thanks,
Lukas
