Re: [PATCH net-next v3 5/8] net: xilinx: tsn: bring up the endpoint MCDMA channels

["Neeli, Srinivas" <srneeli@amd.com>]

Hi,

On 10/20/2026 5:28 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] ep_reset_dma_controller() resets the whole MCDMA core at probe
>    through one channel handle, but the driver does not own every channel…
> - [Low] In drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c, the kernel-
>    doc for struct skbuf_dma_descriptor calls @dma_address the "physical…
>
> --
>
> net: xilinx: tsn: bring up the endpoint MCDMA channels
>
> At probe, this patch requests every TX and RX MCDMA channel named in the
> device tree and holds them until remove. It also resets the MCDMA
> controller once, through the first TX channel. Each channel gets a ring of
> preallocated SKB descriptor slots in ndo_open, and the rings are torn down
> in ndo_stop.
>
>> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
>> index 2adee07f6f51c..ab3d6d8cb9440 100644
>> --- a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
>> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> [ ... ]
>
>> @@ -30,6 +35,47 @@
>>   #define TSN_MAX_VLAN_FRAME_SIZE		(ETH_DATA_LEN + VLAN_ETH_HLEN + \
>>   					 ETH_FCS_LEN)
>>   
>> +#define TX_BD_NUM_DEFAULT		64
>> +#define RX_BD_NUM_DEFAULT		128
>> +
>> +/**
>> + * struct skbuf_dma_descriptor - skb container for each in-flight DMA descriptor
>> + * @sgl: scatter-gather list backing the DMA mapping
>> + * @desc: dmaengine descriptor handle
>> + * @dma_address: physical address of the first sgl entry (RX path)
> [Severity: Low]
>
> This isn't a bug, but is "physical address" the right description for
> @dma_address?
Will address in next version.
> The field is a dma_addr_t. ep_free_dma_chan() passes it to
> dma_unmap_single() as a mapping handle, and in the RX data path patch
> ep_rx_submit_desc() fills it from sg_dma_address() after dma_map_single().
> If an IOMMU sits in front of the MCDMA, the value is an IOVA, not a CPU
> physical address.
>
> The same wording is still there at the end of the series. Would something
> like "DMA address of the first sgl entry" be more accurate?
>
>> + * @skb: SKB owning the buffer
>> + * @sg_len: number of valid entries in @sgl (TX path)
>> + */
> [ ... ]
>
>> +static int ep_reset_dma_controller(struct xlnx_tsn_ep *ep)
>> +{
>> +	struct xilinx_vdma_config cfg = { .reset = 1 };
>> +	int ret;
>> +
>> +	ret = xilinx_vdma_channel_set_config(ep->tx_dma[0], &cfg);
> [Severity: Medium]
>
> Can this reset stall channels on the same MCDMA instance that belong to
> another dmaengine client?
   The MCDMA instance is not shared with any other consumer in this design.
   The whole instance belongs to the endpoint, as the binding example shows.
   If a future IP revision provides per-channel reset, we will use it here
   so that only the endpoint's own channels are affected.
>
> The commit message says:
>
>    "An MCDMA soft reset resets the whole core, so any channel of the
>    instance reaches it."
>
> For MCDMA, every MM2S channel uses ctrl_offset 0, which is the common
> control register. The call chain is:
>
> xlnx_tsn_ep_probe()
>    ep_reset_dma_controller()
>      xilinx_vdma_channel_set_config()
>        xilinx_dma_chan_reset()
>          xilinx_dma_reset()
>
> drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_reset() {
> 	dma_ctrl_set(chan, XILINX_DMA_REG_DMACR, XILINX_DMA_DMACR_RESET);
> 	...
> 	chan->err = false;
> 	chan->idle = true;
> 	chan->desc_pendingcount = 0;
> 	chan->desc_submitcount = 0;
> 	...
> }
>
> The hardware reset hits the whole core, but software state is only reset
> for tx_dma[0].
>
> The endpoint only requests the tx_chanN/rx_chanN entries in its own
> dma-names. Nothing in the binding or the driver stops the other channels of
> the instance from going to another consumer. Suppose one of those channels
> is mid-transfer when the endpoint probes, for example after a probe
> deferral or a module load. It is still marked idle == false and has
> descriptors on active_list. Its next submit then goes through:
>
> drivers/dma/xilinx/xilinx_dma.c:xilinx_mcdma_start_transfer() {
> 	...
> 	if (chan->has_sg && list_empty(&chan->active_list))
> 		xilinx_write(chan, XILINX_MCDMA_CHAN_CDESC_OFFSET(chan->tdest),
> 			     head_desc->async_tx.phys);
> 	...
> 	if (chan->idle)
> 		xilinx_dma_start(chan);
> 	...
> }
>
> CDESC is not reprogrammed and xilinx_dma_start() is skipped. Doesn't that
> leave the engine halted, with no error reported to that client?
>
> I could not confirm that shared MCDMA designs exist in practice. The
> binding example gives the whole instance to the endpoint.
>
> Is the reset needed at all?
   Yes, but not for the DMA channels themselves. On this IP the endpoint
   logic is reset together with the MCDMA core, so this reset is what brings
   the conduit registers into a known state at probe. Without it the
   endpoint would start from whatever  previous state  driver left behind.

Thanks
Neeli Srinivas

> The provider already resets each channel in
> xilinx_dma_chan_probe(). The MCDMA path of
> xilinx_dma_alloc_chan_resources() only allocates BD memory and does not
> program the hardware. So the endpoint's own channels look clean already,
> and the only visible effect of this reset is on channels the endpoint does
> not own.
>
> Calling dmaengine_terminate_sync() from ep_free_dma_chan() on ndo_stop also
> resets the whole core, through the provider's xilinx_dma_terminate_all().
> Any MCDMA client that terminates a channel causes this, so that part comes
> from the provider and not from this patch.
>
>> +	if (ret < 0)
>> +		return dev_err_probe(ep->dev, ret,
>> +				     "failed to reset DMA controller\n");
>> +
>> +	return 0;
>> +}
> [ ... ]
>
>> @@ -286,9 +587,17 @@ static int xlnx_tsn_ep_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto err_free_ndev;
>>   
>> +	ret = ep_request_dma_chans(ep, rx_dma_chan_map);
>> +	if (ret)
>> +		goto err_free_ndev;
>> +
>> +	ret = ep_reset_dma_controller(ep);
>> +	if (ret)
>> +		goto err_release_chans;
>> +
> [ ... ]
>