[PATCH v2 0/3] PCI: Revert / replace the cfg_access_lock lockdep mechanism

[PATCH v2 0/3] PCI: Revert / replace the cfg_access_lock lockdep mechanism

[Dan Williams]

Changes since v1 [1]:
- split out the new warning into its own patch (Bjorn)
- include the provisional fix to the pci_reset_bus() path

[1]: http://lore.kernel.org/r/171709637423.1568751.11773767969980847536.stgit@dwillia2-xfh.jf.intel.com

Hi Bjorn,

Here is the targeted revert of the cfg_access_lock lockdep
infrastructure, but with the new proposed warning for catching "unlocked
pci_bridge_secondary_bus_reset()" events broken out into its own change.
I also included the proposed fix for at least one known case where
pci_bridge_secondary_bus_reset() is being called without
cfg_access_lock.

Given there may be more cases to unwind, and the fact that I am not
convinced patch3 will be problem free I would, as you suggested,
consider patch2 and patch3 v6.11 material. Patch1 is urgent for v6.10-rc
to put out these lockdep false positive reports.

---

Dan Williams (3):
      PCI: Revert the cfg_access_lock lockdep mechanism
      PCI: Warn on missing cfg_access_lock during secondary bus reset
      PCI: Add missing bridge lock to pci_bus_lock()


 drivers/pci/access.c    |    4 ----
 drivers/pci/pci.c       |    8 +++++++-
 drivers/pci/probe.c     |    3 ---
 include/linux/lockdep.h |    5 -----
 include/linux/pci.h     |    2 --
 5 files changed, 7 insertions(+), 15 deletions(-)

base-commit: 56fb6f92854f29dcb6c3dc3ba92eeda1b615e88c

[PATCH v2 1/3] PCI: Revert the cfg_access_lock lockdep mechanism

[Dan Williams]

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.

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
Cc: Jani Saarinen <jani.saarinen@intel.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/pci/access.c    |    4 ----
 drivers/pci/pci.c       |    1 -
 drivers/pci/probe.c     |    3 ---
 include/linux/lockdep.h |    5 -----
 include/linux/pci.h     |    2 --
 5 files changed, 15 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 30f031de9cfe..b123da16b63b 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -289,8 +289,6 @@ 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);
@@ -345,8 +343,6 @@ 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 59e0949fb079..35fb1f17a589 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4883,7 +4883,6 @@ 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 8e696e547565..5fbabb4e3425 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2546,9 +2546,6 @@ 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 5e51b0de4c4b..08b0d1d9d78b 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -297,9 +297,6 @@ 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)
@@ -391,8 +388,6 @@ 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 --git a/include/linux/pci.h b/include/linux/pci.h
index fb004fd4e889..cafc5ab1cbcb 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -413,8 +413,6 @@ struct pci_dev {
 	struct resource driver_exclusive_resource;	 /* driver exclusive resource ranges */
 
 	bool		match_driver;		/* Skip attaching driver */
-	struct lock_class_key cfg_access_key;
-	struct lockdep_map cfg_access_lock;
 
 	unsigned int	transparent:1;		/* Subtractive decode bridge */
 	unsigned int	io_window:1;		/* Bridge has I/O window */

[PATCH v2 2/3] PCI: Warn on missing cfg_access_lock during secondary bus reset

[Dan Williams]

The recent adventure with adding lockdep tracking for cfg_access_lock,
while it yielded many false positives [1], it did catch a true positive in
the pci_reset_bus() path [2].

So, while lockdep is difficult to deploy, open coding a check that
cfg_access_lock is held during the reset is feasible.

While this does not offer a full backtrace, it should be sufficient to
implicate the caller of pci_bridge_secondary_bus_reset() as a path that
needs investigation.

Link: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_134186v1/shard-dg2-1/igt@device_reset@unbind-reset-rebind.html [1]
Link: http://lore.kernel.org/r/cfb50601-5d2a-4676-a958-1bd3f1b06654@intel.com [2]
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/pci/pci.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 35fb1f17a589..8df32a3a0979 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4883,6 +4883,9 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev)
  */
 int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
 {
+	if (!dev->block_cfg_access)
+		pci_warn_once(dev, "unlocked secondary bus reset via: %pS\n",
+			      __builtin_return_address(0));
 	pcibios_reset_secondary_bus(dev);
 
 	return pci_bridge_wait_for_secondary_bus(dev, "bus reset");

[PATCH v2 3/3] PCI: Add missing bridge lock to pci_bus_lock()

[Dan Williams]

One of the true positives that the cfg_access_lock lockdep effort
identified is this sequence:

 WARNING: CPU: 14 PID: 1 at drivers/pci/pci.c:4886 pci_bridge_secondary_bus_reset+0x5d/0x70
 RIP: 0010:pci_bridge_secondary_bus_reset+0x5d/0x70
 Call Trace:
  <TASK>
  ? __warn+0x8c/0x190
  ? pci_bridge_secondary_bus_reset+0x5d/0x70
  ? report_bug+0x1f8/0x200
  ? handle_bug+0x3c/0x70
  ? exc_invalid_op+0x18/0x70
  ? asm_exc_invalid_op+0x1a/0x20
  ? pci_bridge_secondary_bus_reset+0x5d/0x70
  pci_reset_bus+0x1d8/0x270
  vmd_probe+0x778/0xa10
  pci_device_probe+0x95/0x120

Where pci_reset_bus() users are triggering unlocked secondary bus
resets. Ironically pci_bus_reset(), several calls down from
pci_reset_bus(), uses pci_bus_lock() before issuing the reset which
locks everything *but* the bridge itself.

For the same motivation as adding:

    bridge = pci_upstream_bridge(dev);
    if (bridge)
            pci_dev_lock(bridge);

...to pci_reset_function() for the "bus" and "cxl_bus" reset cases, add
pci_dev_lock() for @bus->self to pci_bus_lock().

Reported-by: Imre Deak <imre.deak@intel.com>
Closes: http://lore.kernel.org/r/6657833b3b5ae_14984b29437@dwillia2-xfh.jf.intel.com.notmuch
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/pci/pci.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8df32a3a0979..aac5daad3188 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5444,6 +5444,7 @@ static void pci_bus_lock(struct pci_bus *bus)
 {
 	struct pci_dev *dev;
 
+	pci_dev_lock(bus->self);
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		pci_dev_lock(dev);
 		if (dev->subordinate)
@@ -5461,6 +5462,7 @@ static void pci_bus_unlock(struct pci_bus *bus)
 			pci_bus_unlock(dev->subordinate);
 		pci_dev_unlock(dev);
 	}
+	pci_dev_unlock(bus->self);
 }
 
 /* Return 1 on successful lock, 0 on contention */
@@ -5468,6 +5470,7 @@ static int pci_bus_trylock(struct pci_bus *bus)
 {
 	struct pci_dev *dev;
 
+	pci_dev_lock(bus->self);
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		if (!pci_dev_trylock(dev))
 			goto unlock;
@@ -5486,6 +5489,7 @@ static int pci_bus_trylock(struct pci_bus *bus)
 			pci_bus_unlock(dev->subordinate);
 		pci_dev_unlock(dev);
 	}
+	pci_dev_unlock(bus->self);
 	return 0;
 }
 

Re: [PATCH v2 1/3] PCI: Revert the cfg_access_lock lockdep mechanism

[Dave Jiang]

On 5/30/24 6:04 PM, 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. Solve that narrower
> problem with follow-on patches, and just due to targeted revert for now.
> 
> 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
> Cc: Jani Saarinen <jani.saarinen@intel.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/pci/access.c    |    4 ----
>  drivers/pci/pci.c       |    1 -
>  drivers/pci/probe.c     |    3 ---
>  include/linux/lockdep.h |    5 -----
>  include/linux/pci.h     |    2 --
>  5 files changed, 15 deletions(-)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 30f031de9cfe..b123da16b63b 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -289,8 +289,6 @@ 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);
> @@ -345,8 +343,6 @@ 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 59e0949fb079..35fb1f17a589 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4883,7 +4883,6 @@ 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 8e696e547565..5fbabb4e3425 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2546,9 +2546,6 @@ 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 5e51b0de4c4b..08b0d1d9d78b 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -297,9 +297,6 @@ 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)
> @@ -391,8 +388,6 @@ 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 --git a/include/linux/pci.h b/include/linux/pci.h
> index fb004fd4e889..cafc5ab1cbcb 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -413,8 +413,6 @@ struct pci_dev {
>  	struct resource driver_exclusive_resource;	 /* driver exclusive resource ranges */
>  
>  	bool		match_driver;		/* Skip attaching driver */
> -	struct lock_class_key cfg_access_key;
> -	struct lockdep_map cfg_access_lock;
>  
>  	unsigned int	transparent:1;		/* Subtractive decode bridge */
>  	unsigned int	io_window:1;		/* Bridge has I/O window */
> 

Re: [PATCH v2 2/3] PCI: Warn on missing cfg_access_lock during secondary bus reset

[Dave Jiang]

On 5/30/24 6:04 PM, Dan Williams wrote:
> The recent adventure with adding lockdep tracking for cfg_access_lock,
> while it yielded many false positives [1], it did catch a true positive in
> the pci_reset_bus() path [2].
> 
> So, while lockdep is difficult to deploy, open coding a check that
> cfg_access_lock is held during the reset is feasible.
> 
> While this does not offer a full backtrace, it should be sufficient to
> implicate the caller of pci_bridge_secondary_bus_reset() as a path that
> needs investigation.
> 
> Link: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_134186v1/shard-dg2-1/igt@device_reset@unbind-reset-rebind.html [1]
> Link: http://lore.kernel.org/r/cfb50601-5d2a-4676-a958-1bd3f1b06654@intel.com [2]
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/pci/pci.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 35fb1f17a589..8df32a3a0979 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4883,6 +4883,9 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev)
>   */
>  int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
>  {
> +	if (!dev->block_cfg_access)
> +		pci_warn_once(dev, "unlocked secondary bus reset via: %pS\n",
> +			      __builtin_return_address(0));
>  	pcibios_reset_secondary_bus(dev);
>  
>  	return pci_bridge_wait_for_secondary_bus(dev, "bus reset");
> 

Re: [PATCH v2 3/3] PCI: Add missing bridge lock to pci_bus_lock()

[Dave Jiang]

On 5/30/24 6:04 PM, Dan Williams wrote:
> One of the true positives that the cfg_access_lock lockdep effort
> identified is this sequence:
> 
>  WARNING: CPU: 14 PID: 1 at drivers/pci/pci.c:4886 pci_bridge_secondary_bus_reset+0x5d/0x70
>  RIP: 0010:pci_bridge_secondary_bus_reset+0x5d/0x70
>  Call Trace:
>   <TASK>
>   ? __warn+0x8c/0x190
>   ? pci_bridge_secondary_bus_reset+0x5d/0x70
>   ? report_bug+0x1f8/0x200
>   ? handle_bug+0x3c/0x70
>   ? exc_invalid_op+0x18/0x70
>   ? asm_exc_invalid_op+0x1a/0x20
>   ? pci_bridge_secondary_bus_reset+0x5d/0x70
>   pci_reset_bus+0x1d8/0x270
>   vmd_probe+0x778/0xa10
>   pci_device_probe+0x95/0x120
> 
> Where pci_reset_bus() users are triggering unlocked secondary bus
> resets. Ironically pci_bus_reset(), several calls down from
> pci_reset_bus(), uses pci_bus_lock() before issuing the reset which
> locks everything *but* the bridge itself.
> 
> For the same motivation as adding:
> 
>     bridge = pci_upstream_bridge(dev);
>     if (bridge)
>             pci_dev_lock(bridge);
> 
> ...to pci_reset_function() for the "bus" and "cxl_bus" reset cases, add
> pci_dev_lock() for @bus->self to pci_bus_lock().
> 
> Reported-by: Imre Deak <imre.deak@intel.com>
> Closes: http://lore.kernel.org/r/6657833b3b5ae_14984b29437@dwillia2-xfh.jf.intel.com.notmuch
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/pci/pci.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8df32a3a0979..aac5daad3188 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5444,6 +5444,7 @@ static void pci_bus_lock(struct pci_bus *bus)
>  {
>  	struct pci_dev *dev;
>  
> +	pci_dev_lock(bus->self);
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>  		pci_dev_lock(dev);
>  		if (dev->subordinate)
> @@ -5461,6 +5462,7 @@ static void pci_bus_unlock(struct pci_bus *bus)
>  			pci_bus_unlock(dev->subordinate);
>  		pci_dev_unlock(dev);
>  	}
> +	pci_dev_unlock(bus->self);
>  }
>  
>  /* Return 1 on successful lock, 0 on contention */
> @@ -5468,6 +5470,7 @@ static int pci_bus_trylock(struct pci_bus *bus)
>  {
>  	struct pci_dev *dev;
>  
> +	pci_dev_lock(bus->self);
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>  		if (!pci_dev_trylock(dev))
>  			goto unlock;
> @@ -5486,6 +5489,7 @@ static int pci_bus_trylock(struct pci_bus *bus)
>  			pci_bus_unlock(dev->subordinate);
>  		pci_dev_unlock(dev);
>  	}
> +	pci_dev_unlock(bus->self);
>  	return 0;
>  }
>  
> 

Re: [PATCH v2 0/3] PCI: Revert / replace the cfg_access_lock lockdep mechanism

[Bjorn Helgaas]

On Thu, May 30, 2024 at 06:04:18PM -0700, Dan Williams wrote:
> Changes since v1 [1]:
> - split out the new warning into its own patch (Bjorn)
> - include the provisional fix to the pci_reset_bus() path
> 
> [1]: http://lore.kernel.org/r/171709637423.1568751.11773767969980847536.stgit@dwillia2-xfh.jf.intel.com
> 
> Hi Bjorn,
> 
> Here is the targeted revert of the cfg_access_lock lockdep
> infrastructure, but with the new proposed warning for catching "unlocked
> pci_bridge_secondary_bus_reset()" events broken out into its own change.
> I also included the proposed fix for at least one known case where
> pci_bridge_secondary_bus_reset() is being called without
> cfg_access_lock.
> 
> Given there may be more cases to unwind, and the fact that I am not
> convinced patch3 will be problem free I would, as you suggested,
> consider patch2 and patch3 v6.11 material. Patch1 is urgent for v6.10-rc
> to put out these lockdep false positive reports.
> 
> ---
> 
> Dan Williams (3):
>       PCI: Revert the cfg_access_lock lockdep mechanism
>       PCI: Warn on missing cfg_access_lock during secondary bus reset
>       PCI: Add missing bridge lock to pci_bus_lock()
> 
> 
>  drivers/pci/access.c    |    4 ----
>  drivers/pci/pci.c       |    8 +++++++-
>  drivers/pci/probe.c     |    3 ---
>  include/linux/lockdep.h |    5 -----
>  include/linux/pci.h     |    2 --
>  5 files changed, 7 insertions(+), 15 deletions(-)

Thanks, I applied the first to pci/for-linus for v6.10, and the others
to pci/reset for v6.11.

There will be a trivial conflict in -next and the v6.11 merge window
since pci/reset is based on v6.10-rc1 and contains some of the code
removed by the first patch.

Bjorn

Re: [PATCH v2 0/3] PCI: Revert / replace the cfg_access_lock lockdep mechanism

[Hans de Goede]

Hi,

On 5/31/24 3:04 AM, Dan Williams wrote:
> Changes since v1 [1]:
> - split out the new warning into its own patch (Bjorn)
> - include the provisional fix to the pci_reset_bus() path
> 
> [1]: http://lore.kernel.org/r/171709637423.1568751.11773767969980847536.stgit@dwillia2-xfh.jf.intel.com
> 
> Hi Bjorn,
> 
> Here is the targeted revert of the cfg_access_lock lockdep
> infrastructure, but with the new proposed warning for catching "unlocked
> pci_bridge_secondary_bus_reset()" events broken out into its own change.
> I also included the proposed fix for at least one known case where
> pci_bridge_secondary_bus_reset() is being called without
> cfg_access_lock.
> 
> Given there may be more cases to unwind, and the fact that I am not
> convinced patch3 will be problem free I would, as you suggested,
> consider patch2 and patch3 v6.11 material. Patch1 is urgent for v6.10-rc
> to put out these lockdep false positive reports.

I can confirm that this series fixes the lockdep errors which
I was seeing:

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans





> ---
> 
> Dan Williams (3):
>       PCI: Revert the cfg_access_lock lockdep mechanism
>       PCI: Warn on missing cfg_access_lock during secondary bus reset
>       PCI: Add missing bridge lock to pci_bus_lock()
> 
> 
>  drivers/pci/access.c    |    4 ----
>  drivers/pci/pci.c       |    8 +++++++-
>  drivers/pci/probe.c     |    3 ---
>  include/linux/lockdep.h |    5 -----
>  include/linux/pci.h     |    2 --
>  5 files changed, 7 insertions(+), 15 deletions(-)
> 
> base-commit: 56fb6f92854f29dcb6c3dc3ba92eeda1b615e88c
> 

Re: [PATCH v2 0/3] PCI: Revert / replace the cfg_access_lock lockdep mechanism

[Bjorn Helgaas]

On Mon, Jun 03, 2024 at 09:49:39PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 5/31/24 3:04 AM, Dan Williams wrote:
> > Changes since v1 [1]:
> > - split out the new warning into its own patch (Bjorn)
> > - include the provisional fix to the pci_reset_bus() path
> > 
> > [1]: http://lore.kernel.org/r/171709637423.1568751.11773767969980847536.stgit@dwillia2-xfh.jf.intel.com
> > 
> > Hi Bjorn,
> > 
> > Here is the targeted revert of the cfg_access_lock lockdep
> > infrastructure, but with the new proposed warning for catching "unlocked
> > pci_bridge_secondary_bus_reset()" events broken out into its own change.
> > I also included the proposed fix for at least one known case where
> > pci_bridge_secondary_bus_reset() is being called without
> > cfg_access_lock.
> > 
> > Given there may be more cases to unwind, and the fact that I am not
> > convinced patch3 will be problem free I would, as you suggested,
> > consider patch2 and patch3 v6.11 material. Patch1 is urgent for v6.10-rc
> > to put out these lockdep false positive reports.
> 
> I can confirm that this series fixes the lockdep errors which
> I was seeing:
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com>

Added to the three patches in this series, thanks, Hans!

> > ---
> > 
> > Dan Williams (3):
> >       PCI: Revert the cfg_access_lock lockdep mechanism
> >       PCI: Warn on missing cfg_access_lock during secondary bus reset
> >       PCI: Add missing bridge lock to pci_bus_lock()
> > 
> > 
> >  drivers/pci/access.c    |    4 ----
> >  drivers/pci/pci.c       |    8 +++++++-
> >  drivers/pci/probe.c     |    3 ---
> >  include/linux/lockdep.h |    5 -----
> >  include/linux/pci.h     |    2 --
> >  5 files changed, 7 insertions(+), 15 deletions(-)
> > 
> > base-commit: 56fb6f92854f29dcb6c3dc3ba92eeda1b615e88c
> > 
> 

Re: [PATCH v2 1/3] PCI: Revert the cfg_access_lock lockdep mechanism

[Kalle Valo]

Dan Williams <dan.j.williams@intel.com> writes:

> 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.
>
> 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
> Cc: Jani Saarinen <jani.saarinen@intel.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

In our ath11k test box commit 7e89efc6e9e4 was causing random kernel
crashes. I tested patches 1-3 and did not see anymore crashes so:

Tested-by: Kalle Valo <kvalo@kernel.org>

Unfortunately I didn't realise to test patch 1 alone but I would assume
that's enough to fix the crashes. Please prioritise this patch so that
the regression in Linus' tree is fixed.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 1/3] PCI: Revert the cfg_access_lock lockdep mechanism

[Bjorn Helgaas]

On Tue, Jun 04, 2024 at 11:03:54AM +0300, Kalle Valo wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
> 
> > 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.
> >
> > 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
> > Cc: Jani Saarinen <jani.saarinen@intel.com>
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> In our ath11k test box commit 7e89efc6e9e4 was causing random kernel
> crashes. I tested patches 1-3 and did not see anymore crashes so:
> 
> Tested-by: Kalle Valo <kvalo@kernel.org>

Added to commit logs, thank you!

Re: [PATCH v2 1/3] PCI: Revert the cfg_access_lock lockdep mechanism

[Leon Romanovsky]

On Tue, Jun 04, 2024 at 12:11:21PM -0500, Bjorn Helgaas wrote:
> On Tue, Jun 04, 2024 at 11:03:54AM +0300, Kalle Valo wrote:
> > Dan Williams <dan.j.williams@intel.com> writes:
> > 
> > > 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.
> > >
> > > 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
> > > Cc: Jani Saarinen <jani.saarinen@intel.com>
> > > Cc: Dave Jiang <dave.jiang@intel.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > 
> > In our ath11k test box commit 7e89efc6e9e4 was causing random kernel
> > crashes. I tested patches 1-3 and did not see anymore crashes so:
> > 
> > Tested-by: Kalle Valo <kvalo@kernel.org>
> 
> Added to commit logs, thank you!
> 

Thanks,
Tested-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH v2 3/3] PCI: Add missing bridge lock to pci_bus_lock()

[Keith Busch]

On Thu, May 30, 2024 at 06:04:35PM -0700, Dan Williams wrote:
> @@ -5444,6 +5444,7 @@ static void pci_bus_lock(struct pci_bus *bus)
>  {
>  	struct pci_dev *dev;
>  
> +	pci_dev_lock(bus->self);
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>  		pci_dev_lock(dev);
>  		if (dev->subordinate)

The very next line is:

	pci_bus_lock(dev->subordinate);

The code had just locked "dev". The recursive call will then lock
dev->subordinate->self, which is "dev", the same that was just locked a
moment ago: deadlocked.

Here's an idea:

---
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 46beaf1815fab..4f7cf693857f4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5487,8 +5487,9 @@ static void pci_bus_lock(struct pci_bus *bus)
 
 	pci_dev_lock(bus->self);
 	list_for_each_entry(dev, &bus->devices, bus_list) {
-		pci_dev_lock(dev);
-		if (dev->subordinate)
+		if (!dev->subordinate)
+			pci_dev_lock(dev);
+		else
 			pci_bus_lock(dev->subordinate);
 	}
 }
@@ -5501,7 +5502,8 @@ static void pci_bus_unlock(struct pci_bus *bus)
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		if (dev->subordinate)
 			pci_bus_unlock(dev->subordinate);
-		pci_dev_unlock(dev);
+		else
+			pci_dev_unlock(dev);
 	}
 	pci_dev_unlock(bus->self);
 }
--