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

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, May 30, 2024 at 02:03:09PM -0700, Dave Jiang wrote:
> On 5/30/24 1:52 PM, Bjorn Helgaas wrote:
> > 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().
> 
> Complete revert of 7e89efc6e9e4 will also remove the bridge locking
> which I think we want to keep right?

I dunno, you tell me.  If we want to revert just part of 7e89efc6e9e4,
it would be clearer to do that by itself, then add the new stuff
separately.

> > 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