Skip to content

Commit 57bfaa7

Browse files
author
Myron Stowe
committed
PCI: Revert the cfg_access_lock lockdep mechanism
JIRA: https://issues.redhat.com/browse/RHEL-50255 Upstream Status: c9d52fb commit c9d52fb Author: Dan Williams <dan.j.williams@intel.com> Date: Thu May 30 18:04:24 2024 -0700 PCI: Revert the cfg_access_lock lockdep mechanism 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. Solve that narrower problem with follow-on patches, and just due to targeted revert for now. Link: https://lore.kernel.org/r/171711746402.1628941.14575335981264103013.stgit@dwillia2-xfh.jf.intel.com Fixes: 7e89efc ("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 Signed-off-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Tested-by: Hans de Goede <hdegoede@redhat.com> Tested-by: Kalle Valo <kvalo@kernel.org> Reviewed-by: Dave Jiang <dave.jiang@intel.com> Cc: Jani Saarinen <jani.saarinen@intel.com> Signed-off-by: Myron Stowe <mstowe@redhat.com>
1 parent 815e68c commit 57bfaa7

File tree

5 files changed

+0
-15
lines changed

5 files changed

+0
-15
lines changed

drivers/pci/access.c

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -289,8 +289,6 @@ void pci_cfg_access_lock(struct pci_dev *dev)
289289
{
290290
might_sleep();
291291

292-
lock_map_acquire(&dev->cfg_access_lock);
293-
294292
raw_spin_lock_irq(&pci_lock);
295293
if (dev->block_cfg_access)
296294
pci_wait_cfg(dev);
@@ -345,8 +343,6 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
345343
raw_spin_unlock_irqrestore(&pci_lock, flags);
346344

347345
wake_up_all(&pci_cfg_wait);
348-
349-
lock_map_release(&dev->cfg_access_lock);
350346
}
351347
EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
352348

drivers/pci/pci.c

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4883,7 +4883,6 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev)
48834883
*/
48844884
int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
48854885
{
4886-
lock_map_assert_held(&dev->cfg_access_lock);
48874886
pcibios_reset_secondary_bus(dev);
48884887

48894888
return pci_bridge_wait_for_secondary_bus(dev, "bus reset");

drivers/pci/probe.c

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2546,9 +2546,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
25462546
dev->dev.dma_mask = &dev->dma_mask;
25472547
dev->dev.dma_parms = &dev->dma_parms;
25482548
dev->dev.coherent_dma_mask = 0xffffffffull;
2549-
lockdep_register_key(&dev->cfg_access_key);
2550-
lockdep_init_map(&dev->cfg_access_lock, dev_name(&dev->dev),
2551-
&dev->cfg_access_key, 0);
25522549

25532550
dma_set_max_seg_size(&dev->dev, 65536);
25542551
dma_set_seg_boundary(&dev->dev, 0xffffffff);

include/linux/lockdep.h

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -354,9 +354,6 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
354354
.wait_type_inner = _wait_type, \
355355
.lock_type = LD_LOCK_WAIT_OVERRIDE, }
356356

357-
#define lock_map_assert_held(l) \
358-
lockdep_assert(lock_is_held(l) != LOCK_STATE_NOT_HELD)
359-
360357
#else /* !CONFIG_LOCKDEP */
361358

362359
static inline void lockdep_init_task(struct task_struct *task)
@@ -448,8 +445,6 @@ extern int lockdep_is_held(const void *);
448445
#define DEFINE_WAIT_OVERRIDE_MAP(_name, _wait_type) \
449446
struct lockdep_map __maybe_unused _name = {}
450447

451-
#define lock_map_assert_held(l) do { (void)(l); } while (0)
452-
453448
#endif /* !LOCKDEP */
454449

455450
#ifdef CONFIG_PROVE_LOCKING

include/linux/pci.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -418,8 +418,6 @@ struct pci_dev {
418418
struct resource driver_exclusive_resource; /* driver exclusive resource ranges */
419419

420420
bool match_driver; /* Skip attaching driver */
421-
struct lock_class_key cfg_access_key;
422-
struct lockdep_map cfg_access_lock;
423421

424422
unsigned int transparent:1; /* Subtractive decode bridge */
425423
unsigned int io_window:1; /* Bridge has I/O window */

0 commit comments

Comments
 (0)