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

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

Critical: 0 · High: 2 · Medium: 3 · Low: 0

- [High] The new RX path cannot deliver a single frame on the hardware it
  targets.
- [High] ep_rx_submit_desc()
  (drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c:169-178) records the…
- [Medium] ep_dma_rx_cb() documents and indexes the dmaengine metadata
  window as "[0] = status, [1] = sideband (TID/TDEST/TUSER), [2..] =…
- [Medium] ep_rx_refill() holds the per-channel spinlock with softirqs
  disabled (guard(spinlock_bh)(&xchan->rx_lock), line 191) across a loop…
- [Medium] The commit message states "ndo_stop() sets a closing flag
  under the same per-channel lock the refill takes", but ep_stop() (line…

--

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

This adds the RX side of the Xilinx TSN endpoint: RX buffers are posted to
every RX channel at ndo_open and reaped from the dmaengine completion
callback, which decodes a TUSER port_id out of the descriptor metadata and
passes MAC1/MAC2 frames up with __netif_rx(). A per-channel rx_lock plus a
refill timer are added to serialise refill against teardown.

[Severity: Medium]
The commit message says:

  "Serialise the RX refill against teardown. The completion callback re-arms
   the ring, so ndo_stop() sets a closing flag under the same per-channel
   lock the refill takes, and ep_free_dma_chan() takes that lock as a barrier
   before it terminates the channel."

Is the "under the same per-channel lock" part accurate?  ep_stop() looks
like this:

	netif_tx_disable(ndev);
	WRITE_ONCE(ep->closing, true);
	ep_exit_dmaengine(ep);

The store is not done under any rx_lock; the per-channel lock only shows up
later in ep_free_dma_chan() as a bare lock/unlock barrier, after the flag
has already been published.  Since this claim is the stated reason a single
dmaengine_terminate_sync() suffices, could the wording be adjusted to
describe what the code actually does?

[ ... ]

[ ... ]

[ ... ]

+	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 0;

[Severity: High]
Should ep_rx_submit_desc() check the cookie returned by dmaengine_submit()?
ring_head is advanced and the slot is marked armed before the submit, and
the return value is discarded, so 0 is returned even when the descriptor was
never queued.  The provider can fail:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_tx_submit() {
	...
	if (chan->err) {
		err = xilinx_dma_chan_reset(chan);
		if (err < 0)
			return err;
	}
	...
}

If that happens, three things seem to follow.

The skb and its DMA_FROM_DEVICE mapping stay pinned in the slot with no
completion ever arriving for it.

ep_dma_rx_cb() consumes slots strictly in ring_tail order with no
correlation to the completing descriptor:

	skbuf_dma = ep_get_desc(xchan, xchan->ring_tail & (xchan->ring_size - 1));
	WRITE_ONCE(xchan->ring_tail, xchan->ring_tail + 1);

so every later completion is attributed to the wrong slot.  Can this hand a
buffer the engine never wrote, i.e. freshly allocated dev_alloc_skb() memory
with another descriptor's length applied by skb_put(), to __netif_rx()?

And since ep_rx_refill() judges success only by ring_head != ring_tail, the
falsely advanced head makes refill report success, no retry timer is armed,
and the new ep_init_dmaengine() arming check passes with nothing queued.

The TX path in this series does check dma_submit_error() and rolls the slot
back; would the same handling fit here?

+
+err_unmap_skb:
+	dma_unmap_single(xchan->dma_dev, addr, ep->max_frm_size, DMA_FROM_DEVICE);
+err_free_skb:
+	dev_kfree_skb(skb);
+	return -ENOMEM;
+}
+
+static bool ep_rx_refill(struct xlnx_tsn_ep_dma_chan *xchan, bool arm_timer)
+{
+	int avail, i;
+
+	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: Medium]
Does rx_lock need to cover the whole submit loop?  The guard is taken at
function scope, so with an empty ring avail is ring_size - 1 (127 with
RX_BD_NUM_DEFAULT), and each iteration of ep_rx_submit_desc() does
dev_alloc_skb(), dma_map_single(), dmaengine_prep_slave_sg() and
dmaengine_submit() with softirqs disabled.

Two consequences worth considering.  On the ndo_open path
(ep_init_dmaengine -> ep_rx_refill(chan, false)) this is sleepable process
context, yet every allocation is forced into the GFP_ATOMIC reserve and can
fail ndo_open with -ENOMEM.  A concurrent RX completion on another CPU spins
on rx_lock for the whole burst.

Would moving the skb allocation and mapping outside rx_lock, or dropping and
retaking it per descriptor, work here?

+
+	if (xchan->ring_head != READ_ONCE(xchan->ring_tail))
+		return true;
+
+	if (arm_timer)
+		mod_timer(&xchan->rx_refill_timer, jiffies + EP_RX_REFILL_RETRY);
+
+	return false;
+}

[ ... ]

+	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]
Can this drop every received frame on the AXI MCDMA channels the endpoint
binding uses?  dmaengine_desc_get_metadata_ptr() goes through:

drivers/dma/dmaengine.c:
	ret = desc_check_and_set_metadata_mode(desc, DESC_METADATA_ENGINE);
	if (ret)
		return ERR_PTR(ret);

and xilinx_dma.c only advertises DESC_METADATA_ENGINE for
XDMA_TYPE_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;
	...
}

xilinx_mcdma_prep_slave_sg() also never assigns
desc->async_tx.metadata_ops, unlike the two AXIDMA prep paths.  With
desc_metadata_modes unset, desc_check_and_set_metadata_mode() returns
-ENOTSUPP for every MCDMA completion, so IS_ERR_OR_NULL(metadata) is taken
and the skb is freed with rx_dropped/rx_errors incremented, making the
port_id filtering and __netif_rx() below unreachable.

Would a one-time capability check at open (for example
dmaengine_is_metadata_mode_supported()) be better than a ratelimited
per-packet warning here, and does the MCDMA side need metadata support added
first?

+
+	/* MCDMA metadata: [0] = status, [1] = sideband (TID/TDEST/TUSER), [2..] = app */
+	tuser = metadata[1] & TSN_TUSER_MASK;
+	rx_len = ep->max_frm_size - result->residue;

[Severity: Medium]
Is metadata[1] really the sideband word?  The in-tree provider hands out the
APP window, not the status word:

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 the MCDMA hardware descriptor the status and sideband words sit
before app[]:

struct xilinx_aximcdma_desc_hw {
	...
	u32 control;		/* @0x14 */
	u32 status;		/* @0x18 */
	u32 sideband_status;	/* @0x1C */
	u32 app[XILINX_DMA_NUM_APP_WORDS]; /* @0x20 */
};

So metadata[0] is APP0 and metadata[1] is APP1, which would make the decoded
port_id below unrelated to TUSER: a zero APP1 gives port_id 0
(TSN_TUSER_PORT_EP) and every frame is dropped as an unexpected port_id,
while non-zero garbage can accept a frame and later select the wrong
ep->port_md[port_id] for DSA delivery.  The peer consumer of this same ABI,
axienet_dma_rx_cb(), indexes app_metadata[XILINX_DMA_NUM_APP_WORDS - 1],
which matches the APP-window interpretation.

Also, meta_len and meta_max_len are fetched and then never used.  Should
this check meta_len >= 2 * sizeof(u32) before dereferencing metadata[1]?

+
+	if (rx_len > ep->max_frm_size || rx_len < ETH_HLEN) {

[ ... ]

+	port_id = FIELD_GET(TSN_TUSER_PORT_ID_MASK, tuser);
+	if (port_id != TSN_TUSER_PORT_MAC1 && port_id != TSN_TUSER_PORT_MAC2) {
+		if (net_ratelimit())
+			dev_dbg(ep->dev, "RX dropping unexpected TUSER port_id=%u\n",
+				port_id);
+
+		dev_kfree_skb_any(skb);
+		DEV_STATS_INC(ndev, rx_dropped);
+		goto submit_new;
+	}

[ ... ]

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-patches_v2_external-v2-0-3a40babaff4c%40amd.com