Re: [PATCH v6 02/10] PCI: dw-rockchip: Restore vpcie3v3 regulator handle

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Silent failure when phy_init() or phy_power_on() fails due to dropped dev_err_probe().

Pre-existing issues:
- [High] Repeated devres memory and clock reference leaks during root port resets.
- [High] Failure to create IRQ domain causes potential arbitrary interrupt execution or NULL pointer dereference.
- [High] IRQ handler is active while clocks are disabled on probe error paths, causing a potential crash if the IRQ fires.
- [High] The chained legacy IRQ handler is not cleared on probe failure, leading to a permanent use-after-free and crash.
--

commit f70d8a03f10e09b0300fff37e8aa493a89895114
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

PCI: dw-rockchip: Restore vpcie3v3 regulator handle

This commit reverts a previous cleanup that removed the explicit vpcie3v3 
regulator handle. It restores storing the regulator in the rockchip struct 
and explicitly disables it in the probe error path. This change is needed 
to support disabling the regulator during system suspend in a future patch.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index dc85564e50013..f67ad3c31c35b 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
[ ... ]
> @@ -844,15 +845,22 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
[ ... ]
>  	ret = rockchip_pcie_phy_init(rockchip);
>  	if (ret)
> -		return dev_err_probe(dev, ret,
> -				     "failed to initialize the phy\n");
> +		goto disable_regulator;

[Severity: Medium]
Is it possible that removing this dev_err_probe() call causes failures in
phy_init() or phy_power_on() to happen silently? This seems like it could
make device initialization issues significantly harder to debug when they
occur.

>  
>  	ret = reset_control_deassert(rockchip->rst);
[ ... ]
> @@ -885,6 +893,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);

[Severity: High]
This is a pre-existing issue, but does manually disabling the clocks here on
the error path leave a window where the devm-managed IRQ handler (requested
via devm_request_threaded_irq() earlier in probe) is still active?

If the IRQ fires before devres cleans it up after the function returns, could
the handler attempt to read APB registers with clocks gated, causing a bus
fault and kernel panic?

>  deinit_phy:
>  	rockchip_pcie_phy_deinit(rockchip);
> +disable_regulator:
> +	if (rockchip->vpcie3v3)
> +		regulator_disable(rockchip->vpcie3v3);
>  
>  	return ret;
>  }

While reviewing this patch, I also noticed a few pre-existing issues in the
same file:

> @@ -570,9 +570,9 @@ static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip)
>  {
>  	struct device *dev = rockchip->pci.dev;
>  	int ret;
>  
>  	ret = devm_clk_bulk_get_all(dev, &rockchip->clks);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "failed to get clocks\n");

[Severity: High]
This isn't a bug introduced by this patch, but does calling
devm_clk_bulk_get_all() during root port resets cause a persistent memory
and clock reference leak?

Since rockchip_pcie_clk_init() is called by rockchip_pcie_rc_reset_root_port():

drivers/pci/controller/dwc/pcie-dw-rockchip.c:rockchip_pcie_rc_reset_root_port() {
	...
	ret = rockchip_pcie_clk_init(rockchip);
	if (ret)
		goto deinit_phy;
	...
}

Will repeated root port resets, such as those triggered by hardware
link-down events or userspace sysfs requests, continually allocate devres
structures and leak clock subsystem references, since they are only freed on
unbind?

> @@ -437,6 +437,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
>  {
[ ... ]
>  	ret = rockchip_pcie_init_irq_domain(rockchip);
>  	if (ret < 0)
>  		dev_err(dev, "failed to init irq domain\n");
>  
>  	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
>  					 rockchip);

[Severity: High]
This is a pre-existing issue, but does ignoring the error code from
rockchip_pcie_init_irq_domain() allow rockchip_pcie_intx_handler to be
registered with a NULL irq_domain?

If the legacy interrupt triggers when rockchip->irq_domain is NULL:

drivers/pci/controller/dwc/pcie-dw-rockchip.c:rockchip_pcie_intx_handler() {
	...
	for_each_set_bit(hwirq, &reg, 4)
		generic_handle_domain_irq(rockchip->irq_domain, hwirq);
	...
}

Could generic_handle_domain_irq() fall back to irq_default_domain and
potentially execute completely unrelated interrupt handlers belonging to
other devices if the default domain resolves the hwirq (0-3)?

[Severity: High]
Additionally, this is a pre-existing issue, but is the chained legacy IRQ
handler correctly cleared on probe failure?

If initialization fails later in the probe path, the rockchip structure will
be freed by devres. Because irq_set_chained_handler_and_data() is used
without a corresponding devres cleanup or manual cleanup in the error path,
could a subsequent legacy IRQ trigger a use-after-free when
rockchip_pcie_intx_handler() dereferences the freed rockchip structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-rockchip-pcie-system-suspend-v6-0-fbabcca9921b@collabora.com?part=2