Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

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

authored by

Dan Williams and committed by
Bjorn Helgaas
c9d52fb3 1613e604

-15
-4
drivers/pci/access.c
··· 289 289 { 290 290 might_sleep(); 291 291 292 - lock_map_acquire(&dev->cfg_access_lock); 293 - 294 292 raw_spin_lock_irq(&pci_lock); 295 293 if (dev->block_cfg_access) 296 294 pci_wait_cfg(dev); ··· 343 345 raw_spin_unlock_irqrestore(&pci_lock, flags); 344 346 345 347 wake_up_all(&pci_cfg_wait); 346 - 347 - lock_map_release(&dev->cfg_access_lock); 348 348 } 349 349 EXPORT_SYMBOL_GPL(pci_cfg_access_unlock); 350 350
-1
drivers/pci/pci.c
··· 4883 4883 */ 4884 4884 int pci_bridge_secondary_bus_reset(struct pci_dev *dev) 4885 4885 { 4886 - lock_map_assert_held(&dev->cfg_access_lock); 4887 4886 pcibios_reset_secondary_bus(dev); 4888 4887 4889 4888 return pci_bridge_wait_for_secondary_bus(dev, "bus reset");
-3
drivers/pci/probe.c
··· 2546 2546 dev->dev.dma_mask = &dev->dma_mask; 2547 2547 dev->dev.dma_parms = &dev->dma_parms; 2548 2548 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); 2552 2549 2553 2550 dma_set_max_seg_size(&dev->dev, 65536); 2554 2551 dma_set_seg_boundary(&dev->dev, 0xffffffff);
-5
include/linux/lockdep.h
··· 297 297 .wait_type_inner = _wait_type, \ 298 298 .lock_type = LD_LOCK_WAIT_OVERRIDE, } 299 299 300 - #define lock_map_assert_held(l) \ 301 - lockdep_assert(lock_is_held(l) != LOCK_STATE_NOT_HELD) 302 - 303 300 #else /* !CONFIG_LOCKDEP */ 304 301 305 302 static inline void lockdep_init_task(struct task_struct *task) ··· 387 390 388 391 #define DEFINE_WAIT_OVERRIDE_MAP(_name, _wait_type) \ 389 392 struct lockdep_map __maybe_unused _name = {} 390 - 391 - #define lock_map_assert_held(l) do { (void)(l); } while (0) 392 393 393 394 #endif /* !LOCKDEP */ 394 395
-2
include/linux/pci.h
··· 413 413 struct resource driver_exclusive_resource; /* driver exclusive resource ranges */ 414 414 415 415 bool match_driver; /* Skip attaching driver */ 416 - struct lock_class_key cfg_access_key; 417 - struct lockdep_map cfg_access_lock; 418 416 419 417 unsigned int transparent:1; /* Subtractive decode bridge */ 420 418 unsigned int io_window:1; /* Bridge has I/O window */