[PATCHv2 0/4] pci: slot handling fixes

[PATCHv2 0/4] pci: slot handling fixes

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This series fixes some older issues are the slot lock usage, with two
follow on patches to address multi-function resets in pcie hotplug
slots.

Previous discussions:

  https://lore.kernel.org/linux-pci/20260116184150.3013258-1-kbusch@meta.com/

  https://lore.kernel.org/linux-pci/aXvOOjZQ_EmZDgVs@kbusch-mbp/T/#t

Version change notes:

 * The first patch is unchanged from the original.

 * The second patch adds the local "bridge" to improve code clarity
   (review comments from Dan)

 * The third patch removes pci_slot_resettable in addition to all the
   other slot specific locking and state saving functions. I also
   updated a code comment explaining that all bus devices are saved and
   restored with the slot reset.

 * Included a fourth patch that fixes user suborindate bus resets when the
   bridge is pcie hotplug capable; a previous version came from this
   independent patch here:

  https://lore.kernel.org/linux-pci/20260116154244.1452246-1-kbusch@meta.com/

   And I'm including it in this series since it is all in the same area
   with slot resets. I've made a minor change to the previous version to
   remove the code duplication with pci_bus_error_reset().

Jinhui Guo (1):
  PCI: Fix incorrect unlocking in pci_slot_trylock()

Keith Busch (3):
  pci: fix slot reset device locking
  pci: remove slot specific lock/unlock and save/restore
  pci: make reset_subordinate hotplug safe

 drivers/pci/pci-sysfs.c |   3 +-
 drivers/pci/pci.c       | 213 +++++++++++-----------------------------
 drivers/pci/pci.h       |   2 +-
 3 files changed, 61 insertions(+), 157 deletions(-)

-- 
2.47.3

[PATCHv2 1/4] PCI: Fix incorrect unlocking in pci_slot_trylock()

[Keith Busch]

From: Jinhui Guo <guojinhui.liam@bytedance.com>

Commit a4e772898f8b ("PCI: Add missing bridge lock to pci_bus_lock()")
delegates the bridge device's pci_dev_trylock() to pci_bus_trylock() in
pci_slot_trylock(), but it forgets to remove the corresponding
pci_dev_unlock() when pci_bus_trylock() fails.

Before the commit, the code did:

  if (!pci_dev_trylock(dev)) /* <- lock bridge device */
    goto unlock;
  if (dev->subordinate) {
    if (!pci_bus_trylock(dev->subordinate)) {
      pci_dev_unlock(dev);   /* <- unlock bridge device */
      goto unlock;
    }
  }

After the commit the bridge-device lock is no longer taken, but the
pci_dev_unlock(dev) on the failure path was left in place, leading to
the bug.

This yields one of two errors:
1. A warning that the lock is being unlocked when no one holds it.
2. An incorrect unlock of a lock that belongs to another thread.

Fix it by removing the now-redundant pci_dev_unlock(dev) on the failure
path.

Fixes: a4e772898f8b ("PCI: Add missing bridge lock to pci_bus_lock()")
Cc: stable@vger.kernel.org
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Jinhui Guo <guojinhui.liam@bytedance.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/pci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 13dbb405dc31f..59319e08fca61 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5346,10 +5346,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
 		if (!dev->slot || dev->slot != slot)
 			continue;
 		if (dev->subordinate) {
-			if (!pci_bus_trylock(dev->subordinate)) {
-				pci_dev_unlock(dev);
+			if (!pci_bus_trylock(dev->subordinate))
 				goto unlock;
-			}
 		} else if (!pci_dev_trylock(dev))
 			goto unlock;
 	}
-- 
2.47.3

[PATCHv2 2/4] pci: fix slot reset device locking

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Like pci_bus_lock, pci_slot_lock needs to lock the bridge device to
prevent warnings like:

  pcieport 0000:e2:05.0: unlocked secondary bus reset via: pciehp_reset_slot+0x55/0xa0

Take and release the lock for the bridge providing the slot for the
lock/trylock and unlock routines.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/pci.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 59319e08fca61..57a5b205175f1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5290,10 +5290,9 @@ static int pci_bus_trylock(struct pci_bus *bus)
 /* Do any devices on or below this slot prevent a bus reset? */
 static bool pci_slot_resettable(struct pci_slot *slot)
 {
-	struct pci_dev *dev;
+	struct pci_dev *dev, *bridge = slot->bus->self;
 
-	if (slot->bus->self &&
-	    (slot->bus->self->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET))
+	if (bridge && (bridge->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET))
 		return false;
 
 	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
@@ -5310,7 +5309,10 @@ static bool pci_slot_resettable(struct pci_slot *slot)
 /* Lock devices from the top of the tree down */
 static void pci_slot_lock(struct pci_slot *slot)
 {
-	struct pci_dev *dev;
+	struct pci_dev *dev, *bridge = slot->bus->self;
+
+	if (bridge)
+		pci_dev_lock(bridge);
 
 	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
 		if (!dev->slot || dev->slot != slot)
@@ -5325,7 +5327,7 @@ static void pci_slot_lock(struct pci_slot *slot)
 /* Unlock devices from the bottom of the tree up */
 static void pci_slot_unlock(struct pci_slot *slot)
 {
-	struct pci_dev *dev;
+	struct pci_dev *dev, *bridge = slot->bus->self;
 
 	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
 		if (!dev->slot || dev->slot != slot)
@@ -5335,12 +5337,18 @@ static void pci_slot_unlock(struct pci_slot *slot)
 		else
 			pci_dev_unlock(dev);
 	}
+
+	if (bridge)
+		pci_dev_unlock(bridge);
 }
 
 /* Return 1 on successful lock, 0 on contention */
 static int pci_slot_trylock(struct pci_slot *slot)
 {
-	struct pci_dev *dev;
+	struct pci_dev *dev, *bridge = slot->bus->self;
+
+	if (bridge && !pci_dev_trylock(bridge))
+		return 0;
 
 	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
 		if (!dev->slot || dev->slot != slot)
@@ -5363,6 +5371,9 @@ static int pci_slot_trylock(struct pci_slot *slot)
 		else
 			pci_dev_unlock(dev);
 	}
+
+	if (bridge)
+		pci_dev_unlock(bridge);
 	return 0;
 }
 
-- 
2.47.3

[PATCHv2 3/4] pci: remove slot specific lock/unlock and save/restore

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The Linux pci driver resolves a "slot" to the "D" in the B:D.f (see
PCI_SLOT()). A pcie "slot reset" is a secondary bus reset, which affects
every function on every "D", not just the ones with a matching "slot".
The slot lock/unlock and save/restore functions, however, are only
handling a subset of the functions, breaking the rest.

ARI devices with more than 8 functions fail because their state is not
properly handled, nor is the attached driver notified of the reset. In
the best case, the device will appear unresponsive to the driver,
resulting in unexpected errors. A worse possibility may panic the kernel
if in flight transactions trigger hardware reported errors like this
real observation:

  vfio-pci 0000:01:00.0: resetting
  vfio-pci 0000:01:00.0: reset done
  {1}[Hardware Error]:  Error 1, type: fatal
  {1}[Hardware Error]:   section_type: PCIe error
  {1}[Hardware Error]:   port_type: 0, PCIe end point
  {1}[Hardware Error]:   version: 0.2
  {1}[Hardware Error]:   command: 0x0140, status: 0x0010
  {1}[Hardware Error]:   device_id: 0000:01:01.0
  {1}[Hardware Error]:   slot: 0
  {1}[Hardware Error]:   secondary_bus: 0x00
  {1}[Hardware Error]:   vendor_id: 0x1d9b, device_id: 0x0207
  {1}[Hardware Error]:   class_code: 020000
  {1}[Hardware Error]:   bridge: secondary_status: 0x0000, control: 0x0000
  {1}[Hardware Error]:   aer_cor_status: 0x00008000, aer_cor_mask: 0x00002000
  {1}[Hardware Error]:   aer_uncor_status: 0x00010000, aer_uncor_mask: 0x00100000
  {1}[Hardware Error]:   aer_uncor_severity: 0x006f6030
  {1}[Hardware Error]:   TLP Header: 0a412800 00192080 60000004 00000004
  GHES: Fatal hardware error but panic disabled
  Kernel panic - not syncing: GHES: Fatal hardware error

Fix this by properly locking and notifying the entire affected bus
topology, not just specific matching slots. For architectures that
support "slot" specific resets, this patch potentially introduces an
insignificant amount of overhead, but is otherwise harmless.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/pci.c | 152 ++++------------------------------------------
 1 file changed, 13 insertions(+), 139 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 57a5b205175f1..36427fbf7a747 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5218,7 +5218,6 @@ static bool pci_bus_resettable(struct pci_bus *bus)
 {
 	struct pci_dev *dev;
 
-
 	if (bus->self && (bus->self->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET))
 		return false;
 
@@ -5287,96 +5286,6 @@ static int pci_bus_trylock(struct pci_bus *bus)
 	return 0;
 }
 
-/* Do any devices on or below this slot prevent a bus reset? */
-static bool pci_slot_resettable(struct pci_slot *slot)
-{
-	struct pci_dev *dev, *bridge = slot->bus->self;
-
-	if (bridge && (bridge->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET))
-		return false;
-
-	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
-		if (!dev->slot || dev->slot != slot)
-			continue;
-		if (dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET ||
-		    (dev->subordinate && !pci_bus_resettable(dev->subordinate)))
-			return false;
-	}
-
-	return true;
-}
-
-/* Lock devices from the top of the tree down */
-static void pci_slot_lock(struct pci_slot *slot)
-{
-	struct pci_dev *dev, *bridge = slot->bus->self;
-
-	if (bridge)
-		pci_dev_lock(bridge);
-
-	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
-		if (!dev->slot || dev->slot != slot)
-			continue;
-		if (dev->subordinate)
-			pci_bus_lock(dev->subordinate);
-		else
-			pci_dev_lock(dev);
-	}
-}
-
-/* Unlock devices from the bottom of the tree up */
-static void pci_slot_unlock(struct pci_slot *slot)
-{
-	struct pci_dev *dev, *bridge = slot->bus->self;
-
-	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
-		if (!dev->slot || dev->slot != slot)
-			continue;
-		if (dev->subordinate)
-			pci_bus_unlock(dev->subordinate);
-		else
-			pci_dev_unlock(dev);
-	}
-
-	if (bridge)
-		pci_dev_unlock(bridge);
-}
-
-/* Return 1 on successful lock, 0 on contention */
-static int pci_slot_trylock(struct pci_slot *slot)
-{
-	struct pci_dev *dev, *bridge = slot->bus->self;
-
-	if (bridge && !pci_dev_trylock(bridge))
-		return 0;
-
-	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
-		if (!dev->slot || dev->slot != slot)
-			continue;
-		if (dev->subordinate) {
-			if (!pci_bus_trylock(dev->subordinate))
-				goto unlock;
-		} else if (!pci_dev_trylock(dev))
-			goto unlock;
-	}
-	return 1;
-
-unlock:
-	list_for_each_entry_continue_reverse(dev,
-					     &slot->bus->devices, bus_list) {
-		if (!dev->slot || dev->slot != slot)
-			continue;
-		if (dev->subordinate)
-			pci_bus_unlock(dev->subordinate);
-		else
-			pci_dev_unlock(dev);
-	}
-
-	if (bridge)
-		pci_dev_unlock(bridge);
-	return 0;
-}
-
 /*
  * Save and disable devices from the top of the tree down while holding
  * the @dev mutex lock for the entire tree.
@@ -5410,59 +5319,23 @@ static void pci_bus_restore_locked(struct pci_bus *bus)
 	}
 }
 
-/*
- * Save and disable devices from the top of the tree down while holding
- * the @dev mutex lock for the entire tree.
- */
-static void pci_slot_save_and_disable_locked(struct pci_slot *slot)
-{
-	struct pci_dev *dev;
-
-	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
-		if (!dev->slot || dev->slot != slot)
-			continue;
-		pci_dev_save_and_disable(dev);
-		if (dev->subordinate)
-			pci_bus_save_and_disable_locked(dev->subordinate);
-	}
-}
-
-/*
- * Restore devices from top of the tree down while holding @dev mutex lock
- * for the entire tree.  Parent bridges need to be restored before we can
- * get to subordinate devices.
- */
-static void pci_slot_restore_locked(struct pci_slot *slot)
-{
-	struct pci_dev *dev;
-
-	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
-		if (!dev->slot || dev->slot != slot)
-			continue;
-		pci_dev_restore(dev);
-		if (dev->subordinate) {
-			pci_bridge_wait_for_secondary_bus(dev, "slot reset");
-			pci_bus_restore_locked(dev->subordinate);
-		}
-	}
-}
-
 static int pci_slot_reset(struct pci_slot *slot, bool probe)
 {
+	struct pci_bus *bus = slot->bus;
 	int rc;
 
-	if (!slot || !pci_slot_resettable(slot))
+	if (!slot || (bus && !pci_bus_resettable(bus)))
 		return -ENOTTY;
 
-	if (!probe)
-		pci_slot_lock(slot);
+	if (!probe && bus)
+		pci_bus_lock(bus);
 
 	might_sleep();
 
 	rc = pci_reset_hotplug_slot(slot->hotplug, probe);
 
-	if (!probe)
-		pci_slot_unlock(slot);
+	if (!probe && bus)
+		pci_bus_unlock(bus);
 
 	return rc;
 }
@@ -5489,25 +5362,26 @@ EXPORT_SYMBOL_GPL(pci_probe_reset_slot);
  * wrap the bus reset to avoid spurious slot related events such as hotplug.
  * Generally a slot reset should be attempted before a bus reset.  All of the
  * function of the slot and any subordinate buses behind the slot are reset
- * through this function.  PCI config space of all devices in the slot and
- * behind the slot is saved before and restored after reset.
+ * through this function.  PCI config space of all devices below the slot bus
+ * are saved before and restored after reset.
  *
  * Same as above except return -EAGAIN if the slot cannot be locked
  */
 static int __pci_reset_slot(struct pci_slot *slot)
 {
+	struct pci_bus *bus = slot->bus;
 	int rc;
 
 	rc = pci_slot_reset(slot, PCI_RESET_PROBE);
 	if (rc)
 		return rc;
 
-	if (pci_slot_trylock(slot)) {
-		pci_slot_save_and_disable_locked(slot);
+	if (pci_bus_trylock(bus)) {
+		pci_bus_save_and_disable_locked(bus);
 		might_sleep();
 		rc = pci_reset_hotplug_slot(slot->hotplug, PCI_RESET_DO_RESET);
-		pci_slot_restore_locked(slot);
-		pci_slot_unlock(slot);
+		pci_bus_restore_locked(bus);
+		pci_bus_unlock(bus);
 	} else
 		rc = -EAGAIN;
 
-- 
2.47.3

Re: [PATCHv2 3/4] pci: remove slot specific lock/unlock and save/restore

[dan.j.williams]

Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The Linux pci driver resolves a "slot" to the "D" in the B:D.f (see
> PCI_SLOT()). A pcie "slot reset" is a secondary bus reset, which affects
> every function on every "D", not just the ones with a matching "slot".
> The slot lock/unlock and save/restore functions, however, are only
> handling a subset of the functions, breaking the rest.
> 
> ARI devices with more than 8 functions fail because their state is not
> properly handled, nor is the attached driver notified of the reset. In
> the best case, the device will appear unresponsive to the driver,
> resulting in unexpected errors. A worse possibility may panic the kernel
> if in flight transactions trigger hardware reported errors like this
> real observation:
> 
>   vfio-pci 0000:01:00.0: resetting
>   vfio-pci 0000:01:00.0: reset done
>   {1}[Hardware Error]:  Error 1, type: fatal
>   {1}[Hardware Error]:   section_type: PCIe error
>   {1}[Hardware Error]:   port_type: 0, PCIe end point
>   {1}[Hardware Error]:   version: 0.2
>   {1}[Hardware Error]:   command: 0x0140, status: 0x0010
>   {1}[Hardware Error]:   device_id: 0000:01:01.0
>   {1}[Hardware Error]:   slot: 0
>   {1}[Hardware Error]:   secondary_bus: 0x00
>   {1}[Hardware Error]:   vendor_id: 0x1d9b, device_id: 0x0207
>   {1}[Hardware Error]:   class_code: 020000
>   {1}[Hardware Error]:   bridge: secondary_status: 0x0000, control: 0x0000
>   {1}[Hardware Error]:   aer_cor_status: 0x00008000, aer_cor_mask: 0x00002000
>   {1}[Hardware Error]:   aer_uncor_status: 0x00010000, aer_uncor_mask: 0x00100000
>   {1}[Hardware Error]:   aer_uncor_severity: 0x006f6030
>   {1}[Hardware Error]:   TLP Header: 0a412800 00192080 60000004 00000004
>   GHES: Fatal hardware error but panic disabled
>   Kernel panic - not syncing: GHES: Fatal hardware error
> 
> Fix this by properly locking and notifying the entire affected bus
> topology, not just specific matching slots. For architectures that
> support "slot" specific resets, this patch potentially introduces an
> insignificant amount of overhead, but is otherwise harmless.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/pci/pci.c | 152 ++++------------------------------------------
>  1 file changed, 13 insertions(+), 139 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 57a5b205175f1..36427fbf7a747 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5218,7 +5218,6 @@ static bool pci_bus_resettable(struct pci_bus *bus)
>  {
>  	struct pci_dev *dev;
>  
> -
>  	if (bus->self && (bus->self->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET))
>  		return false;
>  
> @@ -5287,96 +5286,6 @@ static int pci_bus_trylock(struct pci_bus *bus)
>  	return 0;
>  }
>  
> -/* Do any devices on or below this slot prevent a bus reset? */
> -static bool pci_slot_resettable(struct pci_slot *slot)
> -{
> -	struct pci_dev *dev, *bridge = slot->bus->self;
> -
> -	if (bridge && (bridge->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET))
> -		return false;
> -
> -	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> -		if (!dev->slot || dev->slot != slot)
> -			continue;
> -		if (dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET ||
> -		    (dev->subordinate && !pci_bus_resettable(dev->subordinate)))
> -			return false;
> -	}
> -
> -	return true;
> -}
> -
> -/* Lock devices from the top of the tree down */
> -static void pci_slot_lock(struct pci_slot *slot)
> -{
> -	struct pci_dev *dev, *bridge = slot->bus->self;
> -
> -	if (bridge)
> -		pci_dev_lock(bridge);
> -
> -	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> -		if (!dev->slot || dev->slot != slot)
> -			continue;
> -		if (dev->subordinate)
> -			pci_bus_lock(dev->subordinate);
> -		else
> -			pci_dev_lock(dev);
> -	}
> -}
> -
> -/* Unlock devices from the bottom of the tree up */
> -static void pci_slot_unlock(struct pci_slot *slot)
> -{
> -	struct pci_dev *dev, *bridge = slot->bus->self;
> -
> -	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> -		if (!dev->slot || dev->slot != slot)
> -			continue;
> -		if (dev->subordinate)
> -			pci_bus_unlock(dev->subordinate);
> -		else
> -			pci_dev_unlock(dev);
> -	}
> -
> -	if (bridge)
> -		pci_dev_unlock(bridge);
> -}
> -
> -/* Return 1 on successful lock, 0 on contention */
> -static int pci_slot_trylock(struct pci_slot *slot)
> -{
> -	struct pci_dev *dev, *bridge = slot->bus->self;
> -
> -	if (bridge && !pci_dev_trylock(bridge))
> -		return 0;
> -
> -	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> -		if (!dev->slot || dev->slot != slot)
> -			continue;
> -		if (dev->subordinate) {
> -			if (!pci_bus_trylock(dev->subordinate))
> -				goto unlock;
> -		} else if (!pci_dev_trylock(dev))
> -			goto unlock;
> -	}
> -	return 1;
> -
> -unlock:
> -	list_for_each_entry_continue_reverse(dev,
> -					     &slot->bus->devices, bus_list) {
> -		if (!dev->slot || dev->slot != slot)
> -			continue;
> -		if (dev->subordinate)
> -			pci_bus_unlock(dev->subordinate);
> -		else
> -			pci_dev_unlock(dev);
> -	}
> -
> -	if (bridge)
> -		pci_dev_unlock(bridge);
> -	return 0;
> -}
> -
>  /*
>   * Save and disable devices from the top of the tree down while holding
>   * the @dev mutex lock for the entire tree.
> @@ -5410,59 +5319,23 @@ static void pci_bus_restore_locked(struct pci_bus *bus)
>  	}
>  }
>  
> -/*
> - * Save and disable devices from the top of the tree down while holding
> - * the @dev mutex lock for the entire tree.
> - */
> -static void pci_slot_save_and_disable_locked(struct pci_slot *slot)
> -{
> -	struct pci_dev *dev;
> -
> -	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> -		if (!dev->slot || dev->slot != slot)
> -			continue;
> -		pci_dev_save_and_disable(dev);
> -		if (dev->subordinate)
> -			pci_bus_save_and_disable_locked(dev->subordinate);
> -	}
> -}
> -
> -/*
> - * Restore devices from top of the tree down while holding @dev mutex lock
> - * for the entire tree.  Parent bridges need to be restored before we can
> - * get to subordinate devices.
> - */
> -static void pci_slot_restore_locked(struct pci_slot *slot)
> -{
> -	struct pci_dev *dev;
> -
> -	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> -		if (!dev->slot || dev->slot != slot)
> -			continue;
> -		pci_dev_restore(dev);
> -		if (dev->subordinate) {
> -			pci_bridge_wait_for_secondary_bus(dev, "slot reset");
> -			pci_bus_restore_locked(dev->subordinate);
> -		}
> -	}
> -}
> -
>  static int pci_slot_reset(struct pci_slot *slot, bool probe)
>  {
> +	struct pci_bus *bus = slot->bus;

Should this be:

    bus = slot ? slot->bus : NULL;

?

Looks like at least pci_reset_bus() could pass a NULL @slot.

>  	int rc;
>  
> -	if (!slot || !pci_slot_resettable(slot))
> +	if (!slot || (bus && !pci_bus_resettable(bus)))

With above and something like this:

@@ -5219,6 +5219,8 @@ static bool pci_bus_resettable(struct pci_bus *bus)
 {
        struct pci_dev *dev;
 
+       if (!bus)
+               return false;
 
        if (bus->self && (bus->self->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET))
                return false;

Can just make that:

	if (pci_bus_resettable(bus))


>  		return -ENOTTY;
>  
> -	if (!probe)
> -		pci_slot_lock(slot);
> +	if (!probe && bus)
> +		pci_bus_lock(bus);

Similar preference for pci_bus_lock() and pci_bus_unlock() to nop
themselves if @bus is NULL. That might want its own lead-in patch... but
only if Bjorn is on board with the making the conditional locking
implicit.

>  
>  	might_sleep();
>  
>  	rc = pci_reset_hotplug_slot(slot->hotplug, probe);
>  
> -	if (!probe)
> -		pci_slot_unlock(slot);
> +	if (!probe && bus)
> +		pci_bus_unlock(bus);
>  
>  	return rc;
>  }
> @@ -5489,25 +5362,26 @@ EXPORT_SYMBOL_GPL(pci_probe_reset_slot);
>   * wrap the bus reset to avoid spurious slot related events such as hotplug.
>   * Generally a slot reset should be attempted before a bus reset.  All of the
>   * function of the slot and any subordinate buses behind the slot are reset
> - * through this function.  PCI config space of all devices in the slot and
> - * behind the slot is saved before and restored after reset.
> + * through this function.  PCI config space of all devices below the slot bus
> + * are saved before and restored after reset.
>   *
>   * Same as above except return -EAGAIN if the slot cannot be locked
>   */
>  static int __pci_reset_slot(struct pci_slot *slot)
>  {
> +	struct pci_bus *bus = slot->bus;
>  	int rc;
>  
>  	rc = pci_slot_reset(slot, PCI_RESET_PROBE);
>  	if (rc)
>  		return rc;
>  
> -	if (pci_slot_trylock(slot)) {
> -		pci_slot_save_and_disable_locked(slot);
> +	if (pci_bus_trylock(bus)) {
> +		pci_bus_save_and_disable_locked(bus);
>  		might_sleep();
>  		rc = pci_reset_hotplug_slot(slot->hotplug, PCI_RESET_DO_RESET);
> -		pci_slot_restore_locked(slot);
> -		pci_slot_unlock(slot);
> +		pci_bus_restore_locked(bus);
> +		pci_bus_unlock(bus);

With at least the @bus initialization fixup above and with or without
the other suggested cleanups you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCHv2 3/4] pci: remove slot specific lock/unlock and save/restore

[Keith Busch]

On Fri, Jan 30, 2026 at 09:58:39PM -0800, dan.j.williams@intel.com wrote:
> Should this be:
> 
>     bus = slot ? slot->bus : NULL;
> 
> ?

Yes, you're right.

> Looks like at least pci_reset_bus() could pass a NULL @slot.
> 
> >  	int rc;
> >  
> > -	if (!slot || !pci_slot_resettable(slot))
> > +	if (!slot || (bus && !pci_bus_resettable(bus)))
> 
> With above and something like this:
> 
> @@ -5219,6 +5219,8 @@ static bool pci_bus_resettable(struct pci_bus *bus)
>  {
>         struct pci_dev *dev;
>  
> +       if (!bus)
> +               return false;

Sounds good.

> >  	int rc;
> >  
> >  	rc = pci_slot_reset(slot, PCI_RESET_PROBE);
> >  	if (rc)
> >  		return rc;
> >  
> > -	if (pci_slot_trylock(slot)) {
> > -		pci_slot_save_and_disable_locked(slot);
> > +	if (pci_bus_trylock(bus)) {
> > +		pci_bus_save_and_disable_locked(bus);
> >  		might_sleep();
> >  		rc = pci_reset_hotplug_slot(slot->hotplug, PCI_RESET_DO_RESET);
> > -		pci_slot_restore_locked(slot);
> > -		pci_slot_unlock(slot);
> > +		pci_bus_restore_locked(bus);
> > +		pci_bus_unlock(bus);
> 
> With at least the @bus initialization fixup above and with or without
> the other suggested cleanups you can add:

Thanks!

Re: [PATCHv2 3/4] pci: remove slot specific lock/unlock and save/restore

[Keith Busch]

On Fri, Jan 30, 2026 at 08:59:52AM -0800, Keith Busch wrote:
> Fix this by properly locking and notifying the entire affected bus
> topology, not just specific matching slots. For architectures that
> support "slot" specific resets, this patch potentially introduces an
> insignificant amount of overhead, but is otherwise harmless.

I have an alternative proposal if anyone thinks this patch is concerning:
it would be simple to make a sentinal value for the "slot" number to
mean "all devices", and then the slot will claim every function found on
a bus. That's the behavior that pciehp wants anyway, since it only
creates a single slot for a bridge to a suborinate bus. If we set up
slots this way, the existing slot specific reset methods will work fine.

---
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index f59baa9129709..d80346d567049 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -79,7 +79,8 @@ static int init_slot(struct controller *ctrl)
 	snprintf(name, SLOT_NAME_SIZE, "%u", PSN(ctrl));
 
 	retval = pci_hp_initialize(&ctrl->hotplug_slot,
-				   ctrl->pcie->port->subordinate, 0, name);
+				   ctrl->pcie->port->subordinate,
+				   PCI_SLOT_ALL_DEVICES, name);
 	if (retval) {
 		ctrl_err(ctrl, "pci_hp_initialize failed: error %d\n", retval);
 		kfree(ops);
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 50fb3eb595fe6..e0fdbbfbc04fc 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -41,6 +41,9 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)
 		return sysfs_emit(buf, "%04x:%02x\n",
 				  pci_domain_nr(slot->bus),
 				  slot->bus->number);
+	if (slot->number == PCI_SLOT_ALL_DEVICES)
+		return sysfs_emit(buf, "%04x:%02x:00\n",
+				  pci_domain_nr(slot->bus),
 
 	return sysfs_emit(buf, "%04x:%02x:%02x\n",
 			  pci_domain_nr(slot->bus),
@@ -73,7 +76,8 @@ static void pci_slot_release(struct kobject *kobj)
 
 	down_read(&pci_bus_sem);
 	list_for_each_entry(dev, &slot->bus->devices, bus_list)
-		if (PCI_SLOT(dev->devfn) == slot->number)
+		if (slot->number == PCI_SLOT_ALL_DEVICES ||
+		    PCI_SLOT(dev->devfn) == slot->number)
 			dev->slot = NULL;
 	up_read(&pci_bus_sem);
 
@@ -166,7 +170,8 @@ void pci_dev_assign_slot(struct pci_dev *dev)
 
 	mutex_lock(&pci_slot_mutex);
 	list_for_each_entry(slot, &dev->bus->slots, list)
-		if (PCI_SLOT(dev->devfn) == slot->number)
+		if (slot->number == PCI_SLOT_ALL_DEVICES ||
+		    PCI_SLOT(dev->devfn) == slot->number)
 			dev->slot = slot;
 	mutex_unlock(&pci_slot_mutex);
 }
@@ -188,7 +193,7 @@ static struct pci_slot *get_slot(struct pci_bus *parent, int slot_nr)
 /**
  * pci_create_slot - create or increment refcount for physical PCI slot
  * @parent: struct pci_bus of parent bridge
- * @slot_nr: PCI_SLOT(pci_dev->devfn) or -1 for placeholder
+ * @slot_nr: PCI_SLOT(pci_dev->devfn), -1 for placeholder, or PCI_SLOT_ALL_DEVICES
  * @name: user visible string presented in /sys/bus/pci/slots/<name>
  * @hotplug: set if caller is hotplug driver, NULL otherwise
  *
@@ -285,7 +297,8 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
 
 	down_read(&pci_bus_sem);
 	list_for_each_entry(dev, &parent->devices, bus_list)
-		if (PCI_SLOT(dev->devfn) == slot_nr)
+		if (slot_nr == PCI_SLOT_ALL_DEVICES ||
+		    PCI_SLOT(dev->devfn) == slot_nr)
 			dev->slot = slot;
 	up_read(&pci_bus_sem);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 864775651c6fa..d9f85025d97f5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -72,12 +72,18 @@
 /* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
 #define PCI_BUS_NUM(x) (((x) >> 8) & 0xff)
 
+/*
+ * PCI_SLOT_ALL_DEVICES indicates a slot that covers all devices on the bus.
+ * Used for PCIe hotplug where the physical slot is the entire subordinate bus.
+ */
+#define PCI_SLOT_ALL_DEVICES	0xfe
+
 /* pci_slot represents a physical slot */
 struct pci_slot {
 	struct pci_bus		*bus;		/* Bus this slot is on */
--

[PATCHv2 4/4] pci: make reset_subordinate hotplug safe

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Use the slot reset method when resetting the bridge if the bus contains
hot plug slots. This fixes spurious hot plug events that are triggered
by the secondary bus reset that bypasses the slot's detection disabling.

Resetting a bridge's subordinate bus can be done like this:

  # echo 1 > /sys/bus/pci/devices/0000:50:01.0/reset_subordinate

Prior to this patch, an example kernel message may show something like:

  pcieport 0000:50:01.0: pciehp: Slot(40): Link Down

With this change, the pciehp driver ignores the link event during the
reset, so may show this message instead:

  pcieport 0000:50:01.0: pciehp: Slot(40): Link Down/Up ignored

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/pci-sysfs.c |  3 +-
 drivers/pci/pci.c       | 70 +++++++++++++++++++++++++++--------------
 drivers/pci/pci.h       |  2 +-
 3 files changed, 48 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index c2df915ad2d29..0e1cef1c1c73b 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -553,7 +553,6 @@ static ssize_t reset_subordinate_store(struct device *dev,
 				const char *buf, size_t count)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	struct pci_bus *bus = pdev->subordinate;
 	unsigned long val;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -563,7 +562,7 @@ static ssize_t reset_subordinate_store(struct device *dev,
 		return -EINVAL;
 
 	if (val) {
-		int ret = __pci_reset_bus(bus);
+		int ret = pci_reset_bridge(pdev);
 
 		if (ret)
 			return ret;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 36427fbf7a747..12cf61a9bbeb1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -50,6 +50,7 @@ EXPORT_SYMBOL(pci_pci_problems);
 unsigned int pci_pm_d3hot_delay;
 
 static void pci_pme_list_scan(struct work_struct *work);
+static int __pci_reset_bridge(struct pci_dev *bridge, bool save);
 
 static LIST_HEAD(pci_pme_list);
 static DEFINE_MUTEX(pci_pme_list_mutex);
@@ -5419,29 +5420,7 @@ static int pci_bus_reset(struct pci_bus *bus, bool probe)
  */
 int pci_bus_error_reset(struct pci_dev *bridge)
 {
-	struct pci_bus *bus = bridge->subordinate;
-	struct pci_slot *slot;
-
-	if (!bus)
-		return -ENOTTY;
-
-	mutex_lock(&pci_slot_mutex);
-	if (list_empty(&bus->slots))
-		goto bus_reset;
-
-	list_for_each_entry(slot, &bus->slots, list)
-		if (pci_probe_reset_slot(slot))
-			goto bus_reset;
-
-	list_for_each_entry(slot, &bus->slots, list)
-		if (pci_slot_reset(slot, PCI_RESET_DO_RESET))
-			goto bus_reset;
-
-	mutex_unlock(&pci_slot_mutex);
-	return 0;
-bus_reset:
-	mutex_unlock(&pci_slot_mutex);
-	return pci_bus_reset(bridge->subordinate, PCI_RESET_DO_RESET);
+	return __pci_reset_bridge(bridge, false);
 }
 
 /**
@@ -5462,7 +5441,7 @@ EXPORT_SYMBOL_GPL(pci_probe_reset_bus);
  *
  * Same as above except return -EAGAIN if the bus cannot be locked
  */
-int __pci_reset_bus(struct pci_bus *bus)
+static int __pci_reset_bus(struct pci_bus *bus)
 {
 	int rc;
 
@@ -5482,6 +5461,49 @@ int __pci_reset_bus(struct pci_bus *bus)
 	return rc;
 }
 
+static int __pci_reset_bridge(struct pci_dev *bridge, bool save)
+{
+	struct pci_bus *bus = bridge->subordinate;
+	struct pci_slot *slot;
+
+	if (!bus)
+		return -ENOTTY;
+
+	mutex_lock(&pci_slot_mutex);
+	if (list_empty(&bus->slots))
+		goto bus_reset;
+
+	list_for_each_entry(slot, &bus->slots, list)
+		if (pci_probe_reset_slot(slot))
+			goto bus_reset;
+
+	list_for_each_entry(slot, &bus->slots, list) {
+		int ret;
+
+		if (save)
+			ret = __pci_reset_slot(slot);
+		else
+			ret = pci_slot_reset(slot, PCI_RESET_DO_RESET);
+
+		if (ret)
+			goto bus_reset;
+	}
+
+	mutex_unlock(&pci_slot_mutex);
+	return 0;
+bus_reset:
+	mutex_unlock(&pci_slot_mutex);
+
+	if (save)
+		return __pci_reset_bus(bus);
+	return pci_bus_reset(bridge->subordinate, PCI_RESET_DO_RESET);
+}
+
+int pci_reset_bridge(struct pci_dev *bridge)
+{
+	return __pci_reset_bridge(bridge, true);
+}
+
 /**
  * pci_reset_bus - Try to reset a PCI bus
  * @pdev: top level PCI device to reset via slot/bus
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 0e67014aa0013..e537769cc3880 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -197,7 +197,7 @@ bool pci_reset_supported(struct pci_dev *dev);
 void pci_init_reset_methods(struct pci_dev *dev);
 int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
 int pci_bus_error_reset(struct pci_dev *dev);
-int __pci_reset_bus(struct pci_bus *bus);
+int pci_reset_bridge(struct pci_dev *dev);
 
 struct pci_cap_saved_data {
 	u16		cap_nr;
-- 
2.47.3

Re: [PATCHv2 4/4] pci: make reset_subordinate hotplug safe

[dan.j.williams]

Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Use the slot reset method when resetting the bridge if the bus contains

What about some renames to make this easier to read, because it is
ironic that this says "Use the slot reset method" when the functional
change is to now start using the __pci_reset_slot() method. If I am
reading this "slot reset vs reset slot" conversion correctly.

Something like:

s/__pci_reset_slot/pci_try_masked_slot_reset/

Since it is a conditional reset, and it is masking slot events.

...but pulling that string also means:

s/__pci_reset_bus/pci_try_reset_bus/

> hot plug slots. This fixes spurious hot plug events that are triggered
> by the secondary bus reset that bypasses the slot's detection disabling.
> 
> Resetting a bridge's subordinate bus can be done like this:
> 
>   # echo 1 > /sys/bus/pci/devices/0000:50:01.0/reset_subordinate
> 
> Prior to this patch, an example kernel message may show something like:
> 
>   pcieport 0000:50:01.0: pciehp: Slot(40): Link Down
> 
> With this change, the pciehp driver ignores the link event during the
> reset, so may show this message instead:
> 
>   pcieport 0000:50:01.0: pciehp: Slot(40): Link Down/Up ignored
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/pci/pci-sysfs.c |  3 +-
>  drivers/pci/pci.c       | 70 +++++++++++++++++++++++++++--------------
>  drivers/pci/pci.h       |  2 +-
>  3 files changed, 48 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index c2df915ad2d29..0e1cef1c1c73b 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -553,7 +553,6 @@ static ssize_t reset_subordinate_store(struct device *dev,
>  				const char *buf, size_t count)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct pci_bus *bus = pdev->subordinate;
>  	unsigned long val;
>  
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -563,7 +562,7 @@ static ssize_t reset_subordinate_store(struct device *dev,
>  		return -EINVAL;
>  
>  	if (val) {
> -		int ret = __pci_reset_bus(bus);
> +		int ret = pci_reset_bridge(pdev);
>  
>  		if (ret)
>  			return ret;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 36427fbf7a747..12cf61a9bbeb1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -50,6 +50,7 @@ EXPORT_SYMBOL(pci_pci_problems);
>  unsigned int pci_pm_d3hot_delay;
>  
>  static void pci_pme_list_scan(struct work_struct *work);
> +static int __pci_reset_bridge(struct pci_dev *bridge, bool save);
>  
>  static LIST_HEAD(pci_pme_list);
>  static DEFINE_MUTEX(pci_pme_list_mutex);
> @@ -5419,29 +5420,7 @@ static int pci_bus_reset(struct pci_bus *bus, bool probe)
>   */
>  int pci_bus_error_reset(struct pci_dev *bridge)
>  {
> -	struct pci_bus *bus = bridge->subordinate;
> -	struct pci_slot *slot;
> -
> -	if (!bus)
> -		return -ENOTTY;
> -
> -	mutex_lock(&pci_slot_mutex);
> -	if (list_empty(&bus->slots))
> -		goto bus_reset;
> -
> -	list_for_each_entry(slot, &bus->slots, list)
> -		if (pci_probe_reset_slot(slot))
> -			goto bus_reset;
> -
> -	list_for_each_entry(slot, &bus->slots, list)
> -		if (pci_slot_reset(slot, PCI_RESET_DO_RESET))
> -			goto bus_reset;
> -
> -	mutex_unlock(&pci_slot_mutex);
> -	return 0;
> -bus_reset:
> -	mutex_unlock(&pci_slot_mutex);
> -	return pci_bus_reset(bridge->subordinate, PCI_RESET_DO_RESET);
> +	return __pci_reset_bridge(bridge, false);

Similar to how PCI_RESET_{DO_RESET,PROBE} makes the ambiguous
"true/false" readable, how about adding something like:

#define PCI_SLOT_RESET_MASKED true
#define PCI_SLOT_RESET_UNMASKED false

>  }
>  
>  /**
> @@ -5462,7 +5441,7 @@ EXPORT_SYMBOL_GPL(pci_probe_reset_bus);
>   *
>   * Same as above except return -EAGAIN if the bus cannot be locked
>   */
> -int __pci_reset_bus(struct pci_bus *bus)
> +static int __pci_reset_bus(struct pci_bus *bus)
>  {
>  	int rc;
>  
> @@ -5482,6 +5461,49 @@ int __pci_reset_bus(struct pci_bus *bus)
>  	return rc;
>  }
>  
> +static int __pci_reset_bridge(struct pci_dev *bridge, bool save)
> +{
> +	struct pci_bus *bus = bridge->subordinate;
> +	struct pci_slot *slot;
> +
> +	if (!bus)
> +		return -ENOTTY;
> +
> +	mutex_lock(&pci_slot_mutex);
> +	if (list_empty(&bus->slots))
> +		goto bus_reset;
> +
> +	list_for_each_entry(slot, &bus->slots, list)
> +		if (pci_probe_reset_slot(slot))
> +			goto bus_reset;
> +
> +	list_for_each_entry(slot, &bus->slots, list) {
> +		int ret;
> +
> +		if (save)
> +			ret = __pci_reset_slot(slot);
> +		else
> +			ret = pci_slot_reset(slot, PCI_RESET_DO_RESET);
> +
> +		if (ret)
> +			goto bus_reset;
> +	}
> +
> +	mutex_unlock(&pci_slot_mutex);
> +	return 0;
> +bus_reset:
> +	mutex_unlock(&pci_slot_mutex);
> +
> +	if (save)
> +		return __pci_reset_bus(bus);
> +	return pci_bus_reset(bridge->subordinate, PCI_RESET_DO_RESET);

Logic looks equivalent in the PCI_SLOT_RESET_UNMASKED case, I think it
wants a comment that it turns into a "try_reset" when
PCI_SLOT_RESET_MASKED...

> +}
> +
> +int pci_reset_bridge(struct pci_dev *bridge)
> +{
> +	return __pci_reset_bridge(bridge, true);
> +}

...and maybe that comment is helped by naming this helper
pci_try_reset_bridge()?

I would want a second opinion on the renames before saying they are
absolutely required.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

...note I did not look at anything beyond the claims in the changelog,
like potential unwanted knock-on effect from the new masking.

Re: [PATCHv2 4/4] pci: make reset_subordinate hotplug safe

[Keith Busch]

On Fri, Jan 30, 2026 at 10:42:22PM -0800, dan.j.williams@intel.com wrote:
> Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > Use the slot reset method when resetting the bridge if the bus contains
> 
> What about some renames to make this easier to read, because it is
> ironic that this says "Use the slot reset method" when the functional
> change is to now start using the __pci_reset_slot() method. If I am
> reading this "slot reset vs reset slot" conversion correctly.

Yep, I thnk you've got the sense of the naming issues here.
 
> Something like:
> 
> s/__pci_reset_slot/pci_try_masked_slot_reset/
> 
> Since it is a conditional reset, and it is masking slot events.
> 
> ...but pulling that string also means:
> 
> s/__pci_reset_bus/pci_try_reset_bus/

Yeah, there are a lot of refactoring opportunities here. My experience
with the pci subsystem, though, is that it is a bit conservative (and
perhaps rightfully so) on accepting refactor churn. 

> > -	mutex_unlock(&pci_slot_mutex);
> > -	return pci_bus_reset(bridge->subordinate, PCI_RESET_DO_RESET);
> > +	return __pci_reset_bridge(bridge, false);
> 
> Similar to how PCI_RESET_{DO_RESET,PROBE} makes the ambiguous
> "true/false" readable, how about adding something like:
> 
> #define PCI_SLOT_RESET_MASKED true
> #define PCI_SLOT_RESET_UNMASKED false

Sure, can do. Not sure I like the existing PROBE vs DO_RESET though. I
think the probe should just open code the checks in its own function
(which already exist) so that the DO_RESET doesn't duplicate literally
every check the preceding PROBE does. But that's a refactoring thing.

> > +	mutex_unlock(&pci_slot_mutex);
> > +
> > +	if (save)
> > +		return __pci_reset_bus(bus);
> > +	return pci_bus_reset(bridge->subordinate, PCI_RESET_DO_RESET);
> 
> Logic looks equivalent in the PCI_SLOT_RESET_UNMASKED case, I think it
> wants a comment that it turns into a "try_reset" when
> PCI_SLOT_RESET_MASKED...
> 
> > +}
> > +
> > +int pci_reset_bridge(struct pci_dev *bridge)
> > +{
> > +	return __pci_reset_bridge(bridge, true);
> > +}
> 
> ...and maybe that comment is helped by naming this helper
> pci_try_reset_bridge()?

Sounds good.
 
> I would want a second opinion on the renames before saying they are
> absolutely required.

Thanks for the reviews. I'll think on this a it and mix in your
suggestions for the next version.

> ...note I did not look at anything beyond the claims in the changelog,
> like potential unwanted knock-on effect from the new masking.

I'm more concerned about the previous patch in this series. I have no
experience with slots that can reset a subset of attached bus devices,
but apparently they exist. Not that it should cause a problem beyond
extra locking and config space access that would't be necessary, which
should just cost a few unnecessary cpu cycles.

Re: [PATCHv2 0/4] pci: slot handling fixes

[Bjorn Helgaas]

On Fri, Jan 30, 2026 at 08:59:49AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This series fixes some older issues are the slot lock usage, with two
> follow on patches to address multi-function resets in pcie hotplug
> slots.
> 
> Previous discussions:
> 
>   https://lore.kernel.org/linux-pci/20260116184150.3013258-1-kbusch@meta.com/
> 
>   https://lore.kernel.org/linux-pci/aXvOOjZQ_EmZDgVs@kbusch-mbp/T/#t
> 
> Version change notes:
> 
>  * The first patch is unchanged from the original.
> 
>  * The second patch adds the local "bridge" to improve code clarity
>    (review comments from Dan)
> 
>  * The third patch removes pci_slot_resettable in addition to all the
>    other slot specific locking and state saving functions. I also
>    updated a code comment explaining that all bus devices are saved and
>    restored with the slot reset.
> 
>  * Included a fourth patch that fixes user suborindate bus resets when the
>    bridge is pcie hotplug capable; a previous version came from this
>    independent patch here:
> 
>   https://lore.kernel.org/linux-pci/20260116154244.1452246-1-kbusch@meta.com/
> 
>    And I'm including it in this series since it is all in the same area
>    with slot resets. I've made a minor change to the previous version to
>    remove the code duplication with pci_bus_error_reset().
> 
> Jinhui Guo (1):
>   PCI: Fix incorrect unlocking in pci_slot_trylock()
> 
> Keith Busch (3):
>   pci: fix slot reset device locking
>   pci: remove slot specific lock/unlock and save/restore
>   pci: make reset_subordinate hotplug safe
> 
>  drivers/pci/pci-sysfs.c |   3 +-
>  drivers/pci/pci.c       | 213 +++++++++++-----------------------------
>  drivers/pci/pci.h       |   2 +-
>  3 files changed, 61 insertions(+), 157 deletions(-)

Applied the first two on pci/virtualization for now (the first was
already there):

  67bcd817a012 ("PCI: Fix pci_slot_trylock() error handling")
  c6a26e539649 ("PCI: Fix pci_slot_lock () device locking")

If we get a review for the others I'll add them on top.

Thanks!