Re: [PATCH net-next v6 03/14] net: microchip: fdma: add PCIe ATU support

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 3 · Low: 3

- [Medium] fdma_pci_atu_region_map() (EXPORT_SYMBOL_GPL) is the primitive
  that programs the ATU limit register (region->limit_addr =…
- [Medium] fdma_pci_atu_region_enable() writes the region enable bit
  (BIT(31) at offset 0x4) and returns immediately, and…
- [Medium] fdma_pci_atu_configure_translation() programs lower/upper base
  (0x8/0xc), limit (0x10), lower/upper target (0x14/0x18) and the enable…
- [Low] The commit message says the patch "effectively add[s] support for
  PCIe FDMA", but at this commit nothing in the tree calls…
- [Low] fdma_pci_atu_init() is the designated initializer of struct
  fdma_pci_atu (it even programs hardware, disabling all six outbound…
- [Low] fdma_pci_atu_translate_addr() reads region->base_addr and
  region->target_addr with no lock, no READ_ONCE() and no in_use…

--

net: microchip: fdma: add PCIe ATU support

This adds a new fdma_pci.c/fdma_pci.h library that carves the PCIe endpoint
outbound address space (0x10000000-0x1fffffff) into six equally sized ATU
regions, and adds fdma_alloc_coherent_and_map()/fdma_free_coherent_and_unmap()
helpers that pair a coherent DMA allocation with an ATU region mapping.

A few questions below.

    Add the required functions and helpers that combine the DMA allocation
    with the ATU region mapping, effectively adding support for PCIe FDMA.

[Severity: Low]
Is "effectively adding support for PCIe FDMA" the right wording for this
patch on its own?

At this commit nothing in the tree calls fdma_pci_atu_init(),
fdma_pci_atu_region_map(), fdma_pci_atu_translate_addr(),
fdma_alloc_coherent_and_map() or fdma_free_coherent_and_unmap(), and
FDMA_PCI_DB_ALIGN/FDMA_PCI_DB_SIZE() have no user either.

The wiring appears later in the series, in "net: lan966x: add PCIe FDMA
support" and its follow-ups (lan966x_fdma_pci.c calls fdma_pci_atu_init(),
fdma_alloc_coherent_and_map(), fdma_free_coherent_and_unmap(),
fdma_pci_atu_translate_addr() and FDMA_PCI_DB_SIZE()).

Would "add the infrastructure for PCIe FDMA" describe this patch more
accurately?

[ ... ]

[ ... ]

+#define FDMA_PCI_ATU_ADDR            0x300000
+#define FDMA_PCI_ATU_IDX_SIZE        0x200
+#define FDMA_PCI_ATU_ENA_REG         0x4
+#define FDMA_PCI_ATU_ENA_BIT         BIT(31)
+#define FDMA_PCI_ATU_LWR_BASE_ADDR   0x8
+#define FDMA_PCI_ATU_UPP_BASE_ADDR   0xc
+#define FDMA_PCI_ATU_LIMIT_ADDR      0x10
+#define FDMA_PCI_ATU_LWR_TARGET_ADDR 0x14
+#define FDMA_PCI_ATU_UPP_TARGET_ADDR 0x18

[ ... ]

+static void fdma_pci_atu_region_enable(struct fdma_pci_atu_region *region)
+{
+	writel(FDMA_PCI_ATU_ENA_BIT,
+	       fdma_pci_atu_addr_get(region->atu->addr, FDMA_PCI_ATU_ENA_REG,
+				     region->idx));
+}

[Severity: Medium]
Does the enable bit take effect by the time this writel() retires?

The register block looks like the DesignWare unrolled iATU: the 0x300000
base matches DEFAULT_DBI_ATU_OFFSET, the 0x200 stride matches
PCIE_ATU_UNROLL_BASE(dir, index) = index << 9, and 0x0/0x4/0x8/0xc/0x10/
0x14/0x18 match PCIE_ATU_UNR_REGION_CTRL1/CTRL2/LOWER_BASE/UPPER_BASE/
LOWER_LIMIT/LOWER_TARGET/UPPER_TARGET with PCIE_ATU_ENABLE = BIT(31) in
CTRL2.

For that IP, dw_pcie_prog_outbound_atu() in
drivers/pci/controller/dwc/pcie-designware.c polls until the enable bit
reads back:

	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
		val = dw_pcie_readl_atu(pci, dir, index, PCIE_ATU_REGION_CTRL2);
		if (val & PCIE_ATU_ENABLE)
			return 0;

		mdelay(LINK_WAIT_IATU);
	}

	dev_err(pci->dev, "Outbound iATU is not being enabled\n");

	return -ETIMEDOUT;

with the comment "Make sure ATU enable takes effect before any subsequent
config and I/O accesses".

Here fdma_pci_atu_region_map() returns the region as usable right after
this write, and the caller programs FDMA descriptors with
region->base_addr. Should the enable be read back (and a failure to arm
reported to the caller) instead of being fire-and-forget?

+/* Configure the address translation in the ATU. */
+static void
+fdma_pci_atu_configure_translation(struct fdma_pci_atu_region *region)
+{
+	struct fdma_pci_atu *atu = region->atu;
+	int idx = region->idx;
+
+	writel(lower_32_bits(region->base_addr),
+	       fdma_pci_atu_addr_get(atu->addr,
+				     FDMA_PCI_ATU_LWR_BASE_ADDR, idx));
+
+	writel(upper_32_bits(region->base_addr),
+	       fdma_pci_atu_addr_get(atu->addr,
+				     FDMA_PCI_ATU_UPP_BASE_ADDR, idx));
+
+	/* Upper limit register only needed with REGION_SIZE > 4GB. */
+	writel(region->limit_addr,
+	       fdma_pci_atu_addr_get(atu->addr, FDMA_PCI_ATU_LIMIT_ADDR, idx));

[Severity: Medium]
Should region control 1 (offset 0x0) be programmed here too?

Everything except CTRL1 is written: base, limit, target and the enable bit
in CTRL2. On the DWC unrolled iATU, CTRL1 holds the outbound TLP TYPE (MEM
= 0), TD, PCIE_ATU_FUNC_NUM(pf) and PCIE_ATU_INCREASE_REGION_SIZE
(BIT(13)). dw_pcie_prog_outbound_atu() always writes CTRL1 before enabling
CTRL2:

	val = type | PCIE_ATU_FUNC_NUM(func_no);
	...
	dw_pcie_writel_atu(pci, dir, index, PCIE_ATU_REGION_CTRL1, val);
	dw_pcie_writel_atu(pci, dir, index, PCIE_ATU_REGION_CTRL2,
			   PCIE_ATU_ENABLE);

There is no define for offset 0x0 in this file, and fdma_pci_atu_init()
only clears the enable bit, so each window is armed with whatever CTRL1
value reset, the endpoint bootloader, or a previous OS instance left in
place. With a residual TYPE the FDMA writes go out as the wrong TLP type,
with a stale FUNC_NUM they carry the wrong requester, and with a stale
INCREASE_REGION_SIZE the window end is taken from a register this code
never writes.

Related, is the comment "Upper limit register only needed with REGION_SIZE

4GB" accurate here? That mode is selected by CTRL1 BIT(13), which is

neither set nor cleared, and the upper limit register is never written.

[ ... ]

+/* Find an unused ATU region. */
+static struct fdma_pci_atu_region *
+fdma_pci_atu_region_get_free(struct fdma_pci_atu *atu)
+{
+	struct fdma_pci_atu_region *regions = atu->regions;
+
+	for (int i = 0; i < FDMA_PCI_ATU_REGION_MAX; i++) {
+		if (regions[i].in_use)
+			continue;
+
+		return &regions[i];
+	}
+
+	return ERR_PTR(-ENOSPC);
+}

[ ... ]

+/* Map a host DMA address into a free outbound region. */
+struct fdma_pci_atu_region *
+fdma_pci_atu_region_map(struct fdma_pci_atu *atu, u64 target_addr, int size)
+{
+	struct fdma_pci_atu_region *region;
+
+	if (!atu)
+		return ERR_PTR(-EINVAL);
+
+	if (size <= 0)
+		return ERR_PTR(-EINVAL);
+
+	if (size > fdma_pci_atu_region_size())
+		return ERR_PTR(-ERANGE);
+
+	/* The ATU region base is only ever aligned to FDMA_PCI_ATU_REGION_ALIGN;
+	 * require the same alignment of the host target address, since the ATU
+	 * translates addr - target_addr + base_addr and any misalignment here
+	 * would shift every translated address by the same amount.
+	 */
+	if (!IS_ALIGNED(target_addr, FDMA_PCI_ATU_REGION_ALIGN))
+		return ERR_PTR(-EINVAL);

[Severity: Medium]
Should this validation block also reject a size that is not a multiple of
FDMA_PCI_ATU_REGION_ALIGN?

fdma_pci.h states the rule for both halves:

	/* Outbound regions are 64KB granular (datasheet section 3.24.7.4.1), so both
	 * the region base and the mapped size must be aligned to this.
	 */
	#define FDMA_PCI_ATU_REGION_ALIGN BIT(16)

and the commit message names the consequence of not honouring it: "the
outbound window would extend past the memory the host allocated for DMA".

The size given here becomes the programmed limit:

	region->limit_addr = region->base_addr + size - 1;

so with, say, a PAGE_SIZE-aligned 0x21000 from fdma_get_size() the hardware
widens the limit to the next 64KB boundary and the enabled outbound window
covers up to 64KB-1 of host memory beyond the buffer, which the FDMA engine
can then write into.

The 64KB padding is only done in the convenience wrapper
fdma_alloc_coherent_and_map(), not in this exported primitive that
programs the register, and the commit message advertises the primitive for
reuse by lan969x. Would an IS_ALIGNED(size, FDMA_PCI_ATU_REGION_ALIGN)
check (or rounding here) be better placed alongside the target_addr check?

Also, the header comment for fdma_pci_atu_region_map() is just "Map a host
DMA address into a free ATU region", with no mention that a target_addr
that is not 64KB aligned is rejected with -EINVAL. Could that requirement
be documented at the prototype?

+
+	mutex_lock(&atu->lock);
+
+	region = fdma_pci_atu_region_get_free(atu);
+	if (IS_ERR(region)) {
+		mutex_unlock(&atu->lock);
+		return region;
+	}
+
+	region->target_addr = target_addr;
+	region->limit_addr = region->base_addr + size - 1;
+	region->in_use = true;
+
+	fdma_pci_atu_configure_translation(region);
+	fdma_pci_atu_region_enable(region);
+
+	mutex_unlock(&atu->lock);
+
+	return region;
+}
+EXPORT_SYMBOL_GPL(fdma_pci_atu_region_map);
+
+/* Translate a host DMA address to the corresponding OB address. */
+u64 fdma_pci_atu_translate_addr(struct fdma_pci_atu_region *region, u64 addr)
+{
+	return region->base_addr + (addr - region->target_addr);
+}
+EXPORT_SYMBOL_GPL(fdma_pci_atu_translate_addr);

[Severity: Low]
What serializes these reads against the writers?

fdma_pci_atu_region_map() and fdma_pci_atu_region_unmap() mutate
target_addr/in_use and the hardware window under the sleeping atu->lock,
while this helper reads base_addr/target_addr with no lock, no READ_ONCE()
and no in_use check. Its intended callers are descriptor callbacks
(lan966x_fdma_pci_dataptr_cb()/nextptr_cb(), reached from NAPI poll and
from lan966x_fdma_pci_xmit()), which cannot take a mutex.

Looking at the end of the series, the writers are
lan966x_fdma_pci_reload() and lan966x_fdma_pci_deinit(), both of which
first do napi_disable() and lan966x_fdma_tx_disable_netdev() (netif_tx_disable()
drains in-flight ndo_start_xmit callers), and both are under rtnl, so there
does not seem to be a reachable race today.

Given that, would it help to document at the prototype that the caller must
quiesce DMA and the descriptor paths before unmapping a region, since this
is an exported helper?

+
+/* Initialize ATU, dividing the OB space into equally sized regions. */
+void fdma_pci_atu_init(struct fdma_pci_atu *atu, void __iomem *addr)
+{
+	struct fdma_pci_atu_region *regions = atu->regions;
+	u32 region_size = fdma_pci_atu_region_size();
+
+	atu->addr = addr;
+	mutex_init(&atu->lock);
+
+	for (int i = 0; i < FDMA_PCI_ATU_REGION_MAX; i++) {
+		regions[i].base_addr =
+			FDMA_PCI_ATU_OB_START + (i * region_size);
+		regions[i].idx = i;
+		regions[i].atu = atu;
+
+		fdma_pci_atu_region_disable(&regions[i]);
+	}
+}
+EXPORT_SYMBOL_GPL(fdma_pci_atu_init);

[Severity: Low]
Should this also initialize in_use, target_addr and limit_addr?

This is the designated initializer for struct fdma_pci_atu and it even
touches hardware (disabling all six windows), but it only sets base_addr,
idx and atu. The allocation state that fdma_pci_atu_region_get_free()
consumes is in_use, which is never assigned here, so the helper silently
depends on the embedding structure having been zeroed. If in_use held
garbage, every region would look busy and mapping would fail with -ENOSPC.

The only in-tree caller later in the series passes &lan966x->atu, and
lan966x comes from devm_kzalloc() in lan966x_probe(), so it works today.
Would it be worth clearing the per-region software state here (or noting
the "caller must zero the struct" precondition at the prototype)?

There is also no counterpart to this function: nothing resets the software
state, unmaps still-mapped regions, or calls mutex_destroy(). Is a
fdma_pci_atu_deinit() wanted for symmetry?

[ ... ]

+/* Outbound regions are 64KB granular (datasheet section 3.24.7.4.1), so both
+ * the region base and the mapped size must be aligned to this.
+ */
+#define FDMA_PCI_ATU_REGION_ALIGN BIT(16)

[ ... ]

+/* Initialize ATU, dividing OB space into regions. */
+void fdma_pci_atu_init(struct fdma_pci_atu *atu, void __iomem *addr);
+
+/* Unmap an ATU region, clearing its translation and disabling it. */
+void fdma_pci_atu_region_unmap(struct fdma_pci_atu_region *region);
+
+/* Map a host DMA address into a free ATU region. */
+struct fdma_pci_atu_region *fdma_pci_atu_region_map(struct fdma_pci_atu *atu,
+						    u64 target_addr,
+						    int size);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-lan966x-pci-fdma-v6-0-6f48dab9d671%40microchip.com