[PATCH] PCI: cpcihp: Check pci_hp_add_bridge() return value

[PATCH] PCI: cpcihp: Check pci_hp_add_bridge() return value

[Majed Alkhaleefah]

Check the return value of pci_hp_add_bridge() in cpci_configure_slot(). If
bridge initialization fails, log an error message using err().

Current functionality doesn't consider uninitialized bridges a failure.
Changing this may introduce regressions. As such, intentionally avoid
propagating the failure (e.g., returning -ENODEV).

This patch addresses an item in the cpcihp TODO list.

Compile-tested only.

Signed-off-by: Majed Alkhaleefah <majedalkhaleefah@gmail.com>
---
 drivers/pci/hotplug/cpci_hotplug_pci.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/cpci_hotplug_pci.c b/drivers/pci/hotplug/cpci_hotplug_pci.c
index 6c48066acb44..331c21833c44 100644
--- a/drivers/pci/hotplug/cpci_hotplug_pci.c
+++ b/drivers/pci/hotplug/cpci_hotplug_pci.c
@@ -269,8 +269,12 @@ int cpci_configure_slot(struct slot *slot)
 	parent = slot->dev->bus;
 
 	for_each_pci_bridge(dev, parent) {
-		if (PCI_SLOT(dev->devfn) == PCI_SLOT(slot->devfn))
-			pci_hp_add_bridge(dev);
+		if (PCI_SLOT(dev->devfn) == PCI_SLOT(slot->devfn)) {
+			if (pci_hp_add_bridge(dev) != 0) {
+				err("Failed to initialize bridge %s at slot %02x",
+				    pci_name(dev), slot->number);
+			}
+		}
 	}
 
 	pci_assign_unassigned_bridge_resources(parent->self);
-- 
2.43.0

Re: [PATCH] PCI: cpcihp: Check pci_hp_add_bridge() return value

[Bjorn Helgaas]

On Mon, Feb 23, 2026 at 02:36:46AM +0000, Majed Alkhaleefah wrote:
> Check the return value of pci_hp_add_bridge() in cpci_configure_slot(). If
> bridge initialization fails, log an error message using err().
> 
> Current functionality doesn't consider uninitialized bridges a failure.
> Changing this may introduce regressions. As such, intentionally avoid
> propagating the failure (e.g., returning -ENODEV).
> 
> This patch addresses an item in the cpcihp TODO list.

This sentence could just be dropped and the item removed from the list
in the same patch.

> Compile-tested only.
> 
> Signed-off-by: Majed Alkhaleefah <majedalkhaleefah@gmail.com>
> ---
>  drivers/pci/hotplug/cpci_hotplug_pci.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/cpci_hotplug_pci.c b/drivers/pci/hotplug/cpci_hotplug_pci.c
> index 6c48066acb44..331c21833c44 100644
> --- a/drivers/pci/hotplug/cpci_hotplug_pci.c
> +++ b/drivers/pci/hotplug/cpci_hotplug_pci.c
> @@ -269,8 +269,12 @@ int cpci_configure_slot(struct slot *slot)
>  	parent = slot->dev->bus;
>  
>  	for_each_pci_bridge(dev, parent) {
> -		if (PCI_SLOT(dev->devfn) == PCI_SLOT(slot->devfn))
> -			pci_hp_add_bridge(dev);
> +		if (PCI_SLOT(dev->devfn) == PCI_SLOT(slot->devfn)) {
> +			if (pci_hp_add_bridge(dev) != 0) {
> +				err("Failed to initialize bridge %s at slot %02x",
> +				    pci_name(dev), slot->number);

There are five callers of pci_hp_add_bridge(), and none of them checks
the return value, so I'm not sure it's worth just changing this one.

I suspect something in the pci_hp_add_bridge() path would already log
a message in the event of a failure.  If that's the case, I doubt it's
worth adding another message here (and maybe pci_hp_add_bridge()
should just be made a void function).  If we *do* need a message here,
I would want to update all the callers similarly.

> +			}
> +		}
>  	}
>  
>  	pci_assign_unassigned_bridge_resources(parent->self);
> -- 
> 2.43.0
> 

[PATCH v2] PCI: hotplug: Change return type of pci_hp_add_bridge() to void

[Majed Alkhaleefah]

The function pci_hp_add_bridge() currently returns an int error code.
However, none of its callers actually check or store the return value,
neither do they need to take any special action in the event of a
failure.

The execution path of pci_hp_add_bridge() already contains relevant
error logging. Therefore, adding additional logging/error checking is
redundant.

Make pci_hp_add_bridge() a void function.

Compile-tested only.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Majed Alkhaleefah <majedalkhaleefah@gmail.com>
---
Thank you for the review. Looking at the path of
pci_hp_add_bridge(), adding additional error checking is indeed
redundant, and the function could just be made to return void.

Changes in v2:
- Drop the v1 approach of checking the return value of
  pci_hp_add_bridge() in cpci_configure_slot()
- Remove relevant TODO items
- Make pci_hp_add_bridge() a void function

 drivers/pci/hotplug/TODO | 8 --------
 drivers/pci/pci.h        | 2 +-
 drivers/pci/probe.c      | 9 ++-------
 3 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/hotplug/TODO b/drivers/pci/hotplug/TODO
index 7397374af171..2a4f44f9b629 100644
--- a/drivers/pci/hotplug/TODO
+++ b/drivers/pci/hotplug/TODO
@@ -1,9 +1,5 @@
 Contributions are solicited in particular to remedy the following issues:
 
-cpcihp:
-
-* Returned code from pci_hp_add_bridge() is not checked.
-
 cpqphp:
 
 * The driver spawns a kthread cpqhp_event_thread() which is woken by the
@@ -14,8 +10,6 @@ cpqphp:
 * A large portion of cpqphp_ctrl.c and cpqphp_pci.c concerns resource
   management.  Doesn't this duplicate functionality in the core?
 
-* Returned code from pci_hp_add_bridge() is not checked.
-
 ibmphp:
 
 * Implementations of hotplug_slot_ops callbacks such as get_adapter_present()
@@ -43,8 +37,6 @@ ibmphp:
 * A large portion of ibmphp_res.c and ibmphp_pci.c concerns resource
   management.  Doesn't this duplicate functionality in the core?
 
-* Returned code from pci_hp_add_bridge() is not checked.
-
 shpchp:
 
 * The hardirq handler shpc_isr() queues events on a workqueue.  It can be
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 13d998fbacce..ee35f5582688 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -353,7 +353,7 @@ static inline int pci_proc_detach_bus(struct pci_bus *bus) { return 0; }
 #endif
 
 /* Functions for PCI Hotplug drivers to use */
-int pci_hp_add_bridge(struct pci_dev *dev);
+void pci_hp_add_bridge(struct pci_dev *dev);
 bool pci_hp_spurious_link_change(struct pci_dev *pdev);
 
 #if defined(CONFIG_SYSFS) && defined(HAVE_PCI_LEGACY)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index bccc7a4bdd79..b4270e44a769 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3545,7 +3545,7 @@ void __init pci_sort_breadthfirst(void)
 	bus_sort_breadthfirst(&pci_bus_type, &pci_sort_bf_cmp);
 }
 
-int pci_hp_add_bridge(struct pci_dev *dev)
+void pci_hp_add_bridge(struct pci_dev *dev)
 {
 	struct pci_bus *parent = dev->bus;
 	int busnr, start = parent->busn_res.start;
@@ -3558,7 +3558,7 @@ int pci_hp_add_bridge(struct pci_dev *dev)
 	}
 	if (busnr-- > end) {
 		pci_err(dev, "No bus number available for hot-added bridge\n");
-		return -1;
+		return;
 	}
 
 	/* Scan bridges that are already configured */
@@ -3572,10 +3572,5 @@ int pci_hp_add_bridge(struct pci_dev *dev)
 
 	/* Scan bridges that need to be reconfigured */
 	pci_scan_bridge_extend(parent, dev, busnr, available_buses, 1);
-
-	if (!dev->subordinate)
-		return -1;
-
-	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_hp_add_bridge);
-- 
2.43.0

Re: [PATCH v2] PCI: hotplug: Change return type of pci_hp_add_bridge() to void

[Majed Alkhaleefah]

Hello. Just wanted to ping this v2 patch. Please let me know if
there's anything I should do. Thanks!

On Tue, 24 Feb 2026 at 18:01, Majed Alkhaleefah
<majedalkhaleefah@gmail.com> wrote:
>
> The function pci_hp_add_bridge() currently returns an int error code.
> However, none of its callers actually check or store the return value,
> neither do they need to take any special action in the event of a
> failure.
>
> The execution path of pci_hp_add_bridge() already contains relevant
> error logging. Therefore, adding additional logging/error checking is
> redundant.
>
> Make pci_hp_add_bridge() a void function.
>
> Compile-tested only.
>
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Majed Alkhaleefah <majedalkhaleefah@gmail.com>

[PATCH] PCI: cpcihp: Check pci_hp_add_bridge() return value

[Brody England]

cpci_configure_slot() ignores the return value of pci_hp_add_bridge()
and continues with resource assignment and device addition even if
bridge setup fails.

Check the return value, report the error, and abort slot configuration
if adding the bridge fails.

Compile-tested only.

Signed-off-by: Brody England <brodyengland3@gmail.com>
---
 drivers/pci/hotplug/cpci_hotplug_pci.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/cpci_hotplug_pci.c b/drivers/pci/hotplug/cpci_hotplug_pci.c
index 6c48066acb44..7ae02071e796 100644
--- a/drivers/pci/hotplug/cpci_hotplug_pci.c
+++ b/drivers/pci/hotplug/cpci_hotplug_pci.c
@@ -269,8 +269,14 @@ int cpci_configure_slot(struct slot *slot)
 	parent = slot->dev->bus;
 
 	for_each_pci_bridge(dev, parent) {
-		if (PCI_SLOT(dev->devfn) == PCI_SLOT(slot->devfn))
-			pci_hp_add_bridge(dev);
+		if (PCI_SLOT(dev->devfn) == PCI_SLOT(slot->devfn)) {
+			ret = pci_hp_add_bridge(dev);
+			if (ret) {
+				err("Could not add hotplug bridge for slot %02x",
+				    slot->number);
+				goto out;
+			}
+		}
 	}
 
 	pci_assign_unassigned_bridge_resources(parent->self);
-- 
2.47.3