[PATCH v2 0/3] ppc/pnv: Fix panics and refactor pnv_php.c

[PATCH v2 0/3] ppc/pnv: Fix panics and refactor pnv_php.c

[Aditya Gupta]

Overview
========

Since below upstream commit, powernv systems get a panic during boot when an
OpenCAPI hotplug slot is detected:

  commit 80f9fc2362797538ebd4fd70a1dfa838cc2c2cdb
  Date:   Tue Jul 15 16:36:55 2025 -0500
  
      PCI: pnv_php: Work around switches with broken presence detection

The panic log is present in patch #1

Patch #1: Adds null checks around null pointer dereferences
Patch #2: Rework the driver code to separate PCIe-specific and common code
Patch #3 (optional): Group PCIe functionality together, only code movements

Note: Patch 1 is important and independent of the other patches

Note2: Patch 2 & 3 can be squashed into 1 commit. Intentionally kept patch #2 as
small as possible for ease of review.

Changelog
=========
v1 -> v2:
* Sashiko pointed out various null pointer dereferences including a kernel panic triggered by userspace, fixed them
* Add rework patches, so that PCIe-specific and common code can be separated
  cleanly

Aditya Gupta (3):
  ppc/pnv: Add null checks for OpenCapi PHBs
  ppc/pnv: Refactor PNV PCI hotplug driver
  ppc/pnv: Refactor PNV PCI Hotplug to group PCIe functions

 arch/powerpc/include/asm/pnv-pci.h |  16 +
 drivers/pci/hotplug/pnv_php.c      | 554 ++++++++++++++++-------------
 2 files changed, 328 insertions(+), 242 deletions(-)

-- 
2.54.0

[PATCH v2 1/3] ppc/pnv: Add null checks for OpenCapi PHBs

[Aditya Gupta]

For opencapi phb direct slots, the .pdev for php_slots will be NULL

Various sections of the code in pnv_php can do a null dereference and
crash the kernel.

Originally, the issue was hit during boot:

    [    1.568588] PowerPC PowerNV PCI Hotplug Driver version: 0.1
    [    1.569722] BUG: Kernel NULL pointer dereference at 0x00000074
    [    1.569811] Faulting instruction address: 0xc000000000b75fd0
    [    1.569890] Oops: Kernel access of bad area, sig: 11 [#1]
    [    1.569963] LE PAGE_SIZE=64K MMU=Hash  SMP NR_CPUS=2048 NUMA PowerNV
    ...
    [    1.571492] NIP [c000000000b75fd0] pnv_php_get_adapter_state+0x60/0x154
    [    1.571604] LR [c000000000b75fbc] pnv_php_get_adapter_state+0x4c/0x154
    [    1.571690] Call Trace:
    [    1.571725] [c000c0000688f990] [c000000000b75fbc] pnv_php_get_adapter_state+0x4c/0x154 (unreliable)
    [    1.571783] [c000c0000688fa20] [c000000000b78bd0] pnv_php_enable+0x94/0x378
    [    1.571951] [c000c0000688fac0] [c000000000b7912c] pnv_php_register_one.isra.0+0x11c/0x1e0

This occurs for hotplug slots on root buses where bus->self == NULL,
such as OpenCAPI PHB direct slots. An added debug print (not part of
this patch) confirmed it was opencapi:

    [    1.617227] pnv_php: slot 'OPENCAPI-0009' has NULL pdev (bus 0009:00, parent=NO (root bus))
    [    1.617308] pnv_php: slot 'OPENCAPI-0009' dn->full_name='pciex@603a000000000', compatible='ibm,power10-pau-opencapi-pciex'

This only required null check in 'pnv_php_get_adapter_state', which
caused the kernel to boot.

Even with 'pnv_php_get_adapter_state' null check, there are more
possible null dereferences pointed by sashiko, including cases where
userspace crashes the kernel, such as:

    $ cat /sys/bus/pci/slots/*/attention
    ...
    [  557.036295] Kernel attempted to read user page (6e) - exploit attempt? (uid: 0)
    [  557.036354] BUG: Kernel NULL pointer dereference on read at 0x0000006e
    [  557.036383] Faulting instruction address: 0xc000000000a83334
    [  557.036413] Oops: Kernel access of bad area, sig: 11 [#1]
    [  557.036449] LE PAGE_SIZE=64K MMU=Hash  SMP NR_CPUS=2048 NUMA PowerNV
    ...
    [  557.037749] [c000000046707a20] [c000000046707b90] 0xc000000046707b90 (unreliable)
    [  557.037795] [c000000046707a70] [0000000000000001] 0x1
    [  557.037850] [c000000046707ab0] [c000000000acb00c] attention_read_file+0x54/0xa8
    [  557.037910] [c000000046707b30] [c000000000abfbfc] pci_slot_attr_show+0x3c/0x58
    [  557.037977] [c000000046707b50] [c0000000008181ec] sysfs_kf_seq_show+0xd4/0x204
    [  557.038022] [c000000046707be0] [c000000000815004] kernfs_seq_show+0x44/0x58

Add null checks to prevent the null dereferences.

Cc: stable@vger.kernel.org
Fixes: 80f9fc236279 ("PCI: pnv_php: Work around switches with broken presence detection")
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 drivers/pci/hotplug/pnv_php.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
index ff92a5c301b8..d0f5e8ad1f71 100644
--- a/drivers/pci/hotplug/pnv_php.c
+++ b/drivers/pci/hotplug/pnv_php.c
@@ -47,6 +47,9 @@ static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
 	struct pci_dev *pdev = php_slot->pdev;
 	u16 ctrl;
 
+	if (!pdev)
+		return;
+
 	if (php_slot->irq > 0) {
 		pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
 		ctrl &= ~(PCI_EXP_SLTCTL_HPIE |
@@ -414,7 +417,8 @@ static int pnv_php_get_adapter_state(struct hotplug_slot *slot, u8 *state)
 	 */
 	ret = pnv_pci_get_presence_state(php_slot->id, &presence);
 	if (ret >= 0) {
-		if (pci_pcie_type(php_slot->pdev) == PCI_EXP_TYPE_DOWNSTREAM &&
+		if (php_slot->pdev &&
+			pci_pcie_type(php_slot->pdev) == PCI_EXP_TYPE_DOWNSTREAM &&
 			presence == OPAL_PCI_SLOT_EMPTY) {
 			/*
 			 * Similar to pciehp_hpc, check whether the Link Active
@@ -442,6 +446,11 @@ static int pnv_php_get_raw_indicator_status(struct hotplug_slot *slot, u8 *state
 	struct pci_dev *bridge = php_slot->pdev;
 	u16 status;
 
+	if (!bridge) {
+		*state = 0;
+		return 0;
+	}
+
 	pcie_capability_read_word(bridge, PCI_EXP_SLTCTL, &status);
 	*state = (status & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6;
 	return 0;
@@ -514,11 +523,13 @@ static int pnv_php_activate_slot(struct pnv_php_slot *php_slot,
 			 * fence / freeze.
 			 */
 			SLOT_WARN(php_slot, "Try %d...\n", i + 1);
-			pci_set_pcie_reset_state(php_slot->pdev,
-						 pcie_warm_reset);
-			msleep(250);
-			pci_set_pcie_reset_state(php_slot->pdev,
-						 pcie_deassert_reset);
+			if (php_slot->pdev) {
+				pci_set_pcie_reset_state(php_slot->pdev,
+							 pcie_warm_reset);
+				msleep(250);
+				pci_set_pcie_reset_state(php_slot->pdev,
+							 pcie_deassert_reset);
+			}
 
 			ret = pnv_php_set_slot_power_state(
 				slot, OPAL_PCI_SLOT_POWER_ON);
@@ -911,6 +922,9 @@ pnv_php_detect_clear_suprise_removal_freeze(struct pnv_php_slot *php_slot)
 	struct eeh_pe *pe;
 	int i, rc;
 
+	if (!pdev)
+		return;
+
 	/*
 	 * When a device is surprise removed from a downstream bridge slot,
 	 * the upstream bridge port can still end up frozen due to related EEH
@@ -1093,6 +1107,9 @@ static void pnv_php_enable_irq(struct pnv_php_slot *php_slot)
 	struct pci_dev *pdev = php_slot->pdev;
 	int irq, ret;
 
+	if (!pdev)
+		return;
+
 	/*
 	 * The MSI/MSIx interrupt might have been occupied by other
 	 * drivers. Don't populate the surprise hotplug capability
-- 
2.54.0

Re: [PATCH v2 1/3] ppc/pnv: Add null checks for OpenCapi PHBs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Pre-existing NULL pointer dereference in the error path of `pnv_php_alloc_slot()` due to an uninitialized bus pointer.
- [High] Unbalanced `pci_enable_device()` refcount leak during setup failures and normal teardown.
- [High] Concurrent accesses to `pnv_php_enable_slot` and `pnv_php_disable_slot` can cause double-frees due to lack of state locking.
- [Medium] Improper error code in `pnv_php_reset_slot()` prematurely aborts PCI reset method discovery.
--

commit 534a90517227ad6163f935501ac2f0ed9cf60b83
Author: Aditya Gupta <adityag@linux.ibm.com>

ppc/pnv: Add null checks for OpenCapi PHBs

This patch adds null checks for php_slot->pdev across several PCI hotplug
operations in pnv_php.c. It prevents NULL pointer dereferences for OpenCAPI
PHB direct slots where the bus has no self device, fixing kernel crashes
observed during boot and hotplug events.

Cc: stable@vger.kernel.org
Fixes: 80f9fc236279 ("PCI: pnv_php: Work around switches with broken presence detection")

> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
> index ff92a5c301b81..d0f5e8ad1f716 100644
> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c
> @@ -47,6 +47,9 @@ static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
>  	struct pci_dev *pdev = php_slot->pdev;
>  	u16 ctrl;
>  
> +	if (!pdev)
> +		return;
> +
>  	if (php_slot->irq > 0) {
>  		pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
>  		ctrl &= ~(PCI_EXP_SLTCTL_HPIE |
>  			  PCI_EXP_SLTCTL_PDCE |
>  			  PCI_EXP_SLTCTL_DLLSCE);
>  		pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, ctrl);
>  
>  		free_irq(php_slot->irq, php_slot);

[Severity: High]
This is a pre-existing issue, but can concurrent accesses to the slot
operations cause a double-free here?

If pnv_php_disable_slot() is called concurrently by two threads (for example,
via sysfs writes to the power attribute racing with the workqueue event
handler), both threads can bypass the state check because php_slot->state is
not locked.

They could both execute pnv_php_disable_all_downstream_irqs() and reach
this point. Since both would see php_slot->irq > 0, they would both call
free_irq(php_slot->irq, php_slot).

Should there be some synchronization around the state transitions?

[ ... ]

> @@ -1093,6 +1107,9 @@ static void pnv_php_enable_irq(struct pnv_php_slot *php_slot)
>  	struct pci_dev *pdev = php_slot->pdev;
>  	int irq, ret;
>  
> +	if (!pdev)
> +		return;
> +
>  	/*
>  	 * The MSI/MSIx interrupt might have been occupied by other
>  	 * drivers. Don't populate the surprise hotplug capability
>  	 * in that case.
>  	 */
>  	if (pci_dev_msi_enabled(pdev))
>  		return;
>  
>  	ret = pci_enable_device(pdev);
>  	if (ret) {
>  		SLOT_WARN(php_slot, "Error %d enabling device\n", ret);
>  		return;
>  	}

[Severity: High]
This is a pre-existing issue, but does this leak the enable_cnt refcount?

pci_enable_device(pdev) increments the device's enable count. If the
subsequent IRQ initialization steps fail, this function returns without
calling pci_disable_device().

Additionally, during normal slot teardown, pnv_php_disable_irq() is called
with disable_device = false, meaning pci_disable_device() is skipped during
hot-unplug. This would permanently prevent the device from entering low-power
states and block clean driver unbinding.

Is there a missing pci_disable_device() in the error paths and unplug flow?

While reviewing this code, I also noticed a few other pre-existing issues in
this file:

drivers/pci/hotplug/pnv_php.c:pnv_php_reset_slot() {
    ...
	/*
	 * The CAPI folks want pnv_php to drive OpenCAPI slots
	 * which don't have a bridge. Only claim to support
	 * reset_slot() if we have a bridge device (for now...)
	 */
	if (probe)
		return !bridge;
    ...
}

[Severity: Medium]
This is a pre-existing issue, but does returning 1 here abort the PCI reset
method discovery loop prematurely?

The PCI core's __pci_reset_function_locked() expects -ENOTTY to indicate a
reset method is unsupported. When !bridge evaluates to 1, the PCI core sees
rc != -ENOTTY and aborts the entire loop, which prevents falling back to
other reset methods like FLR or PM reset.

---

drivers/pci/hotplug/pnv_php.c:pnv_php_alloc_slot() {
    ...
	if (!php_slot->wq) {
		SLOT_WARN(php_slot, "Cannot alloc workqueue\n");
		goto free_id;
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but could this SLOT_WARN cause a NULL pointer
dereference?

The php_slot structure is zero-initialized on allocation. If
alloc_workqueue() fails, SLOT_WARN evaluates php_slot->pdev (which is NULL)
and takes the false branch to use &php_slot->bus->dev.

However, php_slot->bus has not been assigned yet. Computing &(NULL)->dev
yields an invalid pointer offset that is passed to the underlying dev_warn()
function, which will crash when it attempts to dereference it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527180816.2749186-1-adityag@linux.ibm.com?part=1

[PATCH v2 2/3] ppc/pnv: Refactor PNV PCI hotplug driver

[Aditya Gupta]

Currently the pnv_php driver handles both PCIe and OpenCAPI slots.

The slots has many common functionality, but many operations are pcie
specific, and assume the slot having a parent device, which isn't the
case with opencapi slots

This requires handling the case of parent device being NULL, at many
places, which can be hard to maintain and add code to.

Instead, have PCIe/OpenCAPI operations as .backend_ops in pnv_php_slot,
so that PCIe code is cleanly separated.
With this, future patches can just edit the PCIe/OpenCAPI specific ops,
instead of editing the common code.

No functional change is intended other than reset_slot for OpenCAPI
slots returning -ENODEV when not probing.

Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 arch/powerpc/include/asm/pnv-pci.h |  16 +++
 drivers/pci/hotplug/pnv_php.c      | 160 +++++++++++++++++++----------
 2 files changed, 123 insertions(+), 53 deletions(-)

diff --git a/arch/powerpc/include/asm/pnv-pci.h b/arch/powerpc/include/asm/pnv-pci.h
index 7e9a479951a3..f1020f1e61cd 100644
--- a/arch/powerpc/include/asm/pnv-pci.h
+++ b/arch/powerpc/include/asm/pnv-pci.h
@@ -27,6 +27,9 @@ extern int pnv_pci_set_power_state(uint64_t id, uint8_t state,
 int64_t pnv_opal_pci_msi_eoi(struct irq_data *d);
 bool is_pnv_opal_msi(struct irq_chip *chip);
 
+/* To be set for hotplug operations for PCIe/OpenCAPI */
+struct pnv_php_backend_ops;
+
 struct pnv_php_slot {
 	struct hotplug_slot		slot;
 	uint64_t			id;
@@ -50,10 +53,23 @@ struct pnv_php_slot {
 	void				*fdt;
 	void				*dt;
 	struct of_changeset		ocs;
+	const struct pnv_php_backend_ops *backend_ops;
 	struct pnv_php_slot		*parent;
 	struct list_head		children;
 	struct list_head		link;
 };
+
+struct pnv_php_backend_ops {
+	void (*enable_irq)(struct pnv_php_slot *slot);
+	void (*disable_irq)(struct pnv_php_slot *slot, bool disable_device, bool disable_msi);
+	void (*fixup_presence_state)(struct pnv_php_slot *slot, u8 *presence);
+	void  (*get_attention_state)(struct pnv_php_slot *slot, u8 *state);
+	void  (*set_attention_state)(struct pnv_php_slot *slot, u8 state);
+	void (*fundamental_reset)(struct pnv_php_slot *slot);
+	void (*detect_surprise_removal)(struct pnv_php_slot *slot);
+	int  (*reset_slot)(struct pnv_php_slot *slot, bool probe);
+};
+
 extern struct pnv_php_slot *pnv_php_find_slot(struct device_node *dn);
 extern int pnv_php_set_slot_power_state(struct hotplug_slot *slot,
 					uint8_t state);
diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
index d0f5e8ad1f71..997412eea486 100644
--- a/drivers/pci/hotplug/pnv_php.c
+++ b/drivers/pci/hotplug/pnv_php.c
@@ -39,17 +39,53 @@ static void pnv_php_register(struct device_node *dn);
 static void pnv_php_unregister_one(struct device_node *dn);
 static void pnv_php_unregister(struct device_node *dn);
 
-static void pnv_php_enable_irq(struct pnv_php_slot *php_slot);
+static void pcie_enable_irq(struct pnv_php_slot *php_slot);
+static int pcie_check_link_active(struct pci_dev *pdev);
+static void pcie_detect_surprise_removal(struct pnv_php_slot *php_slot);
+
+static void pcie_fixup_presence_state(struct pnv_php_slot *php_slot, u8 *presence)
+{
+	if (pci_pcie_type(php_slot->pdev) == PCI_EXP_TYPE_DOWNSTREAM &&
+	    *presence == OPAL_PCI_SLOT_EMPTY) {
+		/*
+		 * Similar to pciehp_hpc, check whether the Link Active
+		 * bit is set to account for broken downstream bridges
+		 * that don't properly assert Presence Detect State, as
+		 * was observed on the Microsemi Switchtec PM8533 PFX
+		 * [11f8:8533].
+		 */
+		if (pcie_check_link_active(php_slot->pdev) > 0)
+			*presence = OPAL_PCI_SLOT_PRESENT;
+	}
+}
+
+static void pcie_fundamental_reset(struct pnv_php_slot *php_slot)
+{
+	pci_set_pcie_reset_state(php_slot->pdev, pcie_warm_reset);
+	msleep(250);
+	pci_set_pcie_reset_state(php_slot->pdev, pcie_deassert_reset);
+}
+
+static void pnv_php_enable_irq(struct pnv_php_slot *php_slot)
+{
+	if (php_slot->backend_ops->enable_irq)
+		php_slot->backend_ops->enable_irq(php_slot);
+}
 
 static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
 				bool disable_device, bool disable_msi)
+{
+	if (php_slot->backend_ops->disable_irq)
+		php_slot->backend_ops->disable_irq(php_slot, disable_device,
+				disable_msi);
+}
+
+static void pcie_disable_irq(struct pnv_php_slot *php_slot,
+				bool disable_device, bool disable_msi)
 {
 	struct pci_dev *pdev = php_slot->pdev;
 	u16 ctrl;
 
-	if (!pdev)
-		return;
-
 	if (php_slot->irq > 0) {
 		pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
 		ctrl &= ~(PCI_EXP_SLTCTL_HPIE |
@@ -417,19 +453,8 @@ static int pnv_php_get_adapter_state(struct hotplug_slot *slot, u8 *state)
 	 */
 	ret = pnv_pci_get_presence_state(php_slot->id, &presence);
 	if (ret >= 0) {
-		if (php_slot->pdev &&
-			pci_pcie_type(php_slot->pdev) == PCI_EXP_TYPE_DOWNSTREAM &&
-			presence == OPAL_PCI_SLOT_EMPTY) {
-			/*
-			 * Similar to pciehp_hpc, check whether the Link Active
-			 * bit is set to account for broken downstream bridges
-			 * that don't properly assert Presence Detect State, as
-			 * was observed on the Microsemi Switchtec PM8533 PFX
-			 * [11f8:8533].
-			 */
-			if (pcie_check_link_active(php_slot->pdev) > 0)
-				presence = OPAL_PCI_SLOT_PRESENT;
-		}
+		if (php_slot->backend_ops->fixup_presence_state)
+			php_slot->backend_ops->fixup_presence_state(php_slot, &presence);
 
 		*state = presence;
 		ret = 0;
@@ -440,28 +465,24 @@ static int pnv_php_get_adapter_state(struct hotplug_slot *slot, u8 *state)
 	return ret;
 }
 
-static int pnv_php_get_raw_indicator_status(struct hotplug_slot *slot, u8 *state)
+static void pcie_get_attention_state(struct pnv_php_slot *php_slot, u8 *state)
 {
-	struct pnv_php_slot *php_slot = to_pnv_php_slot(slot);
 	struct pci_dev *bridge = php_slot->pdev;
 	u16 status;
 
-	if (!bridge) {
-		*state = 0;
-		return 0;
-	}
-
 	pcie_capability_read_word(bridge, PCI_EXP_SLTCTL, &status);
 	*state = (status & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6;
-	return 0;
 }
 
-
 static int pnv_php_get_attention_state(struct hotplug_slot *slot, u8 *state)
 {
 	struct pnv_php_slot *php_slot = to_pnv_php_slot(slot);
 
-	pnv_php_get_raw_indicator_status(slot, &php_slot->attention_state);
+	if (php_slot->backend_ops->get_attention_state)
+		php_slot->backend_ops->get_attention_state(php_slot, &php_slot->attention_state);
+	else
+		php_slot->attention_state = 0;
+
 	*state = php_slot->attention_state;
 	return 0;
 }
@@ -469,13 +490,19 @@ static int pnv_php_get_attention_state(struct hotplug_slot *slot, u8 *state)
 static int pnv_php_set_attention_state(struct hotplug_slot *slot, u8 state)
 {
 	struct pnv_php_slot *php_slot = to_pnv_php_slot(slot);
+
+	if (php_slot->backend_ops->set_attention_state)
+		php_slot->backend_ops->set_attention_state(php_slot, state);
+
+	return 0;
+}
+
+static void pcie_set_attention_state(struct pnv_php_slot *php_slot, u8 state)
+{
 	struct pci_dev *bridge = php_slot->pdev;
 	u16 new, mask;
 
 	php_slot->attention_state = state;
-	if (!bridge)
-		return 0;
-
 	mask = PCI_EXP_SLTCTL_AIC;
 
 	if (state)
@@ -484,8 +511,6 @@ static int pnv_php_set_attention_state(struct hotplug_slot *slot, u8 state)
 		new = PCI_EXP_SLTCTL_ATTN_IND_OFF;
 
 	pcie_capability_clear_and_set_word(bridge, PCI_EXP_SLTCTL, mask, new);
-
-	return 0;
 }
 
 static int pnv_php_activate_slot(struct pnv_php_slot *php_slot,
@@ -523,13 +548,8 @@ static int pnv_php_activate_slot(struct pnv_php_slot *php_slot,
 			 * fence / freeze.
 			 */
 			SLOT_WARN(php_slot, "Try %d...\n", i + 1);
-			if (php_slot->pdev) {
-				pci_set_pcie_reset_state(php_slot->pdev,
-							 pcie_warm_reset);
-				msleep(250);
-				pci_set_pcie_reset_state(php_slot->pdev,
-							 pcie_deassert_reset);
-			}
+			if (php_slot->backend_ops->fundamental_reset)
+				php_slot->backend_ops->fundamental_reset(php_slot);
 
 			ret = pnv_php_set_slot_power_state(
 				slot, OPAL_PCI_SLOT_POWER_ON);
@@ -633,16 +653,18 @@ static int pnv_php_enable(struct pnv_php_slot *php_slot, bool rescan)
 static int pnv_php_reset_slot(struct hotplug_slot *slot, bool probe)
 {
 	struct pnv_php_slot *php_slot = to_pnv_php_slot(slot);
+	if (php_slot->backend_ops->reset_slot)
+		return php_slot->backend_ops->reset_slot(php_slot, probe);
+	return probe ? 0 : -ENODEV;
+}
+
+static int pcie_reset_slot(struct pnv_php_slot *php_slot, bool probe)
+{
 	struct pci_dev *bridge = php_slot->pdev;
 	uint16_t sts;
 
-	/*
-	 * The CAPI folks want pnv_php to drive OpenCAPI slots
-	 * which don't have a bridge. Only claim to support
-	 * reset_slot() if we have a bridge device (for now...)
-	 */
 	if (probe)
-		return !bridge;
+		return 0;
 
 	/* mask our interrupt while resetting the bridge */
 	if (php_slot->irq > 0)
@@ -778,6 +800,26 @@ static void pnv_php_release(struct pnv_php_slot *php_slot)
 	pnv_php_put_slot(php_slot->parent);
 }
 
+static const struct pnv_php_backend_ops pnv_php_pcie_ops = {
+	.enable_irq              = pcie_enable_irq,
+	.disable_irq             = pcie_disable_irq,
+	.get_attention_state     = pcie_get_attention_state,
+	.set_attention_state     = pcie_set_attention_state,
+	.fixup_presence_state    = pcie_fixup_presence_state,
+	.fundamental_reset       = pcie_fundamental_reset,
+	.detect_surprise_removal = pcie_detect_surprise_removal,
+	.reset_slot              = pcie_reset_slot,
+};
+
+static int opencapi_reset_slot(struct pnv_php_slot *slot, bool probe)
+{
+	return probe ? 1 : -ENODEV;
+}
+
+static const struct pnv_php_backend_ops pnv_php_opencapi_ops = {
+	.reset_slot              = opencapi_reset_slot,
+};
+
 static struct pnv_php_slot *pnv_php_alloc_slot(struct device_node *dn)
 {
 	struct pnv_php_slot *php_slot;
@@ -830,6 +872,12 @@ static struct pnv_php_slot *pnv_php_alloc_slot(struct device_node *dn)
 	php_slot->power_state_check     = false;
 	php_slot->slot.ops              = &php_slot_ops;
 
+	/* OpenCAPI slots don't have a parent bridge */
+	if (php_slot->pdev)
+		php_slot->backend_ops = &pnv_php_pcie_ops;
+	else
+		php_slot->backend_ops = &pnv_php_opencapi_ops;
+
 	INIT_LIST_HEAD(&php_slot->children);
 	INIT_LIST_HEAD(&php_slot->link);
 
@@ -886,7 +934,7 @@ static int pnv_php_register_slot(struct pnv_php_slot *php_slot)
 	return 0;
 }
 
-static int pnv_php_enable_msix(struct pnv_php_slot *php_slot)
+static int pcie_enable_msix(struct pnv_php_slot *php_slot)
 {
 	struct pci_dev *pdev = php_slot->pdev;
 	struct msix_entry entry;
@@ -915,7 +963,13 @@ static int pnv_php_enable_msix(struct pnv_php_slot *php_slot)
 }
 
 static void
-pnv_php_detect_clear_suprise_removal_freeze(struct pnv_php_slot *php_slot)
+pnv_php_detect_clear_surprise_removal_freeze(struct pnv_php_slot *php_slot)
+{
+	if (php_slot->backend_ops->detect_surprise_removal)
+		php_slot->backend_ops->detect_surprise_removal(php_slot);
+}
+
+static void pcie_detect_surprise_removal(struct pnv_php_slot *php_slot)
 {
 	struct pci_dev *pdev = php_slot->pdev;
 	struct eeh_dev *edev;
@@ -972,7 +1026,7 @@ static void pnv_php_event_handler(struct work_struct *work)
 		pnv_php_enable_slot(&php_slot->slot);
 	} else {
 		pnv_php_disable_slot(&php_slot->slot);
-		pnv_php_detect_clear_suprise_removal_freeze(php_slot);
+		pnv_php_detect_clear_surprise_removal_freeze(php_slot);
 	}
 
 	kfree(event);
@@ -1055,7 +1109,7 @@ static irqreturn_t pnv_php_interrupt(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq)
+static void pcie_init_irq(struct pnv_php_slot *php_slot, int irq)
 {
 	struct pci_dev *pdev = php_slot->pdev;
 	u32 broken_pdc = 0;
@@ -1102,7 +1156,7 @@ static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq)
 	php_slot->irq = irq;
 }
 
-static void pnv_php_enable_irq(struct pnv_php_slot *php_slot)
+static void pcie_enable_irq(struct pnv_php_slot *php_slot)
 {
 	struct pci_dev *pdev = php_slot->pdev;
 	int irq, ret;
@@ -1127,9 +1181,9 @@ static void pnv_php_enable_irq(struct pnv_php_slot *php_slot)
 	pci_set_master(pdev);
 
 	/* Enable MSIx interrupt */
-	irq = pnv_php_enable_msix(php_slot);
+	irq = pcie_enable_msix(php_slot);
 	if (irq > 0) {
-		pnv_php_init_irq(php_slot, irq);
+		pcie_init_irq(php_slot, irq);
 		return;
 	}
 
@@ -1140,7 +1194,7 @@ static void pnv_php_enable_irq(struct pnv_php_slot *php_slot)
 	ret = pci_enable_msi(pdev);
 	if (!ret || pdev->irq) {
 		irq = pdev->irq;
-		pnv_php_init_irq(php_slot, irq);
+		pcie_init_irq(php_slot, irq);
 	}
 }
 
-- 
2.54.0

Re: [PATCH v2 2/3] ppc/pnv: Refactor PNV PCI hotplug driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unprotected state modifications and TOCTOU race in slot enable/disable paths.
- [High] Lockless access to pci_bus->devices list from hardirq context.
--

commit cf3f614d9d225b084eb92a8c44dcbe6b7a0d3932
Author: Aditya Gupta <adityag@linux.ibm.com>

ppc/pnv: Refactor PNV PCI hotplug driver

This patch refactors the PNV PCI hotplug driver to separate PCIe and OpenCAPI
slot operations into backend_ops. It isolates PCIe-specific code so future
updates can edit specific ops instead of common code.

> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
> index d0f5e8ad1f716..997412eea4863 100644
> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c

[ ... ]

> @@ -972,7 +1026,7 @@ static void pnv_php_event_handler(struct work_struct *work)
>  		pnv_php_enable_slot(&php_slot->slot);
>  	} else {
>  		pnv_php_disable_slot(&php_slot->slot);
> -		pnv_php_detect_clear_suprise_removal_freeze(php_slot);
> +		pnv_php_detect_clear_surprise_removal_freeze(php_slot);
>  	}
>  
>  	kfree(event);

[Severity: High]
This isn't a bug introduced by this patch, but can this sequence lead to a
race condition?

Looking at pnv_php_disable_slot(), it checks php_slot->state without holding
any synchronization locks. If a sysfs write and a surprise hotplug event (via
this event handler workqueue) trigger concurrently, can both threads pass the
state check and proceed to tear down the slot?

It seems this could lead to double-unregistering and calling pnv_php_release()
twice, which would result in a double list_del() on php_slot->link and a
kernel panic.

[ ... ]

> @@ -1055,7 +1109,7 @@ static irqreturn_t pnv_php_interrupt(int irq, void *data)
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This is a pre-existing issue, but does the interrupt handler have a lockless
access issue here?

In pnv_php_interrupt(), the handler accesses php_slot->bus->devices via
list_first_entry_or_null() without holding pci_bus_sem or taking a reference
on the returned device.

Concurrently, pnv_php_disable_slot() calls pci_hp_remove_devices() which
unbinds and frees the child devices. Since the root slot IRQ is intentionally
left active during this removal, if the interrupt fires and resolves the child
device just before the hotplug core frees it, could this cause a use-after-free
in hardirq context when passing the pointer to pci_dev_to_eeh_dev()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527180816.2749186-1-adityag@linux.ibm.com?part=2

[PATCH v2 3/3] ppc/pnv: Refactor PNV PCI Hotplug to group PCIe functions

[Aditya Gupta]

With the refactor change to pnv_php.c, pnv_php_* and pcie_* are mixed
up.

Move and group pcie, opencapi, and pnv_php*.

No functional change. Only moves code, and no logic has been modified.

Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
 drivers/pci/hotplug/pnv_php.c | 559 +++++++++++++++++-----------------
 1 file changed, 279 insertions(+), 280 deletions(-)

diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
index 997412eea486..dd8e00b879e1 100644
--- a/drivers/pci/hotplug/pnv_php.c
+++ b/drivers/pci/hotplug/pnv_php.c
@@ -38,46 +38,125 @@ static DEFINE_SPINLOCK(pnv_php_lock);
 static void pnv_php_register(struct device_node *dn);
 static void pnv_php_unregister_one(struct device_node *dn);
 static void pnv_php_unregister(struct device_node *dn);
+static irqreturn_t pnv_php_interrupt(int irq, void *data);
+static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
+				bool disable_device, bool disable_msi);
 
-static void pcie_enable_irq(struct pnv_php_slot *php_slot);
-static int pcie_check_link_active(struct pci_dev *pdev);
-static void pcie_detect_surprise_removal(struct pnv_php_slot *php_slot);
-
-static void pcie_fixup_presence_state(struct pnv_php_slot *php_slot, u8 *presence)
+static void pcie_init_irq(struct pnv_php_slot *php_slot, int irq)
 {
-	if (pci_pcie_type(php_slot->pdev) == PCI_EXP_TYPE_DOWNSTREAM &&
-	    *presence == OPAL_PCI_SLOT_EMPTY) {
-		/*
-		 * Similar to pciehp_hpc, check whether the Link Active
-		 * bit is set to account for broken downstream bridges
-		 * that don't properly assert Presence Detect State, as
-		 * was observed on the Microsemi Switchtec PM8533 PFX
-		 * [11f8:8533].
-		 */
-		if (pcie_check_link_active(php_slot->pdev) > 0)
-			*presence = OPAL_PCI_SLOT_PRESENT;
+	struct pci_dev *pdev = php_slot->pdev;
+	u32 broken_pdc = 0;
+	u16 sts, ctrl;
+	int ret;
+
+	/* Check PDC (Presence Detection Change) is broken or not */
+	ret = of_property_read_u32(php_slot->dn, "ibm,slot-broken-pdc",
+				   &broken_pdc);
+	if (!ret && broken_pdc)
+		php_slot->flags |= PNV_PHP_FLAG_BROKEN_PDC;
+
+	/* Clear pending interrupts */
+	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &sts);
+	if (php_slot->flags & PNV_PHP_FLAG_BROKEN_PDC)
+		sts |= PCI_EXP_SLTSTA_DLLSC;
+	else
+		sts |= (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC);
+	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, sts);
+
+	/* Request the interrupt */
+	ret = request_irq(irq, pnv_php_interrupt, IRQF_SHARED,
+			  php_slot->name, php_slot);
+	if (ret) {
+		pnv_php_disable_irq(php_slot, true, true);
+		SLOT_WARN(php_slot, "Error %d enabling IRQ %d\n", ret, irq);
+		return;
 	}
-}
 
-static void pcie_fundamental_reset(struct pnv_php_slot *php_slot)
-{
-	pci_set_pcie_reset_state(php_slot->pdev, pcie_warm_reset);
-	msleep(250);
-	pci_set_pcie_reset_state(php_slot->pdev, pcie_deassert_reset);
+	/* Enable the interrupts */
+	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
+	if (php_slot->flags & PNV_PHP_FLAG_BROKEN_PDC) {
+		ctrl &= ~PCI_EXP_SLTCTL_PDCE;
+		ctrl |= (PCI_EXP_SLTCTL_HPIE |
+			 PCI_EXP_SLTCTL_DLLSCE);
+	} else {
+		ctrl |= (PCI_EXP_SLTCTL_HPIE |
+			 PCI_EXP_SLTCTL_PDCE |
+			 PCI_EXP_SLTCTL_DLLSCE);
+	}
+	pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, ctrl);
+
+	/* The interrupt is initialized successfully when @irq is valid */
+	php_slot->irq = irq;
 }
 
-static void pnv_php_enable_irq(struct pnv_php_slot *php_slot)
+static int pcie_enable_msix(struct pnv_php_slot *php_slot)
 {
-	if (php_slot->backend_ops->enable_irq)
-		php_slot->backend_ops->enable_irq(php_slot);
+	struct pci_dev *pdev = php_slot->pdev;
+	struct msix_entry entry;
+	int nr_entries, ret;
+	u16 pcie_flag;
+
+	/* Get total number of MSIx entries */
+	nr_entries = pci_msix_vec_count(pdev);
+	if (nr_entries < 0)
+		return nr_entries;
+
+	/* Check hotplug MSIx entry is in range */
+	pcie_capability_read_word(pdev, PCI_EXP_FLAGS, &pcie_flag);
+	entry.entry = FIELD_GET(PCI_EXP_FLAGS_IRQ, pcie_flag);
+	if (entry.entry >= nr_entries)
+		return -ERANGE;
+
+	/* Enable MSIx */
+	ret = pci_enable_msix_exact(pdev, &entry, 1);
+	if (ret) {
+		SLOT_WARN(php_slot, "Error %d enabling MSIx\n", ret);
+		return ret;
+	}
+
+	return entry.vector;
 }
 
-static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
-				bool disable_device, bool disable_msi)
+static void pcie_enable_irq(struct pnv_php_slot *php_slot)
 {
-	if (php_slot->backend_ops->disable_irq)
-		php_slot->backend_ops->disable_irq(php_slot, disable_device,
-				disable_msi);
+	struct pci_dev *pdev = php_slot->pdev;
+	int irq, ret;
+
+	if (!pdev)
+		return;
+
+	/*
+	 * The MSI/MSIx interrupt might have been occupied by other
+	 * drivers. Don't populate the surprise hotplug capability
+	 * in that case.
+	 */
+	if (pci_dev_msi_enabled(pdev))
+		return;
+
+	ret = pci_enable_device(pdev);
+	if (ret) {
+		SLOT_WARN(php_slot, "Error %d enabling device\n", ret);
+		return;
+	}
+
+	pci_set_master(pdev);
+
+	/* Enable MSIx interrupt */
+	irq = pcie_enable_msix(php_slot);
+	if (irq > 0) {
+		pcie_init_irq(php_slot, irq);
+		return;
+	}
+
+	/*
+	 * Use MSI if MSIx doesn't work. Fail back to legacy INTx
+	 * if MSI doesn't work either
+	 */
+	ret = pci_enable_msi(pdev);
+	if (!ret || pdev->irq) {
+		irq = pdev->irq;
+		pcie_init_irq(php_slot, irq);
+	}
 }
 
 static void pcie_disable_irq(struct pnv_php_slot *php_slot,
@@ -104,8 +183,176 @@ static void pcie_disable_irq(struct pnv_php_slot *php_slot,
 			pci_disable_msi(pdev);
 	}
 
-	if (disable_device)
-		pci_disable_device(pdev);
+	if (disable_device)
+		pci_disable_device(pdev);
+}
+
+static void pcie_get_attention_state(struct pnv_php_slot *php_slot, u8 *state)
+{
+	struct pci_dev *bridge = php_slot->pdev;
+	u16 status;
+
+	pcie_capability_read_word(bridge, PCI_EXP_SLTCTL, &status);
+	*state = (status & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6;
+}
+
+static void pcie_set_attention_state(struct pnv_php_slot *php_slot, u8 state)
+{
+	struct pci_dev *bridge = php_slot->pdev;
+	u16 new, mask;
+
+	php_slot->attention_state = state;
+	mask = PCI_EXP_SLTCTL_AIC;
+
+	if (state)
+		new = FIELD_PREP(PCI_EXP_SLTCTL_AIC, state);
+	else
+		new = PCI_EXP_SLTCTL_ATTN_IND_OFF;
+
+	pcie_capability_clear_and_set_word(bridge, PCI_EXP_SLTCTL, mask, new);
+}
+
+static int pcie_check_link_active(struct pci_dev *pdev)
+{
+	u16 lnk_status;
+	int ret;
+
+	ret = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
+	if (ret == PCIBIOS_DEVICE_NOT_FOUND || PCI_POSSIBLE_ERROR(lnk_status))
+		return -ENODEV;
+
+	ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
+
+	return ret;
+}
+
+static void pcie_fixup_presence_state(struct pnv_php_slot *php_slot, u8 *presence)
+{
+	if (pci_pcie_type(php_slot->pdev) == PCI_EXP_TYPE_DOWNSTREAM &&
+	    *presence == OPAL_PCI_SLOT_EMPTY) {
+		/*
+		 * Similar to pciehp_hpc, check whether the Link Active
+		 * bit is set to account for broken downstream bridges
+		 * that don't properly assert Presence Detect State, as
+		 * was observed on the Microsemi Switchtec PM8533 PFX
+		 * [11f8:8533].
+		 */
+		if (pcie_check_link_active(php_slot->pdev) > 0)
+			*presence = OPAL_PCI_SLOT_PRESENT;
+	}
+}
+
+static void pcie_fundamental_reset(struct pnv_php_slot *php_slot)
+{
+	pci_set_pcie_reset_state(php_slot->pdev, pcie_warm_reset);
+	msleep(250);
+	pci_set_pcie_reset_state(php_slot->pdev, pcie_deassert_reset);
+}
+
+static void pcie_detect_surprise_removal(struct pnv_php_slot *php_slot)
+{
+	struct pci_dev *pdev = php_slot->pdev;
+	struct eeh_dev *edev;
+	struct eeh_pe *pe;
+	int i, rc;
+
+	if (!pdev)
+		return;
+
+	/*
+	 * When a device is surprise removed from a downstream bridge slot,
+	 * the upstream bridge port can still end up frozen due to related EEH
+	 * events, which will in turn block the MSI interrupts for slot hotplug
+	 * detection.
+	 *
+	 * Detect and thaw any frozen upstream PE after slot deactivation.
+	 */
+	edev = pci_dev_to_eeh_dev(pdev);
+	pe = edev ? edev->pe : NULL;
+	rc = eeh_pe_get_state(pe);
+	if ((rc == -ENODEV) || (rc == -ENOENT)) {
+		SLOT_WARN(
+			php_slot,
+			"Upstream bridge PE state unknown, hotplug detect may fail\n");
+	} else {
+		if (pe->state & EEH_PE_ISOLATED) {
+			SLOT_WARN(
+				php_slot,
+				"Upstream bridge PE %02x frozen, thawing...\n",
+				pe->addr);
+			for (i = 0; i < 3; i++)
+				if (!eeh_unfreeze_pe(pe))
+					break;
+			if (i >= 3)
+				SLOT_WARN(
+					php_slot,
+					"Unable to thaw PE %02x, hotplug detect will fail!\n",
+					pe->addr);
+			else
+				SLOT_WARN(php_slot,
+					  "PE %02x thawed successfully\n",
+					  pe->addr);
+		}
+	}
+}
+
+static int pcie_reset_slot(struct pnv_php_slot *php_slot, bool probe)
+{
+	struct pci_dev *bridge = php_slot->pdev;
+	uint16_t sts;
+
+	if (probe)
+		return 0;
+
+	/* mask our interrupt while resetting the bridge */
+	if (php_slot->irq > 0)
+		disable_irq(php_slot->irq);
+
+	pci_bridge_secondary_bus_reset(bridge);
+
+	/* clear any state changes that happened due to the reset */
+	pcie_capability_read_word(php_slot->pdev, PCI_EXP_SLTSTA, &sts);
+	sts &= (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC);
+	pcie_capability_write_word(php_slot->pdev, PCI_EXP_SLTSTA, sts);
+
+	if (php_slot->irq > 0)
+		enable_irq(php_slot->irq);
+
+	return 0;
+}
+
+static const struct pnv_php_backend_ops pnv_php_pcie_ops = {
+	.enable_irq              = pcie_enable_irq,
+	.disable_irq             = pcie_disable_irq,
+	.get_attention_state     = pcie_get_attention_state,
+	.set_attention_state     = pcie_set_attention_state,
+	.fixup_presence_state    = pcie_fixup_presence_state,
+	.fundamental_reset       = pcie_fundamental_reset,
+	.detect_surprise_removal = pcie_detect_surprise_removal,
+	.reset_slot              = pcie_reset_slot,
+};
+
+static int opencapi_reset_slot(struct pnv_php_slot *slot, bool probe)
+{
+	return probe ? 1 : -ENODEV;
+}
+
+static const struct pnv_php_backend_ops pnv_php_opencapi_ops = {
+	.reset_slot              = opencapi_reset_slot,
+};
+
+static void pnv_php_enable_irq(struct pnv_php_slot *php_slot)
+{
+	if (php_slot->backend_ops->enable_irq)
+		php_slot->backend_ops->enable_irq(php_slot);
+}
+
+static void pnv_php_disable_irq(struct pnv_php_slot *php_slot,
+				bool disable_device, bool disable_msi)
+{
+	if (php_slot->backend_ops->disable_irq)
+		php_slot->backend_ops->disable_irq(php_slot, disable_device,
+				disable_msi);
 }
 
 static void pnv_php_free_slot(struct kref *kref)
@@ -427,20 +674,6 @@ static int pnv_php_get_power_state(struct hotplug_slot *slot, u8 *state)
 	return 0;
 }
 
-static int pcie_check_link_active(struct pci_dev *pdev)
-{
-	u16 lnk_status;
-	int ret;
-
-	ret = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
-	if (ret == PCIBIOS_DEVICE_NOT_FOUND || PCI_POSSIBLE_ERROR(lnk_status))
-		return -ENODEV;
-
-	ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
-
-	return ret;
-}
-
 static int pnv_php_get_adapter_state(struct hotplug_slot *slot, u8 *state)
 {
 	struct pnv_php_slot *php_slot = to_pnv_php_slot(slot);
@@ -465,15 +698,6 @@ static int pnv_php_get_adapter_state(struct hotplug_slot *slot, u8 *state)
 	return ret;
 }
 
-static void pcie_get_attention_state(struct pnv_php_slot *php_slot, u8 *state)
-{
-	struct pci_dev *bridge = php_slot->pdev;
-	u16 status;
-
-	pcie_capability_read_word(bridge, PCI_EXP_SLTCTL, &status);
-	*state = (status & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6;
-}
-
 static int pnv_php_get_attention_state(struct hotplug_slot *slot, u8 *state)
 {
 	struct pnv_php_slot *php_slot = to_pnv_php_slot(slot);
@@ -497,22 +721,6 @@ static int pnv_php_set_attention_state(struct hotplug_slot *slot, u8 state)
 	return 0;
 }
 
-static void pcie_set_attention_state(struct pnv_php_slot *php_slot, u8 state)
-{
-	struct pci_dev *bridge = php_slot->pdev;
-	u16 new, mask;
-
-	php_slot->attention_state = state;
-	mask = PCI_EXP_SLTCTL_AIC;
-
-	if (state)
-		new = FIELD_PREP(PCI_EXP_SLTCTL_AIC, state);
-	else
-		new = PCI_EXP_SLTCTL_ATTN_IND_OFF;
-
-	pcie_capability_clear_and_set_word(bridge, PCI_EXP_SLTCTL, mask, new);
-}
-
 static int pnv_php_activate_slot(struct pnv_php_slot *php_slot,
 				 struct hotplug_slot *slot)
 {
@@ -658,31 +866,6 @@ static int pnv_php_reset_slot(struct hotplug_slot *slot, bool probe)
 	return probe ? 0 : -ENODEV;
 }
 
-static int pcie_reset_slot(struct pnv_php_slot *php_slot, bool probe)
-{
-	struct pci_dev *bridge = php_slot->pdev;
-	uint16_t sts;
-
-	if (probe)
-		return 0;
-
-	/* mask our interrupt while resetting the bridge */
-	if (php_slot->irq > 0)
-		disable_irq(php_slot->irq);
-
-	pci_bridge_secondary_bus_reset(bridge);
-
-	/* clear any state changes that happened due to the reset */
-	pcie_capability_read_word(php_slot->pdev, PCI_EXP_SLTSTA, &sts);
-	sts &= (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC);
-	pcie_capability_write_word(php_slot->pdev, PCI_EXP_SLTSTA, sts);
-
-	if (php_slot->irq > 0)
-		enable_irq(php_slot->irq);
-
-	return 0;
-}
-
 static int pnv_php_enable_slot(struct hotplug_slot *slot)
 {
 	struct pnv_php_slot *php_slot = to_pnv_php_slot(slot);
@@ -800,26 +983,6 @@ static void pnv_php_release(struct pnv_php_slot *php_slot)
 	pnv_php_put_slot(php_slot->parent);
 }
 
-static const struct pnv_php_backend_ops pnv_php_pcie_ops = {
-	.enable_irq              = pcie_enable_irq,
-	.disable_irq             = pcie_disable_irq,
-	.get_attention_state     = pcie_get_attention_state,
-	.set_attention_state     = pcie_set_attention_state,
-	.fixup_presence_state    = pcie_fixup_presence_state,
-	.fundamental_reset       = pcie_fundamental_reset,
-	.detect_surprise_removal = pcie_detect_surprise_removal,
-	.reset_slot              = pcie_reset_slot,
-};
-
-static int opencapi_reset_slot(struct pnv_php_slot *slot, bool probe)
-{
-	return probe ? 1 : -ENODEV;
-}
-
-static const struct pnv_php_backend_ops pnv_php_opencapi_ops = {
-	.reset_slot              = opencapi_reset_slot,
-};
-
 static struct pnv_php_slot *pnv_php_alloc_slot(struct device_node *dn)
 {
 	struct pnv_php_slot *php_slot;
@@ -934,34 +1097,6 @@ static int pnv_php_register_slot(struct pnv_php_slot *php_slot)
 	return 0;
 }
 
-static int pcie_enable_msix(struct pnv_php_slot *php_slot)
-{
-	struct pci_dev *pdev = php_slot->pdev;
-	struct msix_entry entry;
-	int nr_entries, ret;
-	u16 pcie_flag;
-
-	/* Get total number of MSIx entries */
-	nr_entries = pci_msix_vec_count(pdev);
-	if (nr_entries < 0)
-		return nr_entries;
-
-	/* Check hotplug MSIx entry is in range */
-	pcie_capability_read_word(pdev, PCI_EXP_FLAGS, &pcie_flag);
-	entry.entry = FIELD_GET(PCI_EXP_FLAGS_IRQ, pcie_flag);
-	if (entry.entry >= nr_entries)
-		return -ERANGE;
-
-	/* Enable MSIx */
-	ret = pci_enable_msix_exact(pdev, &entry, 1);
-	if (ret) {
-		SLOT_WARN(php_slot, "Error %d enabling MSIx\n", ret);
-		return ret;
-	}
-
-	return entry.vector;
-}
-
 static void
 pnv_php_detect_clear_surprise_removal_freeze(struct pnv_php_slot *php_slot)
 {
@@ -969,53 +1104,6 @@ pnv_php_detect_clear_surprise_removal_freeze(struct pnv_php_slot *php_slot)
 		php_slot->backend_ops->detect_surprise_removal(php_slot);
 }
 
-static void pcie_detect_surprise_removal(struct pnv_php_slot *php_slot)
-{
-	struct pci_dev *pdev = php_slot->pdev;
-	struct eeh_dev *edev;
-	struct eeh_pe *pe;
-	int i, rc;
-
-	if (!pdev)
-		return;
-
-	/*
-	 * When a device is surprise removed from a downstream bridge slot,
-	 * the upstream bridge port can still end up frozen due to related EEH
-	 * events, which will in turn block the MSI interrupts for slot hotplug
-	 * detection.
-	 *
-	 * Detect and thaw any frozen upstream PE after slot deactivation.
-	 */
-	edev = pci_dev_to_eeh_dev(pdev);
-	pe = edev ? edev->pe : NULL;
-	rc = eeh_pe_get_state(pe);
-	if ((rc == -ENODEV) || (rc == -ENOENT)) {
-		SLOT_WARN(
-			php_slot,
-			"Upstream bridge PE state unknown, hotplug detect may fail\n");
-	} else {
-		if (pe->state & EEH_PE_ISOLATED) {
-			SLOT_WARN(
-				php_slot,
-				"Upstream bridge PE %02x frozen, thawing...\n",
-				pe->addr);
-			for (i = 0; i < 3; i++)
-				if (!eeh_unfreeze_pe(pe))
-					break;
-			if (i >= 3)
-				SLOT_WARN(
-					php_slot,
-					"Unable to thaw PE %02x, hotplug detect will fail!\n",
-					pe->addr);
-			else
-				SLOT_WARN(php_slot,
-					  "PE %02x thawed successfully\n",
-					  pe->addr);
-		}
-	}
-}
-
 static void pnv_php_event_handler(struct work_struct *work)
 {
 	struct pnv_php_event *event =
@@ -1109,95 +1197,6 @@ static irqreturn_t pnv_php_interrupt(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void pcie_init_irq(struct pnv_php_slot *php_slot, int irq)
-{
-	struct pci_dev *pdev = php_slot->pdev;
-	u32 broken_pdc = 0;
-	u16 sts, ctrl;
-	int ret;
-
-	/* Check PDC (Presence Detection Change) is broken or not */
-	ret = of_property_read_u32(php_slot->dn, "ibm,slot-broken-pdc",
-				   &broken_pdc);
-	if (!ret && broken_pdc)
-		php_slot->flags |= PNV_PHP_FLAG_BROKEN_PDC;
-
-	/* Clear pending interrupts */
-	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &sts);
-	if (php_slot->flags & PNV_PHP_FLAG_BROKEN_PDC)
-		sts |= PCI_EXP_SLTSTA_DLLSC;
-	else
-		sts |= (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC);
-	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, sts);
-
-	/* Request the interrupt */
-	ret = request_irq(irq, pnv_php_interrupt, IRQF_SHARED,
-			  php_slot->name, php_slot);
-	if (ret) {
-		pnv_php_disable_irq(php_slot, true, true);
-		SLOT_WARN(php_slot, "Error %d enabling IRQ %d\n", ret, irq);
-		return;
-	}
-
-	/* Enable the interrupts */
-	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
-	if (php_slot->flags & PNV_PHP_FLAG_BROKEN_PDC) {
-		ctrl &= ~PCI_EXP_SLTCTL_PDCE;
-		ctrl |= (PCI_EXP_SLTCTL_HPIE |
-			 PCI_EXP_SLTCTL_DLLSCE);
-	} else {
-		ctrl |= (PCI_EXP_SLTCTL_HPIE |
-			 PCI_EXP_SLTCTL_PDCE |
-			 PCI_EXP_SLTCTL_DLLSCE);
-	}
-	pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, ctrl);
-
-	/* The interrupt is initialized successfully when @irq is valid */
-	php_slot->irq = irq;
-}
-
-static void pcie_enable_irq(struct pnv_php_slot *php_slot)
-{
-	struct pci_dev *pdev = php_slot->pdev;
-	int irq, ret;
-
-	if (!pdev)
-		return;
-
-	/*
-	 * The MSI/MSIx interrupt might have been occupied by other
-	 * drivers. Don't populate the surprise hotplug capability
-	 * in that case.
-	 */
-	if (pci_dev_msi_enabled(pdev))
-		return;
-
-	ret = pci_enable_device(pdev);
-	if (ret) {
-		SLOT_WARN(php_slot, "Error %d enabling device\n", ret);
-		return;
-	}
-
-	pci_set_master(pdev);
-
-	/* Enable MSIx interrupt */
-	irq = pcie_enable_msix(php_slot);
-	if (irq > 0) {
-		pcie_init_irq(php_slot, irq);
-		return;
-	}
-
-	/*
-	 * Use MSI if MSIx doesn't work. Fail back to legacy INTx
-	 * if MSI doesn't work either
-	 */
-	ret = pci_enable_msi(pdev);
-	if (!ret || pdev->irq) {
-		irq = pdev->irq;
-		pcie_init_irq(php_slot, irq);
-	}
-}
-
 static int pnv_php_register_one(struct device_node *dn)
 {
 	struct pnv_php_slot *php_slot;
-- 
2.54.0

Re: [PATCH v2 3/3] ppc/pnv: Refactor PNV PCI Hotplug to group PCIe functions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect read-modify-write pattern on the Write-1-to-Clear (RW1C) register PCI_EXP_SLTSTA.
- [High] Unlocked read-modify-write on the PCI_EXP_SLTCTL register leading to potential lost updates.
- [High] Device enable count and MSI vectors leak on teardown, and pci_clear_master() is missing.
- [High] Use-After-Free and double-free in the error path of pnv_php_register_one().
- [High] NULL pointer dereference when logging a workqueue allocation error in pnv_php_alloc_slot().
- [High] Lockless access to the PCI bus device list in the hardirq context.
- [High] Race condition during IRQ initialization exposes a window where interrupts cannot be disabled during a slot reset.
- [High] Unbounded dynamic memory allocation in the interrupt handler can cause OOM.
- [High] Calling disable_irq() on a potentially shared interrupt line.
--

commit 61762d5f66d3944b865702b98018365475b6c067
Author: Aditya Gupta <adityag@linux.ibm.com>

ppc/pnv: Refactor PNV PCI Hotplug to group PCIe functions

This commit refactors pnv_php.c to group PCIe functions together without
functional changes.

> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
> index 997412eea4863..dd8e00b879e16 100644
> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c
[ ... ]
> +	/* Clear pending interrupts */
> +	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &sts);
> +	if (php_slot->flags & PNV_PHP_FLAG_BROKEN_PDC)
> +		sts |= PCI_EXP_SLTSTA_DLLSC;
> +	else
> +		sts |= (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC);
> +	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, sts);

[Severity: High]
This is a pre-existing issue, but does this read-modify-write pattern clear
unintended status bits?

Since PCI_EXP_SLTSTA is a Write-1-to-Clear (RW1C) register, writing back the
read value writes '1's to other previously set status bits (like Command
Completed), which might inadvertently clear them before they can be handled.

[ ... ]
> +	/* Request the interrupt */
> +	ret = request_irq(irq, pnv_php_interrupt, IRQF_SHARED,
> +			  php_slot->name, php_slot);
> +	if (ret) {
> +		pnv_php_disable_irq(php_slot, true, true);
> +		SLOT_WARN(php_slot, "Error %d enabling IRQ %d\n", ret, irq);
> +		return;
> +	}
> +
> +	/* Enable the interrupts */
> +	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
> +	if (php_slot->flags & PNV_PHP_FLAG_BROKEN_PDC) {
> +		ctrl &= ~PCI_EXP_SLTCTL_PDCE;
> +		ctrl |= (PCI_EXP_SLTCTL_HPIE |
> +			 PCI_EXP_SLTCTL_DLLSCE);
> +	} else {
> +		ctrl |= (PCI_EXP_SLTCTL_HPIE |
> +			 PCI_EXP_SLTCTL_PDCE |
> +			 PCI_EXP_SLTCTL_DLLSCE);
> +	}
> +	pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, ctrl);
> +
> +	/* The interrupt is initialized successfully when @irq is valid */
> +	php_slot->irq = irq;

[Severity: High]
This is a pre-existing issue, but is there a race condition during IRQ
initialization that exposes a window where interrupts cannot be disabled?

Hardware interrupts are unmasked in PCI_EXP_SLTCTL before php_slot->irq = irq
is assigned. A concurrent slot reset via sysfs in this small time window
will evaluate php_slot->irq > 0 as false, bypassing disable_irq().

Could resetting the bridge without disabling the interrupt lead to an
interrupt storm?

[ ... ]
>  static void pcie_disable_irq(struct pnv_php_slot *php_slot,
>  				bool disable_device, bool disable_msi)
>  {
[ ... ]
>  	if (disable_device || disable_msi) {
>  		if (pdev->msix_enabled)
>  			pci_disable_msix(pdev);
>  		else if (pdev->msi_enabled)
>  			pci_disable_msi(pdev);
>  	}
>  
> -	if (disable_device)
> -		pci_disable_device(pdev);
> +	if (disable_device)
> +		pci_disable_device(pdev);
>  }

[Severity: High]
This is a pre-existing issue, but does this teardown path leak the device
enable count and MSI vectors?

When pnv_php_free_slot() calls pnv_php_disable_irq(..., false, false), the
disable_device and disable_msi flags are false. This skips
pci_disable_device() and pci_disable_msix()/msi(), leaking resources.

Additionally, should pci_clear_master() be called prior to device disablement
here?

[ ... ]
> +static void pcie_set_attention_state(struct pnv_php_slot *php_slot, u8 state)
> +{
> +	struct pci_dev *bridge = php_slot->pdev;
> +	u16 new, mask;
> +
> +	php_slot->attention_state = state;
> +	mask = PCI_EXP_SLTCTL_AIC;
> +
> +	if (state)
> +		new = FIELD_PREP(PCI_EXP_SLTCTL_AIC, state);
> +	else
> +		new = PCI_EXP_SLTCTL_ATTN_IND_OFF;
> +
> +	pcie_capability_clear_and_set_word(bridge, PCI_EXP_SLTCTL, mask, new);

[Severity: High]
This is a pre-existing issue, but can this unlocked read-modify-write on the
PCI_EXP_SLTCTL register lead to lost updates?

Concurrent modifications (for example, sysfs state changes vs slot teardown)
might overwrite register updates here, potentially leaving interrupts
incorrectly configured.

[ ... ]
> +static int pcie_reset_slot(struct pnv_php_slot *php_slot, bool probe)
> +{
> +	struct pci_dev *bridge = php_slot->pdev;
> +	uint16_t sts;
> +
> +	if (probe)
> +		return 0;
> +
> +	/* mask our interrupt while resetting the bridge */
> +	if (php_slot->irq > 0)
> +		disable_irq(php_slot->irq);

[Severity: High]
This is a pre-existing issue, but does calling disable_irq() here affect
other devices sharing the same interrupt line?

Because the IRQ is requested with IRQF_SHARED (and may fall back to legacy
INTx), disable_irq() masks the line at the interrupt controller, which might
starve all other independent devices sharing that interrupt.

[ ... ]
> -static const struct pnv_php_backend_ops pnv_php_opencapi_ops = {
> -	.reset_slot              = opencapi_reset_slot,
> -};
> -
>  static struct pnv_php_slot *pnv_php_alloc_slot(struct device_node *dn)
>  {
>  	struct pnv_php_slot *php_slot;

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer dereference
when logging a workqueue allocation error?

If alloc_workqueue() fails, SLOT_WARN() is invoked. The SLOT_WARN() macro
evaluates php_slot->pdev (which is NULL at this point) and falls back to
dev_warn(&(php_slot)->bus->dev, ...).

Because php_slot->bus has not yet been initialized, accessing bus->dev
dereferences a NULL pointer.

[ ... ]
> @@ -1109,95 +1197,6 @@ static irqreturn_t pnv_php_interrupt(int irq, void *data)
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This is a pre-existing issue, but is it safe to perform lockless access to
the PCI bus device list in the hardirq context here?

The hardirq handler uses list_first_entry_or_null(&php_slot->bus->devices, ...)
to iterate the PCI bus device list. Traversing this list safely requires
holding the pci_bus_sem read lock, which cannot be acquired in hardirq
context.

Could this allow a race condition with concurrent device hotplug operations,
potentially corrupting list traversal?

[Severity: High]
Additionally, this is a pre-existing issue, but can this unbounded dynamic
memory allocation in the interrupt handler cause OOM?

A pnv_php_event is allocated with GFP_ATOMIC for every event. Without rate
limiting or coalescing, could a malfunctioning PCIe endpoint that generates
an interrupt storm rapidly exhaust the atomic memory pool?

[ ... ]
> -static void pcie_enable_irq(struct pnv_php_slot *php_slot)
[ ... ]
> -}
> -
>  static int pnv_php_register_one(struct device_node *dn)
>  {
>  	struct pnv_php_slot *php_slot;

[Severity: High]
This is a pre-existing issue, but is there a use-after-free and double-free
in the error path for this function?

If pnv_php_enable() fails, the code branches to the unregister_slot label,
invoking pnv_php_unregister_one() which unregisters the slot and drops its
reference, freeing it.

Execution then falls through to the free_slot label, where pnv_php_put_slot()
is called on the already-freed pointer. Could this cause a double-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527180816.2749186-1-adityag@linux.ibm.com?part=3