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

[Dave Jiang <dave.jiang@intel.com>]

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