Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Lukas Wunner <lukas@wunner.de>]

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