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

[Daniel Machon <daniel.machon@microchip.com>]

> Thank you for your contribution! Sashiko AI review found 8 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 3 (1) · Low: 5 (1)
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] Double fetch of the RX BLOCKL.
> - [Medium] lan966x_fdma_pci_napi_poll() limits RX consumption only by
>   `counter < weight` and tracks refill progress only through the wrapped…
> - [Medium] When lan966x_fdma_pci_get_next_dcb() finds no free TX DCB,
>   lan966x_fdma_pci_xmit() calls netif_stop_queue(dev) and returns…
> - [Low] At this commit the in-tree PCIe overlay
>   (drivers/misc/lan966x_pci.dtso) has no "fdma" interrupt.
> - [Low] The iomap entry { TARGET_PCIE_DBI, 0x400000, 0 } derives
>   regs[TARGET_PCIE_DBI] 4 MB into IO range 0.
> - [Low] lan966x_fdma_pci_resize() always returns -EOPNOTSUPP, so once
>   PCIe FDMA is active every ndo_change_mtu call is rolled back.
> - [Low] In lan966x_fdma_pci_napi_poll(), the FDMA_ERROR branch says "No
>   rx_dropped increment here because src_port is invalid".
> - [Low] With lan966x->ptp left at 0 on PCIe, lan966x_ptp_init() never
>   initialises port->tx_skbs or the PTP locks, while the "ptp" IRQ…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the new PCIe
>   backend inherits it.
> - [Medium] __fdma_dcb_add() writes fdma->last_dcb->nextptr (publishing
>   the new DCB) before it writes the new DCB's own nextptr =…
> - [Low] lan966x_fdma_pci_hw_offload() reads ip_hdr(skb)->protocol and
>   ipv6_hdr(skb)->daddr without checking that skb->len covers an IP…
> 
> --
> 
> net: lan966x: add PCIe FDMA support
> 
> This commit adds a PCIe FDMA backend for lan966x. It uses contiguous
> coherent DMA buffers mapped through the endpoint's ATU and copies frames
> with memcpy. The new ops are selected when lan966x_is_pci() is true, and
> XDP and PTP are disabled on the PCIe path.
> 
> > 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 000000000000..f511e7061314
> > --- /dev/null
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> 
> [ ... ]
> 
> > +static bool lan966x_fdma_pci_hw_offload(struct lan966x *lan966x, u32 port,
> > +					struct sk_buff **pskb)
> > +{
> 
> [ ... ]
> 
> > +	if (skb->protocol == htons(ETH_P_IP) &&
> > +	    ip_hdr(skb)->protocol == IPPROTO_IGMP)
> > +		return false;
> > +
> > +	if (IS_ENABLED(CONFIG_IPV6) &&
> > +	    skb->protocol == htons(ETH_P_IPV6) &&
> > +	    ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr) &&
> > +	    !ipv6_mc_check_mld(skb))
> > +		return false;
> 
> [Severity: Low]
> This isn't a bug introduced by this patch, because the same code already
> exists in lan966x_hw_offload() in lan966x_main.c. Still,
> ip_hdr(skb)->protocol and ipv6_hdr(skb)->daddr are read here without
> checking that skb->len covers an IP header.
> 
> On a runt frame, or on a frame shrunk by an XDP program once XDP support
> lands later in the series, could this read uninitialized skb tailroom?
> 
> The reads stay inside the skb head allocation, and any minimum-size
> Ethernet frame covers both fields. So only the offload_fwd_mark decision
> for an already malformed packet is affected.
> 
> > +static struct sk_buff *lan966x_fdma_pci_rx_get_frame(struct lan966x_rx *rx,
> > +						     u64 src_port)
> > +{
> 
> [ ... ]
> 
> > +	/* Get the received frame and create an SKB for it. */
> > +	db = fdma_db_next_get(fdma);
> > +	data_len = fdma_db_len_get(db);
> > +
> > +	skb = napi_alloc_skb(&lan966x->napi, data_len);
> > +	if (unlikely(!skb))
> > +		return NULL;
> > +
> > +	memcpy(skb->data,
> > +	       fdma_dataptr_virt_addr_contiguous(fdma,
> > +						 fdma->dcb_index,
> > +						 fdma->db_index),
> > +						 data_len);
> 
> [Severity: Medium]
> BLOCKL is validated in lan966x_fdma_pci_rx_check_frame() through
> lan966x_fdma_pci_rx_size_fits(). Here it is read again from the DCB
> status in coherent DMA memory.
> 
> Can the value passed to napi_alloc_skb() and memcpy() differ from the
> value that was checked?
> 
> It looks like "net: lan966x: add PCIe FDMA XDP support" later in the
> series fixes this. That patch reads blockl once in rx_check_frame() and
> passes data and data_len to rx_get_frame(). Would it make sense to fold
> that change into this patch?
> 
> At this commit the path isn't enabled in-tree yet, because the overlay
> has no "fdma" interrupt.

Not enabled, yes. Non-issue.

> 
> [ ... ]
> 
> > +static int lan966x_fdma_pci_xmit(struct sk_buff *skb, __be32 *ifh,
> > +				 struct net_device *dev)
> > +{
> 
> [ ... ]
> 
> > +	next_to_use = lan966x_fdma_pci_get_next_dcb(fdma);
> > +
> > +	if (next_to_use < 0) {
> > +		netif_stop_queue(dev);
> > +		return NETDEV_TX_BUSY;
> > +	}
> 
> [Severity: Medium]
> netif_stop_queue() only stops TX queue 0. Each port netdev is created in
> lan966x_probe_port() with 8 TX queues:
> 
> 	dev = devm_alloc_etherdev_mqs(lan966x->dev,
> 				      sizeof(struct lan966x_port),
> 				      NUM_PRIO_QUEUES, 1);
> 
> There is no ndo_select_queue, so skbs are spread over all 8 queues.
> lan966x_fdma_wakeup_netdev(), called from the PCIe NAPI poll, also only
> checks and wakes queue 0.
> 
> When the shared TX ring is full and an skb arrives on one of queues 1 to
> 7, does that queue ever get stopped?
> 
> It looks like sch_direct_xmit() would requeue the skb and reschedule the
> qdisc. net_tx_action would then keep retrying, taking tx_lock and
> scanning the whole DCB ring each time, until the hardware completes a
> DCB.
> 
> Would netif_tx_stop_all_queues() and netif_tx_wake_all_queues() be a
> better fit here? The platform lan966x_fdma_xmit() has the same pattern,
> and lan966x_fdma_pci_xmit_xdpf() from "net: lan966x: add PCIe FDMA XDP
> support" repeats it.
> 

This is pre-existing behaviour. The lan966x platform path, sparx5 and lan969x
all do it the same way. I dont think this is a bug, so if anything, it should go
to net-next with at patchset for all platforms together

> [ ... ]
> 
> > +	/* 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: Medium]
> This is a pre-existing issue in the shared fdma_api.c helper and was not
> introduced by this patch, but the new backend depends on it.
> __fdma_dcb_add() links the new DCB into the chain before terminating it,
> and there is no DMA write barrier between the two steps:
> 
> drivers/net/ethernet/microchip/fdma/fdma_api.c:__fdma_dcb_add() {
>     ...
> 	fdma->last_dcb->nextptr = cpu_to_le64(nextptr);
> 
> 	fdma->last_dcb = dcb;
> 
> 	dcb->nextptr = cpu_to_le64(FDMA_DCB_INVALID_DATA);
> 	dcb->info = cpu_to_le64(info);
>     ...
> }
> 
> If the FDMA channel is still walking the chain, could it follow the new
> link and read a stale nextptr or info from the new DCB?
> 
> The window is short, and whether the hardware acts on it depends on its
> prefetch behavior. When the channel is stopped, the writel() doorbell in
> lan966x_fdma_tx_start() or lan966x_fdma_rx_reload() orders the earlier
> stores.
> 
> [ ... ]
> 
> > +static int lan966x_fdma_pci_napi_poll(struct napi_struct *napi, int weight)
> > +{
> 
> [ ... ]
> 
> > +	dcb_reload = fdma->dcb_index;
> > +
> > +	/* Get all received skbs. */
> > +	while (counter < weight) {
> > +		if (!fdma_has_frames(fdma))
> > +			break;
> > +		/* Order DONE read before DCB/frame reads below. */
> > +		dma_rmb();
> > +		counter++;
> > +		switch (lan966x_fdma_pci_rx_check_frame(rx, &src_port)) {
> > +		case FDMA_PASS:
> > +			break;
> > +		case FDMA_ERROR:
> > +			/* No rx_dropped increment here because src_port is
> > +			 * invalid.
> > +			 */
> 
> [Severity: Low]
> Is this comment accurate for every FDMA_ERROR return?
> lan966x_fdma_pci_rx_check_frame() also returns FDMA_ERROR after src_port
> and ports[src_port] have already been validated:
> 
> 	blockl = fdma_db_len_get(db);
> 	if (!lan966x_fdma_pci_rx_size_fits(fdma, blockl))
> 		return FDMA_ERROR;
> 
> In that case a frame with a bad BLOCKL on a valid port is dropped without
> being counted in rx_dropped or rx_length_errors. This code is unchanged
> at the end of the series.
> 
> > +			fdma_dcb_advance(fdma);
> > +			continue;
> > +		}
> 
> [ ... ]
> 
> > +	while (dcb_reload != fdma->dcb_index) {
> > +		old_dcb = dcb_reload;
> > +		dcb_reload++;
> > +		dcb_reload &= fdma->n_dcbs - 1;
> 
> [Severity: Medium]
> The RX loop is bounded only by counter < weight. Refill progress is
> tracked only through the wrapped dcb_reload and dcb_index values.
> 
> What happens if weight >= n_dcbs and every DCB is DONE? n_dcbs is
> FDMA_DCB_MAX, which is 512 here and 256 after "net: lan966x: add PCIe
> FDMA MTU change support".
> 
> fdma_dcb_advance() would wrap dcb_index back to dcb_reload. DONE bits are
> only cleared by this refill loop, so the RX loop would then reprocess
> stale DONE descriptors and pass the same frames to napi_gro_receive()
> again.
> 
> When weight is a multiple of n_dcbs, dcb_reload equals fdma->dcb_index at
> this point. The refill loop then doesn't run at all, and the ring is left
> un-armed.
> 
> The normal NAPI weight of 64 can't reach this, but busy polling can.
> __napi_busy_loop() passes the socket's budget straight to napi->poll, and
> SO_BUSY_POLL_BUDGET accepts values up to U16_MAX with CAP_NET_ADMIN.
> 
> The native lan966x_fdma_napi_poll() has the same structure. This is
> still present at the end of the series.

This is a re-raise of v7 finding that was answered and dismissed.

> 
> [ ... ]
> 
> > +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;
> 
> [Severity: Low]
> At this commit the in-tree PCIe overlay (drivers/misc/lan966x_pci.dtso)
> only declares:
> 
> 	interrupt-names = "xtr", "ana";
> 
> As a result lan966x->fdma stays false on the PCIe instance, and this
> function returns right away. Is it intended that the new backend is
> dormant at this commit?
> 
> The "fdma" interrupt is added later in the series by "misc: lan966x-pci:
> dts: add fdma interrupt to overlay". Until then the driver keeps using
> register-based I/O.
> 
> [ ... ]
> 
> > +	lan966x->tx.lan966x = lan966x;
> > +	tx_fdma->channel_id = FDMA_INJ_CHANNEL;
> > +	tx_fdma->n_dcbs = FDMA_DCB_MAX;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, because the platform
> lan966x_fdma_init() uses the same ordering, but the new PCIe backend
> inherits it.
> 
> lan966x_probe() sets lan966x->fdma = true, registers every port netdev,
> and enables NAPI (lan966x_port_init() -> lan966x_fdma_netdev_init()), all
> before it calls lan966x->ops->fdma_init().
> 
> Here tx_fdma->n_dcbs is set without holding tx_lock. tx_fdma->dcbs is
> only allocated later, in lan966x_fdma_pci_tx_alloc(). That happens after
> the GFP_KERNEL coherent allocation in lan966x_fdma_pci_rx_alloc(), which
> can sleep.
> 
> If a port is opened and transmits during that window, can the following
> happen?
> 
> lan966x_port_xmit()
>     spin_lock(&lan966x->tx_lock);
>     if (port->lan966x->fdma)
>         lan966x->ops->fdma_xmit()
>             lan966x_fdma_pci_xmit()
>                 lan966x_fdma_pci_get_next_dcb()
>                     fdma_db_get(fdma, i, 0)   <- fdma->dcbs is NULL
> 
> Later in the same window, xmit could also pick DCBs that
> fdma_dcbs_init() has already marked DONE while last_dcb is still being
> set up. It could also start the injection channel before
> lan966x_fdma_llp_configure() has programmed the TX LLP.
> 
> Would either of these close the window?
> 
> - Calling fdma_init before the ports are registered.
> - Publishing n_dcbs under tx_lock, only after the rings are allocated.
> 

This is a re-raise of v7 finding that was answered and dismissed. Probe order is
pre-existing and not something this series can fix.

> [ ... ]
> 
> > +static int lan966x_fdma_pci_resize(struct lan966x *lan966x)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> 
> [Severity: Low]
> lan966x_port_change_mtu() rolls back the MTU whenever
> lan966x->ops->fdma_resize() fails. With this stub, won't every
> ndo_change_mtu call fail once PCIe FDMA is active?
> 
> The next patch, "net: lan966x: add PCIe FDMA MTU change support",
> replaces the stub with a real implementation. At this commit the path is
> dormant, because the overlay has no "fdma" interrupt.
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > index 024ce9f9916c..de2202786826 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > @@ -50,6 +50,7 @@ struct lan966x_main_io_resource {
> >  static const struct lan966x_main_io_resource lan966x_main_iomap[] =  {
> >  	{ TARGET_CPU,                   0xc0000, 0 }, /* 0xe00c0000 */
> >  	{ TARGET_FDMA,                  0xc0400, 0 }, /* 0xe00c0400 */
> > +	{ TARGET_PCIE_DBI,             0x400000, 0 }, /* 0xe0400000 */
> 
> [Severity: Low]
> This entry places regs[TARGET_PCIE_DBI] 4 MB into IO range 0. At this
> commit the overlay's "cpu" reg is only 1 MB:
> 
> 	reg = <0xe0000000 0x0100000>,
> 
> Wouldn't the iATU accesses from fdma_pci_atu_init(), and the region map
> and unmap, fall outside the ioremap window?
> 
> This can't be reached at this commit, because lan966x_fdma_pci_init()
> returns early without an "fdma" interrupt. Later in the series, "misc:
> lan966x-pci: dts: extend cpu reg to cover PCIE DBI space" enlarges the
> reg to 0x0800000, and it comes before the "fdma" interrupt is added. On
> the SoC instance the pointer is computed but never used.
> 
> [ ... ]
> 
> > @@ -1187,7 +1190,9 @@ static int lan966x_probe(struct platform_device *pdev)
> >  		if (err)
> >  			return dev_err_probe(&pdev->dev, err, "Unable to use ptp irq");
> > 
> > -		lan966x->ptp = 1;
> > +		/* PTP is not supported on the PCIe path yet. */
> > +		if (!lan966x_is_pci(lan966x))
> > +			lan966x->ptp = 1;
> >  	}
> 
> [Severity: Low]
> With lan966x->ptp left at 0 on PCIe, lan966x_ptp_init() never
> initializes port->tx_skbs or the PTP locks. If a "ptp" interrupt exists,
> lan966x_ptp_irq_handler has already been registered just above. Could
> that handler then touch uninitialized PTP state?
> 
> This can't happen in-tree, because drivers/misc/lan966x_pci.dtso doesn't
> declare a "ptp" interrupt. Would it be more robust to also skip the ptp
> IRQ request on PCIe?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-lan966x-pci-fdma-v9-0-a17caf60f89a%40microchip.com