On Wed, Jun 25, 2025 at 9:37 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().
>
> 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.
Yes, please!
