pci: slot handling fixes

From: Keith Busch <[email protected]>

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 <[email protected]>

---
 drivers/pci/pci.c | 152 ++++------------------------------------------
 1 file changed, 13 insertions(+), 139 deletions(-)

drivers/pci/pci.c +0 -121

index 57a5b205175f1..36427fbf7a747 100644
@@ -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);

drivers/pci/pci.c +5 -4

 	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;
 

Should this be:

    bus = slot ? slot->bus : NULL;

?

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

drivers/pci/pci.c static void pci_bus_restore_locked(struct pci_bus *bus) · line 5327

Quoted 4 lines

drivers/pci/pci.c +1 -1

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

With above and something like this:

(quoted diff) +0 -0

 >  	int rc;

Quoted 5 lines

drivers/pci/pci.c +0 -0

 	rc = pci_slot_reset(slot, PCI_RESET_PROBE);
 	if (rc)
 		return rc;

drivers/pci/pci.c EXPORT_SYMBOL_GPL(pci_probe_reset_slot); · line 5384

Quoted 10 lines

drivers/pci/pci.c +4 -4

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

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.

From: Keith Busch <[email protected]>

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 <[email protected]>

---
 drivers/pci/pci-sysfs.c |  3 +-
 drivers/pci/pci.c       | 70 +++++++++++++++++++++++++++--------------
 drivers/pci/pci.h       |  2 +-
 3 files changed, 48 insertions(+), 27 deletions(-)

drivers/pci/pci-sysfs.c +1 -2

index c2df915ad2d29..0e1cef1c1c73b 100644
@@ -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;

drivers/pci/pci.c +1 -14

index 36427fbf7a747..12cf61a9bbeb1 100644
@@ -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;
-

drivers/pci/pci.c +27 -1

 }
 
 /**

@@ -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;

Quoted 3 lines · Keith Busch

From: Keith Busch <[email protected]>

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/

drivers/pci/pci.c static int pci_bus_reset(struct pci_bus *bus, bool probe) · line 5423

Quoted 3 lines

drivers/pci/pci.c +1 -2

-	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

drivers/pci/pci.c int __pci_reset_bus(struct pci_bus *bus) · line 5499

Quoted 5 lines

drivers/pci/pci.c +5 -0

+	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...

drivers/pci/pci.c int __pci_reset_bus(struct pci_bus *bus) · line 5505

Quoted 6 lines

drivers/pci/pci.c +6 -0

+}
+
+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.

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

From: Keith Busch <[email protected]>

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/[email protected]/

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/[email protected]/

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(-)