Re: [PATCH 1/2] 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 minor loop byte count calculation causes hardware configuration errors and potential division by zero panic.
- [High] Direct C bitfield assignment to an __iomem pointer causes undefined behavior and potential Machine Check Exceptions.
--

commit 843ae9154206865fe98175ccec56b456c8445f35
Author: Maoyi Xie <maoyixie.tju@gmail.com>

dmaengine: mpc512x: fix dead empty check in mpc_dma_prep_slave_sg()
    
mpc_dma_prep_slave_sg() reads mchan->free with list_first_entry()
and then tests the returned pointer against NULL. list_first_entry()
never returns NULL. On an empty free list it returns an aliased
pointer derived from the list head, making the recovery path dead code.

> 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 in mpc_dma_prep_slave_sg(),
the minor loop byte count is calculated using the register enum value
tcd->ssize instead of the actual transfer width:

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;
...
}

For a 1-byte transfer width, tcd->ssize is 0, causing tcd->nbytes to be 0.
If a client submits a scatterlist with length 0, the IS_ALIGNED(len,
tcd->nbytes) check passes (since 0 & -1 == 0), resulting in a division by
zero kernel panic when calculating iter.

Even for valid lengths, 1-byte transfers always fail the alignment check,
and transfers with other widths receive an incorrect minor loop
configuration, causing functional failures.

Could we update this to use the actual transfer width instead of the enum
value?


[Severity: High]
This is also a pre-existing issue, but in mpc_dma_execute(), a bitfield on
a memory-mapped I/O structure is modified directly via C assignment:

drivers/dma/mpc512x_dma.c:mpc_dma_execute() {
...
	/* Send first descriptor in chain into hardware */
	memcpy_toio(&mdma->tcd[cid], first->tcd, sizeof(struct mpc_dma_tcd));

	if (first != prev)
		mdma->tcd[cid].e_sg = 1;
...
}

Since mdma->tcd is an __iomem pointer, does dereferencing it directly with
standard C assignments invoke undefined behavior and circumvent necessary
I/O accessors?

As struct mpc_dma_tcd is marked __packed__, the compiler may generate
byte-sized read-modify-write instructions. On PowerPC architectures, such
non-32-bit accesses to memory-mapped device registers can trigger Machine
Check Exceptions (MCE), leading to system crashes.

Is there a safer way to update this hardware register using appropriate
I/O accessors?

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