Re: [PATCH RFC 11/13] dmaengine: sdxi: Add DMA engine provider

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 05 Sep 2025 13:48:34 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Add support for memcpy and interrupt capabilities. Register one
> channel per SDXI function discovered for now.
> 
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>
A few superficial comments inline.

Good to see support for this standard device btw.

Thanks,

Jonathan

> ---
>  drivers/dma/sdxi/device.c |   4 +
>  drivers/dma/sdxi/dma.c    | 409 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/dma/sdxi/dma.h    |  12 ++
>  3 files changed, 425 insertions(+)

> diff --git a/drivers/dma/sdxi/dma.c b/drivers/dma/sdxi/dma.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ad8515deba53898b2b4ea0d38c40042b566abe1f
> --- /dev/null
> +++ b/drivers/dma/sdxi/dma.c

> +static int sdxi_dma_start_desc(struct sdxi_dma_desc *dma_desc)
> +{
> +	struct sdxi_dev *sdxi;
> +	struct sdxi_cmd *sdxi_cmd;
> +	struct sdxi_cxt *cxt;
> +	struct sdxi_desc desc;
> +	struct sdxi_copy copy;
> +	struct sdxi_cst_blk *cst_blk;
> +	dma_addr_t cst_blk_dma;
> +	int err;
> +
> +	sdxi_cmd = &dma_desc->sdxi_cmd;
> +	sdxi = sdxi_cmd->cxt->sdxi;
> +
> +	cxt = dma_desc->cxt;
Probably makes sense to combine with the declarations above.

> +
> +	if (sdxi_cmd->len > MAX_DMA_COPY_BYTES)
> +		return -EINVAL;
> +
> +	copy = (typeof(copy)) {
> +		.src = sdxi_cmd->src_addr,
> +		.dst = sdxi_cmd->dst_addr,
> +		.src_akey = 0,
> +		.dst_akey = 0,
> +		.len = sdxi_cmd->len,
> +	};
> +
> +	err = sdxi_encode_copy(&desc, &copy);
> +	if (err)
> +		return err;
> +
> +	err = sdxi_encode_copy(&desc, &copy);
> +	if (err)
> +		return err;
> +
> +	/* FIXME convert to pool */
> +	cst_blk = dma_alloc_coherent(sdxi_to_dev(sdxi), sizeof(*cst_blk),
> +				     &cst_blk_dma, GFP_NOWAIT);
> +	if (!cst_blk)
> +		return -ENOMEM;
> +
> +	cst_blk->signal = cpu_to_le64(0xff);
> +
> +	sdxi_cmd->cst_blk = cst_blk;
> +	sdxi_cmd->cst_blk_dma = cst_blk_dma;
> +	sdxi_cmd->ret = 0; /* TODO: get desc submit status & update ret value */
> +
> +	sdxi_desc_set_csb(&desc, cst_blk_dma);
> +	err = sdxi_submit_desc(cxt, &desc);
> +	if (err)
> +		goto free_cst_blk;
> +
> +	sdxi->tdata.cmd = sdxi_cmd; /* FIXME: this is not compatible w/multiple clients */
> +	dma_desc->issued_to_hw = 1;
> +	return 0;
> +free_cst_blk:
> +	dma_free_coherent(sdxi_to_dev(sdxi), sizeof(*cst_blk),
> +			  cst_blk, cst_blk_dma);
> +	return err;
> +}

> +static struct sdxi_dma_desc *sdxi_handle_active_desc(struct sdxi_dma_chan *chan,
> +						     struct sdxi_dma_desc *desc)
> +{
> +	struct dma_async_tx_descriptor *tx_desc;
> +	struct virt_dma_desc *vd;
> +	unsigned long flags;
> +
> +	/* Loop over descriptors until one is found with commands */
> +	do {
> +		if (desc) {
> +			if (!desc->issued_to_hw) {
> +				/* No errors, keep going */
> +				if (desc->status != DMA_ERROR)
> +					return desc;
> +			}
> +
> +			tx_desc = &desc->vd.tx;
> +			vd = &desc->vd;
> +		} else {
> +			tx_desc = NULL;
> +		}
> +
> +		spin_lock_irqsave(&chan->vc.lock, flags);
> +
> +		if (desc) {
> +

No blank line here.

> +			if (desc->status != DMA_COMPLETE) {
> +				if (desc->status != DMA_ERROR)
> +					desc->status = DMA_COMPLETE;
> +
> +				dma_cookie_complete(tx_desc);
> +				dma_descriptor_unmap(tx_desc);
> +				list_del(&desc->vd.node);
> +			} else {
> +				/* Don't handle it twice */
> +				tx_desc = NULL;
> +			}
> +		}
> +
> +		desc = sdxi_next_dma_desc(chan);
> +
> +		spin_unlock_irqrestore(&chan->vc.lock, flags);
> +
> +		if (tx_desc) {
> +			dmaengine_desc_get_callback_invoke(tx_desc, NULL);
> +			dma_run_dependencies(tx_desc);
> +			vchan_vdesc_fini(vd);
> +		}
> +	} while (desc);
> +
> +	return NULL;
> +}
> +
> +static void sdxi_cmd_callback(void *data, int err)
> +{
> +	struct sdxi_dma_desc *desc = data;
> +	struct dma_chan *dma_chan;
> +	struct sdxi_dma_chan *chan;
> +	int ret;
> +
> +	if (err == -EINPROGRESS)
> +		return;
> +
> +	dma_chan = desc->vd.tx.chan;
> +	chan = to_sdxi_dma_chan(dma_chan);
> +
> +	if (err)
> +		desc->status = DMA_ERROR;
> +
> +	while (true) {
> +		/* Check for DMA descriptor completion */
> +		desc = sdxi_handle_active_desc(chan, desc);
> +
> +		/* Don't submit cmd if no descriptor or DMA is paused */
> +		if (!desc)
perhaps return?
> +			break;
> +
> +		ret = sdxi_dma_start_desc(desc);
> +		if (!ret)
> +			break;
Perhaps return to make it clear that there is nothing else to do.
> +
> +		desc->status = DMA_ERROR;
> +	}
> +}
> +

> +
> +static struct sdxi_dma_desc *sdxi_dma_create_desc(struct dma_chan *dma_chan,
> +						  dma_addr_t dst,
> +						  dma_addr_t src,
> +						  unsigned int len,
> +						  unsigned long flags)
> +{
> +	struct sdxi_dma_chan *chan = to_sdxi_dma_chan(dma_chan);
> +	struct sdxi_dma_desc *desc;
> +	struct sdxi_cmd *sdxi_cmd;
> +
> +	desc = sdxi_dma_alloc_dma_desc(chan, flags);
> +	if (!desc)
> +		return NULL;
> +
> +	sdxi_cmd = &desc->sdxi_cmd;
Maybe
	*sdxi_cmd = (struct sdxi_cmd) {
		.ctx = chan->ctx,
etc

	};
> +	sdxi_cmd->cxt = chan->cxt;
> +	sdxi_cmd->cxt->sdxi = chan->cxt->sdxi;
> +	sdxi_cmd->src_addr = src;
> +	sdxi_cmd->dst_addr = dst;
> +	sdxi_cmd->len = len;
> +	sdxi_cmd->sdxi_cmd_callback = sdxi_cmd_callback;
> +	sdxi_cmd->data = desc;
> +
> +	return desc;
> +}

> +
> +static void sdxi_check_trans_status(struct sdxi_dma_chan *chan)
> +{
> +	struct sdxi_cxt *cxt = chan->cxt;
> +	struct sdxi_cmd *cmd;
> +
> +	if (!cxt)
> +		return;
> +
> +	cmd = cxt->sdxi->tdata.cmd;
> +
> +	if (le64_to_cpu(cmd->cst_blk->signal) == 0xfe)

Given that's a magic looking value, I think this 0xfe needs a define.

> +		sdxi_cmd_callback(cmd->data, cmd->ret);
> +}

> +
> +int sdxi_dma_register(struct sdxi_cxt *dma_cxt)
> +{
> +	struct sdxi_dma_chan *chan;
> +	struct sdxi_dev *sdxi = dma_cxt->sdxi;
> +	struct device *dev = sdxi_to_dev(sdxi);
> +	struct dma_device *dma_dev = &sdxi->dma_dev;
> +	int ret = 0;
> +
> +	sdxi->sdxi_dma_chan = devm_kzalloc(dev, sizeof(*sdxi->sdxi_dma_chan),
> +					   GFP_KERNEL);
This results in a mix of manual cleanup and devm.  That's generally something
we want to avoid because it makes code hard to review for race conditions etc.
I'd consider using custom actions and devm_add_action_or_reset() to ensure
that everything up to the first thing you want to not manage is done with
devm and ensure everything after that is done by hand.

Or use devm for everything.

> +	if (!sdxi->sdxi_dma_chan)
> +		return -ENOMEM;
> +
> +	sdxi->sdxi_dma_chan->cxt = dma_cxt;
> +
> +	dma_dev->dev = dev;
> +	dma_dev->src_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
> +	dma_dev->dst_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
> +	dma_dev->directions = BIT(DMA_MEM_TO_MEM);
> +	dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
> +	dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
> +	dma_cap_set(DMA_INTERRUPT, dma_dev->cap_mask);
> +
> +	dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
> +
> +	INIT_LIST_HEAD(&dma_dev->channels);
> +
> +	chan = sdxi->sdxi_dma_chan;
> +	chan->cxt->sdxi = sdxi;
> +
> +	/* Set base and prep routines */
> +	dma_dev->device_free_chan_resources = sdxi_dma_free_chan_resources;
> +	dma_dev->device_prep_dma_memcpy = sdxi_dma_prep_memcpy;
> +	dma_dev->device_prep_dma_interrupt = sdxi_prep_dma_interrupt;
> +	dma_dev->device_issue_pending = sdxi_dma_issue_pending;
> +	dma_dev->device_tx_status = sdxi_tx_status;
> +	dma_dev->device_terminate_all = sdxi_dma_terminate_all;
> +	dma_dev->device_synchronize = sdxi_dma_synchronize;
> +
> +	chan->vc.desc_free = sdxi_do_cleanup;
> +	vchan_init(&chan->vc, dma_dev);
> +
> +	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +
> +	ret = dma_async_device_register(dma_dev);
> +	if (ret)
> +		goto err_reg;
> +
> +	return 0;
> +
> +err_reg:

Just return early unless there is something to do.

> +	return ret;
> +}
> +
> +void sdxi_dma_unregister(struct sdxi_cxt *dma_cxt)
> +{
> +	dma_async_device_unregister(&dma_cxt->sdxi->dma_dev);
> +}