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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The ATU outbound window limit is statically configured to its maximum size instead of being tightly constrained to the allocated DMA buffer size.
- [Medium] Hardware ATU regions are not explicitly disabled during initialization, risking stray DMAs from stale configuration.
- [Low] The block comment detailing the ATU regions contains a mathematical error, contradicting the actual hardware configuration.
--

commit 474737a9138a576d895670ba7334d761b2fb9464
Author: Daniel Machon <daniel.machon@microchip.com>
net: microchip: fdma: add PCIe ATU support

When lan966x or lan969x operates as a PCIe endpoint, the internal FDMA
engine cannot directly access host memory. Instead, DMA addresses must
be translated through the PCIe Address Translation Unit (ATU), which
provides outbound windows mapping internal addresses to PCIe bus addresses.

> 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..1bd41eaa58a4b
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/fdma/fdma_pci.c
> @@ -0,0 +1,182 @@
[ ... ]
> + * +-------------+------------+------------+-----+------------+
> + * | Index       | Region 0   | Region 1   | ... | Region 5   |
> + * +-------------+------------+------------+-----+------------+
> + * | Base addr   | 0x10000000 | 0x12aa0000 | ... | 0x1d500000 |

[Severity: Low]
This isn't a bug, but there appears to be a mathematical error in the
comment. The computed region size is round_down((0x1fffffff - 0x10000000) /
6, BIT(16)) which equals 0x2aa0000. Using this size, Region 5's base address
would be 0x10000000 + (5 * 0x2aa0000) = 0x1d520000, not 0x1d500000.

> + * | Limit addr  | 0x12a9ffff | 0x1553ffff | ... | 0x1ff9ffff |
> + * | Target addr | host dma   | host dma   | ... | host dma   |
> + * +-------------+------------+------------+-----+------------+

[ ... ]

> +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(-E2BIG);
> +
> +	region = fdma_pci_atu_region_get_free(atu);
> +	if (IS_ERR(region))
> +		return region;
> +
> +	region->target_addr = target_addr;
> +	region->in_use = true;
> +
> +	/* Enable first, according to datasheet section 3.24.7.4.1 */
> +	fdma_pci_atu_region_enable(region);
> +	fdma_pci_atu_configure_translation(region);
> +
> +	return region;
> +}

[Severity: Medium]
Could this dynamically program the limit_addr to base_addr + size - 1
to enforce strict DMA bounds, rather than using the statically
configured ~42MB limit?

Since the actual DMA buffer allocations are typically much smaller, the
current approach massively over-provisions the DMA window. Could a
hardware bug or malicious endpoint firmware exploit this to perform
out-of-bounds DMAs and silently corrupt adjacent host physical memory?

[ ... ]

> +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;
> +
> +	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].limit_addr =
> +			regions[i].base_addr + region_size - 1;
> +		regions[i].idx = i;
> +		regions[i].atu = atu;
> +	}
> +}

[Severity: Medium]
Should the initialization explicitly disable all hardware ATU regions by
clearing their enable bits?

Because the PCIe endpoint controller cannot be hard-reset by the driver
without dropping the PCIe link to the host, the ATU registers might
retain state from the bootloader or a previous kernel crash. Could
stray DMAs to host memory occur before the kernel explicitly reclaims
and reconfigures these regions if they are left enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-lan966x-pci-fdma-v5-0-ca56197ae05b@microchip.com?part=3