Re: [PATCH v9 4/5] PCI: qcom: Add support for resetting the Root Port due to link down event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unprotected iteration over PCI bus devices risks list corruption and use-after-free.
- [High] Accidental routing of MSI interrupts to the global IRQ thread causes an interrupt storm.
- [Medium] Platform devices created for power controllers are leaked if the newly added global IRQ request fails.
--

--- Patch [4]: [PATCH v9 4/5] PCI: qcom: Add support for resetting the Root Port due to link down event ---
commit 89d5e2336d3e173c1acc29f1a3ce080e06d174b3
Author: Manivannan Sadhasivam <mani@kernel.org>

PCI: qcom: Add support for resetting the Root Port due to link down event

This commit adds handling for PCIe link down events on Qcom controllers
by resetting the Root Port. It enables the link down event in the 
PARF_INT_ALL_MASK register and requests a global IRQ to detect and recover
from these events.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
[ ... ]
> @@ -1734,6 +1754,75 @@ static int qcom_pcie_set_max_opp(struct device *dev)
[ ... ]
> +static int qcom_pcie_reset_root_port(struct pci_host_bridge *bridge,
> +				  struct pci_dev *pdev)
> +{
[ ... ]
> +	/*
> +	 * Re-enable global IRQ events as the PARF_INT_ALL_MASK register is
> +	 * non-sticky.
> +	 */
> +	if (pcie->global_irq)
> +		writel_relaxed(PARF_INT_ALL_LINK_DOWN | PARF_INT_MSI_DEV_0_7,
> +				pcie->parf + PARF_INT_ALL_MASK);

[Severity: High]
Does unmasking PARF_INT_MSI_DEV_0_7 here (and during probe) inadvertently
route downstream MSI interrupts to the global IRQ thread instead of the
standard MSI handler?

Since modern endpoints can generate thousands of MSIs per second, could
this cause an interrupt storm that saturates the CPU when the global
threaded handler is woken up for every single MSI event?

> +
> +	qcom_pcie_start_link(pci);
[ ... ]
> +static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
> +{
> +	struct qcom_pcie *pcie = data;
> +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> +	struct device *dev = pcie->pci->dev;
> +	struct pci_dev *port;
> +	unsigned long status = readl_relaxed(pcie->parf + PARF_INT_ALL_STATUS);
> +
> +	writel_relaxed(status, pcie->parf + PARF_INT_ALL_CLEAR);

[Severity: High]
Since PARF_INT_MSI_DEV_0_7 is unmasked, will this unconditional read and
clear of PARF_INT_ALL_STATUS steal the MSI events from the actual DWC MSI
handler? Might this cause MSIs to be silently dropped?

> +
> +	if (test_and_clear_bit(INT_ALL_LINK_DOWN, &status)) {
> +		dev_dbg(dev, "Received Link down event\n");
> +		for_each_pci_bridge(port, pp->bridge->bus) {

[Severity: High]
Is it safe to iterate over pp->bridge->bus->devices without holding the
pci_bus_sem reader lock?

If a user triggers the removal of the root port device via sysfs concurrently
with a link down event, could this cause list corruption or a use-after-free?

Also, does the loop need to take a reference to the port device via
pci_dev_get() before passing it to pci_host_handle_link_down() to ensure
the device structure isn't freed while being handled?

> +			if (pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
> +				pci_host_handle_link_down(port);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
[ ... ]
> @@ -2135,6 +2245,32 @@ static int qcom_pcie_probe(struct platform_device *pdev)
[ ... ]
> +		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +						qcom_pcie_global_irq_thread,
> +						IRQF_ONESHOT, name, pcie);
> +		if (ret) {
> +			dev_err_probe(&pdev->dev, ret,
> +				      "Failed to request Global IRQ\n");
> +			goto err_host_deinit;

[Severity: Medium]
Can this error path cause a resource leak of platform devices?

When qcom_pcie_host_init() succeeds earlier in probe, it creates power
controller devices via pci_pwrctrl_create_devices(). If the newly added
global IRQ request then fails here, it jumps to err_host_deinit which
eventually calls qcom_pcie_host_deinit().

However, qcom_pcie_host_deinit() explicitly assumes it is only called
during system suspend and skips calling pci_pwrctrl_destroy_devices():

	if (!pci->pp.skip_pwrctrl_off) {
		/*
		 * No need to destroy pwrctrl devices as this function only
		 * gets called during system suspend as of now.
		 */
		pci_pwrctrl_power_off_devices(pci->dev);
	}

Will this leave the power controller devices permanently leaked on probe
failure?

> +		}
> +
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-pci-port-reset-v9-0-53570b92064d@oss.qualcomm.com?part=4