Re: [PATCH v8 10/14] PCI: tegra194: Free up EP resources during remove()

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

New issues:
- [High] Calling `dw_pcie_ep_deinit()` after the hardware is manually powered down causes a synchronous external abort when accessing eDMA registers.

Pre-existing issues:
- [High] The shared IRQ handlers tegra_pcie_ep_hard_irq() and tegra_pcie_rp_irq_handler() access memory-mapped registers without verifying if the device is active.
- [High] The shared PCIe IRQ handler remains active while hardware is powered down during and after device removal, risking an SError crash.
- [Medium] An early return in `tegra_pcie_dw_remove()` for RC mode skips vital cleanup steps, leaking the BPMP reference and leaving a GPIO asserted.
- [Low] The `dev_pm_ops` structure and its callbacks lack the `pm_ptr()` and `pm_sleep_ptr()` wrappers, causing dead code when PM is disabled.
--

--- Patch [10]: [PATCH v8 10/14] PCI: tegra194: Free up EP resources during remove() ---
commit fd329e127a60d994cdbbfaa29a5a96dddb4a9df8
Author: Vidya Sagar <[email protected]>

PCI: tegra194: Free up EP resources during remove()

Free up the resources during remove() that were acquired by the DesignWare
driver for the Endpoint mode during probe().

> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index f66a93f0cf658..d4bbef07964fe 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -2251,6 +2251,7 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
>  static void tegra_pcie_dw_remove(struct platform_device *pdev)
>  {
>  	struct tegra_pcie_dw *pcie = platform_get_drvdata(pdev);
> +	struct dw_pcie_ep *ep = &pcie->pci.ep;
>  
>  	if (pcie->of_data->mode == DW_PCIE_RC_TYPE) {
>  		if (!pcie->link_state)
>  			return;

[Severity: Medium]
This is a pre-existing issue, but does this early return in
tegra_pcie_dw_remove() skip vital cleanup steps?

During probe, an inactive link returns -ENOMEDIUM which is ignored, allowing
probe to succeed with !pcie->link_state.

When returning early here for RC mode without an active link, it bypasses the
trailing cleanup sequence, which leaves tegra_bpmp_put() uncalled and the
pex_refclk_sel_gpiod GPIO asserted.

> @@ -2262,6 +2263,7 @@ static void tegra_pcie_dw_remove(struct platform_device *pdev)
>  	} else {
>  		disable_irq(pcie->pex_rst_irq);
>  		pex_ep_event_pex_rst_assert(pcie);

[Severity: High]
This is a pre-existing issue, but does powering down the controller here leave
the shared PCIe IRQ handler active?

Since the main PCIe interrupt is registered using IRQF_SHARED, turning off the
controller's power by asserting resets and disabling clocks without explicitly
freeing or disabling the main IRQ creates a race condition.

Can an active interrupt handler execute and read memory-mapped registers on
powered-down hardware before devres frees the IRQ after remove() returns?

> +		dw_pcie_ep_deinit(ep);

[Severity: High]
Does calling dw_pcie_ep_deinit() after the hardware is manually powered down
cause a synchronous external abort when accessing eDMA registers?

When pex_ep_event_pex_rst_assert() is called right before this, it completely
powers down the PCIe IP by gating clocks and asserting hardware resets.

Then dw_pcie_ep_deinit() attempts memory-mapped writes to the eDMA registers
of the powered-down controller via dw_pcie_edma_remove() and
dw_edma_core_off().

Could dw_pcie_ep_deinit() be called before asserting the reset?

>  	}
>  
>  	pm_runtime_disable(pcie->dev);

[Severity: High]
This is a pre-existing issue, but do the shared IRQ handlers access
memory-mapped registers without verifying if the device is active?

In drivers/pci/controller/dwc/pcie-tegra194.c:tegra_pcie_ep_hard_irq():

	status_l0 = appl_readl(pcie, APPL_INTR_STATUS_L0);

And similarly in drivers/pci/controller/dwc/pcie-tegra194.c:tegra_pcie_rp_irq_handler():

	val = appl_readl(pcie, APPL_INTR_STATUS_L0);

Since these IRQ handlers are registered with IRQF_SHARED, they can be
triggered by external or unrelated devices while the PCIe controller is
runtime-suspended.

Would reading the APPL_INTR_STATUS_L0 register while clocks are gated cause a
fatal system crash?

Should these handlers call pm_runtime_get_if_active() before reading registers?

[Severity: Low]
This is a pre-existing issue, but does the dev_pm_ops structure lack the
pm_ptr() and pm_sleep_ptr() wrappers?

In drivers/pci/controller/dwc/pcie-tegra194.c:

static const struct dev_pm_ops tegra_pcie_dw_pm_ops = {
	.suspend = tegra_pcie_dw_suspend,
	...
};

static struct platform_driver tegra_pcie_dw_driver = {
	...
	.driver = {
		...
		.pm = &tegra_pcie_dw_pm_ops,
	},
};

Since the callbacks and dev_pm_ops structure are assigned unconditionally,
does this cause dead code to be compiled into the kernel when CONFIG_PM or
CONFIG_PM_SLEEP is disabled?

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