pci slot reset handling fixes
v5 cover pci slot reset handling fixes
From: Keith Busch <[email protected]>
Previous version:
https://lore.kernel.org/linux-pci/[email protected]/
Changes from previous version:
* Fix subject line to match convenction (s/pci/PCI)
* Added example kernel panic logs for fatal hardware errors observed without the slot handling fixes
* Rearraged some code in the last patch to avoid the need for a forward declaration.
* Expanded on the comment explaing the difference between the restore values for the bridge reset
Keith Busch (3):
PCI: rename __pci_bus_reset and __pci_slot_reset
PCI: allow all bus devices to use the same slot
PCI: make reset_subordinate hotplug safe
drivers/pci/hotplug/pciehp_core.c | 3 +- drivers/pci/pci-sysfs.c | 3 +- drivers/pci/pci.c | 97 ++++++++++++++++++++----------- drivers/pci/pci.h | 2 +- drivers/pci/slot.c | 27 +++++++-- include/linux/pci.h | 8 ++- 6 files changed, 97 insertions(+), 43 deletions(-)
Signature · 1 line
2.47.3
PATCH v5 1/3 PCI: rename __pci_bus_reset and __pci_slot_reset
From: Keith Busch <[email protected]>
Make the code a little easier to navigate with more descriptive function names. The two renamed functions here "try" to do to a reset, so make that clear in the name to distinguish them from other similarly named functions.
--- drivers/pci/pci-sysfs.c | 2 +- drivers/pci/pci.c | 10 +++++----- drivers/pci/pci.h | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)
drivers/pci/pci-sysfs.c +1 -1
@@ -563,7 +563,7 @@ static ssize_t reset_subordinate_store(struct device *dev,return -EINVAL ;if (val) { -int ret =__pci_reset_bus (bus); +int ret =pci_try_reset_bus (bus);if (ret)return ret;
drivers/pci/pci.c +5 -5
@@ -5541,7 +5541,7 @@ int pci_probe_reset_slot(struct pci_slot *slot)EXPORT_SYMBOL_GPL (pci_probe_reset_slot);/** -* __pci_reset_slot - Try to reset a PCI slot +* pci_try_reset_slot - Try to reset a PCI slot * @slot: PCI slot to reset * * A PCI bus may host multiple slots, each slot may support a reset mechanism
@@ -5555,7 +5555,7 @@ EXPORT_SYMBOL_GPL(pci_probe_reset_slot);* * Same as above except return -EAGAIN if the slot cannot be locked */ -static int __pci_reset_slot (struct pci_slot *slot) +static int pci_try_reset_slot (struct pci_slot *slot) {int rc;
@@ -5644,12 +5644,12 @@ int pci_probe_reset_bus(struct pci_bus *bus)EXPORT_SYMBOL_GPL (pci_probe_reset_bus);/** -* __pci_reset_bus - Try to reset a PCI bus +* pci_try_reset_bus - Try to reset a PCI bus * @bus: top level PCI bus to reset * * Same as above except return -EAGAIN if the bus cannot be locked */ -int __pci_reset_bus (struct pci_bus *bus) +int pci_try_reset_bus (struct pci_bus *bus) {int rc;
@@ -5678,7 +5678,7 @@ int __pci_reset_bus(struct pci_bus *bus)int pci_reset_bus (struct pci_dev *pdev) {return (!pci_probe_reset_slot (pdev->slot)) ? -__pci_reset_slot (pdev->slot) :__pci_reset_bus (pdev->bus); +pci_try_reset_slot (pdev->slot) :pci_try_reset_bus (pdev->bus); }EXPORT_SYMBOL_GPL (pci_reset_bus);
drivers/pci/pci.h +1 -1
@@ -231,7 +231,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_try_reset_bus (struct pci_bus *bus);struct pci_cap_saved_data {u16 cap_nr;
Signature · 1 line
2.47.3
PATCH v5 2/3 PCI: allow all bus devices to use the same slot
In reply to Keith BuschFrom: Keith Busch <[email protected]>
A pcie hotplug slot applies to the entire subordinate bus. Thus, pciehp only allocates a single hotplug_slot for the bridge to that bus. The pci slot, though, would only match to functions on device 0, meaning all device beyond that are not matched to any slot even though they share it. A slot reset will break all the missing devices because the handling skips them.
For example, 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 errorAllow a slot to be created to claim all devices on a bus, not just a matching device. This is done by introducing a sentinel value, named PCI_SLOT_ALL_DEVICES, which then has the pci slot match to any device on the bus. This fixes slot resets for pciehp.
Since 0xff already has special meaning, the chosen value for this new feature is 0xfe. This will not clash with any actual slot number since they are limited to 5 bits.
--- drivers/pci/hotplug/pciehp_core.c | 3 ++- drivers/pci/slot.c | 27 +++++++++++++++++++++++---- include/linux/pci.h | 8 +++++++- 3 files changed, 32 insertions(+), 6 deletions(-)
drivers/pci/hotplug/pciehp_core.c +2 -1
@@ -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);
drivers/pci/slot.c +23 -4
@@ -42,6 +42,15 @@ static ssize_t address_read_file(struct pci_slot *slot, char *buf)pci_domain_nr (slot->bus), slot->bus->number); +/* +* Preserve legacy ABI expectations that hotplug drivers that manage +* multiple devices per slot emit 0 for the device number. +*/ +if (slot->number ==PCI_SLOT_ALL_DEVICES ) +return sysfs_emit (buf,"%04x:%02x:00\n" , +pci_domain_nr (slot->bus), + slot->bus->number); + return sysfs_emit(buf, "%04x:%02x:%02x\n", pci_domain_nr(slot->bus), slot->bus->number,
@@ -73,7 +82,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 +176,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 +199,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 *
@@ -222,6 +233,13 @@ static struct pci_slot *get_slot(struct pci_bus *parent, int slot_nr)* consist solely of a dddd:bb tuple, where dddd is the PCI domain of the * %struct pci_bus and bb is the bus number. In other words, the devfn of * the 'placeholder' slot will not be displayed. +* +* Bus-wide slots: +* For PCIe hotplug, the physical slot encompasses the entire subordinate +* bus, not just a single device number. Pass @slot_nr == PCI_SLOT_ALL_DEVICES +* to create a slot that matches all devices on the bus. Unlike placeholder +* slots, bus-wide slots go through normal slot lookup and reuse existing +* slots if present. */ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr, const char *name,
@@ -285,7 +303,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);
include/linux/pci.h +7 -1
@@ -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 */ struct list_head list;/* Node in list of slots */ struct hotplug_slot *hotplug;/* Hotplug info (move here) */ -unsigned char number;/* PCI_SLOT(pci_dev->devfn) */ +unsigned char number;/* Device nr, or PCI_SLOT_ALL_DEVICES */ struct kobject kobj; };
Signature · 1 line
2.47.3
Re: [PATCHv5 2/3] PCI: allow all bus devices to use the same slot
Quoted 8 lines · Keith Busch
From: Keith Busch <[email protected]>
A pcie hotplug slot applies to the entire subordinate bus. Thus, pciehp only allocates a single
hotplug_slotfor the bridge to that bus. The pci slot, though, would only match to functions on device 0, meaning all device beyond that are not matched to any slot even though they share it. A slot reset will break all the missing devices because the handling skips them.
I tweaked this a bit to (a) refer to the "secondary" bus instead of "subordinate" to avoid confusion with the PCI "subordinate" term ("pci_dev->subordinate" already has this confusion but is harder to fix), and (b) mention ARI, which I think is what causes the problem you're solving:
+ A PCIe hotplug slot applies to the entire secondary bus. Thus, pciehp only
+ allocates a single hotplug_slot for the bridge to that bus. The existing
+ PCI slot, though, would only match to functions on device 0, meaning any
+ devices beyond that, e.g., ARI functions, are not matched to any slot even
+ though they share it.
Quoted 7 lines
For example, 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:
drivers/pci/slot.c static struct pci_slot *get_slot(struct pci_bus *parent, int slot_nr) · line 242
Quoted 10 lines
drivers/pci/slot.c +7
-0@@ -222,6 +233,13 @@ static struct pci_slot *get_slot(struct pci_bus *parent, int slot_nr)* consist solely of a dddd:bb tuple, where dddd is the PCI domain of the * %struct pci_bus and bb is the bus number. In other words, the devfn of * the 'placeholder' slot will not be displayed. +* +* Bus-wide slots: +* For PCIe hotplug, the physical slot encompasses the entire subordinate +* bus, not just a single device number. Pass @slot_nr == PCI_SLOT_ALL_DEVICES +* to create a slot that matches all devices on the bus. Unlike placeholder +* slots, bus-wide slots go through normal slot lookup and reuse existing +* slots if present.
+ * For PCIe hotplug, the physical slot encompasses the entire secondary
+ * bus, not just a single device number. If the device supports ARI and ARI
+ * Forwarding is enabled in the upstream bridge, a multi-function device
+ * may include functions that appear to have several different device
+ * numbers, i.e., PCI_SLOT() values. Pass @slot_nr == PCI_SLOT_ALL_DEVICES
drivers/pci/slot.c line 77
Quoted 12 lines
drivers/pci/slot.c +4
-0*/ struct pci_slot *pci_create_slot (struct pci_bus *parent,int slot_nr,const char *name, +++ 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.
+ * Used for PCIe hotplug where the physical slot is the entire secondary bus,
+ * and, if ARI Forwarding is enabled, functions may appear to be on multiple
+ * devices.
Let me know if I got any of this wrong.
Bjorn
Re: [PATCHv5 2/3] PCI: allow all bus devices to use the same slot
Quoted 15 lines · Bjorn Helgaas
Quoted 8 lines · Keith Busch
From: Keith Busch <[email protected]>
A pcie hotplug slot applies to the entire subordinate bus. Thus, pciehp only allocates a single
hotplug_slotfor the bridge to that bus. The pci slot, though, would only match to functions on device 0, meaning all device beyond that are not matched to any slot even though they share it. A slot reset will break all the missing devices because the handling skips them.I tweaked this a bit to (a) refer to the "secondary" bus instead of "subordinate" to avoid confusion with the PCI "subordinate" term ("
pci_dev->subordinate" already has this confusion but is harder to fix), and (b) mention ARI, which I think is what causes the problem you're solving:
[..]
Quoted 1 line
Let me know if I got any of this wrong.
Looks good to me and matches my understanding of what Keith is fixing.
PATCH v5 3/3 PCI: make reset_subordinate hotplug safe
In reply to Keith BuschFrom: 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
--- drivers/pci/pci-sysfs.c | 3 +- drivers/pci/pci.c | 91 +++++++++++++++++++++++++++-------------- drivers/pci/pci.h | 2 +- 3 files changed, 62 insertions(+), 34 deletions(-)
drivers/pci/pci-sysfs.c +1 -2
@@ -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_try_reset_bus (bus); +int ret =pci_try_reset_bridge (pdev);if (ret)return ret;
drivers/pci/pci.c +60 -31
123 lines not on this page · raw message
drivers/pci/pci.h +1 -1
@@ -231,7 +231,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_try_reset_bus (struct pci_bus *bus); +int pci_try_reset_bridge (struct pci_dev *bridge);struct pci_cap_saved_data {u16 cap_nr;
Signature · 1 line
2.47.3
Hi Bjorn,
It's pretty quiet on this version. Can we consider this one for inclusion?
Thanks
Bjorn Helgaas
AppliedQuoted 24 lines · Keith Busch
From: Keith Busch <[email protected]>
Previous version:
https://lore.kernel.org/linux-pci/[email protected]/
Changes from previous version:
* Fix subject line to match convenction (s/pci/PCI)
* Added example kernel panic logs for fatal hardware errors observed without the slot handling fixes
* Rearraged some code in the last patch to avoid the need for a forward declaration.
* Expanded on the comment explaing the difference between the restore values for the bridge reset
Keith Busch (3):
PCI: rename__pci_bus_resetand__pci_slot_reset
PCI: allow all bus devices to use the same slot
PCI: makereset_subordinatehotplug safe
Quoted 7 lines
drivers/pci/hotplug/pciehp_core.c | 3 +- drivers/pci/pci-sysfs.c | 3 +- drivers/pci/pci.c | 97 ++++++++++++++++++++----------- drivers/pci/pci.h | 2 +- drivers/pci/slot.c | 27 +++++++-- include/linux/pci.h | 8 ++- 6 files changed, 97 insertions(+), 43 deletions(-)
Applied to pci/reset for v7.1, thanks!