Re: [PATCH net-next v8 11/15] net: lan966x: add PCIe FDMA support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: lan966x: missing NAPI initialization in PCIe FDMA path
- [Medium] net: lan966x: RX descriptor ring reload failure when wrap occurs
- [Low] net: lan966x: DMA engine reads uninitialized memory due to FCS padding
--

commit b0974995ea3ce44150d8f413eb0f917b2dc37e02
Author: Daniel Machon <daniel.machon@microchip.com>

net: lan966x: add PCIe FDMA support

This commit adds PCIe FDMA support for the lan966x driver, mapping 
contiguous DMA buffers through the endpoint's ATU. The new path replaces 
per-page DMA mappings with memcpy-based frame transfers. This significantly 
improves throughput over register-based I/O on PCIe cards.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> new file mode 100644
> index 0000000000000..bccd1b8590d74
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> @@ -0,0 +1,421 @@

[ ... ]

> +static int lan966x_fdma_pci_xmit(struct sk_buff *skb, __be32 *ifh,
> +				 struct net_device *dev)
> +{

[ ... ]

> +	/* virt_addr points to the IFH. */
> +	virt_addr = fdma_dataptr_virt_addr_contiguous(fdma, next_to_use, 0);
> +	memcpy(virt_addr, ifh, IFH_LEN_BYTES);
> +	memcpy(virt_addr + IFH_LEN_BYTES, skb->data, skb->len);
> +
> +	/* Order frame write before DCB status write below. */
> +	dma_wmb();
> +
> +	fdma_dcb_add(fdma,
> +		     next_to_use,
> +		     0,
> +		     FDMA_DCB_STATUS_INTR |
> +		     FDMA_DCB_STATUS_SOF |
> +		     FDMA_DCB_STATUS_EOF |
> +		     FDMA_DCB_STATUS_BLOCKO(0) |
> +		     FDMA_DCB_STATUS_BLOCKL(IFH_LEN_BYTES + skb->len + ETH_FCS_LEN));

[Severity: Low]
Does the DMA engine read uninitialized host memory here?

The CPU copies skb->len bytes to the contiguous buffer, but the hardware DCB
is programmed to read skb->len + ETH_FCS_LEN bytes. Even though the hardware
likely overwrites the extra 4 bytes with its own FCS internally so it does
not leak onto the wire, this still instructs the DMA to perform an
out-of-bounds read past the initialized region of the host buffer.

[ ... ]

> +static int lan966x_fdma_pci_napi_poll(struct napi_struct *napi, int weight)
> +{
> +	struct lan966x *lan966x = container_of(napi, struct lan966x, napi);
> +	struct lan966x_rx *rx = &lan966x->rx;
> +	struct fdma *fdma = &rx->fdma;
> +	int dcb_reload, old_dcb;
> +	struct sk_buff *skb;
> +	int counter = 0;
> +	u64 src_port;
> +
> +	/* Wake any stopped TX queues if a TX DCB is available. */
> +	spin_lock(&lan966x->tx_lock);
> +	if (lan966x_fdma_pci_get_next_dcb(&lan966x->tx.fdma) >= 0)
> +		lan966x_fdma_wakeup_netdev(lan966x);
> +	spin_unlock(&lan966x->tx_lock);
> +
> +	dcb_reload = fdma->dcb_index;
> +
> +	/* Get all received skbs. */
> +	while (counter < weight) {
> +		if (!fdma_has_frames(fdma))
> +			break;

[ ... ]

> +		skb = lan966x_fdma_pci_rx_get_frame(rx, src_port);
> +		fdma_dcb_advance(fdma);
> +		if (!skb) {
> +			lan966x->ports[src_port]->dev->stats.rx_dropped++;
> +			continue;
> +		}
> +
> +		napi_gro_receive(&lan966x->napi, skb);
> +	}
> +	while (dcb_reload != fdma->dcb_index) {

[Severity: Medium]
Could this loop condition fail to reload the ring if exactly fdma->n_dcbs
frames are processed during a single poll iteration?

If counter reaches fdma->n_dcbs, fdma->dcb_index will wrap completely
around and equal the cached dcb_reload value. The reload loop condition
would evaluate to false immediately, meaning zero descriptors are replenished.

This would leave the hardware ring marked as done and could cause the driver
to re-process the exact same frames on the next NAPI iteration.

> +		old_dcb = dcb_reload;
> +		dcb_reload++;
> +		dcb_reload &= fdma->n_dcbs - 1;
> +
> +		fdma_dcb_add(fdma,
> +			     old_dcb,
> +			     FDMA_DCB_INFO_DATAL(fdma->db_size - XDP_PACKET_HEADROOM),
> +			     FDMA_DCB_STATUS_INTR);
> +
> +		lan966x_fdma_rx_reload(rx);
> +	}

[ ... ]

> +static int lan966x_fdma_pci_init(struct lan966x *lan966x)
> +{
> +	struct fdma *rx_fdma = &lan966x->rx.fdma;
> +	struct fdma *tx_fdma = &lan966x->tx.fdma;
> +	int err;
> +
> +	if (!lan966x->fdma)
> +		return 0;

[ ... ]

> +	tx_fdma->ops.nextptr_cb = &lan966x_fdma_pci_nextptr_cb;
> +	tx_fdma->ops.dataptr_cb = &lan966x_fdma_pci_dataptr_cb;
> +
> +	err = lan966x_fdma_pci_rx_alloc(&lan966x->rx);
> +	if (err)
> +		return err;

[Severity: High]
Is there a missing NAPI initialization in this path?

In the standard FDMA initialization path, lan966x->napi is initialized using
netif_napi_add(). Here, lan966x_fdma_pci_init sets up the PCIe FDMA path
entirely but appears to omit the NAPI struct initialization.

When the first RX/TX interrupt fires, lan966x_fdma_irq_handler will blindly
call napi_schedule(&lan966x->napi). Because the struct is zero-initialized
and not properly registered, this would lead to a kernel panic when the
softirq handler tries to invoke napi->poll().

> +
> +	err = lan966x_fdma_pci_tx_alloc(&lan966x->tx);
> +	if (err) {
> +		fdma_free_coherent_and_unmap(lan966x->dma_dev, rx_fdma);
> +		return err;
> +	}
> +
> +	lan966x_fdma_rx_start(&lan966x->rx);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-lan966x-pci-fdma-v8-0-201c7b707d8b@microchip.com?part=11