Re: [PATCH v2 06/16] PCI: dwc: rockchip: Use cached PCIe capability offset

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The IRQ domain and chained IRQ handler created in RC mode initialization are never cleaned up if host initialization fails.
- [High] In EP mode, the devres-managed 'sys' interrupt can fire while clocks are disabled during the probe error path.
--

commit 2570eaad60265910abfe106597d119f992b11a1b
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: rockchip: Use cached PCIe capability offset

This commit modifies the initialization sequence to trigger caching of the
PCIe capability offset. It replaces a direct capability search with the
cached helper function dw_pcie_get_pcie_cap().

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 731d93663ccae..be8b6187913df 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c

[ ... ]

> @@ -369,7 +369,7 @@ static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
>  	u32 cap, lnkcap;
>  
>  	/* Enable L0S capability for all SoCs */
> -	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	cap = dw_pcie_get_pcie_cap(pci);
>  	if (cap) {
>  		lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
>  		lnkcap |= PCI_EXP_LNKCAP_ASPM_L0S;

[Severity: High]
This isn't a bug introduced by this patch, but looking at the RC mode
initialization in rockchip_pcie_host_init():

    irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler, rockchip);

The IRQ domain and chained IRQ handler are created here, but there does not
appear to be a corresponding teardown path if host initialization fails later.
If a subsequent step in dw_pcie_host_init() fails, it relies on a .deinit
callback to clean up.

Since rockchip_pcie_host_ops lacks a .deinit callback, does this leak the IRQ
domain and chained handler? When the probe function returns the error and
disables the APB clocks, could a subsequent interrupt on the legacy line
invoke rockchip_pcie_intx_handler() while clocks are disabled, causing a
synchronous external abort?

[Severity: High]
This is a pre-existing issue, but I noticed a potential race on the error path
in EP mode in rockchip_pcie_configure_ep():

    ret = devm_request_threaded_irq(dev, irq, NULL, rockchip_pcie_ep_sys_irq_thread, IRQF_ONESHOT, "pcie-sys-ep", rockchip);

We request the 'sys' interrupt using a devres-managed function. If a later
initialization step like dw_pcie_ep_init_registers() fails, the probe
function jumps to the deinit_clk label and manually disables APB clocks using
clk_bulk_disable_unprepare().

Because devres resources are only freed after the probe function completely
returns, the IRQ remains active while the clocks are turned off. If the
interrupt fires or is pending during this window, will the IRQ thread attempt
to read the PCIE_CLIENT_INTR_STATUS_MISC register without clock access and
trigger a kernel crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530153101.695580-1-18255117159@163.com?part=6