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

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

[Marc Zyngier]

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.

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

[AngeloGioacchino Del Regno]

Il 21/09/25 18:14, Alok Tiwari ha scritto:
> 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.
> 

I think that's a nice improvement - propagating the right error code looks good.

The only thing is - you have to make sure that it never returns NULL, so that
in the controller drivers you always check for `if (IS_ERR(x))` - otherwise with
the current IS_ERR_OR_NULL(x) most of the error paths are wrong.

Cheers,
Angelo

> Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
> ---
>   arch/mips/pci/pci-xtalk-bridge.c                       | 4 ++--
>   drivers/pci/controller/cadence/pci-j721e.c             | 4 ++--
>   drivers/pci/controller/cadence/pcie-cadence-plat.c     | 4 ++--
>   drivers/pci/controller/dwc/pcie-designware-host.c      | 4 ++--
>   drivers/pci/controller/dwc/pcie-qcom.c                 | 4 ++--
>   drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c | 4 ++--
>   drivers/pci/controller/mobiveil/pcie-mobiveil-plat.c   | 4 ++--
>   drivers/pci/controller/pci-aardvark.c                  | 4 ++--
>   drivers/pci/controller/pci-ftpci100.c                  | 4 ++--
>   drivers/pci/controller/pci-host-common.c               | 4 ++--
>   drivers/pci/controller/pci-hyperv.c                    | 4 ++--
>   drivers/pci/controller/pci-ixp4xx.c                    | 4 ++--
>   drivers/pci/controller/pci-loongson.c                  | 4 ++--
>   drivers/pci/controller/pci-mvebu.c                     | 4 ++--
>   drivers/pci/controller/pci-rcar-gen2.c                 | 4 ++--
>   drivers/pci/controller/pci-tegra.c                     | 4 ++--
>   drivers/pci/controller/pci-v3-semi.c                   | 4 ++--
>   drivers/pci/controller/pci-versatile.c                 | 4 ++--
>   drivers/pci/controller/pci-xgene.c                     | 4 ++--
>   drivers/pci/controller/pcie-altera.c                   | 4 ++--
>   drivers/pci/controller/pcie-brcmstb.c                  | 4 ++--
>   drivers/pci/controller/pcie-iproc-bcma.c               | 4 ++--
>   drivers/pci/controller/pcie-iproc-platform.c           | 4 ++--
>   drivers/pci/controller/pcie-mediatek-gen3.c            | 4 ++--
>   drivers/pci/controller/pcie-mediatek.c                 | 4 ++--
>   drivers/pci/controller/pcie-mt7621.c                   | 4 ++--
>   drivers/pci/controller/pcie-rcar-host.c                | 4 ++--
>   drivers/pci/controller/pcie-rockchip-host.c            | 4 ++--
>   drivers/pci/controller/pcie-xilinx-cpm.c               | 4 ++--
>   drivers/pci/controller/pcie-xilinx-dma-pl.c            | 4 ++--
>   drivers/pci/controller/pcie-xilinx-nwl.c               | 4 ++--
>   drivers/pci/controller/pcie-xilinx.c                   | 4 ++--
>   drivers/pci/controller/plda/pcie-plda-host.c           | 4 ++--
>   drivers/pci/probe.c                                    | 8 ++++----
>   34 files changed, 70 insertions(+), 70 deletions(-)
>