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

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

[Enrique Hernández Bello]

Since commit c47f90be4c89 ("PCI: rockchip-host: Fix
rockchip_pcie_host_init_port() PERST# handling"), a JMicron JMB585
behind an rk3399 root port almost never becomes usable: the link trains
normally, but the endpoint's configuration space never answers, so the
device is not enumerated. On this controller a configuration read that
gets no usable completion is reported as an external abort rather than
as an all-ones response, which on arm64 brings the machine down.

The change added an unconditional 100 ms sleep so that PERST# stays
asserted for at least Tpvperl after power becomes valid. The wait is
performed while PERST# is asserted, so it also extends the reset by
100 ms, and this endpoint does not tolerate the longer assertion.

Tpvperl is counted from the supplies becoming valid (PCIe CEM r5.1,
sec 2.9.2). On boards whose PCIe supplies are always-on -- vcc3v3_pcie
on ROCK Pi 4 is regulator-always-on and regulator-boot-on -- power has
been valid since boot, seconds before the driver probes, so the
requirement is already met and the sleep only lengthens the reset.

Record whether the supplies were already enabled before the driver
enabled them, and skip the wait in that case. A supply that is already
on at probe was brought up either by the bootloader or by the regulator
core at boot, both of which precede a PCIe probe by far more than
Tpvperl. When the driver really does bring the rails up, or on resume
where vpcie0v9 has just been re-enabled, the full wait still happens,
as it does if regulator_is_enabled() cannot tell.

Measured on a ROCK Pi 4C with a Radxa Penta SATA HAT (JMB585) by
booting repeatedly and counting how often the endpoint enumerated:

  unmodified ....................................  0 out of 84 boots
  with this patch ...............................  3 out of 3 boots
  other ways of dropping the same wait ..........  16 out of 16 boots

Fisher's exact test, pooling the last two rows against the first, gives
p = 4.1e-21. With the patch the endpoint enumerated on every boot and all
four disks behind it came up.

Each of the three PERST#-related changes that landed together in
v6.11-rc1 was also reverted individually; only removing this wait made
any difference. Moving the wait to before link training is enabled,
rather than removing it, did not help (0 out of 15 boots), which is
what identified the length of the PERST# assertion rather than any
interaction with link training as the cause.

The measurements were taken on 6.18, but the code in question is
unchanged between v6.11 and v7.2.

Fixes: c47f90be4c89 ("PCI: rockchip-host: Fix rockchip_pcie_host_init_port() PERST# handling")
Cc: stable@vger.kernel.org
Signed-off-by: Enrique Hernández Bello <ehbello@gmail.com>
---
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -318,6 +318,7 @@
 	struct	regulator *vpcie1v8; /* 1.8V power supply */
 	struct	regulator *vpcie0v9; /* 0.9V power supply */
 	struct	gpio_desc *perst_gpio;
+	bool	supplies_pre_enabled;
 	u32	lanes;
 	u8      lanes_map;
 	int	link_gen;
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -314,7 +314,9 @@
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
 			    PCIE_CLIENT_CONFIG);
 
-	msleep(PCIE_T_PVPERL_MS);
+	if (!rockchip->supplies_pre_enabled)
+		msleep(PCIE_T_PVPERL_MS);
+
 	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
 
 	msleep(PCIE_RESET_CONFIG_WAIT_MS);
@@ -614,6 +616,23 @@
 	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;
+
 	if (!IS_ERR(rockchip->vpcie12v)) {
 		err = regulator_enable(rockchip->vpcie12v);
 		if (err) {
@@ -890,6 +909,9 @@
 	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;
+
 	err = regulator_enable(rockchip->vpcie0v9);
 	if (err) {
 		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
-- 
2.43.0

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

[Enrique Hernández Bello]

Since commit c47f90be4c89 ("PCI: rockchip-host: Fix
rockchip_pcie_host_init_port() PERST# handling"), a JMicron JMB585
behind an rk3399 root port almost never becomes usable: the link trains
normally, but the endpoint's configuration space never answers, so the
device is not enumerated. On this controller a configuration read that
gets no usable completion is reported as an external abort rather than
as an all-ones response, which on arm64 brings the machine down.

The change added an unconditional 100 ms sleep so that PERST# stays
asserted for at least Tpvperl after power becomes valid. The wait is
performed while PERST# is asserted, so it also extends the reset by
100 ms, and this endpoint does not tolerate the longer assertion.

Tpvperl is counted from the supplies becoming valid (PCIe CEM r5.1,
sec 2.9.2). On boards whose PCIe supplies are always-on -- vcc3v3_pcie
on ROCK Pi 4 is regulator-always-on and regulator-boot-on -- power has
been valid since boot, seconds before the driver probes, so the
requirement is already met and the sleep only lengthens the reset.

Record whether the supplies were already enabled before the driver
enabled them, and skip the wait in that case. A supply that is already
on at probe was brought up either by the bootloader or by the regulator
core at boot, both of which precede a PCIe probe by far more than
Tpvperl. When the driver really does bring the rails up, or on resume
where vpcie0v9 has just been re-enabled, the full wait still happens,
as it does if regulator_is_enabled() cannot tell.

Measured on a ROCK Pi 4C with a Radxa Penta SATA HAT (JMB585) by
booting repeatedly and counting how often the endpoint enumerated:

  unmodified ....................................  0 out of 84 boots
  with this patch ...............................  3 out of 3 boots
  other ways of dropping the same wait ..........  16 out of 16 boots

Fisher's exact test, pooling the last two rows against the first, gives
p = 4.1e-21. With the patch the endpoint enumerated on every boot and all
four disks behind it came up.

Each of the three PERST#-related changes that landed together in
v6.11-rc1 was also reverted individually; only removing this wait made
any difference. Moving the wait to before link training is enabled,
rather than removing it, did not help (0 out of 15 boots), which is
what identified the length of the PERST# assertion rather than any
interaction with link training as the cause.

The measurements were taken on 6.18, but the code in question is
unchanged between v6.11 and v7.2.

Fixes: c47f90be4c89 ("PCI: rockchip-host: Fix rockchip_pcie_host_init_port() PERST# handling")
Cc: stable@vger.kernel.org
Signed-off-by: Enrique Hernández Bello <ehbello@gmail.com>
---
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -318,6 +318,7 @@
 	struct	regulator *vpcie1v8; /* 1.8V power supply */
 	struct	regulator *vpcie0v9; /* 0.9V power supply */
 	struct	gpio_desc *perst_gpio;
+	bool	supplies_pre_enabled;
 	u32	lanes;
 	u8      lanes_map;
 	int	link_gen;
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -314,7 +314,9 @@
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
 			    PCIE_CLIENT_CONFIG);
 
-	msleep(PCIE_T_PVPERL_MS);
+	if (!rockchip->supplies_pre_enabled)
+		msleep(PCIE_T_PVPERL_MS);
+
 	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
 
 	msleep(PCIE_RESET_CONFIG_WAIT_MS);
@@ -614,6 +616,23 @@
 	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;
+
 	if (!IS_ERR(rockchip->vpcie12v)) {
 		err = regulator_enable(rockchip->vpcie12v);
 		if (err) {
@@ -890,6 +909,9 @@
 	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;
+
 	err = regulator_enable(rockchip->vpcie0v9);
 	if (err) {
 		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
-- 
2.43.0

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[sashiko-bot]

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