On Thu, Jun 26, 2025 at 7:30 AM Lukas Wunner <lukas@xxxxxxxxx> wrote: > > 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(). > > > > I worry a little that the fix is equally subtle and we could easily > > reintroduce this issue with future code reorganization. > > I think this fix makes sense regardless of whether or not > the return value of pci_bridge_d3_possible() is cached: > > Right now pciehp_is_native() reads the Hot-Plug Capable bit from > the register even though the bit is cached in pci_dev->is_hotplug_bridge > and pci_bridge_d3_possible() only calls pciehp_is_native() if that flag > is set. In other words, pciehp_is_native() is re-checking the condition > under which it was called. That's just nonsensical and superfluous. Agreed. > There's only one other caller of pciehp_is_native() and that's > hotplug_is_native(). Only that other caller needs the register read, > so it should be moved there. > > So I think the question of whether the pci_bridge_d3_possible() return > value should be cached is orthogonal to this patch. +1
