Re: [PATCH v9 9/9] PCI: dw-rockchip: Add system PM support

[Sebastian Reichel <sebastian.reichel@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]

Hi,

On Sat, Nov 07, 2026 at 10:06:40PM +0530, Manivannan Sadhasivam wrote:
> > +static int rockchip_pcie_suspend(struct device *dev)
> > +{
> > +	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> > +	struct dw_pcie *pci = &rockchip->pci;
> > +	int ret;
> > +
> > +	if (rockchip->data->mode == DW_PCIE_EP_TYPE)
> > +		return 0;
> > +
> > +	if (pci->suspended)
> > +		return 0;
> 
> This check looks wrong. 'suspended' flag is only set at the end of
> dw_pcie_suspend_noirq() and is used as an indication to glue suspend callbacks
> to turn off the resources or not. here is no way this can be set at the start
> of tne glue suspend callback.
> 
> I'll drop this check while applying.

I added this as a workaround in v8 for the issue Sashiko found in v7
(see down below). We probably will never see it in reality, so I'm
fine with dropping. Thanks for applying.

Greetings,

-- Sebastian

-------------------------------------------------------------------

[Severity: High]
What happens on a subsequent suspend if rockchip_pcie_resume() fails?
During suspend, dw_pcie_suspend_noirq() sets pci->suspended = true, and
rockchip_pcie_suspend() then powers off the PHY, disables clocks, asserts
reset, and disables rockchip->vpcie3v3.
If a later call to rockchip_pcie_resume() fails (for example, if
dw_pcie_resume_noirq() returns -ETIMEDOUT when dw_pcie_wait_for_link() times
out, or if regulator, PHY, reset, or clock initialization fails), the error
unwind path in rockchip_pcie_resume() disables the clocks, asserts reset,
deinitializes the PHY, and disables rockchip->vpcie3v3 while pci->suspended
remains true:
drivers/pci/controller/dwc/pcie-dw-rockchip.c:rockchip_pcie_resume() {
	...
	ret = dw_pcie_resume_noirq(pci);
	if (ret) {
		dev_err(dev, "failed to resume: %d\n", ret);
		goto err_clear_rst_gpio;
	}
	return 0;
err_clear_rst_gpio:
	gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
err_disable_clk:
	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
err_assert_reset:
	reset_control_assert(rockchip->rst);
err_deinit_phy:
	rockchip_pcie_phy_deinit(rockchip);
err_disable_regulator:
	if (rockchip->vpcie3v3)
		regulator_disable(rockchip->vpcie3v3);
	return ret;
}
Because the PM core logs the noirq resume error, clears is_noirq_suspended,
and continues resuming the system, won't child PCI device resume and the next
call to rockchip_pcie_suspend() read APB/DBI registers while the controller
clocks are disabled and reset is asserted (causing an SError on ARM64), and
then call rockchip_pcie_phy_deinit(), clk_bulk_disable_unprepare(), and
regulator_disable() a second time?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]