Re: [PATCH net-next v3 6/8] net: xilinx: tsn: add the endpoint RX data path

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 1 · High: 1 · Medium: 2 · Low: 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).

- [Critical] RX buffers are freed and unmapped while the MCDMA engine
  still owns them, so incoming DMA lands in freed memory.
- [High] The RX path cannot deliver any frame on the in-tree Xilinx AXI
  MCDMA provider.
- [Medium] After an MCDMA RX channel error, RX on that queue stops
  permanently while the interface stays up.
- [Medium] A partially armed RX batch can hold received frames for an
  unbounded time under sparse traffic.
- [Low] ep_rx_refill() decides whether 'at least one descriptor is armed'
  by testing ring_head != READ_ONCE(ring_tail) after…

--

net: xilinx: tsn: add the endpoint RX data path

This commit posts RX buffers to every MCDMA RX channel on open. It reaps
them from the dmaengine completion callback and passes MAC1/MAC2 frames up
the stack with __netif_rx(). Refill runs from the completion callback, with
a retry timer for a drained ring and a closing flag plus a per-channel lock
to serialise refill against teardown.

> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> index ab3d6d8cb9440..0a13e04d40575 100644
> --- a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c

[ ... ]

> @@ -112,6 +135,181 @@ ep_get_desc(struct xlnx_tsn_ep_dma_chan *xchan, int idx)

[ ... ]

> +static bool ep_rx_refill(struct xlnx_tsn_ep_dma_chan *xchan, bool arm_timer)
> +{
> +	int avail, i;
> +
> +	scoped_guard(spinlock_bh, &xchan->rx_lock) {
> +		if (READ_ONCE(xchan->ep->closing))
> +			return false;
> +
> +		avail = CIRC_SPACE(xchan->ring_head, READ_ONCE(xchan->ring_tail),
> +				   xchan->ring_size);
> +		for (i = 0; i < avail; i++) {
> +			if (ep_rx_submit_desc(xchan))
> +				break;
> +		}
> +		dma_async_issue_pending(xchan->chan);

[Severity: Critical]
Can this give RX buffers back to the allocator while the MCDMA engine still
owns them?

On open, this loop posts CIRC_SPACE(0, 0, 128) = 127 descriptors in one
batch. xilinx_mcdma_start_transfer() only reprograms the IRQ coalescing
threshold when the batch is small enough:

drivers/dma/xilinx/xilinx_dma.c:xilinx_mcdma_start_transfer() {
    ...
	if (chan->desc_pendingcount <= XILINX_MCDMA_COALESCE_MAX) {
		reg &= ~XILINX_MCDMA_COALESCE_MASK;
		reg |= chan->desc_pendingcount <<
			XILINX_MCDMA_COALESCE_SHIFT;
	}
    ...
}

XILINX_MCDMA_COALESCE_MAX is 24. With 127 pending, the threshold keeps its
previous or reset value, which is well below 127.

When the first IOC fires, xilinx_mcdma_irq_handler() calls
xilinx_dma_complete_descriptor(). That function only checks per-BD
completion for AXIDMA:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_complete_descriptor() {
    ...
	list_for_each_entry_safe(desc, next, &chan->active_list, node) {
		if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
			...
			if (!(seg->hw.status & XILINX_DMA_BD_COMP_MASK) && chan->has_sg)
				break;
		}
		...
		list_add_tail(&desc->node, &chan->done_list);
	}
}

For MCDMA, every active descriptor goes to done_list, and the tasklet runs
ep_dma_rx_cb() for each of them. For every BD the hardware has not filled
yet, ep_dma_rx_cb() unconditionally calls dma_unmap_single() and then frees
the skb.

The S2MM engine's CURDESC..TAILDESC chain still points at those BDs and
buffer addresses. Would the next received frames then be written into
freed skb memory?

The descriptor segments are also recycled, and the next refill rewrites
them while the engine is running. active_list is empty by then, so
xilinx_mcdma_start_transfer() also rewrites CDESC on a channel that already
has RUNSTOP set:

	if (chan->has_sg && list_empty(&chan->active_list))
		xilinx_write(chan, XILINX_MCDMA_CHAN_CDESC_OFFSET(chan->tdest),
			     head_desc->async_tx.phys);

The same thing looks possible in steady state, whenever a small refill
sets the threshold to 1 while other descriptors are still active.

The commit message says dmaengine "delivers one descriptor per callback".
That assumes per-BD completion, and this provider doesn't appear to
provide it for MCDMA.

[Severity: Medium]
Could a partially armed batch hold received frames for an unbounded time
when traffic is sparse?

This loop can post between 2 and 24 descriptors in one call. That happens
if dev_alloc_skb() fails partway through, or if a callback refills several
slots at once. xilinx_mcdma_start_transfer() then sets the coalescing
threshold to that count, and no IOC is raised until that many frames have
arrived.

xilinx_mcdma_irq_handler() only logs the delay interrupt and reaps nothing:

	if (status & XILINX_MCDMA_IRQ_DELAY_MASK) {
		...
		dev_dbg(chan->dev, "Inter-packet latency too long\n");
	}

No delay timer appears to be programmed for MCDMA. ep_rx_refill() returns
true because the ring is non-empty, so the refill timer isn't armed either.

Would low-rate traffic such as ARP, PTP or control frames sit in host
memory until enough further frames arrive, or indefinitely if traffic
stops?

> +
> +		if (xchan->ring_head != READ_ONCE(xchan->ring_tail))
> +			return true;

[Severity: Medium]
What recovers this RX queue after an MCDMA channel error?

On XILINX_MCDMA_IRQ_ERR_MASK, xilinx_mcdma_irq_handler() sets
chan->err = true. An error-only interrupt completes no descriptor, because
completion only happens under XILINX_MCDMA_IRQ_IOC_MASK. So no
ep_dma_rx_cb() runs, and xilinx_mcdma_start_transfer() returns early while
chan->err is set.

The outstanding descriptors keep ring_head != ring_tail here. The refill
timer is never armed and nothing new is submitted. The only automatic
reset is in xilinx_dma_tx_submit():

	if (chan->err) {
		...
		err = xilinx_dma_chan_reset(chan);

That reset needs a new submit, and none ever happens.

Does the queue then stay halted until ifdown/ifup, with the interface up
and no driver-level counter showing the problem?

> +
> +		if (arm_timer)
> +			mod_timer(&xchan->rx_refill_timer,
> +				  jiffies + EP_RX_REFILL_RETRY);
> +	}
> +
> +	return false;
> +}

[ ... ]

> +static void ep_dma_rx_cb(void *data, const struct dmaengine_result *result)
> +{

[ ... ]

> +	metadata = dmaengine_desc_get_metadata_ptr(skbuf_dma->desc,
> +						   &meta_len,
> +						   &meta_max_len);
> +	if (IS_ERR_OR_NULL(metadata)) {
> +		if (net_ratelimit())
> +			dev_warn(ep->dev, "Failed to get RX metadata pointer\n");
> +
> +		dev_kfree_skb_any(skb);
> +		DEV_STATS_INC(ndev, rx_dropped);
> +		DEV_STATS_INC(ndev, rx_errors);
> +		goto submit_new;
> +	}
> +
> +	/* MCDMA metadata: [0] = status, [1] = sideband (TID/TDEST/TUSER), [2..] = app */
> +	tuser = metadata[1] & TSN_TUSER_MASK;

[Severity: High]
Can any frame reach __netif_rx() with the in-tree MCDMA provider?

For XDMA_TYPE_AXIMCDMA, xilinx_dma_probe() installs
xilinx_mcdma_prep_slave_sg(), and that function never sets
async_tx.metadata_ops. DESC_METADATA_ENGINE is only set when
has_axistream_connected is true, and that flag is only read for AXIDMA:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_probe() {
    ...
	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
		xdev->has_axistream_connected =
			of_property_read_bool(node, "xlnx,axistream-connected");
	}
    ...
	if (xdev->has_axistream_connected)
		xdev->common.desc_metadata_modes = DESC_METADATA_ENGINE;
    ...
}

So dmaengine_desc_get_metadata_ptr() looks like it always returns an
ERR_PTR for these descriptors. The error comes either from
desc_check_and_set_metadata_mode() or from the NULL metadata_ops check.

If so, every DMA_TRANS_NOERROR completion is dropped here with "Failed to
get RX metadata pointer". ndo_open still succeeds, so the interface comes
up with RX dead.

Wiring up the existing hook wouldn't fix this on its own.
xilinx_dma_get_metadata_ptr() casts to the AXIDMA segment type and returns
the app words, which start at BD offset 0x20:

	seg = list_first_entry(&desc->segments,
			       struct xilinx_axidma_tx_segment, node);
	return seg->hw.app;

In struct xilinx_aximcdma_desc_hw, status is at 0x18 and sideband_status
(TID/TDEST/TUSER) is at 0x1C. The "[0] = status, [1] = sideband" layout
assumed here doesn't match any get_ptr in the tree. metadata[1] would be
an app word, and the TUSER port_id filter would act on unrelated bits.

metadata[1] is also read without checking meta_len.

No patch in the series touches drivers/dma/. The same code is still there
at the end of the series, in "net: xilinx: tsn: deliver endpoint RX frames
to DSA user ports".

[ ... ]

> @@ -305,9 +523,19 @@ static int ep_init_dmaengine(struct xlnx_tsn_ep *ep)
>  		rx_allocated++;
>  	}
>  
> +	for (i = 0; i < ep->num_rx_queues; i++) {
> +		if (!ep_rx_refill(ep->rx_chans[i], false)) {
> +			dev_err(ep->dev, "RX channel %d: no descriptors armed\n",
> +				i);
> +			ret = -ENOMEM;
> +			goto err_free_chans;
> +		}
> +	}

[Severity: Low]
Can ep_rx_refill() return false here even though a descriptor was armed?

ep_rx_refill() decides success by testing ring_head !=
READ_ONCE(ring_tail) after dma_async_issue_pending(). ep_dma_rx_cb()
advances ring_tail outside rx_lock, before it calls ep_rx_refill() and
blocks on the lock:

	WRITE_ONCE(xchan->ring_tail, xchan->ring_tail + 1);

The callback runs in the provider tasklet, possibly on another CPU, and
spin_lock_bh() only blocks BH locally.

Suppose only one slot gets armed, because dev_alloc_skb() (GFP_ATOMIC) or
prep fails on the second slot. If that frame completes before the
head/tail test, head == tail and ep_rx_refill() returns false. ndo_open
then fails with -ENOMEM and "no descriptors armed", even though the frame
has already gone up through __netif_rx().

Teardown after that failure looks safe. closing is set before
ep_free_dma_chan() takes the lock and shuts down the timer.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-patches_v2_external-v3-0-e33d3960ca1e%40amd.com