Re: [PATCH] PCI: rockchip: Skip the Tpvperl wait when power is already valid

[sashiko-bot@kernel.org]

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

New issues:
- [High] The driver unconditionally forces the 100ms T_PVPERL wait on resume, which will cause the same endpoint enumeration failure on boards where the 0.9V regulator remains on during suspend.
- [Medium] The logic to skip the PCIe T_PVPERL wait assumes that any enabled regulator was brought up by the bootloader and has been stable for >100ms, which fails for shared regulators enabled by recently probed drivers.

Pre-existing issues:
- [High] The chained IRQ handler for the 'legacy' interrupt is never removed on probe failure or driver removal, leading to a Use-After-Free.
--

commit 4d72f1c6e9bdfdfe2b1710bb4e36490f981a6798
Author: Enrique Hernández Bello <ehbello@gmail.com>

PCI: rockchip: Skip the Tpvperl wait when power is already valid

This commit aims to fix PCIe endpoint enumeration failures on the rk3399 by
skipping the 100ms T_PVPERL wait if the PCIe regulators are already enabled
when the driver probes. It checks regulator_is_enabled() on the supplies and
sets a flag to skip the wait, while forcing the wait during system resume.

> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index d203c4876d304..2c88e836dd2a6 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c

[ ... ]

> @@ -614,6 +616,23 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>  	struct device *dev = rockchip->dev;
>  	int err;
>  
> +	/*
> +	 * Tpvperl is counted from the supplies becoming valid, and the wait
> +	 * for it happens with PERST# asserted, so it also lengthens the reset.
> +	 * A supply that is already enabled before this driver enables it was
> +	 * brought up either by the bootloader or by the regulator core at boot,
> +	 * both of which precede this probe by far more than Tpvperl, so the
> +	 * requirement is already met and the wait can be skipped. Treat an
> +	 * error from regulator_is_enabled() as "not known to be on" and wait.
> +	 */
> +	rockchip->supplies_pre_enabled =
> +		(IS_ERR(rockchip->vpcie12v) ||
> +		 regulator_is_enabled(rockchip->vpcie12v) > 0) &&
> +		(IS_ERR(rockchip->vpcie3v3) ||
> +		 regulator_is_enabled(rockchip->vpcie3v3) > 0) &&
> +		regulator_is_enabled(rockchip->vpcie1v8) > 0 &&
> +		regulator_is_enabled(rockchip->vpcie0v9) > 0;

[Severity: Medium]
Could this logic cause a violation of the PCIe CEM T_PVPERL requirement?

If a PCIe supply (e.g., the 0.9V or 1.8V regulator) is shared with another IP
block like a USB PHY, and that driver probes and enables the regulator a few
milliseconds before the PCIe controller probes, regulator_is_enabled() will
return true. 

Does this mean the PCIe driver will incorrectly assume the regulator was
enabled by the bootloader >100ms ago, skip the mandatory T_PVPERL wait, and
de-assert PERST# prematurely, preventing PCIe endpoint enumeration?

[ ... ]

> @@ -890,6 +909,9 @@ static int rockchip_pcie_resume_noirq(struct device *dev)
>  	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>  	int err;
>  
> +	/* The 0.9V supply was turned off on suspend, so Tpvperl applies. */
> +	rockchip->supplies_pre_enabled = false;
> +

[Severity: High]
Does this hardcoded assumption break endpoint enumeration on resume for boards
with always-on regulators?

In rockchip_pcie_suspend_noirq(), regulator_disable(rockchip->vpcie0v9) only
decrements the regulator use-count. On boards where this regulator is marked
regulator-always-on or is shared and remains in use, the power rail stays
physically on during suspend.

On resume, unconditionally setting supplies_pre_enabled = false forces
rockchip_pcie_host_init_port() to sleep for 100ms with PERST# asserted. Since
power was already valid, won't this cause a 100ms warm reset, which is the
exact endpoint enumeration failure this patch intends to fix?

[ ... ]

[Severity: High]
This is a pre-existing issue, but can the legacy IRQ handler cause a
use-after-free on probe failure or driver removal?

In rockchip_pcie_setup_irq(), the chained handler is registered:

	irq_set_chained_handler_and_data(irq,
					 rockchip_pcie_intx_handler,
					 rockchip);

However, there is no corresponding irq_set_chained_handler_and_data(..., NULL)
call in the error path of rockchip_pcie_probe() or in rockchip_pcie_remove().

If pci_host_probe() fails, the devres framework automatically frees the
bridge and rockchip memory. If the interrupt line is shared or spuriously
triggered after removal, won't rockchip_pcie_intx_handler() dereference the
freed rockchip pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911104952.4190994-1-ehbello@gmail.com?part=1