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

[Jakub Kicinski <kuba@kernel.org>]

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;

[ ... ]

> +	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: