Re: [PATCH v8 09/14] PCI: tegra194: Allow system suspend when the Endpoint link is not up

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PM callbacks are asymmetric, missing a `.resume` callback, which leaves `pex_rst_irq` permanently disabled if the system suspend process is aborted.
- [High] Time-of-check to time-of-use (TOCTOU) race in `tegra_pcie_dw_suspend()` allows the system to suspend while the Endpoint link is active.
--

--- Patch [9]: [PATCH v8 09/14] PCI: tegra194: Allow system suspend when the Endpoint link is not up ---
commit 9f02485e505b10cc3b88208af0183e49b85c7e80
Author: Vidya Sagar <[email protected]>

PCI: tegra194: Allow system suspend when the Endpoint link is not up

Host software initiates the L2 sequence. PCIe link is kept in L2 state
during suspend. If Endpoint mode is enabled and the link is up, the
software cannot proceed with suspend. However, when the PCIe Endpoint
driver is probed, but the PCIe link is not up, Tegra can go into suspend
state. So, allow system to suspend in this case.

> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index f1985fca988ee..f66a93f0cf658 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -2270,16 +2270,28 @@ static void tegra_pcie_dw_remove(struct platform_device *pdev)
>  		gpiod_set_value(pcie->pex_refclk_sel_gpiod, 0);
>  }
>  
> -static int tegra_pcie_dw_suspend_late(struct device *dev)
> +static int tegra_pcie_dw_suspend(struct device *dev)
>  {
>  	struct tegra_pcie_dw *pcie = dev_get_drvdata(dev);
> -	u32 val;
>  
>  	if (pcie->of_data->mode == DW_PCIE_EP_TYPE) {
> -		dev_err(dev, "Failed to Suspend as Tegra PCIe is in EP mode\n");
> -		return -EPERM;
> +		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);

[Severity: High]
Is there a race condition between checking the endpoint state and disabling
the IRQ here?

If the host deasserts PERST# just after ep_state is checked but before
disable_irq() executes, the tegra_pcie_ep_pex_rst_irq threaded IRQ handler
could run concurrently.

This would allow the endpoint state to change to EP_STATE_ENABLED,
bypassing the safety check and allowing the system to suspend while the
link is active.

Could the IRQ be disabled prior to checking ep_state, and re-enabled on
the error path if suspend is not permitted?

> +		return 0;
>  	}
>  
> +	return 0;
> +}
> +

[ ... ]

> @@ -2345,8 +2363,8 @@ static int tegra_pcie_dw_resume_early(struct device *dev)
>  	u32 val;
>  
>  	if (pcie->of_data->mode == DW_PCIE_EP_TYPE) {
> -		dev_err(dev, "Suspend is not supported in EP mode");
> -		return -ENOTSUPP;
> +		enable_irq(pcie->pex_rst_irq);
> +		return 0;
>  	}
>  
>  	if (!pcie->link_state)

[ ... ]

> @@ -2451,6 +2469,7 @@ 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,
>  	.suspend_late = tegra_pcie_dw_suspend_late,
>  	.suspend_noirq = tegra_pcie_dw_suspend_noirq,
>  	.resume_noirq = tegra_pcie_dw_resume_noirq,

[Severity: High]
Does this introduce an asymmetry in the PM callbacks by placing disable_irq()
in .suspend but enable_irq() in .resume_early?

If a system suspend is aborted due to a failure in another device's suspend
callback, the PM core rolls back by calling .resume on successfully
suspended devices. The .resume_early phase would be skipped entirely.

Because there is no .resume callback defined here to pair with .suspend,
pex_rst_irq would remain permanently disabled after a failed suspend,
breaking the PCIe endpoint PERST# reset functionality.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9