Re: [PATCH] PCI: Revert / replace the cfg_access_lock lockdep mechanism

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, May 30, 2024 at 12:53:46PM -0700, Dan Williams wrote:
> Dan Williams wrote:
> > While the experiment did reveal that there are additional places that
> > are missing the lock during secondary bus reset, one of the places that
> > needs to take cfg_access_lock (pci_bus_lock()) is not prepared for
> > lockdep annotation.
> > 
> > Specifically, pci_bus_lock() takes pci_dev_lock() recursively and is
> > currently dependent on the fact that the device_lock() is marked
> > lockdep_set_novalidate_class(&dev->mutex). Otherwise, without that
> > annotation, pci_bus_lock() would need to use something like a new
> > pci_dev_lock_nested() helper, a scheme to track a PCI device's depth in
> > the topology, and a hope that the depth of a PCI tree never exceeds the
> > max value for a lockdep subclass.
> > 
> > The alternative to ripping out the lockdep coverage would be to deploy a
> > dynamic lock key for every PCI device. Unfortunately, there is evidence
> > that increasing the number of keys that lockdep needs to track to be
> > per-PCI-device is prohibitively expensive for something like the
> > cfg_access_lock.
> > 
> > The main motivation for adding the annotation in the first place was to
> > catch unlocked secondary bus resets, not necessarily catch lock ordering
> > problems between cfg_access_lock and other locks.
> > 
> > Replace the lockdep tracking with a pci_warn_once() for that primary
> > concern.
> > 
> > Fixes: 7e89efc6e9e4 ("PCI: Lock upstream bridge for pci_reset_function()")
> > Reported-by: Imre Deak <imre.deak@intel.com>
> > Closes: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_134186v1/shard-dg2-1/igt@device_reset@unbind-reset-rebind.html
> > Cc: Jani Saarinen <jani.saarinen@intel.com>
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Bjorn, this against mainline, not your tree where I see you already have
> "PCI: Make cfg_access_lock lockdep key a singleton" queued up. The
> "overkill" justification for making it singleton is valid, but then
> means that it has all the same problems as the device lock that needs to
> be marked lockdep_set_novalidate_class().
> 
> Let me know if you want this rebased on your for-linus branch.
> 
> Note that the pci_warn_once() will trigger on all pci_bus_reset() users
> unless / until pci_bus_lock() additionally locks the bridge itself ala:
> 
> http://lore.kernel.org/r/6657833b3b5ae_14984b29437@dwillia2-xfh.jf.intel.com.notmuch
> 
> Apologies for the thrash, this has been a useful exercise for finding
> some of these gaps, but ultimately not possible to carry forward
> without more invasive changes.

No problem, this is a complicated locking scenario.  These fixes are
the only thing on my for-linus branch (which I regard as a draft
rather than being immutable) and I haven't asked Linus to pull them
yet, so I'll just drop both:

  ac445566fcf9 ("PCI: Make cfg_access_lock lockdep key a singleton")
  f941b9182c54 ("PCI: Fix missing lockdep annotation for pci_cfg_access_trylock()")

I think the clearest way to do this would be to do a simple revert of
7e89efc6e9e4, followed by a second patch to add the pci_warn_once().

The revert would definitely be v6.10 material.  The pci_warn_once()
might be v6.11 material.  Or if you think it will find significant
bugs, maybe that's v6.10 material as well, but it'll be easier to make
that argument if it's in a separate patch.

Bjorn