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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The RX path permanently stalls if memory allocation or DMA mapping fails, as there is no mechanism to recover an empty RX ring.
--

--- Patch [6]: [PATCH 06/20] net: xilinx: tsn: add the endpoint RX data path ---
commit 913acc776e2acc162774e82ae72c55335c3d4e98
Author: Srinivas Neeli <[email protected]>

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

This commit adds the endpoint RX data path for the Xilinx TSN driver. It
implements RX buffer allocation, submission to the DMA engine, and handling
of received frames via a completion callback.

> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> index fa21d59ade165..207ca3242a123 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 @@
> +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));
> +	if (!skbuf_dma)
> +		return;
> +
> +	skb = dev_alloc_skb(ep->max_frm_size);
> +	if (!skb)
> +		return;

[Severity: High]
Does this silently drop the descriptor if memory allocation fails?

Since xchan->ring_head isn't advanced and the descriptor isn't re-armed,
transient memory pressure will permanently reduce the number of in-flight
descriptors. A similar silent drop occurs if dma_map_single() fails just
below this block.

[ ... ]

> +static void ep_dma_rx_cb(void *data, const struct dmaengine_result *result)
> +{

[ ... ]

> +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: High]
If the ring completely drains due to allocation failures in
ep_rx_submit_desc(), will it ever recover?

Because refill is strictly driven by the hardware RX completion interrupt
here in ep_dma_rx_cb(), an empty ring will never generate another interrupt,
leading to a permanent stall of the RX path. Is there a fallback mechanism
like NAPI polling, a timer, or a workqueue to recover the ring when memory
becomes available again?

[ ... ]

> @@ -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;
>  
>  err_free_chans:

[Severity: High]
Does this allow the interface to open in a wedged state?

If ep_rx_submit_desc() fails during initialization due to memory allocation 
or DMA mapping errors, the failures are silently ignored, leaving the RX 
ring partially or fully empty on startup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6