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

[netdev-bot+sashiko@kernel.org]

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?

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 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? 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;
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-patches_v2_external-v3-0-e33d3960ca1e%40amd.com