linux-kernel - Re: [PATCH RFC] PCI: Convert devm_pci_alloc_host

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87ms6nyb7x.wl-maz@kernel.org>
Date: Sun, 21 Sep 2025 18:19:46 +0100
From: Marc Zyngier <maz@...nel.org>
To: Alok Tiwari <alok.a.tiwari@...cle.com>
Cc: thomas.petazzoni@...tlin.com,
	pali@...nel.org,
	lpieralisi@...nel.org,
	kwilczynski@...nel.org,
	mani@...nel.org,
	robh@...nel.org,
	bhelgaas@...gle.com,
	joyce.ooi@...el.com,
	alyssa@...enzweig.io,
	jim2101024@...il.com,
	florian.fainelli@...adcom.com,
	bcm-kernel-feedback-list@...adcom.com,
	rjui@...adcom.com,
	sbranden@...adcom.com,
	ryder.lee@...iatek.com,
	jianjun.wang@...iatek.com,
	sergio.paracuellos@...il.com,
	matthias.bgg@...il.com,
	angelogioacchino.delregno@...labora.com,
	marek.vasut+renesas@...il.com,
	yoshihiro.shimoda.uh@...esas.com,
	geert+renesas@...der.be,
	magnus.damm@...il.com,
	shawn.lin@...k-chips.com,
	heiko@...ech.de,
	michal.simek@....com,
	bharat.kumar.gogada@....com,
	will@...nel.org,
	kys@...rosoft.com,
	haiyangz@...rosoft.com,
	wei.liu@...nel.org,
	decui@...rosoft.com,
	linus.walleij@...aro.org,
	thierry.reding@...il.com,
	jonathanh@...dia.com,
	rric@...nel.org,
	nirmal.patel@...ux.intel.com,
	toan@...amperecomputing.com,
	jonathan.derrick@...ux.dev,
	linux-pci@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	linux-rpi-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	linux-renesas-soc@...r.kernel.org,
	linux-rockchip@...ts.infradead.org,
	linux-hyperv@...r.kernel.org,
	linux-tegra@...r.kernel.org
Subject: Re: [PATCH RFC] PCI: Convert devm_pci_alloc_host_bridge() users to error-pointer returns

On Sun, 21 Sep 2025 17:14:07 +0100,
Alok Tiwari <alok.a.tiwari@...cle.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.