PCI hotplug driver fixes

PATCH Superseded Krishna Kumar 4 participants 10 messages Jun 24, 2024 – Jun 29, 2024

Subsystems (2)

PowerPC PCI

lore

Share & tools

lore

Krishna Kumar

author

2024-06-24 12:11 UTC #0 lore raw

v3 cover PCI hotplug driver fixes

The fix of Powerpc hotplug driver (drivers/pci/hotplug/pnv_php.c) addresses below two issues.

1. Kernel Crash during hot unplug of bridge/switch slot.

2. Bridge Support Enablement - Previously, when we do a hot-unplug operation on a bridge slot, all the ports and devices behind the bridge-ports would be hot-unplugged/offline, but when we do a hot-plug operation on the same bridge slot, all the ports and devices behind the bridge would not get hot-plugged/online. In this case, Only the first port of the bridge gets enabled and the remaining port/devices remain unplugged/offline. After the fix, The hot-unplug and hot-plug operations on the slot associated with the bridge started behaving correctly and became in sync. Now, after the hot plug operation on the same slot, all the bridge ports and devices behind the bridge become hot-plugged/online/restored in the same manner as it was before the hot-unplug operation.

Krishna Kumar (2):
pci/hotplug/pnv_php: Fix hotplug driver crash on Powernv
powerpc: hotplug driver bridge support

 arch/powerpc/include/asm/ppc-pci.h |  4 ++++
 arch/powerpc/kernel/pci-hotplug.c  |  5 ++---
 arch/powerpc/kernel/pci_dn.c       | 32 ++++++++++++++++++++++++++++++
 drivers/pci/hotplug/pnv_php.c      |  3 +--
 4 files changed, 39 insertions(+), 5 deletions(-)

Changelog:
==========

v3: 24 June 2024
- Removed the DPC keyword from description in cover letter and patch2

v2: 14 May 2024
- Used of_property_read_u32() in place of of_get_property() and

    of_read_number(). [patch2]

- Removed some unnecessary variable and changed the function return type from void* to void. [patch2]
- Removed the export declaration of pci_traverse_sibling_nodes_and_scan_slot() as its not needed. [patch2]

Signature · 1 line

2.45.0

Krishna Kumar

author

2024-06-24 12:11 UTC #1 lore raw

PATCH v3 1/2 pci/hotplug/pnv_php: Fix hotplug driver crash on Powernv

Description of the problem: The hotplug driver for powerpc (pci/hotplug/pnv_php.c) gives kernel crash when we try to hot-unplug/disable the PCIe switch/bridge from the PHB.

Root Cause of Crash: The crash is due to the reason that, though the msi data structure has been released during disable/hot-unplug path and it has been assigned with NULL, still during unregistartion the code was again trying to explicitly disable the msi which causes the Null pointer dereference and kernel crash.

Proposed Fix : The fix is to correct the check during unregistration path so that the code should not try to invoke pci_disable_msi/msix() if its data structure is already freed.

Cc Michael Ellerman <[email protected]>

Cc Nicholas Piggin <[email protected]>

Cc Christophe Leroy <[email protected]>

Cc "Aneesh Kumar K.V" <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Gaurav Batra <[email protected]>

Cc Nathan Lynch <[email protected]>

Cc Brian King <[email protected]>

Signed-off-by Krishna Kumar <[email protected]>

---
 drivers/pci/hotplug/pnv_php.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/pci/hotplug/pnv_php.c +1 -2

index 694349be9d0a..573a41869c15 100644
@@ -40,7 +40,6 @@ static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
 				bool disable_device)
 {
 	struct pci_dev *pdev = php_slot->pdev;
-	int irq = php_slot->irq;
 	u16 ctrl;
 
 	if (php_slot->irq > 0) {

@@ -59,7 +58,7 @@ static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
 		php_slot->wq = NULL;
 	}
 
-	if (disable_device || irq > 0) {
+	if (disable_device) {
 		if (pdev->msix_enabled)
 			pci_disable_msix(pdev);
 		else if (pdev->msi_enabled)

Signature · 1 line

2.45.0

Bjorn Helgaas

Acked-by on v3 1/2

2024-06-26 15:21 UTC #2 lore raw

Re: [PATCH v3 1/2] pci/hotplug/pnv_php: Fix hotplug driver crash on Powernv

I expect this series would go through the powerpc tree since that's where most of the chance is.

Quoted 9 lines · Krishna Kumar

Description of the problem: The hotplug driver for powerpc (pci/hotplug/pnv_php.c) gives kernel crash when we try to hot-unplug/disable the PCIe switch/bridge from the PHB.
Root Cause of Crash: The crash is due to the reason that, though the msi data structure has been released during disable/hot-unplug path and it has been assigned with NULL, still during unregistartion the code was again trying to explicitly disable the msi which causes the Null pointer dereference and kernel crash.

s/unregistartion/unregistration/
s/Null/NULL/ to match previous use
s/msi/MSI/ to match spec usage

Quoted 3 lines

Proposed Fix : The fix is to correct the check during unregistration path so that the code should not try to invoke pci_disable_msi/msix() if its data structure is already freed.

s/Proposed Fix : The fix is to// ... Just say what the patch does.

If/when the powerpc folks like this, add my:

Acked-by Bjorn Helgaas <[email protected]>

Quoted 38 lines

Cc Michael Ellerman <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Christophe Leroy <[email protected]>
Cc "Aneesh Kumar K.V" <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Gaurav Batra <[email protected]>
Cc Nathan Lynch <[email protected]>
Cc Brian King <[email protected]>
Signed-off-by Krishna Kumar <[email protected]>
---
 drivers/pci/hotplug/pnv_php.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/pci/hotplug/pnv_php.c +1 -2
index 694349be9d0a..573a41869c15 100644
@@ -40,7 +40,6 @@ static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
 				bool disable_device)
 {
 	struct pci_dev *pdev = php_slot->pdev;
-	int irq = php_slot->irq;
 	u16 ctrl;
 
 	if (php_slot->irq > 0) {
@@ -59,7 +58,7 @@ static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
 		php_slot->wq = NULL;
 	}
 
-	if (disable_device || irq > 0) {
+	if (disable_device) {
 		if (pdev->msix_enabled)
 			pci_disable_msix(pdev);
 		else if (pdev->msi_enabled)
Signature · 1 line
2.45.0

Michael Ellerman

reviewer

2024-06-28 05:34 UTC #3 lore raw

Re: [PATCH v3 1/2] pci/hotplug/pnv_php: Fix hotplug driver crash on Powernv

Quoted 2 lines · Bjorn Helgaas

I expect this series would go through the powerpc tree since that's where most of the chance is.

Thanks, yeah I'll plan to merge v4 with your comments addressed.

cheers

Quoted 63 lines · Krishna Kumar

Quoted 9 lines
Description of the problem: The hotplug driver for powerpc (pci/hotplug/pnv_php.c) gives kernel crash when we try to hot-unplug/disable the PCIe switch/bridge from the PHB.
Root Cause of Crash: The crash is due to the reason that, though the msi data structure has been released during disable/hot-unplug path and it has been assigned with NULL, still during unregistartion the code was again trying to explicitly disable the msi which causes the Null pointer dereference and kernel crash.
s/unregistartion/unregistration/
s/Null/NULL/ to match previous use
s/msi/MSI/ to match spec usage
Quoted 3 lines
Proposed Fix : The fix is to correct the check during unregistration path so that the code should not try to invoke pci_disable_msi/msix() if its data structure is already freed.
s/Proposed Fix : The fix is to// ... Just say what the patch does.
If/when the powerpc folks like this, add my:
Acked-by Bjorn Helgaas <[email protected]>
Quoted 38 lines
Cc Michael Ellerman <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Christophe Leroy <[email protected]>
Cc "Aneesh Kumar K.V" <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Gaurav Batra <[email protected]>
Cc Nathan Lynch <[email protected]>
Cc Brian King <[email protected]>
Signed-off-by Krishna Kumar <[email protected]>
---
 drivers/pci/hotplug/pnv_php.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/pci/hotplug/pnv_php.c +1 -2
index 694349be9d0a..573a41869c15 100644
@@ -40,7 +40,6 @@ static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
 				bool disable_device)
 {
 	struct pci_dev *pdev = php_slot->pdev;
-	int irq = php_slot->irq;
 	u16 ctrl;
 
 	if (php_slot->irq > 0) {
@@ -59,7 +58,7 @@ static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
 		php_slot->wq = NULL;
 	}
 
-	if (disable_device || irq > 0) {
+	if (disable_device) {
 		if (pdev->msix_enabled)
 			pci_disable_msix(pdev);
 		else if (pdev->msi_enabled)
Signature · 1 line
2.45.0

Shawn Anastasio

Tested-by on v3 1/2

2024-06-27 17:08 UTC #4 lore raw

Re: [PATCH v3 1/2] pci/hotplug/pnv_php: Fix hotplug driver crash on Powernv

In reply to Krishna Kumar

Hi Krishna,

Quoted 23 lines · Krishna Kumar

Description of the problem: The hotplug driver for powerpc (pci/hotplug/pnv_php.c) gives kernel crash when we try to hot-unplug/disable the PCIe switch/bridge from the PHB.
Root Cause of Crash: The crash is due to the reason that, though the msi data structure has been released during disable/hot-unplug path and it has been assigned with NULL, still during unregistartion the code was again trying to explicitly disable the msi which causes the Null pointer dereference and kernel crash.
Proposed Fix : The fix is to correct the check during unregistration path so that the code should not try to invoke pci_disable_msi/msix() if its data structure is already freed.
Cc Michael Ellerman <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Christophe Leroy <[email protected]>
Cc "Aneesh Kumar K.V" <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Gaurav Batra <[email protected]>
Cc Nathan Lynch <[email protected]>
Cc Brian King <[email protected]>

Quoted 1 line

Signed-off-by Krishna Kumar <[email protected]>

As with v1, I can confirm that this patch solves the panic encountered when hotplugging PCIe bridges on POWER9.

Tested-by Shawn Anastasio <[email protected]>

Thanks,
Shawn

Michael Ellerman

reviewer

2024-06-28 04:48 UTC #5 lore raw

Re: [PATCH v3 1/2] pci/hotplug/pnv_php: Fix hotplug driver crash on Powernv

Quoted 26 lines · Shawn Anastasio

Hi Krishna,
Quoted 23 lines · Krishna Kumar
Description of the problem: The hotplug driver for powerpc (pci/hotplug/pnv_php.c) gives kernel crash when we try to hot-unplug/disable the PCIe switch/bridge from the PHB.
Root Cause of Crash: The crash is due to the reason that, though the msi data structure has been released during disable/hot-unplug path and it has been assigned with NULL, still during unregistartion the code was again trying to explicitly disable the msi which causes the Null pointer dereference and kernel crash.
Proposed Fix : The fix is to correct the check during unregistration path so that the code should not try to invoke pci_disable_msi/msix() if its data structure is already freed.
Cc Michael Ellerman <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Christophe Leroy <[email protected]>
Cc "Aneesh Kumar K.V" <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Gaurav Batra <[email protected]>
Cc Nathan Lynch <[email protected]>
Cc Brian King <[email protected]>

Quoted 4 lines

Quoted 1 line
Signed-off-by Krishna Kumar <[email protected]>
As with v1, I can confirm that this patch solves the panic encountered when hotplugging PCIe bridges on POWER9.

Was the panic reported anywhere? So we can link to the report in the commit.

cheers

Shawn Anastasio

2024-06-28 19:22 UTC #6 lore raw

Re: [PATCH v3 1/2] pci/hotplug/pnv_php: Fix hotplug driver crash on Powernv

Hi Michael,

Quoted 2 lines · Michael Ellerman

Was the panic reported anywhere? So we can link to the report in the commit.

It was indeed -- here is the link to the thread from last December:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-December/267034.html

Quoted 1 line

cheers

Thanks,
Shawn

Michael Ellerman

reviewer

2024-06-29 07:30 UTC #7 lore raw

Re: [PATCH v3 1/2] pci/hotplug/pnv_php: Fix hotplug driver crash on Powernv

Quoted 8 lines · Shawn Anastasio

Hi Michael,
Quoted 2 lines · Michael Ellerman
Was the panic reported anywhere? So we can link to the report in the commit.
It was indeed -- here is the link to the thread from last December:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-December/267034.html

Thanks.

cheers

Krishna Kumar

author

2024-06-24 12:11 UTC #8 lore raw

PATCH v3 2/2 powerpc: hotplug driver bridge support

In reply to Krishna Kumar

There is an issue with the hotplug operation when it's done on the bridge/switch slot. The bridge-port and devices behind the bridge, which become offline by hot-unplug operation, don't get hot-plugged/enabled by doing hot-plug operation on that slot. Only the first port of the bridge gets enabled and the remaining port/devices remain unplugged. The hot plug/unplug operation is done by the hotplug driver (drivers/pci/hotplug/pnv_php.c).

Root Cause Analysis: This behavior is due to missing code for the switch/bridge. The existing driver depends on pci_hp_add_devices() function for device enablement. This function calls pci_scan_slot() on only one device-node/port of the bridge, not on all the siblings' device-node/port.

The missing code needs to be added which will find all the sibling device-nodes/bridge-ports and will run explicit pci_scan_slot() on those. A new function has been added for this purpose which gets invoked from pci_hp_add_devices(). This new function pci_traverse_sibling_nodes_and_scan_slot() gets all the sibling bridge-ports by traversal and explicitly invokes pci_scan_slot on them.

Cc Michael Ellerman <[email protected]>

Cc Nicholas Piggin <[email protected]>

Cc Christophe Leroy <[email protected]>

Cc "Aneesh Kumar K.V" <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Gaurav Batra <[email protected]>

Cc Nathan Lynch <[email protected]>

Cc Brian King <[email protected]>

Signed-off-by Krishna Kumar <[email protected]>

---
 arch/powerpc/include/asm/ppc-pci.h |  4 ++++
 arch/powerpc/kernel/pci-hotplug.c  |  5 ++---
 arch/powerpc/kernel/pci_dn.c       | 32 ++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 3 deletions(-)

arch/powerpc/include/asm/ppc-pci.h +4 -0

index a8b7e8682f5b..83db8d0798ac 100644
@@ -28,6 +28,10 @@ struct pci_dn;
 void *pci_traverse_device_nodes(struct device_node *start,
 				void *(*fn)(struct device_node *, void *),
 				void *data);
+
+void pci_traverse_sibling_nodes_and_scan_slot(struct device_node *start,
+					       struct pci_bus *bus);
+
 extern void pci_devs_phb_init_dynamic(struct pci_controller *phb);
 
 #if defined(CONFIG_IOMMU_API) && (defined(CONFIG_PPC_PSERIES) || \

arch/powerpc/kernel/pci-hotplug.c +2 -3

index 0fe251c6ac2c..639a3d592fe2 100644
@@ -106,7 +106,7 @@ EXPORT_SYMBOL_GPL(pci_hp_remove_devices);
  */
 void pci_hp_add_devices(struct pci_bus *bus)
 {
-	int slotno, mode, max;
+	int mode, max;
 	struct pci_dev *dev;
 	struct pci_controller *phb;
 	struct device_node *dn = pci_bus_to_OF_node(bus);

@@ -129,8 +129,7 @@ void pci_hp_add_devices(struct pci_bus *bus)
 		 * order for fully rescan all the way down to pick them up.
 		 * They can have been removed during partial hotplug.
 		 */
-		slotno = PCI_SLOT(PCI_DN(dn->child)->devfn);
-		pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
+		pci_traverse_sibling_nodes_and_scan_slot(dn, bus);
 		max = bus->busn_res.start;
 		/*
 		 * Scan bridges that are already configured. We don't touch

arch/powerpc/kernel/pci_dn.c +32 -0

index 38561d6a2079..bea612759832 100644
@@ -493,4 +493,36 @@ static void pci_dev_pdn_setup(struct pci_dev *pdev)
 	pdn = pci_get_pdn(pdev);
 	pdev->dev.archdata.pci_data = pdn;
 }
+
+void pci_traverse_sibling_nodes_and_scan_slot(struct device_node *start, struct pci_bus *bus)
+{
+	struct device_node *dn;
+	int slotno;
+
+	u32 class = 0;
+
+	if (!of_property_read_u32(start->child, "class-code", &class)) {
+		/* Call of pci_scan_slot for non-bridge/EP case */
+		if (!((class >> 8) == PCI_CLASS_BRIDGE_PCI)) {
+			slotno = PCI_SLOT(PCI_DN(start->child)->devfn);
+			pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
+			return;
+		}
+	}
+
+	/* Iterate all siblings */
+	for_each_child_of_node(start, dn) {
+		class = 0;
+
+		if (!of_property_read_u32(start->child, "class-code", &class)) {
+			/* Call of pci_scan_slot on each sibling-nodes/bridge-ports */
+			if ((class >> 8) == PCI_CLASS_BRIDGE_PCI) {
+				slotno = PCI_SLOT(PCI_DN(dn)->devfn);
+				pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
+			}
+		}
+	}
+
+}
+
 DECLARE_PCI_FIXUP_EARLY(PCI_ANY_ID, PCI_ANY_ID, pci_dev_pdn_setup);

Signature · 1 line

2.45.0

Shawn Anastasio

Tested-by on v3 2/2

2024-06-27 17:11 UTC #9 lore raw

Re: [PATCH v3 2/2] powerpc: hotplug driver bridge support

Hi Krishna,

Quoted 30 lines · Krishna Kumar

There is an issue with the hotplug operation when it's done on the bridge/switch slot. The bridge-port and devices behind the bridge, which become offline by hot-unplug operation, don't get hot-plugged/enabled by doing hot-plug operation on that slot. Only the first port of the bridge gets enabled and the remaining port/devices remain unplugged. The hot plug/unplug operation is done by the hotplug driver (drivers/pci/hotplug/pnv_php.c).
Root Cause Analysis: This behavior is due to missing code for the switch/bridge. The existing driver depends on pci_hp_add_devices() function for device enablement. This function calls pci_scan_slot() on only one device-node/port of the bridge, not on all the siblings' device-node/port.
The missing code needs to be added which will find all the sibling device-nodes/bridge-ports and will run explicit pci_scan_slot() on those. A new function has been added for this purpose which gets invoked from pci_hp_add_devices(). This new function pci_traverse_sibling_nodes_and_scan_slot() gets all the sibling bridge-ports by traversal and explicitly invokes pci_scan_slot on them.
Cc Michael Ellerman <[email protected]>
Cc Nicholas Piggin <[email protected]>
Cc Christophe Leroy <[email protected]>
Cc "Aneesh Kumar K.V" <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Gaurav Batra <[email protected]>
Cc Nathan Lynch <[email protected]>
Cc Brian King <[email protected]>

Quoted 1 line

Signed-off-by Krishna Kumar <[email protected]>

Other than the case with NVMe devices failing that we discussed in v1's thread, I can confirm that this patch resolves many of the issues we've encountered with PCIe hotplug on POWER9.

Tested-by Shawn Anastasio <[email protected]>

Thanks,
Shawn