Re: [PATCH net-next v4 7/8] net: xilinx: tsn: add the endpoint TX data path
From: sashiko-bot
Date: Sat Nov 14 2026 - 07:05:45 EST
Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] net: xilinx: tsn: ep_stop() terminates TX DMA before
netif_tx_disable()
- [Medium] net: xilinx: tsn: missing TX DMA error recovery in
ep_start_xmit()
- [Medium] net: xilinx: tsn: ep_start_xmit() TX flow control ignores DMA
segment limit
--
commit a8a4f14fcbe22e1b33cd5c8b571aab05e71576de
Author: Srinivas Neeli <srinivas.neeli@xxxxxxx>
net: xilinx: tsn: add the endpoint TX data path
This commit adds the transmit path and TX DMA completion handling for the
Xilinx TSN endpoint driver.
> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> index f179062c7d148..6e2721a74f287 100644
> --- a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
[ ... ]
> @@ -311,8 +316,151 @@ static void ep_dma_rx_cb(void *data, const struct dmaengine_result *result)
> ep_rx_refill(xchan, true);
> }
>
> +static void ep_dma_tx_cb(void *data, const struct dmaengine_result *result)
> +{
> + struct xlnx_tsn_ep_dma_chan *xchan = data;
> + struct skbuf_dma_descriptor *skbuf_dma;
> + struct netdev_queue *txq;
> + struct net_device *ndev;
> + struct scatterlist *sgl;
> + struct sk_buff *skb;
> + int sg_len;
> + int len;
> +
> + scoped_guard(spinlock_bh, &xchan->tx_lock) {
> + skbuf_dma = ep_get_desc(xchan,
> + xchan->ring_tail & (xchan->ring_size - 1));
> + if (!skbuf_dma || !skbuf_dma->skb)
> + return;
> +
> + skb = skbuf_dma->skb;
> + sgl = skbuf_dma->sgl;
> + sg_len = skbuf_dma->sg_len;
> +
> + dma_unmap_sg(xchan->dma_dev, sgl, sg_len, DMA_TO_DEVICE);
> +
> + skbuf_dma->skb = NULL;
> + WRITE_ONCE(xchan->ring_tail, xchan->ring_tail + 1);
> + }
> +
> + ndev = skb->dev;
> + txq = netdev_get_tx_queue(ndev, skb_get_queue_mapping(skb));
> + len = skb->len;
> +
> + if (unlikely(result->result != DMA_TRANS_NOERROR)) {
> + DEV_STATS_INC(ndev, tx_errors);
> + dev_kfree_skb_any(skb);
> + } else {
> + DEV_STATS_INC(ndev, tx_packets);
> + DEV_STATS_ADD(ndev, tx_bytes, len);
> + dev_consume_skb_any(skb);
> + }
> +
> + __netif_txq_completed_wake(txq, 1, len,
> + CIRC_SPACE(READ_ONCE(xchan->ring_head),
> + READ_ONCE(xchan->ring_tail),
> + xchan->ring_size), 2,
> + READ_ONCE(xchan->ep->closing));
> +}
[Severity: Medium]
Can an AXI MCDMA channel error leave a stopped TX queue permanently
wedged?
When xilinx_mcdma_irq_handler() handles XILINX_MCDMA_IRQ_ERR_MASK, it sets
chan->err = true, and xilinx_dma_complete_descriptor() only moves
descriptors to done_list when XILINX_DMA_BD_COMP_MASK is set. In-flight
descriptors that do not complete never invoke ep_dma_tx_cb(), so
xchan->ring_tail does not advance and __netif_txq_completed_wake() is
never called. If the subqueue was stopped by netif_txq_maybe_stop() or
BQL, ep_start_xmit() will not run again to reset the channel via
xilinx_dma_tx_submit(), and ep_netdev_ops defines no ndo_tx_timeout
callback to recover the channel and drain xchan->skb_ring.
> +
> +static netdev_tx_t ep_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> + struct dma_async_tx_descriptor *dma_tx_desc;
> + struct xlnx_tsn_ep *ep = netdev_priv(ndev);
> + struct skbuf_dma_descriptor *skbuf_dma;
> + int queue = skb_get_queue_mapping(skb);
> + struct xlnx_tsn_ep_dma_chan *xchan;
> + struct netdev_queue *txq;
> + int sg_len, nents, ret;
> + dma_cookie_t cookie;
> +
> + if (unlikely(queue >= ep->num_tx_queues)) {
> + if (net_ratelimit())
> + netdev_warn(ndev, "Invalid TX queue %d (max %u)\n",
> + queue, ep->num_tx_queues);
> + goto err_drop_skb;
> + }
> +
> + if (ep->tx_dma_chan_map[queue] == TSN_DMA_CH_INVALID) {
> + if (net_ratelimit())
> + netdev_warn(ndev, "Logical TX queue %d has invalid DMA mapping\n",
> + queue);
> + goto err_drop_skb;
> + }
> +
> + xchan = ep->tx_chans[queue];
> +
> + sg_len = skb_shinfo(skb)->nr_frags + 1;
> + txq = netdev_get_tx_queue(ndev, queue);
> +
> + spin_lock_bh(&xchan->tx_lock);
> + if (CIRC_SPACE(xchan->ring_head, READ_ONCE(xchan->ring_tail),
> + xchan->ring_size) <= 1) {
> + netif_txq_try_stop(txq,
> + CIRC_SPACE(xchan->ring_head,
> + READ_ONCE(xchan->ring_tail),
> + xchan->ring_size),
> + 2);
> + spin_unlock_bh(&xchan->tx_lock);
> + if (net_ratelimit())
> + netdev_warn(ndev, "TSN TX ring full\n");
> +
> + return NETDEV_TX_BUSY;
> + }
> +
> + skbuf_dma = ep_get_desc(xchan, xchan->ring_head & (xchan->ring_size - 1));
> + if (!skbuf_dma) {
> + spin_unlock_bh(&xchan->tx_lock);
> + goto err_drop_skb;
> + }
> + spin_unlock_bh(&xchan->tx_lock);
> +
> + sg_init_table(skbuf_dma->sgl, sg_len);
> + ret = skb_to_sgvec(skb, skbuf_dma->sgl, 0, skb->len);
> + if (ret < 0)
> + goto err_drop_skb;
> + sg_len = ret;
> +
> + nents = dma_map_sg(xchan->dma_dev, skbuf_dma->sgl, sg_len, DMA_TO_DEVICE);
> + if (!nents)
> + goto err_drop_skb;
> +
> + dma_tx_desc = dmaengine_prep_slave_sg(xchan->chan, skbuf_dma->sgl,
> + nents, DMA_MEM_TO_DEV,
> + DMA_PREP_INTERRUPT);
> + if (!dma_tx_desc)
> + goto err_unmap_sg;
[Severity: Medium]
Can dmaengine_prep_slave_sg() exhaust the MCDMA channel's segment pool and
drop frames while the software ring still has space?
The driver advertises NETIF_F_SG and allows up to MAX_SKB_FRAGS + 1 (18)
scatterlist entries per frame, while queue flow control only checks
CIRC_SPACE() on the 64-frame software ring (up to 62 frames in flight).
In drivers/dma/xilinx/xilinx_dma.c, each MCDMA channel has a pool of
XILINX_DMA_NUM_DESCS (512) segments in chan->free_seg_list. When
multi-fragment frames in flight consume all 512 segments before 62 frames
are queued, dmaengine_prep_slave_sg() returns NULL and ep_start_xmit()
drops the packet instead of stopping the TX queue.
[ ... ]
> @@ -343,10 +491,16 @@ static int ep_open(struct net_device *ndev)
> static int ep_stop(struct net_device *ndev)
> {
> struct xlnx_tsn_ep *ep = netdev_priv(ndev);
> + unsigned int i;
>
> - netif_tx_disable(ndev);
> WRITE_ONCE(ep->closing, true);
> + for (i = 0; i < ep->num_tx_queues; i++)
> + dmaengine_terminate_sync(ep->tx_chans[i]->chan);
> +
> + netif_tx_disable(ndev);
> ep_exit_dmaengine(ep);
> + for (i = 0; i < ndev->num_tx_queues; i++)
> + netdev_tx_reset_subqueue(ndev, i);
>
> return 0;
> }
[Severity: High]
Can ep_start_xmit() race with dmaengine_terminate_sync() when ep_stop()
terminates the TX channels before calling netif_tx_disable()?
Until netif_tx_disable() acquires HARD_TX_LOCK and sets
__QUEUE_STATE_DRV_XOFF, transmit paths that bypass qdisc deactivation
(such as AF_PACKET with PACKET_QDISC_BYPASS via __dev_direct_xmit(), or a
queue using noqueue) can still enter ep_start_xmit(), which does not check
ep->closing:
CPU0 CPU1
ep_stop() __dev_direct_xmit()
WRITE_ONCE(ep->closing, true) ep_start_xmit()
dmaengine_terminate_sync()
xilinx_dma_terminate_all()
xilinx_dma_chan_reset()
chan->terminating = true
xilinx_dma_free_descriptors()
dmaengine_prep_slave_sg()
dmaengine_submit()
xilinx_dma_tx_submit()
chan->terminating = false
WRITE_ONCE(xchan->ring_head, ...)
dma_async_issue_pending()
netif_tx_disable()
Calling dma_async_issue_pending() concurrently with
dmaengine_terminate_sync() violates the dmaengine_synchronize() API
contract and races with the unlocked channel reset and descriptor freeing
in xilinx_dma_terminate_all(). In addition, because
xilinx_dma_terminate_all() discards earlier in-flight descriptors without
running ep_dma_tx_cb(), their SKBs remain at xchan->ring_tail; if the
newly issued transfer completes before ep_exit_dmaengine() frees the
channel, ep_dma_tx_cb() pops the stale slot at ring_tail, unmapping and
freeing the wrong skb and passing its length to
__netif_txq_completed_wake().
Calling netif_tx_disable(ndev) after WRITE_ONCE(ep->closing, true) and
before the dmaengine_terminate_sync() loop would serialize against
ep_start_xmit() while still preventing any concurrent ep_dma_tx_cb() from
waking a stopped queue.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-patches_v2_external-v4-0-56bb26f042a4@xxxxxxx?part=7