On 5/30/24 2:08 PM, Bjorn Helgaas wrote:
> 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@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?
>
> 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.
Unless Dan objects I think we should do a partial revert and only remove the lockdep bits.
>
>>> 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
