Re: [PATCH] PCI: dwc: Add sysfs for local loopback interface

On 8/3/2026 3:17 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
> - [Critical] `pcibios_align_resource` is used as an alignment callback but receives a `struct device *` instead of a `struct pci_dev *`.
> - [High] The inbound iATU is programmed with the CPU physical address instead of the PCIe bus address.
> - [High] Inbound ATU window configuration is overwritten and permanently disabled, silently destroying prior hardware mappings.
> - [High] Hardware recovery via `dw_pcie_resume_noirq()` will unconditionally fail and mask test outcomes.
> - [High] The inbound iATU window programmed for the loopback test is not disabled if the controller requires reinitialization, leaving a hardware mapping to freed memory active.
> - [High] Missing pm_runtime_get_sync() before accessing hardware registers in the sysfs handler.
> - [High] Calling `deinit` and `resume_noirq` during active system operation without blocking concurrent config space accesses will cause a system crash.
> - [Medium] The loopback test uses `kmalloc()` to allocate an arbitrarily large test buffer (up to 1 MiB), which is highly susceptible to allocation failure due to fragmentation.
> - [Medium] TOCTOU race in link status check can cause disruption of an active link.
> --
>
> --- Patch [1]: [PATCH] PCI: dwc: Add sysfs for local loopback interface ---
> commit 4f4d57ae54b60f668971d41c20d2d0f542109200
> Author: Krishna Chaitanya Chundru <[email protected]>
>
> PCI: dwc: Add sysfs for local loopback interface
>
> This patch adds a loopback sysfs interface to trigger a local
> data-integrity test on DesignWare PCIe root ports. It works by allocating
> a DMA buffer, redirecting PCIe writes to it via an inbound iATU window,
> and verifying the data byte-by-byte. It also includes a recovery mechanism
> to handle LTSSM state issues after the test completes.
>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 06722259d2e37..f0cfe9e885204 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> [ ... ]
>
>> @@ -567,6 +569,297 @@ static int dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
>>  	return 0;
>>  }
>>  
>> +#define DW_PCIE_LB_BUF_SIZE_MIN	SZ_4K
>> +#define DW_PCIE_LB_BUF_SIZE_MAX	SZ_1M
>> +#define DW_PCIE_LB_SETTLE_MS		100
>> +#define DW_PCIE_LB_LTSSM_SETTLE_US	(DW_PCIE_LB_SETTLE_MS * USEC_PER_MSEC)
>> +#define DW_PCIE_LB_LTSSM_POLL_US	1000
>> +
>> +static int dw_pcie_loopback_run(struct dw_pcie_rp *pp, size_t buf_size)
>> +{
>> +	struct resource lb_res = { .name = "pcie-loopback",
>> +				   .flags = IORESOURCE_MEM };
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	enum dw_pcie_ltssm ltssm;
>> +	void __iomem *src_base;
>> +	u32 plc, gen3, pipe_lb;
>> +	dma_addr_t dst_dma;
>> +	void *dst_virt;
>> +	int ret, rret;
>> +	void *tx_buf;
>> +	int ib_index;
>> +
>> +	ib_index = pci->num_ib_windows - 1;
> [Severity: High]
> Does this code safely reserve the inbound ATU window? It seems the index
> is hardcoded to the last available window without checking if it is already
> in use.
>
> If the platform driver or PCIe host core had previously programmed this
> window (for example, to map system RAM for incoming endpoint DMA or to
> route MSI interrupts), could this unconditionally overwrite and permanently
> destroy that configuration?
Ack,  I will create new element to track the number of inbound iatu's used.
>> +
>> +	ret = pci_bus_alloc_resource(pp->bridge->bus, &lb_res,
>> +				     buf_size, buf_size,
>> +				     PCIBIOS_MIN_MEM, 0,
>> +				     pcibios_align_resource,
>> +				     &pp->bridge->dev);
> [Severity: Critical]
> Is the alignment data parameter passed to pci_bus_alloc_resource()
> correct here?
>
> The pcibios_align_resource callback expects a struct pci_dev pointer, but
> &pp->bridge->dev is a struct device pointer. Architecture implementations
> often cast this parameter directly to struct pci_dev and access its fields.
>
> Since struct device is embedded inside struct pci_dev at a non-zero offset,
> could this cause out-of-bounds memory accesses?
ack.
>> +	if (ret) {
>> +		dev_err(pci->dev, "loopback: failed to alloc PCIe MEM resource: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	src_base = ioremap(lb_res.start, buf_size);
>> +	if (!src_base) {
>> +		dev_err(pci->dev, "loopback: ioremap of PCIe source window failed\n");
>> +		ret = -ENOMEM;
>> +		goto err_release_res;
>> +	}
>> +
>> +	dst_virt = dma_alloc_coherent(pci->dev, buf_size, &dst_dma, GFP_KERNEL);
>> +	if (!dst_virt) {
>> +		ret = -ENOMEM;
>> +		goto err_iounmap;
>> +	}
>> +
>> +	ret = dw_pcie_prog_inbound_atu(pci, ib_index, PCIE_TLP_TYPE_MEM_RDWR,
>> +				       dst_dma, lb_res.start, buf_size);
> [Severity: High]
> Does the inbound ATU programming use the correct address space?
>
> pci_bus_alloc_resource() populates lb_res.start with a CPU physical
> address, and this is passed directly to dw_pcie_prog_inbound_atu() as the
> PCIe bus address.
>
> On platforms where the CPU physical address space has an offset from the
> PCIe bus address space (such as those specifying dma-ranges), will the ATU
> expect the wrong address and cause the loopback test to fail?
I will check from dma-ranges perspective and fix it if required.
>
>> +	if (ret) {
>> +		dev_err(pci->dev, "loopback: inbound iATU programming failed: %d\n", ret);
>> +		goto err_free_dma;
>> +	}
>> +
>> +	plc = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
>> +
>> +	gen3 = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
>> +	dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF,
>> +			   gen3 | GEN3_RELATED_OFF_GEN3_EQ_DISABLE);
>> +
>> +	pipe_lb = dw_pcie_readl_dbi(pci, PCIE_PIPE_LOOPBACK_CONTROL);
>> +	dw_pcie_writel_dbi(pci, PCIE_PIPE_LOOPBACK_CONTROL,
>> +			   pipe_lb | PCIE_PIPE_LOOPBACK_EN);
>> +
>> +	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc | PORT_LINK_LOOPBACK_EN);
>> +
>> +	msleep(DW_PCIE_LB_SETTLE_MS);
>> +
>> +	tx_buf = kmalloc(buf_size, GFP_KERNEL);
> [Severity: Medium]
> Could this memory allocation fail on systems with fragmented memory?
>
> The loopback test can request a buffer up to 1 MiB via the sysfs attribute,
> which requires an order-8 physically contiguous page block.
>
> Since tx_buf is solely used as a virtual buffer to hold random data before
> copying via memcpy_toio(), would it be more reliable to use kvmalloc() to
> avoid disruptive page allocator warnings?
ack
>> +	if (!tx_buf) {
>> +		ret = -ENOMEM;
>> +		goto err_restore_link;
>> +	}
> [ ... ]
>
>> +err_restore_link:
>> +	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc);
>> +	dw_pcie_writel_dbi(pci, PCIE_PIPE_LOOPBACK_CONTROL, pipe_lb);
>> +	dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, gen3);
>> +
>> +	if (read_poll_timeout(dw_pcie_get_ltssm, ltssm,
>> +			      ltssm == DW_PCIE_LTSSM_DETECT_QUIET,
>> +			      DW_PCIE_LB_LTSSM_POLL_US, DW_PCIE_LB_LTSSM_SETTLE_US,
>> +			      false, pci)) {
>> +		/*
>> +		 * LTSSM has been observed stuck outside DETECT_QUIET on some
>> +		 * platforms; only a full controller reinit (mirroring system
>> +		 * suspend/resume) reliably recovers it.
>> +		 */
>> +		dev_warn(pci->dev,
>> +			 "loopback: LTSSM did not settle in DETECT_QUIET (in %s), reinitializing controller\n",
>> +			 dw_pcie_ltssm_status_string(ltssm));
>> +
>> +		if (pp->ops->deinit)
>> +			pp->ops->deinit(pp);
>> +
>> +		pci->suspended = true;
> [Severity: High]
> Does powering down the controller here pose a risk of a system crash?
>
> This recovery runs in a sysfs handler while the rest of the system is fully
> active, without blocking concurrent accesses to the PCIe config space.
>
> If another task attempts a config read (for example, from lspci) while the
> clocks are disabled by the deinit callback, could the unclocked access
> trigger an AXI bus fault?
Will not expect user to do this test and in parallel he will run lspci and also
expects this deinit and reinit happens quickly before axi detects this as a
error.
>> +
>> +		rret = dw_pcie_resume_noirq(pci);
>> +		if (rret) {
>> +			dev_err(pci->dev, "loopback: controller reinit failed: %d\n", rret);
>> +			if (!ret)
>> +				ret = rret;
>> +		}
> [Severity: High]
> Will this hardware recovery unconditionally fail?
>
> The loopback test runs strictly when the PCIe link is down (no endpoint is
> connected). However, dw_pcie_resume_noirq() starts the link and waits for
> it to become UP. Because no endpoint is physically connected, this wait
> will time out and return -ETIMEDOUT.
>
> This seems to overwrite a potentially successful test result with -ETIMEDOUT
> and abandon the controller in a broken, suspended state.
ack 
>> +
>> +		ltssm = dw_pcie_get_ltssm(pci);
>> +	} else {
>> +		dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, ib_index);
>> +	}
>> +
>> +	dev_info(pci->dev, "PCIe LTSSM state after loopback exit: %s\n",
>> +		 dw_pcie_ltssm_status_string(ltssm));
>> +
>> +err_free_dma:
>> +	dma_free_coherent(pci->dev, buf_size, dst_virt, dst_dma);
> [Severity: High]
> Does this leave a hardware mapping to freed memory active?
>
> When the LTSSM poll times out, the code skips the else block containing
> dw_pcie_disable_atu(). Execution falls through to err_free_dma, where
> dma_free_coherent() frees the DMA buffer back to the kernel.
>
> If the link later becomes active, could incoming memory writes from an
> endpoint corrupt kernel memory through the still-active inbound ATU window?
>
> [ ... ]
as part resume_noirq IATU will be cleared.
>> +static ssize_t run_store(struct device *dev,
>> +			 struct device_attribute *attr,
>> +			 const char *buf, size_t count)
>> +{
>> +	struct dw_pcie_loopback *lb =
>> +		container_of(attr, struct dw_pcie_loopback, attr_run);
>> +	struct dw_pcie_rp *pp = lb->pp;
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	size_t buf_size;
>> +	int ret;
>> +
>> +	if (!sysfs_streq(buf, "local"))
>> +		return -EINVAL;
>> +
>> +	if (dw_pcie_link_up(pci)) {
>> +		dev_err(pci->dev,
>> +			"loopback: refusing to run while the link is up\n");
>> +		return -EBUSY;
>> +	}
> [Severity: High]
> Are the power domains and clocks guaranteed to be active here?
>
> The run_store() handler directly checks the link state by reading the
> PCIE_PORT_DEBUG1 MMIO register via dw_pcie_link_up(), and then proceeds
> to modify numerous controller registers in dw_pcie_loopback_run().
>
> Since the test intentionally runs when the link is down, is it possible
> the driver core has runtime-suspended the PCIe controller to save power?
> Accessing MMIO registers without calling pm_runtime_get_sync() first
> could trigger a synchronous bus fault.
>
> [Severity: Medium]
> Is there a race condition between checking the link status and executing
> the test?
>
> The code locklessly checks dw_pcie_link_up() here, and later acquires
> lb->lock and performs sleeping allocations in dw_pcie_loopback_run(),
> such as dma_alloc_coherent() and kmalloc().
>
> If an endpoint is hotplugged or finishes link training during this sleep
> window, the link will transition to the UP state. Will the loopback test
> then blindly overwrite the PCIE_PORT_LINK_CONTROL register, forcefully
> disrupting the newly active link?
User might not do link init and do a hotplug of a devcie.

- Krishna Chaitanya.
>> +
>> +	if (mutex_lock_interruptible(&lb->lock))
>> +		return -ERESTARTSYS;
> [ ... ]
>