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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] PCI: dw-rockchip: premature rst_gpio deassertion in rockchip_pcie_resume()
- [High] PCI: dw-rockchip: unclocked MMIO and double disable in rockchip_pcie_suspend()
- [Medium] PCI: dw-rockchip: wrong IRQ mask and order in rockchip_pcie_resume()
- [Low] PCI: dw-rockchip: signed shift overflow in rockchip_pcie_resume()
--

commit 941dec7d380bdb3a689bc84885404d2f2e8eef67
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

PCI: dw-rockchip: Add system PM support

This commit adds noirq system suspend and resume callbacks for Rockchip
DesignWare PCIe controllers in Root Complex mode. It saves and restores the
legacy interrupt mask and sequences the PCIe link, PERST# GPIO, PHY, clocks,
reset line, and 3.3V regulator across suspend and resume.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 2e759f92b5c6f..75b40091bb953 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c

[ ... ]

> @@ -1037,6 +1039,102 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
>  	return ret;
>  }
>  
> +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;
> +
> +	rockchip->intx = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_MASK_LEGACY);
> +
> +	ret = dw_pcie_suspend_noirq(pci);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
> +	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);

[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?

> +
> +	return 0;
> +}
> +
> +static int rockchip_pcie_resume(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 (rockchip->vpcie3v3) {
> +		ret = regulator_enable(rockchip->vpcie3v3);
> +		if (ret) {
> +			dev_err(dev, "regulator enable failed: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	ret = rockchip_pcie_phy_init(rockchip);
> +	if (ret) {
> +		dev_err(dev, "phy init failed: %d\n", ret);
> +		goto err_disable_regulator;
> +	}
> +
> +	ret = reset_control_deassert(rockchip->rst);
> +	if (ret) {
> +		dev_err(dev, "reset deassert failed: %d\n", ret);
> +		goto err_deinit_phy;
> +	}
> +
> +	ret = clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks);
> +	if (ret) {
> +		dev_err(dev, "clock init failed: %d\n", ret);
> +		goto err_assert_reset;
> +	}
> +
> +	rockchip_pcie_writel_apb(rockchip, FIELD_PREP_WM16(0xffff, rockchip->intx),
> +				 PCIE_CLIENT_INTR_MASK_LEGACY);

[Severity: Low]
Could passing 0xffff to FIELD_PREP_WM16() trigger a signed shift overflow in
rockchip_pcie_resume()?

FIELD_PREP_WM16() preserves the type of _mask and shifts it left by 16:

include/linux/hw_bitfield.h:
#define FIELD_PREP_WM16(_mask, _val)					     \
	({								     \
		typeof(_val) __val = _val;				     \
		typeof(_mask) __mask = _mask;				     \
		__BF_FIELD_CHECK(__mask, ((u16)0U), __val,		     \
				 "HWORD_UPDATE: ");			     \
		(((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) | \
		((__mask) << 16);					     \
	})

Because 0xffff is a signed int literal with bit 15 set, (__mask) << 16 shifts
into the sign bit of a 32-bit signed int, triggering a UBSAN
shift-out-of-bounds warning when CONFIG_UBSAN_SHIFT is enabled. Should this
use PCIE_INTR_MASK (GENMASK(7, 0)) or an unsigned mask instead?

> +
> +	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);

[Severity: Medium]
Should rockchip_pcie_resume() be calling rockchip_pcie_unmask_dll_indicator()
here in Root Complex mode and before dw_pcie_resume_noirq()?

Looking at rockchip_pcie_unmask_dll_indicator():

drivers/pci/controller/dwc/pcie-dw-rockchip.c:rockchip_pcie_unmask_dll_indicator() {
	u32 val;

	/* unmask DLL up/down indicator and hot reset/link-down reset */
	val = FIELD_PREP_WM16(PCIE_RDLH_LINK_UP_CHGED, 0) |
	      FIELD_PREP_WM16(PCIE_LINK_REQ_RST_NOT_INT, 0);
	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
}

In RC mode, rockchip_pcie_configure_rc() and
rockchip_pcie_rc_reset_root_port() only unmask PCIE_LINK_REQ_RST_NOT_INT, and
rockchip_pcie_configure_rc() only does so after dw_pcie_host_init() has
configured the RC and brought up the link:

drivers/pci/controller/dwc/pcie-dw-rockchip.c:rockchip_pcie_configure_rc() {
	...
	ret = dw_pcie_host_init(pp);
	if (ret) {
		dev_err(dev, "failed to initialize host\n");
		return ret;
	}

	/* unmask hot reset/link-down reset */
	val = FIELD_PREP_WM16(PCIE_LINK_REQ_RST_NOT_INT, 0);
	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
}

Because rockchip_pcie_rc_sys_irq_thread() only handles
PCIE_LINK_REQ_RST_NOT_INT (whereas PCIE_RDLH_LINK_UP_CHGED is handled by
rockchip_pcie_ep_sys_irq_thread()), won't calling
rockchip_pcie_unmask_dll_indicator() before dw_pcie_resume_noirq() cause
spurious pcie-sys-rc interrupts on link-up changes and allow link transitions
during resume link training to latch PCIE_LINK_REQ_RST_NOT_INT and trigger
pci_host_handle_link_down() once IRQs are re-enabled?

> +
> +	ret = gpiod_set_value_cansleep(rockchip->rst_gpio, 1);
> +	if (ret) {
> +		dev_err(dev, "failed to set reset gpio: %d\n", ret);
> +		goto err_disable_clk;
> +	}
> +
> +	ret = dw_pcie_resume_noirq(pci);

[Severity: High]
Does setting rockchip->rst_gpio to 1 before dw_pcie_resume_noirq() cause a
premature PERST# deassertion and reset glitch on resume?

In rockchip_pcie_resume(), gpiod_set_value_cansleep(rockchip->rst_gpio, 1)
deasserts PERST# before dw_pcie_resume_noirq() runs. Immediately afterward,
dw_pcie_resume_noirq() calls dw_pcie_setup_rc() and
dw_pcie_start_link() -> rockchip_pcie_start_link(), which drives
rockchip->rst_gpio back to 0, enables LTSSM, waits PCIE_T_PVPERL_MS (100ms),
and then drives rockchip->rst_gpio to 1:

drivers/pci/controller/dwc/pcie-dw-rockchip.c:rockchip_pcie_start_link() {
	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);

	/* Reset device */
	gpiod_set_value_cansleep(rockchip->rst_gpio, 0);

	rockchip_pcie_enable_ltssm(rockchip);
	...
	msleep(PCIE_T_PVPERL_MS);

	rockchip_pcie_ltssm_trace(rockchip, true);

	gpiod_set_value_cansleep(rockchip->rst_gpio, 1);

	return 0;
}

Won't driving rockchip->rst_gpio high before rockchip_pcie_start_link()
produce a 0 -> 1 -> 0 -> 1 glitch on PERST# before the RC is configured,
before LTSSM and refclk are enabled, and before PCIE_T_PVPERL_MS has elapsed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-rockchip-pcie-system-suspend-v7-0-6ae6875939a2@collabora.com?part=9