Re: [PATCH RFC] PCI: Convert devm_pci_alloc_host_bridge() users to error-pointer returns

[Marc Zyngier <maz@kernel.org>]

On Sun, 21 Sep 2025 17:14:07 +0100,
Alok Tiwari <alok.a.tiwari@oracle.com> wrote:
> 
> devm_pci_alloc_host_bridge() and pci_alloc_host_bridge() previously
> returned NULL on failure, forcing callers to special-case NULL handling
> and often hardcode -ENOMEM as the error.
> 
> This series updates devm_pci_alloc_host_bridge() to consistently return
> error pointers (ERR_PTR) with the actual error code, instead of NULL.
> All callers across PCI host controller drivers are updated to use
> IS_ERR_OR_NULL()/PTR_ERR() instead of NULL checks and hardcoded -ENOMEM.
> 
> Benefits:
>   - Standardizes error handling with Linux kernel ERR_PTR()/PTR_ERR()
>     conventions.
>   - Ensures that the actual error code from lower-level helpers is
>     propagated back to the caller.
>   - Removes ambiguity between NULL and error pointer returns.
>
> Touched drivers include:
>  cadence (J721E, cadence-plat)
>  dwc (designware, qcom)
>  mobiveil (layerscape-gen4, mobiveil-plat)
>  aardvark, ftpci100, ixp4xx, loongson, mvebu, rcar, tegra, v3-semi,
>  versatile, xgene, altera, brcmstb, iproc, mediatek, mt7621, xilinx,
>  plda, and others
> 
> This patch updates error handling across these host controller drivers
>  so that callers consistently receive ERR_PTR() instead of NULL.

Not quite.

> diff --git a/arch/mips/pci/pci-xtalk-bridge.c b/arch/mips/pci/pci-xtalk-bridge.c
> index e00c38620d14..c2c8ed8ecac1 100644
> --- a/arch/mips/pci/pci-xtalk-bridge.c
> +++ b/arch/mips/pci/pci-xtalk-bridge.c
> @@ -636,8 +636,8 @@ static int bridge_probe(struct platform_device *pdev)
>  	pci_set_flags(PCI_PROBE_ONLY);
>  
>  	host = devm_pci_alloc_host_bridge(dev, sizeof(*bc));
> -	if (!host) {
> -		err = -ENOMEM;
> +	if (IS_ERR_OR_NULL(host)) {
> +		err = PTR_ERR(host);

Under which circumstances can NULL still be returned? Because applying
PTR_ERR() to a NULL pointer looks like a pretty bad idea.

>  		goto err_remove_domain;
>  	}
>  

[...]

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index f41128f91ca7..e627f36b7683 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -686,18 +686,18 @@ struct pci_host_bridge *devm_pci_alloc_host_bridge(struct device *dev,
>  
>  	bridge = pci_alloc_host_bridge(priv);
>  	if (!bridge)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	bridge->dev.parent = dev;
>  
>  	ret = devm_add_action_or_reset(dev, devm_pci_alloc_host_bridge_release,
>  				       bridge);
>  	if (ret)
> -		return NULL;
> +		return ERR_PTR(ret);
>  
>  	ret = devm_of_pci_bridge_init(dev, bridge);
>  	if (ret)
> -		return NULL;
> +		return ERR_PTR(ret);
>  
>  	return bridge;
>  }
> @@ -3198,7 +3198,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  
>  	bridge = pci_alloc_host_bridge(0);
>  	if (!bridge)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	bridge->dev.parent = parent;
>  

And what about the code that comes after that if we fail to register
the bus? The remaining "return NULL", which will then be interpreted
as 0 in any user of this function, leading to a worse situation than
what we have now.

Also, things like pci_scan_root_bus() have the following pattern:

	b = pci_create_root_bus(parent, bus, ops, sysdata, resources);
	if (!b)
		return NULL;

which will end with prejudice given what you have introduced.

If you are going to touch this sort of things, at least make it
consistent, analyse *all* code paths, and provide documentation.

	M.

-- 
Jazz isn't dead. It just smells funny.