Re: [PATCH v2 2/4] PCI: rzg3s-host: Use shared reset controls for power domain resets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing chained IRQ handler cleanup in error paths leads to Use-After-Free of the host structure.
- [High] The `rzg3s_pcie_resume_noirq` callback unconditionally executes `fsleep(5000)` while local interrupts are disabled, leading to a system hang or scheduling BUG.
--

commit 41ddeeaac37e089f4b9cce52ea971db3b995ab04
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

PCI: rzg3s-host: Use shared reset controls for power domain resets

Switch to shared reset controls for PCIe power resets to prepare for
RZ/V2H(P) support. On this platform, multiple PCIe controllers share
the same reset line, requiring shared ownership of the reset control.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index d86e7516dcc29..a5192e4b58dfb 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c

[ ... ]

While reviewing this code, I noticed a potential regression that was not
introduced by this patch.

In rzg3s_pcie_init_irqdomain(), the driver loops over PCI_NUM_INTX and
installs chained handlers for INTx interrupts:

drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_init_irqdomain() {
    ...
	for (int i = 0; i < PCI_NUM_INTX; i++) {
        ...
		irq_set_chained_handler_and_data(irq, rzg3s_pcie_intx_irq_handler,
						 host);
    ...
}

However, if an error occurs later in the initialization sequence, the error
handling paths do not tear down these chained handlers by calling
irq_set_chained_handler_and_data(irq, NULL, NULL).

Since the host structure is allocated via devm_pci_alloc_host_bridge(), it is
automatically freed when the probe function fails.

If any of the mapped INTx interrupts trigger after probe failure, the kernel
will execute the stale rzg3s_pcie_intx_irq_handler which dereferences the
freed host pointer.

Can this lead to a use-after-free regression on the host structure?


I also noticed another issue in the resume path:

drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_resume_noirq() {
    ...
	ret = rzg3s_pcie_power_resets_deassert(host);
    ...
}

drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_power_resets_deassert() {
    ...
	fsleep(5000);
    ...
}

rzg3s_pcie_resume_noirq() is registered as a .resume_noirq PM callback via
NOIRQ_SYSTEM_SLEEP_PM_OPS.

The PM core executes noirq phase callbacks with local CPU interrupts explicitly
disabled.

Within rzg3s_pcie_resume_noirq(), the driver calls
rzg3s_pcie_power_resets_deassert(), which unconditionally executes
fsleep(5000).

For a 5000us delay, fsleep() delegates to usleep_range(), which uses
schedule_hrtimeout() and sleeps the task.

Because timer interrupts are disabled on the local CPU in the noirq phase,
the hrtimer may never fire to wake the task.

Could sleeping with interrupts disabled here trigger a scheduling bug or lead
to a silent system hang regression during resume?

If a delay is strictly required in the noirq phase, should a busy-wait
function like mdelay(5) be used instead?

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