Re: [PATCH v2 3/4] dmaengine: dw-axi-dmac: Fix CH_CFG2 channel priority position

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Spurious dev_err logs and potential duplicate execution of DMA descriptors due to active descriptors remaining on the `desc_issued` list.
- [High] Missing NULL check on the error path in axi_desc_put() leads to a dma_pool_free() crash and an atomic counter underflow.
--

commit ecdc36132aeb3706e71e4d58d69cbfe272b69e28
Author: Jia Wang <wangjia@ultrarisc.com>

dmaengine: dw-axi-dmac: Fix CH_CFG2 channel priority position

The driver currently shifts the priority value by 20, which programs the
wrong bits for the CH_CFG2 channel priority field. This commit corrects
the field position by using bit 15 instead.

> diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac.h b/drivers/dma/dw-axi-dmac/dw-axi-dmac.h
> index 67cc199e24d1f..97451bb8b16a7 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac.h
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac.h
> @@ -327,7 +327,7 @@ enum {
>  #define CH_CFG2_H_TT_FC_POS		0
>  #define CH_CFG2_H_HS_SEL_SRC_POS	3
>  #define CH_CFG2_H_HS_SEL_DST_POS	4
> -#define CH_CFG2_H_PRIORITY_POS		20
> +#define CH_CFG2_H_PRIORITY_POS		15
>  
>  /**
>   * DW AXI DMA channel interrupts

While reviewing this patch, I noticed a couple of pre-existing issues in
the dw-axi-dmac driver:

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dma_chan_issue_pending() {
...
	if (vchan_issue_pending(&chan->vc))
		axi_chan_start_first_queued(chan);
...
}

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_chan_start_first_queued() {
...
	vd = vchan_next_desc(&chan->vc);
	if (!vd)
		return;

	desc = vd_to_axi_desc(vd);
...
	axi_chan_block_xfer_start(chan, desc);
}

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_chan_block_xfer_start() {
...
	if (unlikely(axi_chan_is_hw_enable(chan))) {
		dev_err(chan2dev(chan), "%s is non-idle!\n",
			axi_chan_name(chan));

		return;
	}
...
}

[Severity: High]
This isn't a bug introduced by this patch, but if a client queues a second
transfer and calls dma_chan_issue_pending() while the first is actively
running, does vchan_issue_pending() leave the active descriptor in the
issued list?

If vchan_next_desc() peeks at the head of the issued list without removing
it, does axi_chan_start_first_queued() fetch the already running first
descriptor?

If so, would axi_chan_block_xfer_start() detect the hardware is active and
print a spurious dev_err, or erroneously restart the already completed
descriptor if the hardware just finished but the interrupt hasn't fired yet?


drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_desc_put() {
...
	for (descs_put = 0; descs_put < count; descs_put++) {
		hw_desc = &desc->hw_desc[descs_put];
		dma_pool_free(chan->desc_pool, hw_desc->lli, hw_desc->llp);
	}

	kfree(desc->hw_desc);
	kfree(desc);
	atomic_sub(descs_put, &chan->descs_allocated);
...
}

[Severity: High]
This is a pre-existing issue, but if memory is exhausted during allocation
and axi_desc_get() returns NULL and goes to the error path, could
hw_desc->lli be NULL when dma_pool_free() is called here?

Does dma_pool_free() safely handle a NULL virtual address, or could this
cause a crash when it tries to push the block onto the pool's free list?

Also, does subtracting descs_put (which reaches the full count) from
chan->descs_allocated cause an atomic counter underflow if only a partial
allocation succeeded?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-dma-fix-v2-0-d7f0459ebb14@ultrarisc.com?part=3