diff mbox series

[v5,2/4] PCI: Add check for CXL Secondary Bus Reset

Message ID 20240429223610.1341811-3-dave.jiang@intel.com
State Superseded
Headers show
Series PCI: Add Secondary Bus Reset (SBR) support for CXL | expand

Commit Message

Dave Jiang April 29, 2024, 10:35 p.m. UTC
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@huawei.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v5:
- Add locking of upstream bridge.
---
 drivers/pci/pci.c             | 57 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/pci_regs.h |  5 +++
 2 files changed, 62 insertions(+)

Comments

Dan Williams May 1, 2024, 7:47 p.m. UTC | #1
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@huawei.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> 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
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e5f243dd4288..d33228088b0a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4927,10 +4927,55 @@  static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
 	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
 }
 
+static u16 cxl_port_dvsec(struct pci_dev *dev)
+{
+	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
+					 PCI_DVSEC_CXL_PORT);
+}
+
+static bool cxl_sbr_masked(struct pci_dev *dev)
+{
+	u16 dvsec, reg;
+	int rc;
+
+	/*
+	 * No DVSEC found, either is not a CXL port, or not connected in which
+	 * case mask state is a nop (CXL r3.1 sec 9.12.3 "Enumerating CXL RPs
+	 * and DSPs"
+	 */
+	dvsec = cxl_port_dvsec(dev);
+	if (!dvsec)
+		return false;
+
+	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_PORT_CTL, &reg);
+	if (rc || PCI_POSSIBLE_ERROR(reg))
+		return false;
+
+	/*
+	 * CXL spec r3.1 8.1.5.2
+	 * When 0, SBR bit in Bridge Control register of this Port has no effect.
+	 * When 1, the Port shall generate hot reset when SBR bit in Bridge
+	 * Control gets set to 1.
+	 */
+	if (reg & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR)
+		return false;
+
+	return true;
+}
+
 static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
 {
+	struct pci_dev *bridge = pci_upstream_bridge(dev);
 	int rc;
 
+	/* If it's a CXL port and the SBR control is masked, fail the SBR */
+	if (bridge && cxl_sbr_masked(bridge)) {
+		if (probe)
+			return 0;
+
+		return -ENOTTY;
+	}
+
 	rc = pci_dev_reset_slot_function(dev, probe);
 	if (rc != -ENOTTY)
 		return rc;
@@ -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);
+
 	pci_dev_lock(dev);
 	pci_dev_save_and_disable(dev);
 
@@ -5258,6 +5312,9 @@  int pci_reset_function(struct pci_dev *dev)
 	pci_dev_restore(dev);
 	pci_dev_unlock(dev);
 
+	if (bridge)
+		pci_dev_unlock(bridge);
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(pci_reset_function);
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index a39193213ff2..d61fa43662e3 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1148,4 +1148,9 @@ 
 #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL		0x00ff0000
 #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX	0xff000000
 
+/* Compute Express Link (CXL) */
+#define PCI_DVSEC_CXL_PORT				3
+#define PCI_DVSEC_CXL_PORT_CTL				0x0c
+#define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR		0x00000001
+
 #endif /* LINUX_PCI_REGS_H */