Dave Jiang wrote: > Per CXL spec r3.1 8.1.5.2, Secondary Bus Reset (SBR) is masked unless the > "Unmask SBR" bit is set. Add a check to the PCI secondary bus reset > path to fail the CXL SBR request if the "Unmask SBR" bit is clear in > the CXL Port Control Extensions register by returning -ENOTTY. > > Otherwise when the "Unmask SBR" bit is set to 0 (default), the bus_reset > would appear to have executed successfully. However the operation is > actually masked. The intention is to inform the user that SBR for the CXL > device is masked and will not go through. > > If the "Unmask SBR" bit is set to 1, then the bus reset will execute > successfully. > > Add the locking of the upstream bridge in pci_reset_function() to ensure > the locking order of locking the bridge then locking the device. The > bridge configuration needs to be consistent for a CXL device. This should > not impact PCI devices. > > Link: https://lore.kernel.org/linux-cxl/20240220203956.GA1502351@bhelgaas/ > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> > Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx> > Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx> > --- > v5: > - Add locking of upstream bridge. [..] > @@ -5245,11 +5290,20 @@ void pci_init_reset_methods(struct pci_dev *dev) > */ > int pci_reset_function(struct pci_dev *dev) > { > + struct pci_dev *bridge; > int rc; > > if (!pci_reset_supported(dev)) > return -ENOTTY; > > + bridge = pci_upstream_bridge(dev); > + /* > + * If there's no upstream bridge, then no locking is needed since there is no > + * upstream bridge configuration to hold consistent. > + */ > + if (bridge) > + pci_dev_lock(bridge); > + This change deserves to be broken out and merged separately because it is fixing a long standing locking gap for missing pci_cfg_access_lock() while manipulating bridge reset registers and configuration during pci_reset_bus_function(). Yes, the CXL reset type adds SBR mask management, but that does not change the fact that PCIe SBR is inconsistently locked. This builds for me and highlights the expectation: diff --git a/drivers/pci/access.c b/drivers/pci/access.c index 6449056b57dd..36f10c7f9ef5 100644 --- a/drivers/pci/access.c +++ b/drivers/pci/access.c @@ -275,6 +275,8 @@ void pci_cfg_access_lock(struct pci_dev *dev) { might_sleep(); + lock_map_acquire(&dev->cfg_access_lock); + raw_spin_lock_irq(&pci_lock); if (dev->block_cfg_access) pci_wait_cfg(dev); @@ -329,6 +331,8 @@ void pci_cfg_access_unlock(struct pci_dev *dev) raw_spin_unlock_irqrestore(&pci_lock, flags); wake_up_all(&pci_cfg_wait); + + lock_map_release(&dev->cfg_access_lock); } EXPORT_SYMBOL_GPL(pci_cfg_access_unlock); diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index e5f243dd4288..63086d97eb90 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4879,6 +4879,7 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev) */ int pci_bridge_secondary_bus_reset(struct pci_dev *dev) { + lock_map_assert_held(&dev->cfg_access_lock); pcibios_reset_secondary_bus(dev); return pci_bridge_wait_for_secondary_bus(dev, "bus reset"); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index d89b67541d02..2f178cd6e723 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2544,6 +2544,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) dev->dev.dma_mask = &dev->dma_mask; dev->dev.dma_parms = &dev->dma_parms; dev->dev.coherent_dma_mask = 0xffffffffull; + lockdep_register_key(&dev->cfg_access_key); + lockdep_init_map(&dev->cfg_access_lock, dev_name(&dev->dev), + &dev->cfg_access_key, 0); dma_set_max_seg_size(&dev->dev, 65536); dma_set_seg_boundary(&dev->dev, 0xffffffff); diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index 08b0d1d9d78b..5e51b0de4c4b 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -297,6 +297,9 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie); .wait_type_inner = _wait_type, \ .lock_type = LD_LOCK_WAIT_OVERRIDE, } +#define lock_map_assert_held(l) \ + lockdep_assert(lock_is_held(l) != LOCK_STATE_NOT_HELD) + #else /* !CONFIG_LOCKDEP */ static inline void lockdep_init_task(struct task_struct *task) @@ -388,6 +391,8 @@ extern int lockdep_is_held(const void *); #define DEFINE_WAIT_OVERRIDE_MAP(_name, _wait_type) \ struct lockdep_map __maybe_unused _name = {} +#define lock_map_assert_held(l) do { (void)(l); } while (0) + #endif /* !LOCKDEP */ #ifdef CONFIG_PROVE_LOCKING
