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

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 inste=
ad of the PCIe bus address.
- [High] Inbound ATU window configuration is overwritten and permanently di=
sabled, silently destroying prior hardware mappings.
- [High] Hardware recovery via `dw_pcie_resume_noirq()` will unconditionall=
y fail and mask test outcomes.
- [High] The inbound iATU window programmed for the loopback test is not di=
sabled if the controller requires reinitialization, leaving a hardware mapp=
ing 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 cras=
h.
- [Medium] The loopback test uses `kmalloc()` to allocate an arbitrarily la=
rge test buffer (up to 1 MiB), which is highly susceptible to allocation fa=
ilure due to fragmentation.
- [Medium] TOCTOU race in link status check can cause disruption of an acti=
ve 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_pci=
e_rp *pp)
>  	return 0;
>  }
> =20
> +#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 =3D { .name =3D "pcie-loopback",
> +				   .flags =3D IORESOURCE_MEM };
> +	struct dw_pcie *pci =3D 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 =3D 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?

> +
> +	ret =3D 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?

> +	if (ret) {
> +		dev_err(pci->dev, "loopback: failed to alloc PCIe MEM resource: %d\n",=
 ret);
> +		return ret;
> +	}
> +
> +	src_base =3D ioremap(lb_res.start, buf_size);
> +	if (!src_base) {
> +		dev_err(pci->dev, "loopback: ioremap of PCIe source window failed\n");
> +		ret =3D -ENOMEM;
> +		goto err_release_res;
> +	}
> +
> +	dst_virt =3D dma_alloc_coherent(pci->dev, buf_size, &dst_dma, GFP_KERNE=
L);
> +	if (!dst_virt) {
> +		ret =3D -ENOMEM;
> +		goto err_iounmap;
> +	}
> +
> +	ret =3D 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?

> +	if (ret) {
> +		dev_err(pci->dev, "loopback: inbound iATU programming failed: %d\n", r=
et);
> +		goto err_free_dma;
> +	}
> +
> +	plc =3D dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> +
> +	gen3 =3D 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 =3D 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_LOOPBAC=
K_EN);
> +
> +	msleep(DW_PCIE_LB_SETTLE_MS);
> +
> +	tx_buf =3D 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?

> +	if (!tx_buf) {
> +		ret =3D -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 =3D=3D 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), reinitializ=
ing controller\n",
> +			 dw_pcie_ltssm_status_string(ltssm));
> +
> +		if (pp->ops->deinit)
> +			pp->ops->deinit(pp);
> +
> +		pci->suspended =3D 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?

> +
> +		rret =3D dw_pcie_resume_noirq(pci);
> +		if (rret) {
> +			dev_err(pci->dev, "loopback: controller reinit failed: %d\n", rret);
> +			if (!ret)
> +				ret =3D 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.

> +
> +		ltssm =3D 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?

[ ... ]

> +static ssize_t run_store(struct device *dev,
> +			 struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	struct dw_pcie_loopback *lb =3D
> +		container_of(attr, struct dw_pcie_loopback, attr_run);
> +	struct dw_pcie_rp *pp =3D lb->pp;
> +	struct dw_pcie *pci =3D 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?

> +
> +	if (mutex_lock_interruptible(&lb->lock))
> +		return -ERESTARTSYS;

[ ... ]

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803-loopback-v=
[email protected]?part=3D1