Re: [PATCH v2 03/10] PCI: tegra194: Fix Endpoint PERST# IRQ suspend race

[Frank Li <Frank.li@oss.nxp.com>]

On Wed, Oct 21, 2026 at 11:43:42AM +0530, Manikanta Maddireddy wrote:
> Endpoint system suspend is allowed only while the endpoint is disabled.
> Checking ep_state before disabling the PERST# IRQ leaves a window in which
> PERST# deassertion starts the threaded handler after the check. The
> handler can enable the controller, clocks and PHYs before disable_irq()
> returns, but suspend still succeeds based on the earlier disabled state.
> The system then proceeds into suspend with an active endpoint that should
> have caused the suspend callback to reject the transition.
>
> Disable the PERST# IRQ before checking ep_state. disable_irq() waits for
> an already-running threaded handler to finish, so the state check sees
> its completed transition and no new handler can enable the endpoint.
> Re-enable the IRQ immediately if the endpoint is enabled and reject
> suspend. Otherwise, keep it disabled until the normal resume callback.
> This also balances the IRQ disable when another device aborts suspend
> and PM rolls back through .resume without calling .resume_early.
>
> This preserves the existing policy that PERST# is not serviced while the
> endpoint is suspended. It serializes the software state check with the
> IRQ thread; it does not make PERST# a wakeup source or guarantee delivery
> of transitions during suspension.
>
> Wrap the PM ops pointer with pm_sleep_ptr() so the callbacks are used
> only when system sleep is enabled.
>
> Fixes: c76f8eae7d46 ("PCI: tegra194: Allow system suspend when the Endpoint link is not up")
> Link: https://lore.kernel.org/r/20260324190755.1094879-10-mmaddireddy@nvidia.com
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Changes V1 -> V2:
> - Expand the commit message to explain the active-endpoint suspend race,
>   IRQ-thread synchronization and suspend rollback handling.
> - Clarify that PERST# is not serviced while the endpoint is suspended.
> - Add a comment explaining why the IRQ thread is drained before checking
>   endpoint state.
>
>  drivers/pci/controller/dwc/pcie-tegra194.c | 34 ++++++++++++++--------
>  1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 89dbef83dd9e..0f895c766a71 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -2374,19 +2374,30 @@ static int tegra_pcie_dw_suspend(struct device *dev)
>  {
>  	struct tegra_pcie_dw *pcie = dev_get_drvdata(dev);
>
> -	if (pcie->of_data->mode == DW_PCIE_EP_TYPE) {
> -		if (pcie->ep_state == EP_STATE_ENABLED) {
> -			dev_err(dev, "Tegra PCIe is in EP mode, suspend not allowed\n");
> -			return -EPERM;
> -		}
> -
> -		disable_irq(pcie->pex_rst_irq);
> +	if (pcie->of_data->mode != DW_PCIE_EP_TYPE)
>  		return 0;
> +
> +	/* Drain the PERST# thread before checking whether the endpoint is idle. */
> +	disable_irq(pcie->pex_rst_irq);
> +	if (pcie->ep_state == EP_STATE_ENABLED) {
> +		enable_irq(pcie->pex_rst_irq);
> +		dev_err(dev, "Tegra PCIe is in EP mode, suspend not allowed\n");
> +		return -EPERM;
>  	}
>
>  	return 0;
>  }
>
> +static int tegra_pcie_dw_resume(struct device *dev)
> +{
> +	struct tegra_pcie_dw *pcie = dev_get_drvdata(dev);
> +
> +	if (pcie->of_data->mode == DW_PCIE_EP_TYPE)
> +		enable_irq(pcie->pex_rst_irq);
> +
> +	return 0;
> +}
> +
>  static int tegra_pcie_dw_suspend_late(struct device *dev)
>  {
>  	struct tegra_pcie_dw *pcie = dev_get_drvdata(dev);
> @@ -2462,10 +2473,8 @@ static int tegra_pcie_dw_resume_early(struct device *dev)
>  	struct tegra_pcie_dw *pcie = dev_get_drvdata(dev);
>  	u32 val;
>
> -	if (pcie->of_data->mode == DW_PCIE_EP_TYPE) {
> -		enable_irq(pcie->pex_rst_irq);
> +	if (pcie->of_data->mode == DW_PCIE_EP_TYPE)
>  		return 0;
> -	}
>
>  	if (!pcie->link_state)
>  		return 0;
> @@ -2569,10 +2578,11 @@ static const struct of_device_id tegra_pcie_dw_of_match[] = {
>
>  static const struct dev_pm_ops tegra_pcie_dw_pm_ops = {
>  	.suspend = tegra_pcie_dw_suspend,
> +	.resume = tegra_pcie_dw_resume,
>  	.suspend_late = tegra_pcie_dw_suspend_late,
> +	.resume_early = tegra_pcie_dw_resume_early,
>  	.suspend_noirq = tegra_pcie_dw_suspend_noirq,
>  	.resume_noirq = tegra_pcie_dw_resume_noirq,
> -	.resume_early = tegra_pcie_dw_resume_early,
>  };
>
>  static struct platform_driver tegra_pcie_dw_driver = {
> @@ -2581,7 +2591,7 @@ static struct platform_driver tegra_pcie_dw_driver = {
>  	.shutdown = tegra_pcie_dw_shutdown,
>  	.driver = {
>  		.name	= "tegra194-pcie",
> -		.pm = &tegra_pcie_dw_pm_ops,
> +		.pm = pm_sleep_ptr(&tegra_pcie_dw_pm_ops),
>  		.of_match_table = tegra_pcie_dw_of_match,
>  	},
>  };
> --
> 2.34.1
>