Re: [PATCH net-next v4 7/8] net: xilinx: tsn: add the endpoint TX data path

[sashiko-bot@kernel.org]

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@amd.com>

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@amd.com?part=7