[PATCH v1] PCI: cpqphp: Check pci_hp_add_bridge() return value

[PATCH v1] PCI: cpqphp: Check pci_hp_add_bridge() return value

[Fahmy Hassan]

cpqhp_configure_device() calls pci_hp_add_bridge() without checking
its return value. If pci_hp_add_bridge() fails (e.g. no bus number
is available for the hot-added bridge), dev->subordinate is left
unset, so the subsequent pci_bus_add_devices(child) call is
correctly skipped -- but the failure itself goes completely
unreported, making such problems very hard to diagnose in the field.

Log an error via the driver's existing err() macro when
pci_hp_add_bridge() fails, and remove the now-resolved TODO entry.

Signed-off-by: Fahmy Hassan <fahmymohammed@gmail.com>
---
 drivers/pci/hotplug/cpqphp_pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
index 81c58b1ec200..34803f36ba65 100644
--- a/drivers/pci/hotplug/cpqphp_pci.c
+++ b/drivers/pci/hotplug/cpqphp_pci.c
@@ -100,7 +100,9 @@ int cpqhp_configure_device(struct controller *ctrl, struct pci_func *func)
 	}
 
 	if (func->pci_dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
-		pci_hp_add_bridge(func->pci_dev);
+		if (pci_hp_add_bridge(func->pci_dev))
+			err("bus/device/function %x/%x/%x: pci_hp_add_bridge() failed\n",
+			    func->bus, func->device, func->function);
 		child = func->pci_dev->subordinate;
 		if (child)
 			pci_bus_add_devices(child);
-- 
2.53.0

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

[Bjorn Helgaas]

On Tue, Sep 08, 2026 at 07:30:28PM -0600, Fahmy Hassan wrote:
> cpqhp_configure_device() calls pci_hp_add_bridge() without checking
> its return value. If pci_hp_add_bridge() fails (e.g. no bus number
> is available for the hot-added bridge), dev->subordinate is left
> unset, so the subsequent pci_bus_add_devices(child) call is
> correctly skipped -- but the failure itself goes completely
> unreported, making such problems very hard to diagnose in the field.
> 
> Log an error via the driver's existing err() macro when
> pci_hp_add_bridge() fails, and remove the now-resolved TODO entry.

The patch doesn't seem to remove the TODO.

There are four other drivers that call pci_hp_add_bridge(), and none
of them check the return value either.  We should leave it alone or
fix them all, so the review effort can be applied to all of them at
once.

> Signed-off-by: Fahmy Hassan <fahmymohammed@gmail.com>
> ---
>  drivers/pci/hotplug/cpqphp_pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
> index 81c58b1ec200..34803f36ba65 100644
> --- a/drivers/pci/hotplug/cpqphp_pci.c
> +++ b/drivers/pci/hotplug/cpqphp_pci.c
> @@ -100,7 +100,9 @@ int cpqhp_configure_device(struct controller *ctrl, struct pci_func *func)
>  	}
>  
>  	if (func->pci_dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> -		pci_hp_add_bridge(func->pci_dev);
> +		if (pci_hp_add_bridge(func->pci_dev))
> +			err("bus/device/function %x/%x/%x: pci_hp_add_bridge() failed\n",
> +			    func->bus, func->device, func->function);
>  		child = func->pci_dev->subordinate;
>  		if (child)
>  			pci_bus_add_devices(child);
> -- 
> 2.53.0
> 

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

[Fahmy Hassan]

On Tue, Sep 08, 2026 at 07:41 PM -0600, Bjorn Helgaas wrote:
> The patch doesn't seem to remove the TODO.

Sorry about that -- the patch you received was generated before I
amended the commit to actually include the TODO removal; the archived
patch file was stale on my end, not something dropped from the actual
fix. Fixed in v2.

> There are four other drivers that call pci_hp_add_bridge(), and none
> of them check the return value either.  We should leave it alone or
> fix them all, so the review effort can be applied to all of them at
> once.

Makes sense. I've turned this into a v2 series that checks the return
value the same way in cpqphp, cpcihp, ibmphp, pciehp, and shpchp,
logging via each driver's existing error macro.

One more correction, from an automated review I also got on v1: my
commit message said the failure "goes completely unreported", which
overstated it -- pci_hp_add_bridge() already logs the "no bus number
available" case via pci_err(). It only stays silent for the other
failure path (subordinate bus not created after scanning). Since the
function returns a bare -1 for either case, the caller can't tell
which one happened, so the new per-driver log will occasionally
overlap with that existing message for the bus-number case
specifically. I've reworded the v2 commit messages to describe this
accurately instead.

v2 coming shortly.

Thanks,
Fahmy

[PATCH v2 0/5] PCI: hotplug: Check pci_hp_add_bridge() return value

[Fahmy Hassan]

This is v2 of a fix originally sent as a single-driver patch against
cpqphp. Bjorn pointed out that pci_hp_add_bridge() is called unchecked
by four other hotplug drivers too, and asked that this be reviewed as
one series rather than driver by driver, so v2 expands the fix to
cover all five callers: cpqphp, cpcihp, ibmphp, pciehp, and shpchp.
Each patch checks the return value and logs via that driver's own
existing error macro, and removes the corresponding now-resolved TODO
entry where one existed.

v1 -> v2:
 - Expanded from a single cpqphp patch to all five drivers that call
   pci_hp_add_bridge(), per Bjorn's review.
 - Fixed the TODO removal that was missing from the v1 patch email
   (a stale archived patch file on my end, not a dropped hunk).
 - Reworded the commit messages: pci_hp_add_bridge() already logs one
   of its two failure paths (no bus number available) via pci_err();
   only the other path (subordinate bus not created after scanning)
   was actually unreported. v1's message overstated this as
   "completely unreported".

Fahmy Hassan (5):
  PCI: cpqphp: Check pci_hp_add_bridge() return value
  PCI: cpcihp: Check pci_hp_add_bridge() return value
  PCI: ibmphp: Check pci_hp_add_bridge() return value
  PCI: pciehp: Check pci_hp_add_bridge() return value
  PCI: shpchp: Check pci_hp_add_bridge() return value

 drivers/pci/hotplug/TODO               | 8 --------
 drivers/pci/hotplug/cpci_hotplug_pci.c | 5 +++--
 drivers/pci/hotplug/cpqphp_pci.c       | 4 +++-
 drivers/pci/hotplug/ibmphp_core.c      | 3 ++-
 drivers/pci/hotplug/pciehp_pci.c       | 3 ++-
 drivers/pci/hotplug/shpchp_pci.c       | 5 +++--
 6 files changed, 13 insertions(+), 15 deletions(-)

-- 
2.53.0

[PATCH v2 1/5] PCI: cpqphp: Check pci_hp_add_bridge() return value

[Fahmy Hassan]

cpqhp_configure_device() calls pci_hp_add_bridge() without checking
its return value. pci_hp_add_bridge() already logs an error for one
failure path (no bus number available for the hot-added bridge), but
returns silently if the bridge's subordinate bus isn't created after
scanning -- that path goes completely unreported, and either way the
caller currently has no way to notice or react to the failure.

Log an error via the driver's existing err() macro when
pci_hp_add_bridge() fails, identifying the device involved, and
remove the now-resolved TODO entry.

Signed-off-by: Fahmy Hassan <fahmymohammed@gmail.com>
---
 drivers/pci/hotplug/TODO         | 2 --
 drivers/pci/hotplug/cpqphp_pci.c | 4 +++-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/TODO b/drivers/pci/hotplug/TODO
index 7397374af171..215ab36df11d 100644
--- a/drivers/pci/hotplug/TODO
+++ b/drivers/pci/hotplug/TODO
@@ -14,8 +14,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()
diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
index 81c58b1ec200..34803f36ba65 100644
--- a/drivers/pci/hotplug/cpqphp_pci.c
+++ b/drivers/pci/hotplug/cpqphp_pci.c
@@ -100,7 +100,9 @@ int cpqhp_configure_device(struct controller *ctrl, struct pci_func *func)
 	}
 
 	if (func->pci_dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
-		pci_hp_add_bridge(func->pci_dev);
+		if (pci_hp_add_bridge(func->pci_dev))
+			err("bus/device/function %x/%x/%x: pci_hp_add_bridge() failed\n",
+			    func->bus, func->device, func->function);
 		child = func->pci_dev->subordinate;
 		if (child)
 			pci_bus_add_devices(child);
-- 
2.53.0

[PATCH v2 2/5] PCI: cpcihp: Check pci_hp_add_bridge() return value

[Fahmy Hassan]

cpci_configure_slot() calls pci_hp_add_bridge() for each bridge on the
newly added slot without checking its return value. pci_hp_add_bridge()
already logs an error for one failure path (no bus number available
for the hot-added bridge), but returns silently if the bridge's
subordinate bus isn't created after scanning -- that path goes
completely unreported, and either way the caller currently has no way
to notice or react to the failure.

Log an error via the driver's existing err() macro when
pci_hp_add_bridge() fails, identifying the device involved, and
remove the now-resolved TODO entry.

Signed-off-by: Fahmy Hassan <fahmymohammed@gmail.com>
---
 drivers/pci/hotplug/TODO               | 4 ----
 drivers/pci/hotplug/cpci_hotplug_pci.c | 5 +++--
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/hotplug/TODO b/drivers/pci/hotplug/TODO
index 215ab36df11d..0a8e23546e5e 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
diff --git a/drivers/pci/hotplug/cpci_hotplug_pci.c b/drivers/pci/hotplug/cpci_hotplug_pci.c
index 6c48066acb44..3deeec8dd206 100644
--- a/drivers/pci/hotplug/cpci_hotplug_pci.c
+++ b/drivers/pci/hotplug/cpci_hotplug_pci.c
@@ -269,8 +269,9 @@ 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) &&
+		    pci_hp_add_bridge(dev))
+			err("pci_hp_add_bridge(%s) failed", pci_name(dev));
 	}
 
 	pci_assign_unassigned_bridge_resources(parent->self);
-- 
2.53.0

[PATCH v2 3/5] PCI: ibmphp: Check pci_hp_add_bridge() return value

[Fahmy Hassan]

ibm_configure_device() calls pci_hp_add_bridge() without checking its
return value. pci_hp_add_bridge() already logs an error for one
failure path (no bus number available for the hot-added bridge), but
returns silently if the bridge's subordinate bus isn't created after
scanning -- that path goes completely unreported, and either way the
caller currently has no way to notice or react to the failure.

Log an error via the driver's existing err() macro when
pci_hp_add_bridge() fails, identifying the device involved, and
remove the now-resolved TODO entry.

Signed-off-by: Fahmy Hassan <fahmymohammed@gmail.com>
---
 drivers/pci/hotplug/TODO          | 2 --
 drivers/pci/hotplug/ibmphp_core.c | 3 ++-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/TODO b/drivers/pci/hotplug/TODO
index 0a8e23546e5e..2a4f44f9b629 100644
--- a/drivers/pci/hotplug/TODO
+++ b/drivers/pci/hotplug/TODO
@@ -37,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/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c
index aca86c092d4a..b653d3068677 100644
--- a/drivers/pci/hotplug/ibmphp_core.c
+++ b/drivers/pci/hotplug/ibmphp_core.c
@@ -690,7 +690,8 @@ static int ibm_configure_device(struct pci_func *func)
 		}
 	}
 	if (!(flag) && (func->dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)) {
-		pci_hp_add_bridge(func->dev);
+		if (pci_hp_add_bridge(func->dev))
+			err("pci_hp_add_bridge(%s) failed\n", pci_name(func->dev));
 		child = func->dev->subordinate;
 		if (child)
 			pci_bus_add_devices(child);
-- 
2.53.0

[PATCH v2 4/5] PCI: pciehp: Check pci_hp_add_bridge() return value

[Fahmy Hassan]

pciehp_configure_device() calls pci_hp_add_bridge() for each bridge on
the newly added slot without checking its return value.
pci_hp_add_bridge() already logs an error for one failure path (no
bus number available for the hot-added bridge), but returns silently
if the bridge's subordinate bus isn't created after scanning -- that
path goes completely unreported, and either way the caller currently
has no way to notice or react to the failure.

Log an error via the driver's existing ctrl_err() macro when
pci_hp_add_bridge() fails, identifying the device involved.

Signed-off-by: Fahmy Hassan <fahmymohammed@gmail.com>
---
 drivers/pci/hotplug/pciehp_pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index 65e50bee1a8c..cbdfde557f3b 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -59,7 +59,8 @@ int pciehp_configure_device(struct controller *ctrl)
 	}
 
 	for_each_pci_bridge(dev, parent)
-		pci_hp_add_bridge(dev);
+		if (pci_hp_add_bridge(dev))
+			ctrl_err(ctrl, "pci_hp_add_bridge(%s) failed\n", pci_name(dev));
 
 	pci_assign_unassigned_bridge_resources(bridge);
 	pcie_bus_configure_settings(parent);
-- 
2.53.0

[PATCH v2 5/5] PCI: shpchp: Check pci_hp_add_bridge() return value

[Fahmy Hassan]

shpchp_configure_device() calls pci_hp_add_bridge() for each bridge on
the newly added slot without checking its return value.
pci_hp_add_bridge() already logs an error for one failure path (no
bus number available for the hot-added bridge), but returns silently
if the bridge's subordinate bus isn't created after scanning -- that
path goes completely unreported, and either way the caller currently
has no way to notice or react to the failure.

Log an error via the driver's existing ctrl_err() macro when
pci_hp_add_bridge() fails, identifying the device involved.

Signed-off-by: Fahmy Hassan <fahmymohammed@gmail.com>
---
 drivers/pci/hotplug/shpchp_pci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/shpchp_pci.c b/drivers/pci/hotplug/shpchp_pci.c
index 36db0c3c4ea6..b9b18879b8ab 100644
--- a/drivers/pci/hotplug/shpchp_pci.c
+++ b/drivers/pci/hotplug/shpchp_pci.c
@@ -48,8 +48,9 @@ int shpchp_configure_device(struct slot *p_slot)
 	}
 
 	for_each_pci_bridge(dev, parent) {
-		if (PCI_SLOT(dev->devfn) == p_slot->device)
-			pci_hp_add_bridge(dev);
+		if (PCI_SLOT(dev->devfn) == p_slot->device &&
+		    pci_hp_add_bridge(dev))
+			ctrl_err(ctrl, "pci_hp_add_bridge(%s) failed\n", pci_name(dev));
 	}
 
 	pci_assign_unassigned_bridge_resources(bridge);
-- 
2.53.0

Re: [PATCH v2 4/5] PCI: pciehp: Check pci_hp_add_bridge() return value

[Lukas Wunner]

On Tue, Sep 08, 2026 at 08:22:25PM -0600, Fahmy Hassan wrote:
> pciehp_configure_device() calls pci_hp_add_bridge() for each bridge on
> the newly added slot without checking its return value.
> pci_hp_add_bridge() already logs an error for one failure path (no
> bus number available for the hot-added bridge), but returns silently
> if the bridge's subordinate bus isn't created after scanning -- that
> path goes completely unreported, and either way the caller currently
> has no way to notice or react to the failure.
> 
> Log an error via the driver's existing ctrl_err() macro when
> pci_hp_add_bridge() fails, identifying the device involved.

The only change here is to log something on error.  That can be done
in pci_hp_add_bridge() itself without having to amend every caller.

However pci_hp_add_bridge() already logs an error for the one failure
case that merits an error message.

The return value is normally evaluated to do something, such as
bailing out or unwinding some earlier actions.  But in this case,
I'm not seeing a need to do that.

So I believe this change isn't necessary, at least as far as pciehp
is concerned.

Thanks,

Lukas