[PATCH] PCI: dwc: uniphier: use fwnode API for legacy-interrupt-controller

[PATCH] PCI: dwc: uniphier: use fwnode API for legacy-interrupt-controller

[Rosen Penev]

Convert uniphier_pcie_config_intx_irq() from the OF-specific to the
generic firmware-node API: of_get_child_by_name() to
device_get_named_child_node(), irq_of_parse_and_map() to
fwnode_irq_get(), of_fwnode_handle() on the child node to passing the
fwnode directly, and of_node_put() to fwnode_handle_put().

Unlike irq_of_parse_and_map(), fwnode_irq_get() returns a positive IRQ
or a negative errno and never 0 (it rewrites 0 to -EINVAL), so check
pp->irq < 0 and propagate the error. pp->irq is an int, so the negative
value is preserved. Drop the now-unused np / dev_fwnode() locals, since
device_get_named_child_node() operates on the device directly.

Built for arm64 (defconfig + CONFIG_PCIE_UNIPHIER) with LLVM=1;
drivers/pci/controller/dwc/pcie-uniphier.o compiles cleanly.

Assisted-by: opencode:hy3-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/pci/controller/dwc/pcie-uniphier.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index 297e7a3d9b36..6516264fdfc1 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -262,27 +262,27 @@ static int uniphier_pcie_config_intx_irq(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct uniphier_pcie *pcie = to_uniphier_pcie(pci);
-	struct device_node *np = pci->dev->of_node;
-	struct device_node *np_intc;
+	struct device *dev = pci->dev;
+	struct fwnode_handle *fw_intc;
 	int ret = 0;
 
-	np_intc = of_get_child_by_name(np, "legacy-interrupt-controller");
-	if (!np_intc) {
-		dev_err(pci->dev, "Failed to get legacy-interrupt-controller node\n");
+	fw_intc = device_get_named_child_node(dev, "legacy-interrupt-controller");
+	if (!fw_intc) {
+		dev_err(dev, "Failed to get legacy-interrupt-controller node\n");
 		return -EINVAL;
 	}
 
-	pp->irq = irq_of_parse_and_map(np_intc, 0);
-	if (!pp->irq) {
-		dev_err(pci->dev, "Failed to get an IRQ entry in legacy-interrupt-controller\n");
+	pp->irq = fwnode_irq_get(fw_intc, 0);
+	if (pp->irq < 0) {
+		dev_err(dev, "Failed to get an IRQ entry in legacy-interrupt-controller\n");
 		ret = -EINVAL;
 		goto out_put_node;
 	}
 
-	pcie->intx_irq_domain = irq_domain_create_linear(of_fwnode_handle(np_intc), PCI_NUM_INTX,
+	pcie->intx_irq_domain = irq_domain_create_linear(fw_intc, PCI_NUM_INTX,
 						&uniphier_intx_domain_ops, pp);
 	if (!pcie->intx_irq_domain) {
-		dev_err(pci->dev, "Failed to get INTx domain\n");
+		dev_err(dev, "Failed to get INTx domain\n");
 		ret = -ENODEV;
 		goto out_put_node;
 	}
@@ -291,7 +291,7 @@ static int uniphier_pcie_config_intx_irq(struct dw_pcie_rp *pp)
 					 pp);
 
 out_put_node:
-	of_node_put(np_intc);
+	fwnode_handle_put(fw_intc);
 	return ret;
 }
 
-- 
2.55.0

Re: [PATCH] PCI: dwc: uniphier: use fwnode API for legacy-interrupt-controller

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The patch claims to propagate the error code from `fwnode_irq_get()`, but actively masks it by overwriting `ret` with `-EINVAL`, breaking probe deferral.

Pre-existing issues:
- [High] The chained IRQ handler and IRQ domain are not cleaned up if probe fails after `uniphier_pcie_config_intx_irq()` succeeds, leading to a potential use-after-free.
--

--- Patch [1]: [PATCH] PCI: dwc: uniphier: use fwnode API for legacy-interrupt-controller ---
commit 3514429fcfd9e126f916318af07e40df60df2ea1
Author: Rosen Penev <rosenp@gmail.com>

PCI: dwc: uniphier: use fwnode API for legacy-interrupt-controller

This patch converts uniphier_pcie_config_intx_irq() from the OF-specific
API to the generic firmware-node API. It switches out of_get_child_by_name()
and irq_of_parse_and_map() for their fwnode equivalents.

> Unlike irq_of_parse_and_map(), fwnode_irq_get() returns a positive IRQ
> or a negative errno and never 0 (it rewrites 0 to -EINVAL), so check
> pp->irq < 0 and propagate the error. pp->irq is an int, so the negative
> value is preserved.

> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> index 297e7a3d9b368..6516264fdfc18 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
[ ... ]
> @@ -262,27 +262,27 @@ static int uniphier_pcie_config_intx_irq(struct dw_pcie_rp *pp)
[ ... ]
> -	pp->irq = irq_of_parse_and_map(np_intc, 0);
> -	if (!pp->irq) {
> -		dev_err(pci->dev, "Failed to get an IRQ entry in legacy-interrupt-controller\n");
> +	pp->irq = fwnode_irq_get(fw_intc, 0);
> +	if (pp->irq < 0) {
> +		dev_err(dev, "Failed to get an IRQ entry in legacy-interrupt-controller\n");
>  		ret = -EINVAL;

[Severity: Medium]
The commit message states it will "propagate the error", but does this
hardcode the return value to -EINVAL instead?

If fwnode_irq_get() returns -EPROBE_DEFER, will overwriting it with -EINVAL
cause the PCI host driver to permanently fail probe rather than retrying
when the interrupt controller is ready?

>  		goto out_put_node;
>  	}
[ ... ]
> @@ -291,7 +291,7 @@ static int uniphier_pcie_config_intx_irq(struct dw_pcie_rp *pp)
>  					 pp);

[Severity: High]
This is a pre-existing issue, but does this setup lack corresponding cleanup
in the probe failure path?

If a subsequent step in dw_pcie_host_init() fails, the probe aborts and
devres frees the uniphier_pcie struct, which contains the pp pointer.

Since there is no .deinit callback in uniphier_pcie_host_ops to call
irq_set_chained_handler_and_data(pp->irq, NULL, NULL) or remove the IRQ
domain, could a spurious interrupt fire and execute a dangling chained
handler with a freed pp pointer?

>  
>  out_put_node:
> -	of_node_put(np_intc);
> +	fwnode_handle_put(fw_intc);
>  	return ret;
>  }

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