Re: [PATCH 1/3] PCI: rzg3s: Disable refclk on probe failure

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch violates hardware teardown order by disabling the PCIe reference clock before asserting controller resets.

Pre-existing issues:
- [Critical] Missing IRQ cleanup for chained INTx handlers on probe failure leads to use-after-free.
- [High] MMIO register access after PM runtime clock gating in suspend path causes system hang.
--

--- Patch [1]: [PATCH 1/3] PCI: rzg3s: Disable refclk on probe failure ---
commit 0d4ef239e6084c21b4710c1db03e3a6365363533
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: PCI: rzg3s: Disable refclk on probe failure

This commit disables the PCIe reference clock when the probe function fails.
It adds a call to clk_disable_unprepare in the error path, ensuring the
clock is balanced if initialization fails.

Cc: stable@vger.kernel.org

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 66f687304c1cb..bd6471a7fb7a1 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> @@ -1762,6 +1762,7 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
>  	return 0;
>  
>  host_probe_teardown:
> +	clk_disable_unprepare(host->port.refclk);

[Severity: Medium]
Does this new addition violate the hardware teardown order by disabling the
reference clock before asserting controller resets?

During setup, the reference clock is enabled prior to calling
config_post_init() which deasserts resets. In this new error path,
clk_disable_unprepare() is called before host->data->config_deinit(host).

Could disabling the clock while the core is out of reset leave the state
machine hanging or undefined? Should the clock disable be moved after the
config_deinit() call to maintain strict LIFO initialization order?

>  	rzg3s_pcie_teardown_irqdomain(host);

[Severity: Critical]
This is a pre-existing issue, but does the teardown function fail to clean
up chained handlers on probe failure?

Looking at rzg3s_pcie_teardown_irqdomain(), it removes the IRQ domain but
does not unregister the chained INTx hardware handlers:

drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_teardown_irqdomain() {
        if (IS_ENABLED(CONFIG_PCI_MSI))
                rzg3s_pcie_teardown_msi(host);

        irq_domain_remove(host->intx_domain);
}

If pci_host_probe() fails and the devres mechanism frees the host structure,
could a shared or spurious INTx interrupt firing later cause
rzg3s_pcie_intx_irq_handler() to dereference the freed host pointer? Should
this call irq_set_chained_handler_and_data(irq, NULL, NULL) to prevent a
use-after-free?

>  	host->data->config_deinit(host);
>  rpm_put:

[Severity: High]
This is a pre-existing issue, but can the suspend path cause a system hang
due to MMIO access after PM runtime clock gating?

In rzg3s_pcie_suspend_noirq(), the driver disables the device PM domain
clocks before calling the hardware de-initialization routine:

drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_suspend_noirq() {
        ...
        ret = pm_runtime_put_sync(dev);
        if (ret)
                return ret;

        clk_disable_unprepare(port->refclk);

        /* SoC-specific de-initialization */
        ret = data->config_deinit(host);
        ...
}

For the RZ/G3E SoC, the de-initialization callback performs an MMIO write:

drivers/pci/controller/pcie-rzg3s-host.c:rzg3e_pcie_config_deinit() {
        writel_relaxed(0, host->axi + RZG3S_PCI_RESET);
        return 0;
}

Will accessing this MMIO region while the bus clock is gated trigger a fatal
bus fault on ARM64 during system suspend? Should the de-initialization
happen before pm_runtime_put_sync() is called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715223019.3002237-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=1