Re: [PATCH v3 3/4] PCI: tegra194: Notify Endpoint functions on PERST# assert

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI: tegra194: missing IRQ sync in pex_ep_event_pex_rst_assert()
- [High] PCI: tegra194: deadlock between pex_ep_event_pex_rst_assert() and pci_epc_stop()
--

commit 9fcf1c9f540e3e347bfaf4edb431bb510324fd09
Author: Manikanta Maddireddy <mmaddireddy@nvidia.com>

PCI: tegra194: Notify Endpoint functions on PERST# assert

This commit calls pci_epc_linkdown() from pex_ep_event_pex_rst_assert()
before powering down the Tegra194 PCIe endpoint controller on PERST#
assertion so endpoint function drivers can stop outstanding work.

> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 8cb907dbf6356..8dede9531956d 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -1704,6 +1704,9 @@ static void pex_ep_event_pex_rst_assert(struct tegra_pcie_dw *pcie)
>  	if (ret)
>  		dev_info(pcie->dev, "LTSSM state: 0x%x detect timeout: %d\n", val, ret);
>  
> +	/* Avoid the DBI register reinitialization in dw_pcie_ep_linkdown(). */
> +	pci_epc_linkdown(pcie->pci.ep.epc);

[Severity: High]
Can calling pci_epc_linkdown() here race with a pending or running
tegra_pcie_ep_irq_thread() on pcie->pci.pp.irq?

In endpoint mode, tegra_pcie_ep_hard_irq() and tegra_pcie_ep_irq_thread()
run on pcie->pci.pp.irq, while tegra_pcie_ep_pex_rst_irq() runs on the
separate GPIO interrupt pcie->pex_rst_irq:

drivers/pci/controller/dwc/pcie-tegra194.c:tegra_pcie_ep_hard_irq() {
	...
			if (link_status & APPL_LINK_STATUS_RDLH_LINK_UP) {
				dev_dbg(pcie->dev, "Link is up with Host\n");
				set_bit(0, &pcie->link_status);
				return IRQ_WAKE_THREAD;
			}
	...
}

drivers/pci/controller/dwc/pcie-tegra194.c:tegra_pcie_ep_irq_thread() {
	...
	if (test_and_clear_bit(0, &pcie->link_status))
		dw_pcie_ep_linkup(ep);

	tegra_pcie_icc_set(pcie);
	...
}

If tegra_pcie_ep_hard_irq() fires just before PERST# assertion (or before
APPL_CTRL_LTSSM_EN is cleared), sets bit 0 in pcie->link_status, and wakes
tegra_pcie_ep_irq_thread(), pex_ep_event_pex_rst_assert() calls
pci_epc_linkdown() without disabling LTSSM or controller interrupts first,
without calling synchronize_irq(pcie->pci.pp.irq), and without clearing
bit 0 of pcie->link_status.

Wouldn't tegra_pcie_ep_irq_thread() then observe bit 0 set in
pcie->link_status and call dw_pcie_ep_linkup() -> pci_epc_linkup() after
pci_epc_linkdown() has already returned, re-arming endpoint function
workers (such as epf_test->cmd_handler) right before the controller is
powered down, as well as reading DBI registers in tegra_pcie_icc_set()
while pex_ep_event_pex_rst_assert() asserts resets and disables clocks?

[Severity: High]
Also, could calling pci_epc_linkdown() inside the pex_rst_irq threaded
handler deadlock against pci_epc_stop()?

When userspace stops the endpoint controller through configfs,
pci_epc_start_store() -> pci_epc_stop() holds epc->lock across
epc->ops->stop(epc):

drivers/pci/endpoint/pci-epc-core.c:pci_epc_stop() {
	...
	mutex_lock(&epc->lock);
	epc->ops->stop(epc);
	mutex_unlock(&epc->lock);
}

And epc->ops->stop() calls dw_pcie_ep_stop() -> dw_pcie_stop_link() ->
tegra_pcie_dw_stop_link(), which waits for the pex_rst_irq handler to
finish:

drivers/pci/controller/dwc/pcie-tegra194.c:tegra_pcie_dw_stop_link() {
	struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);

	if (pcie->of_data->mode == DW_PCIE_EP_TYPE)
		disable_irq(pcie->pex_rst_irq);
}

Meanwhile, if tegra_pcie_ep_pex_rst_irq() -> pex_ep_event_pex_rst_assert()
is running concurrently and calls pci_epc_linkdown(), endpoint function
link_down callbacks can synchronously wait on or invoke operations that
acquire epc->lock. For example, pci_epf_test_link_down() calls:

drivers/pci/endpoint/functions/pci-epf-test.c:pci_epf_test_link_down() {
	...
	cancel_delayed_work_sync(&epf_test->cmd_handler);
	...
}

while pci_epf_test_cmd_handler() calls pci_epc_map_addr(),
pci_epc_unmap_addr(), pci_epc_raise_irq(), and pci_epc_set_bar(), all of
which take mutex_lock(&epc->lock) (and pci_epf_mhi_link_down() directly
calls mhi_ep_power_down() -> mhi_ep_abort_transfer() ->
mhi_ep_free_host_cfg() -> pci_epf_mhi_unmap_free() ->
pci_epc_unmap_addr()).

Doesn't this create a lock inversion between epc->lock and
disable_irq(pcie->pex_rst_irq)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102111450.4010818-1-mmaddireddy@nvidia.com?part=3