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@xxxxxxxxx>
>>> Closes: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_134186v1/shard-dg2-1/igt@device_reset@xxxxxxxxxxxxxxxxxxxxxxxx
>>> Cc: Jani Saarinen <jani.saarinen@xxxxxxxxx>
>>> Cc: Dave Jiang <dave.jiang@xxxxxxxxx>
>>> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxxxxxxxxx.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
