Re: [PATCH 06/20] net: xilinx: tsn: add the endpoint RX data path

["Neeli, Srinivas" <srneeli@amd.com>]

Hi Jakub,

On 8/9/2026 1:18 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: xilinx: tsn: add the endpoint RX data path
>
> This adds the RX side of the TSN endpoint: RX buffers are posted to every
> RX channel at open time and reaped from the dmaengine completion callback,
> which decodes a TUSER port id out of the descriptor metadata and passes
> MAC1/MAC2 frames up the stack.  The completion callback also refills the
> slot it just consumed, and the channel teardown path grows an extra
> terminate/synchronize/terminate sequence.
>
>> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
>> index fa21d59ade16..207ca3242a12 100644
>> --- a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
>> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> [ ... ]
>
>> @@ -42,6 +43,17 @@
>>   #define TX_BD_NUM_DEFAULT		64
>>   #define RX_BD_NUM_DEFAULT		128
>>   
>> +/*
>> + * The DMA descriptor sideband status word packs TID/TDEST/TUSER together;
>> + * TUSER occupies the low byte, TID/TDEST sit in the upper bits.
>> + */
>> +#define TSN_TUSER_MASK			GENMASK(7, 0)
>> +/* TUSER Input Port ID field (bits [5:4] of the TUSER field) */
>> +#define TSN_TUSER_PORT_ID_MASK		GENMASK(5, 4)
>> +#define TSN_TUSER_PORT_EP		0x0
>> +#define TSN_TUSER_PORT_MAC1		0x1
>> +#define TSN_TUSER_PORT_MAC2		0x2
>> +
>>   /**
>>    * struct skbuf_dma_descriptor - skb container for each in-flight DMA descriptor
>>    * @sgl: scatter-gather list backing the DMA mapping
>> @@ -115,6 +127,147 @@ ep_get_desc(struct xlnx_tsn_ep_dma_chan *xchan, int idx)
>>   	return xchan->skb_ring[idx];
>>   }
>>   
>> +static void ep_dma_rx_cb(void *data, const struct dmaengine_result *result);
>> +
>> +static void ep_rx_submit_desc(struct xlnx_tsn_ep_dma_chan *xchan)
>> +{
>> +	struct dma_async_tx_descriptor *dma_rx_desc;
>> +	struct skbuf_dma_descriptor *skbuf_dma;
>> +	struct xlnx_tsn_ep *ep = xchan->ep;
>> +	struct sk_buff *skb;
>> +	dma_addr_t addr;
>> +
>> +	skbuf_dma = ep_get_desc(xchan, xchan->ring_head & (RX_BD_NUM_DEFAULT - 1));
> [Severity: Low]
> This isn't a bug today, but should this use xchan->ring_size instead of
> RX_BD_NUM_DEFAULT?
>
> struct xlnx_tsn_ep_dma_chan.ring_size is documented as the number of slots
> in skb_ring, is set from the caller-supplied size in ep_alloc_dma_chan(),
> and ep_free_dma_chan() masks with it:
>
>      skbuf_dma = chan->skb_ring[chan->ring_tail & (chan->ring_size - 1)];
>
> The new RX code hardcodes RX_BD_NUM_DEFAULT for the ring mask here, for the
> mask and CIRC_SPACE() in ep_dma_rx_cb(), and for the prefill count in
> ep_init_dmaengine().  The two sides of the same ring now disagree about
> where the size comes from, so a per-channel or ethtool-configurable RX ring
> size would index out of range on one of them.
>
>> +	if (!skbuf_dma)
>> +		return;
>> +
>> +	skb = dev_alloc_skb(ep->max_frm_size);
>> +	if (!skb)
>> +		return;
>> +
>> +	sg_init_table(skbuf_dma->sgl, 1);
>> +	addr = dma_map_single(xchan->dma_dev, skb->data, ep->max_frm_size,
>> +			      DMA_FROM_DEVICE);
>> +	if (unlikely(dma_mapping_error(xchan->dma_dev, addr))) {
>> +		if (net_ratelimit())
>> +			dev_warn(ep->dev, "DMA mapping error on RX submit\n");
>> +
>> +		goto err_free_skb;
>> +	}
>> +	sg_dma_address(skbuf_dma->sgl) = addr;
>> +	sg_dma_len(skbuf_dma->sgl) = ep->max_frm_size;
>> +	dma_rx_desc = dmaengine_prep_slave_sg(xchan->chan, skbuf_dma->sgl,
>> +					      1, DMA_DEV_TO_MEM,
>> +					      DMA_PREP_INTERRUPT);
>> +	if (!dma_rx_desc)
>> +		goto err_unmap_skb;
>> +
>> +	skbuf_dma->skb = skb;
>> +	skbuf_dma->dma_address = sg_dma_address(skbuf_dma->sgl);
>> +	skbuf_dma->desc = dma_rx_desc;
>> +	dma_rx_desc->callback_param = xchan;
>> +	dma_rx_desc->callback_result = ep_dma_rx_cb;
>> +	xchan->ring_head++;
>> +	dmaengine_submit(dma_rx_desc);
>> +
>> +	return;
>> +
>> +err_unmap_skb:
>> +	dma_unmap_single(xchan->dma_dev, addr, ep->max_frm_size, DMA_FROM_DEVICE);
>> +err_free_skb:
>> +	dev_kfree_skb(skb);
>> +}
>> +
>> +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;
>> +	}
> [Severity: High]
> Does this path drop every received frame on the configuration described by
> the binding?
>
> The binding added by this series wires the endpoint to an AXI MCDMA:
>
>      dmas = <&axi_mcdma_0 0>, ... <&axi_mcdma_0 23>;
>
> In drivers/dma/xilinx/xilinx_dma.c, the "xlnx,axistream-connected"
> property is only evaluated for XDMA_TYPE_AXIDMA, desc_metadata_modes is
> only set to DESC_METADATA_ENGINE under that flag, and async_tx.metadata_ops
> is only attached in the AXIDMA prep paths.  xilinx_mcdma_prep_slave_sg(),
> which is installed as device_prep_slave_sg for XDMA_TYPE_AXIMCDMA, never
> sets metadata_ops.
>
> So for an MCDMA instance:
>
>    ep_dma_rx_cb()
>      dmaengine_desc_get_metadata_ptr()
>        desc_check_and_set_metadata_mode()   /* desc_metadata_modes == 0 */
>          -> -ENOTSUPP
>      IS_ERR_OR_NULL(metadata) -> dev_kfree_skb_any() + rx_dropped/rx_errors
>
> Every frame would be freed and counted as an RX error, with only a
> net_ratelimit() warning to explain it.  Should ep_init_dmaengine() check
> dmaengine_is_metadata_mode_supported() while arming the RX channels and
> fail the open, rather than letting ndo_open succeed with a dead RX path?
>
>> +
>> +	/* MCDMA metadata: [0] = status, [1] = sideband (TID/TDEST/TUSER), [2..] = app */
>> +	tuser = metadata[1] & TSN_TUSER_MASK;
> [Severity: Medium]
> Is this metadata layout comment correct, and is metadata[1] the sideband
> word?
>
> xilinx_dma_get_metadata_ptr() returns the app array, not the start of the
> descriptor status words:
>
> drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_get_metadata_ptr() {
>      ...
> 	*max_len = *payload_len = sizeof(u32) * XILINX_DMA_NUM_APP_WORDS;
> 	seg = list_first_entry(&desc->segments,
> 			       struct xilinx_axidma_tx_segment, node);
> 	return seg->hw.app;
> }
>
> And in struct xilinx_aximcdma_desc_hw:
>
> 	u32 status;		/* @0x18 */
> 	u32 sideband_status;	/* @0x1C */
> 	u32 app[XILINX_DMA_NUM_APP_WORDS]; /* @0x20 */
>
> That makes metadata[0] == app[0] and metadata[1] == app[1]; status and
> sideband_status sit before the returned pointer and are not reachable
> through this API.  The existing consumer of the same API agrees that index
> N maps to app[N]:
>
> drivers/net/ethernet/xilinx/xilinx_axienet_main.c:axienet_dma_rx_cb() {
>      ...
> 	rx_len = (app_metadata[LEN_APP] & 0xFFFF);   /* #define LEN_APP 4 */
> }
>
> With the base off by two words, tuser and port_id are decoded from an
> unrelated AXI4-Stream application word, so frames are either all dropped as
> "unexpected TUSER port_id" or, once the later patch adds
>
> 	skb_dst_set_noref(skb, &ep->port_md[port_id]->dst);
>
> attributed to the wrong DSA user port.  Could either the index or the
> comment be corrected so the claimed layout is verifiable?
>
>> +	rx_len = ep->max_frm_size - result->residue;
   These review not on latest dmaengine code. Recently we send few patch 
series to mainline and those are got reviewed by tag.
   once we apply those on top of latest xilinx_dma.c, these two comments 
will eliminate.
https://lore.kernel.org/all/20260713072146.45269-1-srinivas.neeli@amd.com/


I will address remaining  comments in next version.

Thanks
Srinivas Neeli

> [ ... ]
>
>> +	skb_put(skb, rx_len);
>> +	skb->dev = ndev;
>> +	skb->protocol = eth_type_trans(skb, ndev);
>> +	skb->ip_summed = CHECKSUM_NONE;
>> +	__netif_rx(skb);
>> +
>> +	DEV_STATS_INC(ndev, rx_packets);
>> +	DEV_STATS_ADD(ndev, rx_bytes, rx_len);
> [Severity: Low]
> This isn't a bug, but could the changelog say why the RX path is not NAPI
> based?  Frames are pushed with __netif_rx() straight from the dmaengine
> completion callback, so there is no poll budget and no GRO, and every frame
> goes through the per-CPU backlog where overflow is accounted to the core
> drop counters rather than the driver.  The per-packet statistics also use
> the legacy dev->stats atomics (DEV_STATS_INC/DEV_STATS_ADD) in the hot
> path.
>
>> +
>> +submit_new:
>> +	if (READ_ONCE(ep->closing))
>> +		return;
>> +
>> +	avail = CIRC_SPACE(xchan->ring_head, xchan->ring_tail,
>> +			   RX_BD_NUM_DEFAULT);
>> +	for (i = 0; i < avail; i++)
>> +		ep_rx_submit_desc(xchan);
>> +	dma_async_issue_pending(xchan->chan);
>> +}
> [Severity: Medium]
> Can RX stall permanently here if the refills fail?
>
> ep_rx_submit_desc() returns silently on dev_alloc_skb() returning NULL, on
> dma_map_single() failure and on dmaengine_prep_slave_sg() returning NULL.
> None of those advance ring_head, bump a counter, or tell the caller
> anything.
>
> ep_dma_rx_cb() has already done xchan->ring_tail++ unconditionally, so the
> number of outstanding descriptors (ring_head - ring_tail) drops by one for
> every failed refill.  The loop above then performs avail no-ops.
>
> The only callers of ep_rx_submit_desc() are ep_init_dmaengine() and this
> callback; there is no worker, timer or poll routine, as the commit message
> notes:
>
>      Refill each slot from the completion callback unless the interface is
>      closing, which keeps the ring full without a separate refill worker.
>
> So once the outstanding count reaches zero the DMA engine raises no further
> completion, this callback is never entered again, and RX is dead until the
> interface is downed and re-opened.  dev_alloc_skb() here runs in the
> dmaengine tasklet with GFP_ATOMIC, and a persistent mapping failure (IOMMU
> or swiotlb space exhaustion) makes all attempts fail deterministically.
> Would a bounded retry, a work item, or an error return from the arming path
> be preferable?  This still reads the same way at the end of the series.
>
>> +
>>   static netdev_tx_t ep_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>>   {
>>   	dev_kfree_skb(skb);
>> @@ -229,8 +382,18 @@ static void ep_free_dma_chan(struct xlnx_tsn_ep_dma_chan *chan)
>>   	if (!chan)
>>   		return;
>>   
>> -	if (chan->chan)
>> +	if (chan->chan) {
>> +		/* Drain the in-flight completion callback, then synchronize in
>> +		 * case that callback re-armed a descriptor before it observed
>> +		 * ep->closing. xilinx_dma_tx_submit() clears chan->terminating
>> +		 * on every submit, so a single terminate does not cover a
>> +		 * re-arm. The final terminate is a barrier before the channel
>> +		 * memory is freed.
>> +		 */
>>   		dmaengine_terminate_sync(chan->chan);
>> +		dmaengine_synchronize(chan->chan);
>> +		dmaengine_terminate_sync(chan->chan);
>> +	}
> [Severity: High]
> Does this sequence actually close the window, or does it only clean up
> after the damage?
>
> Nothing serializes the re-arm in ep_dma_rx_cb() against the terminate here.
> The check-then-act on ep->closing gives no exclusion:
>
>    tasklet: xilinx_dma_chan_desc_cleanup()
> 	list_for_each_entry_safe(desc, next, &chan->done_list, node) {
> 		...
> 		spin_unlock_irqrestore(&chan->lock, flags);
> 		dmaengine_desc_get_callback_invoke(&desc->async_tx, &result);
> 		spin_lock_irqsave(&chan->lock, flags);
> 		...
> 		xilinx_dma_free_tx_descriptor(chan, desc);
> 		if (chan->terminating)
> 			break;
> 	}
>
>    callback: READ_ONCE(ep->closing) == false
>
>    ip link set down: ep_stop() -> ep_exit_dmaengine() -> ep_free_dma_chan()
> 	  dmaengine_terminate_sync() -> xilinx_dma_terminate_all()
> 		  chan->terminating = true;
> 		  xilinx_dma_free_descriptors();  /* kfree()s done_list
> 						     entries, including the
> 						     iterator's `next` */
>
>    callback: ep_rx_submit_desc() -> dmaengine_submit()
> 	  xilinx_dma_tx_submit()
> 		  chan->terminating = false;
>
>    tasklet resumes: `if (chan->terminating) break;` is now false, so the
>    loop does desc = next and dereferences freed memory.
>
> That looks like a use-after-free plus list_del() on freed memory inside the
> DMA engine.  The comment above shows the cleared terminating flag is known
> about, but the terminate/synchronize/terminate runs after the corrupting
> iteration.  Would a real quiesce (for example refusing the re-arm under the
> same lock that the teardown path takes, or fixing the flag handling in
> xilinx_dma_tx_submit()) be the right shape here?  This also still reads the
> same way at the end of the series.
>
> [Severity: Medium]
> The commit message describes only the RX data path and says nothing about
> changing teardown.  Could it mention that ep_free_dma_chan(), which is
> shared by TX and RX channels, now issues terminate/synchronize/terminate
> instead of a single dmaengine_terminate_sync(), and that the reason is an
> internal behaviour of one dmaengine provider (xilinx_dma_tx_submit()
> clearing chan->terminating)?  As written, the open-coded triple terminate
> becomes silently dead code if that provider is ever fixed, and nobody
> touching drivers/dma/xilinx/xilinx_dma.c has a pointer to the dependency.
>
>>   
>>   	if (chan->is_tx) {
>>   		while (chan->ring_tail != chan->ring_head) {
> [ ... ]
>
>> @@ -329,6 +492,12 @@ static int ep_init_dmaengine(struct xlnx_tsn_ep *ep)
>>   		rx_allocated++;
>>   	}
>>   
>> +	for (i = 0; i < ep->num_rx_queues; i++) {
>> +		for (j = 0; j < RX_BD_NUM_DEFAULT - 1; j++)
>> +			ep_rx_submit_desc(ep->rx_chans[i]);
>> +		dma_async_issue_pending(ep->rx_chans[i]->chan);
>> +	}
>> +
>>   	return 0;
> [Severity: Medium]
> Related to the refill question above: every failure inside
> ep_rx_submit_desc() is discarded here and this returns 0, so ep_open() can
> report success with an under-filled or completely empty RX ring.  Should
> this loop count successful submissions and fail the open when none (or too
> few) were armed?
>
>>   
>>   err_free_chans: