Re: [PATCH] dmaengine: dw-axi-dmac: demote apb_regs warning to debug

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] In `dw_axi_dma_set_hw_channel()`, bitwise operations on a 32-bit signed integer and improper use of `unsigned long` cause severe data loss and Undefined Behavior, permanently corrupting the configuration for channels 4-7 in the 64-bit hardware register.
- [High] In several descriptor preparation functions, the error cleanup path loops over all hardware descriptors to free them using dma_pool_free(), regardless of whether they were successfully allocated. This causes a NULL pointer dereference crash and corrupts atomic tracking counters.
--

commit f6abedf368282d2b40d4a72081898ecaa3428d91
Author: Baineng Shou <shoubaineng@gmail.com>

dmaengine: dw-axi-dmac: demote apb_regs warning to debug

This commit changes the log level of the "apb_regs not initialized" message
from error to debug in dw_axi_dma_set_hw_channel(). This prevents log spam
on supported SoCs that do not carry apb_regs, where reaching this path is
a normal no-op during slave or cyclic transfers.

> diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> index eebed2474210d..dc04b73c5987b 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> @@ -574,7 +574,7 @@ static void dw_axi_dma_set_hw_channel(struct axi_dma_chan *chan, bool set)
>  	unsigned long reg_value, val;

[Severity: High]
This is a pre-existing issue, but does using unsigned long here silently
truncate the 64-bit value returned by lo_hi_readq() on 32-bit platforms?
Assigning the 64-bit result to a 32-bit unsigned long would cause immediate
data loss for the upper 32 bits.

Also, further down in dw_axi_dma_set_hw_channel(), is there a risk of
undefined behavior and unintended zero-extension?

	reg_value &= ~(DMA_APB_HS_SEL_MASK <<
			(chan->id * DMA_APB_HS_SEL_BIT_SIZE));

If DMA_APB_HS_SEL_MASK is a 32-bit signed integer, shifting it by 24 (when
chan->id >= 3) produces a sign-extended 0x00FFFFFF. When promoted to 64-bit,
this zero-extends to 0x0000000000FFFFFFUL, which unintentionally wipes out
the configuration for channels 4-7. For chan->id >= 4, it shifts by 32 or
more, invoking undefined behavior.

>  
>  	if (!chip->apb_regs) {
> -		dev_err(chip->dev, "apb_regs not initialized\n");
> +		dev_dbg(chip->dev, "apb_regs not initialized\n");
>  		return;
>  	}

[Severity: High]
This is another pre-existing issue, but while reviewing the error handling
paths for descriptor allocation, I noticed a potential null pointer
dereference in axi_desc_put().

When functions like dw_axi_dma_chan_prep_cyclic() fail to allocate or set up
hardware descriptors, they jump to their error paths and call axi_desc_put().
The array of hardware descriptors is zero-initialized, and axi_desc_put()
unconditionally loops over all elements to free them:

	dma_pool_free(chan->desc_pool, hw_desc->lli, hw_desc->llp);

If unallocated descriptors are passed in, could hw_desc->lli be null?
Passing a null pointer to dma_pool_free() dereferences it immediately,
crashing the kernel.

Additionally, in the same function:

	atomic_sub(descs_put, &chan->descs_allocated);

Could this corrupt the atomic tracking counter? It appears to subtract the
full array length (descs_put) regardless of how many descriptors were
actually allocated, which would underflow the counter.

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