Re: [PATCH net-next v8 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()
  (drivers/net/ethernet/microchip/fdma/fdma_pci.c) is exported.
- [Low] The ATU region claimed in fdma_alloc_coherent_and_map() is
  released only by fdma_free_coherent_and_unmap().
- [Low] If fdma_pci_atu_region_map() fails, fdma_alloc_coherent_and_map()
  calls fdma_free_coherent() (fdma_api.c:154) but leaves fdma->dcbs and…
- [Low] fdma_pci_atu_region_map() sets the iATU enable bit (CTRL2, offset
  0x4) with a bare writel() and never reads it back.
- [Low] fdma_pci_atu_configure_translation() and fdma_pci_atu_init()
  never write iATU REGION_CTRL1 (offset 0x0: TLP type, function number,…

--

net: microchip: fdma: add PCIe ATU support

This patch adds PCIe ATU support to the shared Microchip FDMA library. It
lets lan966x (and later lan969x) DMA to host memory when running as a PCIe
endpoint. The outbound address space is split into six regions. New
helpers combine coherent DMA allocation with claiming and programming an
ATU region, and the mapped allocation is padded to the 64KB ATU
granularity.

> diff --git a/drivers/net/ethernet/microchip/fdma/fdma_api.c b/drivers/net/ethernet/microchip/fdma/fdma_api.c
> index e78c3590da9e6..a3c9e3097c5c2 100644
> --- a/drivers/net/ethernet/microchip/fdma/fdma_api.c
> +++ b/drivers/net/ethernet/microchip/fdma/fdma_api.c
> @@ -127,6 +127,50 @@ void fdma_free_phys(struct fdma *fdma)
>  }
>  EXPORT_SYMBOL_GPL(fdma_free_phys);
>  
> +#if IS_ENABLED(CONFIG_MCHP_LAN966X_PCI)
> +/* Allocate coherent DMA memory and map it in the ATU. */
> +int fdma_alloc_coherent_and_map(struct device *dev, struct fdma *fdma,
> +				struct fdma_pci_atu *atu)
> +{
> +	struct fdma_pci_atu_region *region;
> +	int err;
> +
> +	if (WARN_ON(fdma->atu_region))
> +		return -EBUSY;
> +

[ ... ]

> +	err = fdma_alloc_coherent(dev, fdma);
> +	if (err)
> +		return err;
> +
> +	region = fdma_pci_atu_region_map(atu, fdma->dma, fdma->size);
> +	if (IS_ERR(region)) {
> +		fdma_free_coherent(dev, fdma);
> +		return PTR_ERR(region);
> +	}

[Severity: Low]
If fdma_pci_atu_region_map() fails here, the buffer is freed, but
fdma->dcbs and fdma->dma still point at it. A later
fdma_free_coherent_and_unmap() or fdma_free_coherent() on the same struct
fdma would then call dma_free_coherent() on this buffer a second time.
Could that happen?

No caller later in the series seems to do this:
lan966x_fdma_pci_rx_alloc()/tx_alloc() just return the error.
lan966x_fdma_pci_init() frees only the rx struct when the tx allocation
fails. lan966x_fdma_pci_reload() puts the old structs back with memcpy()
on failure.

Would clearing fdma->dcbs and fdma->dma after the rollback free still be
worth doing?

> +
> +	fdma->atu_region = region;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fdma_alloc_coherent_and_map);
> +
> +/* Free coherent DMA memory and unmap the memory in the ATU. */
> +void fdma_free_coherent_and_unmap(struct device *dev, struct fdma *fdma)
> +{
> +	fdma_pci_atu_region_unmap(fdma->atu_region);
> +	fdma->atu_region = NULL;
> +	fdma_free_coherent(dev, fdma);
> +}

[Severity: Low]
This is the only path that releases the region claimed in
fdma_alloc_coherent_and_map(). The plain fdma_free_coherent() is still
exported and does not touch fdma->atu_region.

What happens if a by-value copy of struct fdma is freed with
fdma_free_coherent(), as the platform lan966x_fdma_reload() does? The
region would stay in_use, with an enabled outbound window pointing at
freed memory. The next map could also hit WARN_ON(fdma->atu_region).

Later in the series the PCIe path looks correct. lan966x_fdma_pci_reload()
sets rx.fdma.atu_region and tx.fdma.atu_region to NULL before
reallocating, then frees the old copies with
fdma_free_coherent_and_unmap(). The platform lan966x_fdma_reload() only
runs through lan966x_fdma_ops, which never maps ATU regions.

Would it help to document that fdma_free_coherent() must not be used on
a mapped struct fdma?

[ ... ]

> 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 0000000000000..dd1dc46cbc9dc
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/fdma/fdma_pci.c
> @@ -0,0 +1,208 @@

[ ... ]

> +/* Configure the address translation in the ATU. */
> +static void
> +fdma_pci_atu_configure_translation(struct fdma_pci_atu_region *region)
> +{

[ ... ]

> +	/* 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]
This writes base, limit and target, but it never writes REGION_CTRL1
(offset 0x0: TLP type, function number, INCREASE_REGION_SIZE) or
UPPER_LIMIT (offset 0x20). fdma_pci_atu_init() doesn't write them either;
it only clears the enable bit.

Does this code depend on those registers still holding their reset values
(memory request type, function 0, no increased region size)? For
comparison, dw_pcie_prog_outbound_atu() always writes CTRL1 explicitly.

No other agent that programs these registers has been identified, and
this driver never writes them. Would setting CTRL1 and UPPER_LIMIT to 0
during setup 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)
> +{

[ ... ]

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

[ ... ]

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

[Severity: Low]
target_addr is checked for FDMA_PCI_ATU_REGION_ALIGN alignment, but size
is not, and size goes straight into limit_addr here.

The commit message says the hardware widens a limit that is not 64KB
aligned. fdma_pci_atu_region_map() is exported. If a caller passed a size
such as 0x1234, would the outbound window then extend to
target_addr + 0xffff, past the end of the caller's buffer?

This isn't reachable today. The fdma_pci.h comment says "target_addr and
size must be FDMA_PCI_ATU_REGION_ALIGN aligned". The only in-tree caller,
fdma_alloc_coherent_and_map(), pads fdma->size with ALIGN() first.

Could an IS_ALIGNED(size, FDMA_PCI_ATU_REGION_ALIGN) check that returns
-EINVAL go next to the target_addr check, so the documented precondition
is enforced?

> +	region->in_use = true;
> +
> +	fdma_pci_atu_configure_translation(region);
> +	fdma_pci_atu_region_enable(region);

[Severity: Low]
fdma_pci_atu_region_enable() sets the enable bit in CTRL2 (offset 0x4)
with a bare writel(), and success is returned without reading the bit
back. On the other side, fdma_pci_atu_region_unmap() clears the bit with
a posted write, and fdma_free_coherent_and_unmap() frees the buffer right
after.

dw_pcie_prog_outbound_atu() polls CTRL2 after enabling a region, with the
comment "Make sure ATU enable takes effect before any subsequent config
and I/O accesses". Does the lan966x iATU need a similar read-back?

In the series as posted, lan966x_fdma_rx_start() reads registers on the
same endpoint before it activates a channel, and those reads would flush
the enable write. Every unmap caller also first waits for the channels to
go idle via lan966x_fdma_rx_disable()/tx_disable(). So this only matters
if the hardware needs settle time beyond that flush.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-lan966x-pci-fdma-v8-0-201c7b707d8b%40microchip.com