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

[sashiko-bot@kernel.org]

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