Re: [PATCH v2] dmaengine: mpc512x: fix dead empty check in mpc_dma_prep_slave_sg()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect calculation of `tcd->nbytes` using an encoded bus width enum instead of the actual byte count, leading to hardware misconfiguration and potential division by zero.
- [High] Activating the previously dead error recovery path triggers severe concurrency issues, including client deadlocks and cookie state corruption.
--

commit 534ce02bd48f83d7760dd6fe18bd31e5103a4bad
Author: Maoyi Xie <maoyixie.tju@gmail.com>

dmaengine: mpc512x: fix dead empty check in mpc_dma_prep_slave_sg()

This commit changes the list_first_entry() call to
list_first_entry_or_null() in mpc_dma_prep_slave_sg() to correctly
handle an empty free list. This ensures the empty case returns NULL and
allows the existing recovery path to execute.

> diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
> index 0adc8e01057e1..f5934136efc4c 100644
> --- a/drivers/dma/mpc512x_dma.c
> +++ b/drivers/dma/mpc512x_dma.c
> @@ -706,8 +706,8 @@ mpc_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  	for_each_sg(sgl, sg, sg_len, i) {
>  		spin_lock_irqsave(&mchan->lock, iflags);
>  
> -		mdesc = list_first_entry(&mchan->free,
> -						struct mpc_dma_desc, node);
> +		mdesc = list_first_entry_or_null(&mchan->free,
> +						 struct mpc_dma_desc, node);
>  		if (!mdesc) {
>  			spin_unlock_irqrestore(&mchan->lock, iflags);
>  			/* Try to free completed descriptors */

[Severity: High]
This isn't a bug introduced by this patch, but activating the recovery path
exposes a design flaw. Is it safe to call mpc_dma_process_completed()
from process context here?

As seen just below this diff, the recovery path calls
mpc_dma_process_completed(mdma). DMA clients often hold a spinlock during
dmaengine_prep_slave_sg() and expect to acquire it in their callback. Since
mpc_dma_process_completed() invokes client callbacks inline, could this
lead to a double-lock deadlock?

Additionally, mpc_dma_process_completed() drops mchan->lock to run
callbacks. Concurrently, mpc_dma_tasklet can execute, process newer
completions, and update mchan->chan.completed_cookie. When process
context resumes, it reacquires the lock and overwrites completed_cookie
with its older last_cookie, rolling the DMA state backwards.

Furthermore, this recovery path iterates over and flushes completions for
all DMA channels, which might disrupt other clients.

[ ... ]

[Severity: High]
This is a pre-existing issue, but further down in mpc_dma_prep_slave_sg(), is
the minor loop byte count calculated correctly?

drivers/dma/mpc512x_dma.c:mpc_dma_prep_slave_sg() {
...
		tcd->ssize = buswidth_to_dmatsize(mchan->swidth);
		tcd->dsize = buswidth_to_dmatsize(mchan->dwidth);

		if (mdma->is_mpc8308) {
...
		} else {
			len = sg_dma_len(sg);
			tcd->nbytes = tcd_nunits * tcd->ssize;
			if (!IS_ALIGNED(len, tcd->nbytes))
				goto err_prep;

			iter = len / tcd->nbytes;
...
}

The value tcd->ssize is an encoded hardware value (log2 of the bus width),
not the actual byte size. If mchan->swidth is 1 byte,
buswidth_to_dmatsize(1) returns 0, making tcd->ssize 0 and
tcd->nbytes 0.

If len == 0, IS_ALIGNED(0, 0) is true, which could trigger a division by
zero when calculating iter.

If len > 0, it unconditionally fails the alignment check, which would render
1-byte transfers broken. For larger widths (e.g., 4 bytes), tcd->ssize is 2,
causing the hardware to be misconfigured with an incorrect transfer size.

Additionally, for DMA_MEM_TO_DEV, does this incorrectly mix the peripheral
maxburst with the memory bus width?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522150214.95651-1-maoyixie.tju@gmail.com?part=1