Re: [PATCH v6 09/10] PCI: dw-rockchip: Add system PM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] System resume unconditionally re-initializes the host, leaking the existing IRQ domain and breaking legacy INTx interrupts while executing invalid GFP_KERNEL allocations in an atomic context.
- [High] The PM operations `rockchip_pcie_suspend` and `rockchip_pcie_resume` are registered as `noirq` callbacks but call functions that can sleep.
- [High] Missing check for the return value of reset_control_deassert() in rockchip_pcie_resume().
- [Medium] Missing `reset_control_assert` in the error handling path of `rockchip_pcie_resume`.
- [Medium] The order of initializing and tearing down the regulator, PHY, and clocks in system PM callbacks contradicts the proven sequence used during driver probe.
--

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

PCI: dw-rockchip: Add system PM support

Add system PM support for Rockchip PCIe Designware Controllers.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 47d9cab909bce..e675918ba1848 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c

[ ... ]

> +static int rockchip_pcie_suspend(struct device *dev)
> +{

[ ... ]

> +	ret = dw_pcie_suspend_noirq(pci);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_set_value_cansleep(rockchip->rst_gpio, 0);

[Severity: High]
Since rockchip_pcie_suspend() is registered via NOIRQ_SYSTEM_SLEEP_PM_OPS(),
it executes as a noirq callback where local interrupts are disabled.

Is it safe to call sleepable functions in this atomic context? Functions like
gpiod_set_value_cansleep() and clk_bulk_disable_unprepare() (which takes a
mutex) might trigger might_sleep splats or deadlocks.

> +	rockchip_pcie_phy_deinit(rockchip);
> +	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> +	reset_control_assert(rockchip->rst);
> +	if (rockchip->vpcie3v3)
> +		regulator_disable(rockchip->vpcie3v3);
> +
> +	return 0;
> +}
> +
> +static int rockchip_pcie_resume(struct device *dev)
> +{

[ ... ]

> +	ret = clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks);

[Severity: Medium]
Does this sequence invert the hardware initialization dependencies used during
driver probe?

During probe, the driver enables the regulator, initializes the PHY, deasserts
reset, and then enables the clocks. Here in rockchip_pcie_resume(), the clocks
are enabled first, followed by the regulator and PHY. Could this inverted
order cause link-training bugs or PHY lockups?

> +	if (ret) {
> +		dev_err(dev, "clock init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (rockchip->vpcie3v3) {
> +		ret = regulator_enable(rockchip->vpcie3v3);
> +		if (ret)
> +			goto err_disable_clk;
> +	}
> +
> +	ret = rockchip_pcie_phy_init(rockchip);
> +	if (ret) {
> +		dev_err(dev, "phy init failed: %d\n", ret);
> +		goto err_disable_regulator;
> +	}
> +
> +	reset_control_deassert(rockchip->rst);

[Severity: High]
Does this code handle reset_control_deassert() failures?

If reset_control_deassert() fails, the driver immediately proceeds to write to
the IP block's APB registers. If the hardware reset was not actually released,
accessing these registers could trigger a synchronous external abort and kernel
panic.

> +
> +	rockchip_pcie_writel_apb(rockchip, FIELD_PREP_WM16(0xffff, rockchip->intx),
> +				 PCIE_CLIENT_INTR_MASK_LEGACY);
> +
> +	rockchip_pcie_enable_enhanced_ltssm_control_mode(rockchip, 0);
> +	rockchip_pcie_set_controller_mode(rockchip, PCIE_CLIENT_MODE_RC);
> +	rockchip_pcie_unmask_dll_indicator(rockchip);
> +
> +	gpiod_set_value_cansleep(rockchip->rst_gpio, 1);
> +
> +	ret = dw_pcie_resume_noirq(pci);

[Severity: High]
Does system resume unconditionally re-initialize the host, leaking the existing
IRQ domain and breaking legacy INTx interrupts?

When dw_pcie_resume_noirq() is called, it invokes the host initialization
callback rockchip_pcie_host_init(), which then calls
rockchip_pcie_init_irq_domain():

rockchip_pcie_host_init()
  rockchip_pcie_init_irq_domain()
    rockchip->irq_domain = irq_domain_create_linear(of_fwnode_handle(intc),
                                                    PCI_NUM_INTX,
                                                    &intx_domain_ops,
                                                    rockchip);

This executes an invalid GFP_KERNEL allocation inside the atomic noirq phase,
overwrites the existing irq_domain pointer (leaking the old domain), and drops
existing INTx mappings.

> +	if (ret) {
> +		dev_err(dev, "failed to resume: %d\n", ret);

[ ... ]

> +err_deinit_phy:
> +	gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
> +	rockchip_pcie_phy_deinit(rockchip);
> +err_disable_regulator:

[Severity: Medium]
Does this error path leave the hardware controller in an inconsistent,
permanently deasserted state?

If dw_pcie_resume_noirq() fails, the error handling block tears down the PHY,
clocks, and regulator, but it seems to miss calling
reset_control_assert(rockchip->rst).

> +	if (rockchip->vpcie3v3)
> +		regulator_disable(rockchip->vpcie3v3);
> +err_disable_clk:
> +	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> +	return ret;
> +}

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