* [PATCH] PCI: dwc: Add sysfs for local loopback interface
@ 2026-08-03 9:34 Krishna Chaitanya Chundru
2026-08-03 9:47 ` sashiko-bot
0 siblings, 1 reply; 2+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-08-03 9:34 UTC (permalink / raw)
To: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas
Cc: linux-pci, linux-kernel, Krishna Chaitanya Chundru
The PCIe Base Specification (r6.0, sec. 4.2.7.10) defines a Loopback
state in the LTSSM: the Loopback Master sets the Loopback bit in TS1
Ordered Sets, and the Loopback Slave reflects received data back
bit-for-bit. DesignWare PCIe IP also exposes a local variant, entirely
within the controller and not requiring a connected endpoint, via
PCIE_PIPE_LOOPBACK_CONTROL and PORT_LINK_LOOPBACK_EN.
This patch focuses on local loopback only. Remote (link-level)
loopback requires the link to be up while the test runs, which means
any client/endpoint drivers using the link would need to be told to
stop transfers beforehand -- up to and including a driver remove -- and
the controller/link would need to be reinitiated afterward. That
coordination needs further discussion, so it is left for a follow-up.
Add a sysfs attribute group 'loopback/' on the platform device with:
run (RW) -- write "local" to run a synchronous, blocking
data-integrity test; reads back "idle" or "busy" to
indicate test state. Refused with -EBUSY while the
link is up, so it cannot disrupt an active link or
other devices behind the same root port.
buf_size (RW) -- transfer size in bytes, rounded up to the next
power-of-two and clamped to a minimum of 4 KiB and a
maximum of 1 MiB (default 4 KiB); cannot be changed
during a test
The test allocates a coherent DMA buffer, programs an inbound iATU
window to redirect PCIe writes to it, writes random data through the
PCIe window, and compares the result byte-by-byte.
On completion the loopback/link-control DBI bits are restored directly
rather than resetting the whole controller, and the LTSSM is polled for
DETECT_QUIET. On some platforms it has been observed (and reproduced
on Qualcomm Eliza hardware) to land elsewhere instead and leave the
controller unable to service further runs; in that case the controller
is recovered via the same ops->deinit() + dw_pcie_resume_noirq()
sequence used across a system suspend/resume cycle, which has been
validated to reliably restore the controller across repeated
back-to-back test runs.
The sysfs group is added/removed via device_add_group()/
device_remove_group() rather than the devm_* variant, with removal
called explicitly at the start of dw_pcie_host_deinit() before the
rest of the bridge is torn down, since device_remove_group() blocks
until any in-flight run_store()/run_show() call returns -- closing a
race where driver unbind/remove could otherwise run concurrently with
a sysfs write.
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
.../testing/sysfs-driver-pcie-designware-loopback | 69 +++++
MAINTAINERS | 1 +
drivers/pci/controller/dwc/pcie-designware-host.c | 298 +++++++++++++++++++++
drivers/pci/controller/dwc/pcie-designware.h | 17 ++
4 files changed, 385 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-driver-pcie-designware-loopback b/Documentation/ABI/testing/sysfs-driver-pcie-designware-loopback
new file mode 100644
index 000000000000..096da7800549
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-pcie-designware-loopback
@@ -0,0 +1,69 @@
+What: /sys/devices/.../loopback/run
+Date: August 2026
+KernelVersion: 7.2
+Contact: linux-pci@vger.kernel.org
+Description:
+ (RW) Diagnostic local loopback test interface for Synopsys
+ DesignWare PCIe root-port controllers.
+
+ Reading this file returns the current test state:
+
+ - ``idle`` - no test is running
+ - ``busy`` - a test is in progress
+
+ Writing ``local`` triggers a synchronous, blocking
+ data-integrity test performed entirely within the local
+ controller, without requiring a connected endpoint. The
+ write does not return until the test completes, and logs
+ ``PASSED`` or ``FAILED`` via dev_info().
+
+ The test allocates a coherent DMA buffer, configures an
+ inbound iATU window to redirect PCIe writes to that buffer,
+ writes random data through the PCIe window, and compares
+ the result byte-by-byte against what was written.
+
+ The test only runs while the PCIe link is down; writing
+ ``local`` while the link is up returns -EBUSY. This avoids
+ disrupting an active link and any devices behind it.
+
+ Only one test may run at a time. Concurrent writes
+ return -EBUSY.
+
+ Example::
+
+ # Run a local loopback test
+ $ echo local > /sys/devices/.../loopback/run
+
+ # Check whether a test is in progress
+ $ cat /sys/devices/.../loopback/run
+ idle
+
+What: /sys/devices/.../loopback/buf_size
+Date: August 2026
+KernelVersion: 7.2
+Contact: linux-pci@vger.kernel.org
+Description:
+ (RW) Size in bytes of the data buffer used by the
+ loopback test.
+
+ Reading this file returns the current buffer size as a
+ decimal integer.
+
+ Writing a decimal integer sets a new buffer size. The
+ value is rounded up to the next power of two and clamped
+ to a minimum of 4096 (4 KiB) and a maximum of 1048576
+ (1 MiB); values above the maximum are rejected with
+ -EINVAL. The new size takes effect on the next test run;
+ it cannot be changed while a test is in progress (returns
+ -EBUSY).
+
+ The default value is 4096.
+
+ Example::
+
+ # Set the test buffer to 64 KiB
+ $ echo 65536 > /sys/devices/.../loopback/buf_size
+
+ # Read back the (rounded) value
+ $ cat /sys/devices/.../loopback/buf_size
+ 65536
diff --git a/MAINTAINERS b/MAINTAINERS
index 0d7987278c07..44a11de7cc8d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20761,6 +20761,7 @@ M: Jingoo Han <jingoohan1@gmail.com>
M: Manivannan Sadhasivam <mani@kernel.org>
L: linux-pci@vger.kernel.org
S: Maintained
+F: Documentation/ABI/testing/sysfs-driver-pcie-designware-loopback
F: Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
F: Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
F: drivers/pci/controller/dwc/*designware*
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 06722259d2e3..f0cfe9e88520 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -19,6 +19,8 @@
#include <linux/pci.h>
#include <linux/pci_regs.h>
#include <linux/platform_device.h>
+#include <linux/random.h>
+#include <linux/sizes.h>
#include "../pci-host-common.h"
#include "../../pci.h"
@@ -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;
+
+ ret = pci_bus_alloc_resource(pp->bridge->bus, &lb_res,
+ buf_size, buf_size,
+ PCIBIOS_MIN_MEM, 0,
+ pcibios_align_resource,
+ &pp->bridge->dev);
+ 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);
+ 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);
+ if (!tx_buf) {
+ ret = -ENOMEM;
+ goto err_restore_link;
+ }
+
+ get_random_bytes(tx_buf, buf_size);
+ memcpy_toio(src_base, tx_buf, buf_size);
+ msleep(DW_PCIE_LB_SETTLE_MS);
+
+ if (memcmp(tx_buf, dst_virt, buf_size))
+ ret = -EIO;
+
+ dev_info(pci->dev, "PCIe local loopback test %s\n", ret ? "FAILED" : "PASSED");
+
+ kfree(tx_buf);
+
+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;
+
+ rret = dw_pcie_resume_noirq(pci);
+ if (rret) {
+ dev_err(pci->dev, "loopback: controller reinit failed: %d\n", rret);
+ if (!ret)
+ ret = rret;
+ }
+
+ 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);
+err_iounmap:
+ iounmap(src_base);
+err_release_res:
+ release_resource(&lb_res);
+
+ return ret;
+}
+
+static ssize_t buf_size_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct dw_pcie_loopback *lb =
+ container_of(attr, struct dw_pcie_loopback, attr_buf_size);
+
+ return sysfs_emit(buf, "%zu\n", lb->buf_size);
+}
+
+static ssize_t buf_size_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_buf_size);
+ struct dw_pcie *pci = to_dw_pcie_from_pp(lb->pp);
+ unsigned long req;
+ size_t new_size;
+ int ret;
+
+ ret = kstrtoul(buf, 0, &req);
+ if (ret)
+ return ret;
+
+ if (!req)
+ return -EINVAL;
+
+ if (req > DW_PCIE_LB_BUF_SIZE_MAX)
+ return -EINVAL;
+
+ /* Round up to next power-of-two, then enforce the 4 KiB minimum */
+ new_size = max_t(size_t, DW_PCIE_LB_BUF_SIZE_MIN,
+ roundup_pow_of_two((size_t)req));
+
+ if (mutex_lock_interruptible(&lb->lock))
+ return -ERESTARTSYS;
+
+ if (lb->busy) {
+ mutex_unlock(&lb->lock);
+ return -EBUSY;
+ }
+
+ lb->buf_size = new_size;
+ mutex_unlock(&lb->lock);
+
+ dev_dbg(pci->dev, "loopback: buf_size set to %zu bytes\n", new_size);
+
+ return count;
+}
+
+static ssize_t run_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct dw_pcie_loopback *lb =
+ container_of(attr, struct dw_pcie_loopback, attr_run);
+
+ return sysfs_emit(buf, "%s\n", lb->busy ? "busy" : "idle");
+}
+
+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;
+ }
+
+ if (mutex_lock_interruptible(&lb->lock))
+ return -ERESTARTSYS;
+
+ if (lb->busy) {
+ dev_warn(pci->dev, "loopback: test already in progress\n");
+ mutex_unlock(&lb->lock);
+ return -EBUSY;
+ }
+
+ lb->busy = true;
+ buf_size = lb->buf_size;
+ mutex_unlock(&lb->lock);
+
+ ret = dw_pcie_loopback_run(pp, buf_size);
+
+ mutex_lock(&lb->lock);
+ lb->busy = false;
+ mutex_unlock(&lb->lock);
+
+ return ret ? ret : count;
+}
+
+static int dw_pcie_loopback_sysfs_init(struct dw_pcie_rp *pp)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct dw_pcie_loopback *lb;
+ int ret;
+
+ lb = devm_kzalloc(pci->dev, sizeof(*lb), GFP_KERNEL);
+ if (!lb)
+ return -ENOMEM;
+
+ lb->pp = pp;
+
+ mutex_init(&lb->lock);
+ lb->buf_size = DW_PCIE_LB_BUF_SIZE_MIN;
+
+ lb->attr_run = (struct device_attribute)
+ __ATTR(run, 0644, run_show, run_store);
+ lb->attr_buf_size = (struct device_attribute)
+ __ATTR(buf_size, 0644, buf_size_show, buf_size_store);
+
+ sysfs_attr_init(&lb->attr_run.attr);
+ sysfs_attr_init(&lb->attr_buf_size.attr);
+
+ lb->attrs[0] = &lb->attr_run.attr;
+ lb->attrs[1] = &lb->attr_buf_size.attr;
+ lb->attrs[2] = NULL;
+ lb->attr_group.name = "loopback";
+ lb->attr_group.attrs = lb->attrs;
+
+ pp->loopback = lb;
+
+ ret = device_add_group(pci->dev, &lb->attr_group);
+ if (ret) {
+ dev_err(pci->dev, "loopback: device_add_group failed: %d\n", ret);
+ pp->loopback = NULL;
+ return ret;
+ }
+
+ return 0;
+}
+
+static void dw_pcie_loopback_sysfs_deinit(struct dw_pcie_rp *pp)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct dw_pcie_loopback *lb = pp->loopback;
+
+ if (!lb)
+ return;
+
+ /*
+ * device_remove_group() blocks until any in-flight run_store()/
+ * run_show() has returned, so pp/pci/bridge are guaranteed not to be
+ * accessed by the loopback code once this returns. Must run before
+ * the rest of dw_pcie_host_deinit() tears down pp->bridge.
+ */
+ device_remove_group(pci->dev, &lb->attr_group);
+ pp->loopback = NULL;
+}
+
int dw_pcie_host_init(struct dw_pcie_rp *pp)
{
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -675,6 +968,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
dwc_pcie_debugfs_init(pci, DW_PCIE_RC_TYPE);
+ if (dw_pcie_loopback_sysfs_init(pp))
+ dev_warn(dev, "failed to create loopback sysfs entry\n");
+
return 0;
err_stop_link:
@@ -703,6 +999,8 @@ void dw_pcie_host_deinit(struct dw_pcie_rp *pp)
{
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ dw_pcie_loopback_sysfs_deinit(pp);
+
dwc_pcie_debugfs_deinit(pci);
pci_lock_rescan_remove();
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index de4b245b1758..dfcbcdf822da 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -88,6 +88,7 @@
#define PORT_AFR_L1_ENTRANCE_LAT_MASK GENMASK(29, 27)
#define PCIE_PORT_LINK_CONTROL 0x710
+#define PORT_LINK_LOOPBACK_EN BIT(2)
#define PORT_LINK_DLL_LINK_EN BIT(5)
#define PORT_LINK_FAST_LINK_MODE BIT(7)
#define PORT_LINK_MODE_MASK GENMASK(21, 16)
@@ -173,6 +174,9 @@
#define COHERENCY_CONTROL_2_OFF 0x8E4
#define COHERENCY_CONTROL_3_OFF 0x8E8
+#define PCIE_PIPE_LOOPBACK_CONTROL 0x8B8
+#define PCIE_PIPE_LOOPBACK_EN BIT(31)
+
#define PCIE_PORT_MULTI_LANE_CTRL 0x8C0
#define PORT_MLTI_UPCFG_SUPPORT BIT(7)
@@ -471,6 +475,19 @@ struct dw_pcie_rp {
bool native_ecam;
bool skip_l23_ready;
bool skip_pwrctrl_off;
+ struct dw_pcie_loopback *loopback;
+};
+
+struct dw_pcie_loopback {
+ struct dw_pcie_rp *pp;
+ /* Protects busy and buf_size against concurrent sysfs access */
+ struct mutex lock;
+ bool busy;
+ size_t buf_size;
+ struct device_attribute attr_run;
+ struct device_attribute attr_buf_size;
+ struct attribute *attrs[3]; /* run, buf_size, NULL */
+ struct attribute_group attr_group;
};
struct dw_pcie_ep_ops {
---
base-commit: 8ba098e6b6ff0db8edf28528d1552be261af30d4
change-id: 20250130-loopback-60e4cd7bbfe7
Best regards,
--
Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] PCI: dwc: Add sysfs for local loopback interface
2026-08-03 9:34 [PATCH] PCI: dwc: Add sysfs for local loopback interface Krishna Chaitanya Chundru
@ 2026-08-03 9:47 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-08-03 9:47 UTC (permalink / raw)
To: Krishna Chaitanya Chundru; +Cc: linux-pci
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 <krishna.chundru@oss.qualcomm.com>
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?
> +
> + 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?
> + 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?
> + 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?
> + 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?
> +
> + 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.
> +
> + 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?
[ ... ]
> +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?
> +
> + if (mutex_lock_interruptible(&lb->lock))
> + return -ERESTARTSYS;
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-loopback-v1-1-508f7f19e47a@oss.qualcomm.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-08-03 9:47 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-03 9:34 [PATCH] PCI: dwc: Add sysfs for local loopback interface Krishna Chaitanya Chundru
2026-08-03 9:47 ` sashiko-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox