On Wed, Jun 25, 2025 at 02:32:31PM -0500, Bjorn Helgaas wrote:
> On Wed, Jun 25, 2025 at 09:37:38AM +0200, Lukas Wunner wrote:
> > On Tue, Jun 24, 2025 at 09:24:07AM -0500, Bjorn Helgaas wrote:
> > > 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.
[...]
> But I feel like I'm missing your point about bugs flying under the
> radar. Having portdrv keep track of whether it did runtime PM setup
> (i.e., the pci_bridge_d3_possible() state at .probe()-time) is
> functionally the same as having struct pci_dev keep track of it, so
> the bugs you're referring to could still fly under the radar.
So the return value of pci_bridge_d3_possible() should never change
over the lifetime of a device. We're also invoking that function
from pci_bridge_d3_update() and the logic would no longer work
if the return value changed.
My point is that we're currently verifying that the return value
hasn't changed by regenerating it in pcie_portdrv_remove().
If it *has* changed, the runtime PM ref imbalance occurs and we get
a warning message.
If we instead cached the value in pcie_portdrv_probe(), we wouldn't
have found this bug.
Does that make sense?
Thanks,
Lukas
