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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] net: xilinx: tsn: RX stall from MCDMA IRQ coalescing in
  ep_rx_refill()
- [Medium] net: xilinx: tsn: permanent RX stall after MCDMA error
  interrupt
--

commit 8a8c3c55f7d1afb981a05d0249b531e8e8eb911c
Author: Srinivas Neeli <srinivas.neeli@amd.com>

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

This commit adds the endpoint RX data path, posting RX buffers to the MCDMA
channels, handling completion callbacks with TUSER port_id filtering, and
refilling the RX ring under a per-channel lock.

> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> index 52d2da04a5d88..f179062c7d148 100644
> --- a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
[ ... ]
> @@ -113,6 +136,181 @@ ep_get_desc(struct xlnx_tsn_ep_dma_chan *xchan, int idx)
[ ... ]
> +static bool ep_rx_refill(struct xlnx_tsn_ep_dma_chan *xchan, bool arm_timer)
> +{
> +	int avail, i;
> +
> +	scoped_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]
Can calling dma_async_issue_pending() once after submitting a batch of 2 to
24 descriptors stall RX under sparse traffic?

Each ep_rx_submit_desc() increments chan->desc_pendingcount in the MCDMA
driver, and dma_async_issue_pending() then programs the channel's hardware
interrupt coalescing threshold from that batch count:

drivers/dma/xilinx/xilinx_dma.c:xilinx_mcdma_start_transfer() {
    ...
	if (chan->desc_pendingcount <= XILINX_MCDMA_COALESCE_MAX) {
		reg &= ~XILINX_MCDMA_COALESCE_MASK;
		reg |= chan->desc_pendingcount <<
			XILINX_MCDMA_COALESCE_SHIFT;
	}

	reg |= XILINX_MCDMA_IRQ_ALL_MASK;
	dma_ctrl_write(chan, XILINX_MCDMA_CHAN_CR_OFFSET(chan->tdest), reg);
    ...
}

If ep_rx_refill() submits between 2 and 24 descriptors (for example when
multiple descriptors complete before refill runs, or when an allocation
fails partway through filling the ring), the hardware waits for that many
frames before raising XILINX_MCDMA_IRQ_IOC_MASK, and later batches larger
than 24 leave that threshold programmed.  Because xilinx_mcdma_irq_handler()
does not complete descriptors on XILINX_MCDMA_IRQ_DELAY_MASK, received
frames fewer than the coalescing threshold sit in the ring without a
completion callback until more traffic arrives.

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

[Severity: Medium]
Does an MCDMA channel error interrupt wedge the RX queue until the interface
is cycled down and up?

When xilinx_mcdma_irq_handler() sees XILINX_MCDMA_IRQ_ERR_MASK without a
completed descriptor (XILINX_DMA_BD_COMP_MASK), it sets chan->err = true and
leaves the active descriptors on chan->active_list rather than moving them
to chan->done_list:

drivers/dma/xilinx/xilinx_dma.c:xilinx_mcdma_irq_handler() {
    ...
	if (status & XILINX_MCDMA_IRQ_ERR_MASK) {
		...
		chan->err = true;
	}
    ...
	if (status & XILINX_MCDMA_IRQ_IOC_MASK) {
		spin_lock(&chan->lock);
		xilinx_dma_complete_descriptor(chan);
    ...
}

Because nothing lands on chan->done_list, ep_dma_rx_cb() is never called for
the in-flight descriptors, xchan->ring_head != xchan->ring_tail remains
true, and rx_refill_timer is not armed.  No further ep_rx_submit_desc() call
runs to trigger xilinx_dma_chan_reset() in xilinx_dma_tx_submit(), leaving
RX on that channel halted while the netdev stays up.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-patches_v2_external-v4-0-56bb26f042a4@amd.com?part=6