Re: [PATCH v4 25/25] dmaengine: sdxi: Add DMA engine provider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] dmaengine: sdxi: devm DMA unregister runs after sdxi_unregister() frees cxts
- [High] dmaengine: sdxi: sdxi_tx_status() loses error state and skips dma_tx_state
- [Medium] dmaengine: sdxi: sdxi_device_init() ignores sdxi_dma_register() error
- [Medium] dmaengine: sdxi: context stop-on-error prevents IRQ reaping and stalls sync
- [Medium] dmaengine: sdxi: premature AKey and vector release on context teardown
- [Medium] dmaengine: sdxi: missing max_buffer check in sdxi_dma_prep_memcpy()
- [Low] dmaengine: sdxi: undocumented dma_channels parameter in kernel-parameters.txt
--

commit cdf3a67512f9931ad28471e39e5814ce577c3d81
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Add DMA engine provider

This commit registers a virt-dma memcpy provider for SDXI functions with a
configurable number of channels per function. It batches copy descriptors
behind fenced completion interrupts and implements channel termination and
synchronization callbacks.

> diff --git a/drivers/dma/sdxi/device.c b/drivers/dma/sdxi/device.c
> index 30c4e687cedf1..265ebea1e350e 100644
> --- a/drivers/dma/sdxi/device.c
> +++ b/drivers/dma/sdxi/device.c
> @@ -23,6 +23,7 @@
>  #include <linux/xarray.h>
>  
>  #include "context.h"
> +#include "dma.h"
>  #include "hw.h"
>  #include "mmio.h"
>  #include "sdxi.h"
> @@ -424,6 +425,7 @@ static int sdxi_device_init(struct sdxi_dev *sdxi)
>  	if (err)
>  		return err;
>  
> +	sdxi_dma_register(sdxi);
>  	return 0;
>  }

[Severity: Medium]
As reported by Sashiko on v3 [1], sdxi_dma_register() returns -ENOMEM when
devm_kzalloc() fails and returns the negative error code from
dmaenginem_async_device_register() when registration fails, but
sdxi_device_init() ignores the return value and unconditionally returns 0.

Should sdxi_device_init() check the return value of sdxi_dma_register() and
clean up via sdxi_dev_stop() on failure, or should sdxi_dma_register() return
void if DMA engine registration is intentionally best-effort?

> diff --git a/drivers/dma/sdxi/dma.c b/drivers/dma/sdxi/dma.c
> new file mode 100644
> index 0000000000000..512855c83da27
> --- /dev/null
> +++ b/drivers/dma/sdxi/dma.c
> @@ -0,0 +1,509 @@

[ ... ]

> +static unsigned short dma_channels = 1;
> +module_param(dma_channels, ushort, 0644);
> +MODULE_PARM_DESC(dma_channels, "DMA channels per function (default: 1)");

[Severity: Low]
This adds the sdxi_core.dma_channels module parameter, which can also be set
on the kernel command line at boot. Should this parameter be documented in
Documentation/admin-guide/kernel-parameters.txt?

[ ... ]

> +static struct dma_async_tx_descriptor *
> +sdxi_dma_prep_memcpy(struct dma_chan *dma_chan, dma_addr_t dst,
> +		     dma_addr_t src, size_t len, unsigned long flags)
> +{
> +	struct sdxi_dma_chan *sdchan = to_sdxi_dma_chan(dma_chan);
> +	struct sdxi_cxt *cxt = sdchan->cxt;
> +	u16 akey_index = sdxi_akey_index(cxt, sdchan->akey);
> +	struct sdxi_dma_txd *txd;
> +	struct sdxi_copy copy = {
> +		.src = src,
> +		.dst = dst,
> +		.src_akey = akey_index,
> +		.dst_akey = akey_index,
> +		.len = len,
> +	};
> +
> +	/*
> +	 * Validate now, with a throwaway encode onto the stack. The real
> +	 * encode into the ring happens at issue.
> +	 */
> +	if (sdxi_encode_copy(&(struct sdxi_desc){}, &copy))
> +		return NULL;

[Severity: Medium]
In sdxi_fn_activate() and sdxi_publish_cxt(), the function and client context
are configured with sdxi->max_buffer, which restricts each copy descriptor to
at most (u64)SZ_2M << sdxi->max_buffer bytes:

drivers/dma/sdxi/device.c:sdxi_fn_activate()
	sdxi->max_buffer = FIELD_GET(SDXI_MMIO_CAP1_MAX_BUFFER, cap1);
	dma_set_max_seg_size(sdxi->dev,
			     min((u64)SZ_2M << sdxi->max_buffer, U32_MAX));

drivers/dma/sdxi/context.c:sdxi_publish_cxt()
	*ent = (typeof(*ent)) {
		...
		.max_buffer  = cxt->sdxi->max_buffer,
		...
	};

However, sdxi_dma_prep_memcpy() only validates copy.len through
sdxi_encode_copy() -> sdxi_encode_size32(), which accepts any length up to
SZ_4G. Because DMA engine memcpy callers such as dmatest and async_memcpy do
not check dma_get_max_seg_size() before calling device_prep_dma_memcpy(), can
a transfer with len > ((u64)SZ_2M << cxt->sdxi->max_buffer) succeed in
sdxi_dma_prep_memcpy() and then fault the hardware context when issued?

> +
> +	txd = kmalloc_obj(*txd, GFP_NOWAIT);
> +	if (!txd)
> +		return NULL;
> +
> +	*txd = (typeof(*txd)) {
> +		.copy = copy,
> +	};
> +
> +	return vchan_tx_prep(&sdchan->vchan, &txd->vdesc, flags);
> +}
> +
> +static enum dma_status sdxi_tx_status(struct dma_chan *chan,
> +				      dma_cookie_t cookie,
> +				      struct dma_tx_state *state)
> +{
> +	struct sdxi_dma_chan *sdchan = to_sdxi_dma_chan(dma_chan);
> +	struct virt_dma_desc *vdesc;
> +
> +	/* Under the lock for a consistent cookie snapshot; the IRQ reaps. */
> +	guard(spinlock_irqsave)(&sdchan->vchan.lock);
> +
> +	/* vchan_find_desc() only returns issued txds, which own a completion. */
> +	vdesc = vchan_find_desc(&sdchan->vchan, cookie);
> +	if (vdesc) {
> +		struct sdxi_dma_txd *txd = to_sdxi_dma_txd(vdesc);
> +
> +		if (sdxi_cst_blk_check(txd->completion.cst_blk) == SDXI_COMPLETION_ERROR)
> +			return DMA_ERROR;
> +	}
> +
> +	return dma_cookie_status(chan, cookie, state);
> +}

[Severity: High]
As reported by Sashiko on v3 [2], when a descriptor finishes with
SDXI_COMPLETION_ERROR, sdxi_dma_reap_locked() removes it from
vchan->desc_issued and calls vchan_cookie_complete():

	if (state == SDXI_COMPLETION_ERROR)
		txd->vdesc.tx_result.result = DMA_TRANS_ABORTED;

	list_del(&vdesc->node);
	sdxi_cst_pool_put(sdchan->cxt->cst_pool, txd->completion.cst_blk);
	txd->completion = (struct sdxi_completion) {};
	vchan_cookie_complete(&txd->vdesc);

Because vchan_cookie_complete() advances chan->completed_cookie to that
descriptor's cookie and vchan_find_desc() only searches vchan->desc_issued,
won't any call to sdxi_tx_status() after the descriptor is reaped fall through
to dma_cookie_status() and report DMA_COMPLETE instead of DMA_ERROR?

Also, when vchan_find_desc() does find an errored descriptor still on
vchan->desc_issued, returning DMA_ERROR directly skips dma_cookie_status()
and leaves *state uninitialized when the caller passes a non-NULL state
pointer (such as in dma_async_is_tx_complete()).

[ ... ]

> +static bool sdxi_dma_idle(struct sdxi_dma_chan *sdchan)
> +{
> +	struct virt_dma_chan *vchan = &sdchan->vchan;
> +
> +	guard(spinlock_irqsave)(&vchan->lock);
> +
> +	sdxi_dma_reap_locked(sdchan);
> +	if (sdxi_dma_drain_locked(sdchan))
> +		sdxi_cxt_kick(sdchan->cxt);
> +
> +	return list_empty(&vchan->desc_terminated) &&
> +	       list_empty(&vchan->desc_issued) &&
> +	       list_empty(&sdchan->pending);
> +}

[Severity: Medium]
Client contexts are configured with stop-on-error enabled in
sdxi_publish_cxt():

drivers/dma/sdxi/context.c:sdxi_publish_cxt()
	ctl_cfg = (typeof(ctl_cfg)) {
		.se              = 1,
		.csa             = 1,
		...
	};

In sdxi_dma_drain_locked(), completion interrupts are generated by a fenced
DSC_INTR descriptor placed at the end of each copy batch. If a copy descriptor
encounters an error, the context stops on that descriptor before executing the
trailing fenced DSC_INTR descriptor or any subsequent copies in the ring.

When that happens, won't the completion interrupt never fire, and won't
sdxi_dma_idle() in sdxi_dma_synchronize() continue waiting on the remaining
issued and pending descriptors until the 10-second SDXI_DMA_SYNC_TIMEOUT_US
expires rather than detecting that cxt->sq->cxt_sts.state has entered
CXTV_STOP_FN or CXTV_ERR_FN?

[ ... ]

> +static int sdxi_dma_alloc_chan_resources(struct dma_chan *dma_chan)
> +{

[ ... ]

> +	err = sdxi_start_cxt(sdchan->cxt);
> +	if (err)
> +		goto free_irq;
> +
> +	return 0;
> +free_irq:
> +	free_irq(sdchan->irq, sdchan);
> +free_akey:
> +	sdxi_free_akey(sdchan->cxt, sdchan->akey);
> +free_vector:
> +	sdxi_free_vector(sdxi, vector);
> +exit_cxt:
> +	sdxi_cxt_exit(sdchan->cxt);
> +	return err;
> +}
> +
> +static void sdxi_dma_free_chan_resources(struct dma_chan *dma_chan)
> +{
> +	struct sdxi_dma_chan *sdchan = to_sdxi_dma_chan(dma_chan);
> +	LIST_HEAD(head);
> +
> +	sdxi_stop_cxt(sdchan->cxt);
> +	free_irq(sdchan->irq, sdchan);
> +	sdxi_free_vector(sdchan->cxt->sdxi, sdchan->vector);
> +	sdxi_free_akey(sdchan->cxt, sdchan->akey);
> +	/* virt-dma does not know about the driver's pending list. */
> +	scoped_guard(spinlock_irqsave, &sdchan->vchan.lock)
> +		list_splice_tail_init(&sdchan->pending, &head);
> +	vchan_dma_desc_free_list(&sdchan->vchan, &head);
> +	vchan_free_chan_resources(to_virt_chan(dma_chan));
> +	sdxi_cxt_exit(sdchan->cxt);
> +}

[Severity: Medium]
In sdxi_dma_free_chan_resources(), if sdxi_stop_cxt() fails or times out (or
is interrupted in sdxi_cxt_reserve()), or if sdxi_cxt_update() fails inside
sdxi_cxt_exit(), sdxi_cxt_exit() retains the context until function shutdown
because the hardware may still reference it:

drivers/dma/sdxi/context.c:sdxi_cxt_exit()
	state = FIELD_GET(SDXI_CXT_STS_STATE, READ_ONCE(cxt->sq->cxt_sts.state));
	if (state != CXTV_STOP_SW && state != CXTV_STOP_FN && state != CXTV_ERR_FN)
		return -EBUSY;
	...
	err = sdxi_cxt_update(cxt);
	if (err) {
		dev_err(sdxi->dev,
			"context %u release failed (%d), retaining resources until function shutdown\n",
			cxt->id, err);
		return err;
	}

	sdxi_free_cxt(cxt);

Because sdxi_dma_free_chan_resources() calls sdxi_free_vector() and
sdxi_free_akey() unconditionally before sdxi_cxt_exit(), can this zero the
DMA-visible AKey entry and release sdchan->vector for reuse by another
channel while the unstopped context is still active in hardware?

Similarly, in sdxi_dma_alloc_chan_resources(), if sdxi_start_cxt() times out
after publishing DSC_CXT_START to the admin ring, jumping to free_irq frees
the AKey and vector and calls sdxi_cxt_exit() without stopping the context,
while DSC_CXT_START may still be pending or may have just transitioned the
context to CXTV_RUN.

> +
> +int sdxi_dma_register(struct sdxi_dev *sdxi)
> +{

[ ... ]

> +	err = dmaenginem_async_device_register(dma_dev);
> +	if (err)
> +		return dev_warn_probe(dev, err, "failed to register dma device\n");
> +
> +	return 0;
> +}

[Severity: High]
Does using dmaenginem_async_device_register() here cause a use-after-free and
double-free during driver unbind or PCI removal?

During probe, sdxi_pci_probe() -> sdxi_register() -> sdxi_device_init() ->
sdxi_dma_register() registers dma_dev with dmaenginem_async_device_register(),
which defers dma_async_device_unregister() to devres_release_all() after the
bus remove callback returns.

When the device is removed or unbound, sdxi_pci_remove() calls
sdxi_unregister() first:

drivers/dma/sdxi/device.c:sdxi_unregister()
	sdxi_dev_stop(sdxi);
	sdxi_client_cxts_exit(sdxi);
	ida_destroy(&sdxi->vectors);

sdxi_client_cxts_exit() frees every client context in sdxi->client_cxts via
sdxi_free_cxt() and destroys the xarray, while dma_dev and any allocated
channels are still registered. When devres_release_all() subsequently runs
dmaenginem_async_device_unregister() -> dma_async_device_unregister() ->
sdxi_dma_free_chan_resources() (or if an interrupt or channel callback runs
before devres teardown), sdxi_dma_free_chan_resources() dereferences the
already-freed sdchan->cxt, accesses the destroyed sdxi->vectors IDA and
sdxi->client_cxts xarray, and frees sdchan->cxt a second time in
sdxi_cxt_exit().

[1] https://lore.kernel.org/r/20260605-sdxi-base-v3-23-4d38ca2bdffe@amd.com
[2] https://lore.kernel.org/r/20260605-sdxi-base-v3-23-4d38ca2bdffe@amd.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-sdxi-base-v4-0-3668ccc7aab8@amd.com?part=25