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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] fdma_pci_atu_region_map() in
  drivers/net/ethernet/microchip/fdma/fdma_pci.c is exported.
- [Low] The alignment comment in fdma_pci_atu_region_map()
  (drivers/net/ethernet/microchip/fdma/fdma_pci.c, lines 151-155) says…
- [Low] fdma_pci_atu_region_enable() writes REGION_CTRL2 = BIT(31) with a
  single posted writel().
- [Low] fdma_pci_atu_configure_translation() programs LOWER/UPPER_BASE,
  LIMIT and LOWER/UPPER_TARGET, and fdma_pci_atu_init() only clears…

--

net: microchip: fdma: add PCIe ATU support

This commit adds PCIe ATU outbound window support to the Microchip FDMA
library. When the switch runs as a PCIe endpoint, the lan966x (and later
lan969x) FDMA engine can then reach host memory. The outbound space is
split into six regions, and new helpers combine a coherent DMA allocation
with claiming and programming a region, padding the allocation to the
64KB ATU granularity.

> diff --git a/drivers/net/ethernet/microchip/fdma/fdma_pci.c b/drivers/net/ethernet/microchip/fdma/fdma_pci.c
> new file mode 100644
> index 000000000000..dd1dc46cbc9d
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/fdma/fdma_pci.c

[ ... ]

> +/* 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));
> +
> +	/* The OB address space lies entirely below 4GB, so the limit always
> +	 * fits the lower limit register and the upper one is left alone.
> +	 */
> +	writel(region->limit_addr,
> +	       fdma_pci_atu_addr_get(atu->addr, FDMA_PCI_ATU_LIMIT_ADDR, idx));

[Severity: Low]
Should REGION_CTRL1 (offset 0x0) and the upper limit register (offset
0x20) be written explicitly, either here or in fdma_pci_atu_init()?

Neither fdma_pci_atu_configure_translation() nor fdma_pci_atu_init()
writes them. fdma_pci_atu_init() only clears the enable bit through
fdma_pci_atu_region_disable(). As a result, the TLP type, function
number, INCREASE_REGION_SIZE and the upper limit all depend on reset
values.

For comparison, dw_pcie_prog_outbound_atu() in
drivers/pci/controller/dwc/pcie-designware.c always writes CTRL1. On
4.60a and later it also writes the upper limit:

	if (dw_pcie_ver_is_ge(pci, 460A))
		dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_UPPER_LIMIT,
				      upper_32_bits(limit_addr));
	...
	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, val);

This driver never changes those registers, so reloading it can't leave
them stale. It would only matter if something else, such as endpoint-side
firmware, had programmed the ATU before the host driver bound. No in-tree
path doing that was found.

Would writing CTRL1 = 0 at init be reasonable hardening?

[ ... ]

> +/* 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)
> +{

[ ... ]

> +	/* 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.
> +	 */

[Severity: Low]
Is the translation direction in this comment reversed?

In struct fdma_pci_atu_region, base_addr is the OB window address and
target_addr is the host DMA address. The file header says the ATU turns
FDMA OB addresses into host DMA addresses, so the hardware computes
target_addr + (addr - base_addr).

The formula in the comment is the host to OB direction. That is what the
software helper fdma_pci_atu_translate_addr() computes:

	return region->base_addr + (addr - region->target_addr);

This does not affect the code's behaviour. Could the comment instead
describe the OB to host translation that the ATU performs?

> +	if (!IS_ALIGNED(target_addr, FDMA_PCI_ATU_REGION_ALIGN))
> +		return ERR_PTR(-EINVAL);

[Severity: Low]
Should size also be checked for FDMA_PCI_ATU_REGION_ALIGN alignment here?

The prototype comment in fdma_pci.h says:

  /* Map a host DMA address into a free ATU region. target_addr and size must be
   * FDMA_PCI_ATU_REGION_ALIGN aligned; a misaligned target_addr returns -EINVAL.
   */

However, fdma_pci_atu_region_map() only rejects a misaligned target_addr.
Any size in the range 0 < size <= fdma_pci_atu_region_size() is accepted
and programmed directly:

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

The commit message says the hardware widens the limit to the 64KB
boundary. The padding that keeps the widened window inside owned memory
is applied only in fdma_alloc_coherent_and_map():

	fdma->size = ALIGN(fdma->size, FDMA_PCI_ATU_REGION_ALIGN);

That wrapper is the only caller in the series, so no current path passes
an unaligned size. A future direct caller of the exported
fdma_pci_atu_region_map() could, though, and that would open a window
past the end of its buffer.

Would an IS_ALIGNED(size, FDMA_PCI_ATU_REGION_ALIGN) check next to the
target_addr check make the function match its documented contract?

> +
> +	mutex_lock(&atu->lock);

[ ... ]

> +	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);

[Severity: Low]
Should the enable bit be read back before fdma_pci_atu_region_map()
returns success?

fdma_pci_atu_region_enable() is a single posted writel() of BIT(31) to
REGION_CTRL2, and nothing checks whether it took effect.
dw_pcie_prog_outbound_atu() programs the same register layout. It polls
for PCIE_ATU_ENABLE and returns -ETIMEDOUT if the bit never appears:

	/*
	 * Make sure ATU enable takes effect before any subsequent config
	 * and I/O accesses.
	 */

Unmap is similar. fdma_pci_atu_region_unmap() posts the disable with no
flushing read, and fdma_free_coherent_and_unmap() then frees the buffer
right away through fdma_free_coherent().

In the lan966x user, fdma_pci_atu_region_map() is followed by
non-posted reads through the same BAR before FDMA_CH_ACTIVATE, and the
channels are polled until inactive before every unmap. That probably
hides this in practice.

Would a readback after the enable and the disable writes make the
ordering explicit?

> +
> +	mutex_unlock(&atu->lock);
> +
> +	return region;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-lan966x-pci-fdma-v9-0-a17caf60f89a%40microchip.com