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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message inaccurately claims to remove a 'now-resolved TODO entry' and states that the failure goes 'completely unreported', resulting in duplicate logs and missing changes.
--

commit b0a51b9f884f314ded07cba26637065948174b4b
Author: Fahmy Hassan <fahmymohammed@gmail.com>

PCI: cpqphp: Check pci_hp_add_bridge() return value

This commit adds an error check and log for pci_hp_add_bridge() in
cpqhp_configure_device(). The commit message notes this fixes a completely
unreported failure and removes a resolved TODO entry.

>     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.

[Severity: Medium]
Is the statement that the failure goes "completely unreported" accurate?

Looking at pci_hp_add_bridge() in drivers/pci/probe.c, it already explicitly
logs an error when no bus number is available:

	if (busnr-- > end) {
		pci_err(dev, "No bus number available for hot-added bridge\n");
		return -1;
	}

Additionally, the commit message mentions removing a "now-resolved TODO
entry," but the patch does not appear to remove any such entry from
cpqhp_configure_device() or the rest of the file. Could the commit message
be updated, or was a chunk of the patch accidentally dropped?

> diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
> index 81c58b1ec200f..34803f36ba65c 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);

[Severity: Medium]
Does adding this err() call result in duplicate logs when
pci_hp_add_bridge() fails?

Since pci_hp_add_bridge() already logs its own error for the missing bus
number case, this new message will be printed immediately after the core PCI
error log, potentially duplicating the error reporting.

>  		child = func->pci_dev->subordinate;
>  		if (child)
>  			pci_bus_add_devices(child);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909013028.508870-1-fahmymohammed@gmail.com?part=1