Re: [PATCH v7 16/19] dmaengine: ti: k3-udma-v2: New driver for K3 BCDMA_V2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Out-of-bounds array access and channel allocation failure in `bcdma_v2_get_bchan()`.
- [High] Unconditional PEER register access in `udma_v2_stop()` for block copy channels (BCHAN).
- [High] Ignored completion ring events due to an early return in `udma_v2_ring_irq_handler()`.
- [High] Protocol violation by asserting EOP on every TR in a scatter-gather list in `udma_prep_slave_sg_tr()`.
- [Medium] Missing locking in `udma_v2_pause()` and `udma_v2_resume()` causes a read-modify-write race on `UDMA_CHAN_RT_CTL_REG`.
- [Low] Hardcoded register offset used instead of predefined macro in `udma_v2_tx_status()`.
--

--- Patch [16]: [PATCH v7 16/19] dmaengine: ti: k3-udma-v2: New driver for K3 BCDMA_V2 ---
commit 4f5633caae3e5cd709118b7e0fd5b3865c70a1dc
Author: Sai Sree Kartheek Adivi <s-adivi@ti.com>
Subject: dmaengine: ti: k3-udma-v2: New driver for K3 BCDMA_V2

This commit adds support for the BCDMA_V2 hardware found in SoCs like AM62L.
It introduces a new driver file tailored to the differences in V2, including
direct IRQs, static channel mapping, and a unified channel space.

> diff --git a/drivers/dma/ti/k3-udma-common.c b/drivers/dma/ti/k3-udma-common.c
> --- a/drivers/dma/ti/k3-udma-common.c
> +++ b/drivers/dma/ti/k3-udma-common.c
[ ... ]
> @@ -342,6 +348,11 @@ udma_prep_slave_sg_tr(struct udma_chan *uc, struct scatterlist *sgl,
>  	else
>  		asel = (u64)uc->config.asel << K3_ADDRESS_ASEL_SHIFT;
>  
> +	if (uc->ud->match_data->type == DMA_TYPE_BCDMA &&
> +	    uc->ud->match_data->version == K3_UDMA_V2 &&
> +	    dir == DMA_MEM_TO_DEV)
> +		extra_flags = CPPI5_TR_CSF_EOP;
> +
>  	tr_req = d->hwdesc[0].tr_req_base;
>  	for_each_sg(sgl, sgent, sglen, i) {
>  		dma_addr_t sg_addr = sg_dma_address(sgent);
> @@ -358,7 +369,7 @@ udma_prep_slave_sg_tr(struct udma_chan *uc, struct scatterlist *sgl,
>  
>  		cppi5_tr_init(&tr_req[tr_idx].flags, CPPI5_TR_TYPE1, false,
>  			      false, CPPI5_TR_EVENT_SIZE_COMPLETION, 0);
> -		cppi5_tr_csf_set(&tr_req[tr_idx].flags, CPPI5_TR_CSF_SUPR_EVT);
> +		cppi5_tr_csf_set(&tr_req[tr_idx].flags, CPPI5_TR_CSF_SUPR_EVT | extra_flags);

[Severity: High]
Does this code inadvertently assert the End Of Packet (EOP) flag on every
Transfer Request (TR) in the scatter-gather list?

In udma_prep_slave_sg_tr(), extra_flags is initialized with CPPI5_TR_CSF_EOP
for DMA_MEM_TO_DEV transfers. By bitwise-ORing it into every TR generated
within the for_each_sg loop, the packet becomes artificially fragmented on
the hardware bus.

Should the EOP bit be reserved exclusively for the final TR of the final SG
segment to avoid protocol violations where peripherals expect a continuous
payload?

[ ... ]
> diff --git a/drivers/dma/ti/k3-udma-v2.c b/drivers/dma/ti/k3-udma-v2.c
> new file mode 100644
> index 0000000000000..e7989b0b34cac
> --- /dev/null
> +++ b/drivers/dma/ti/k3-udma-v2.c
[ ... ]
> +static int udma_v2_stop(struct udma_chan *uc)
> +{
> +	uc->state = UDMA_CHAN_IS_TERMINATING;
> +	reinit_completion(&uc->teardown_completed);
> +
> +	if (uc->config.dir == DMA_DEV_TO_MEM) {
> +		if (!uc->cyclic && !uc->desc)
> +			udma_push_to_ring(uc, -1);
> +	}
> +
> +	udma_chanrt_write(uc, UDMA_CHAN_RT_PEER_REG(8), UDMA_CHAN_RT_PEER_REG8_FLUSH);

[Severity: High]
Can this unconditional PEER register write cause a kernel panic when stopping
block copy channels?

In udma_v2_stop(), there is an MMIO write to UDMA_CHAN_RT_PEER_REG(8). Since
block copy channels (which handle DMA_MEM_TO_MEM transfers) do not have this
PEER register region mapped, this write may trigger an asynchronous external
abort (SError).

Could this be guarded with a check like if (uc->config.ep_type !=
PSIL_EP_NATIVE), similar to how it was handled in the V1 driver?

> +	udma_chanrt_update_bits(uc, UDMA_CHAN_RT_CTL_REG,
> +				UDMA_CHAN_RT_CTL_EN | UDMA_CHAN_RT_CTL_TDOWN,
> +				UDMA_CHAN_RT_CTL_EN | UDMA_CHAN_RT_CTL_TDOWN);
> +
> +	return 0;
> +}
[ ... ]
> +static irqreturn_t udma_v2_ring_irq_handler(int irq, void *data)
> +{
[ ... ]
> +	if (!intr_status)
> +		return IRQ_NONE;	/* Not this channel's interrupt */
> +
> +	reg = udma_chanrt_read(uc, UDMA_CHAN_RT_CTL_REG);
> +
> +	if (intr_status & K3_RINGACC_RT_INT_STATUS_TR) {
> +		/* check teardown status */
> +		if ((reg & UDMA_CHAN_RT_CTL_TDOWN) && !(reg & UDMA_CHAN_RT_CTL_EN))
> +			complete_all(&uc->teardown_completed);
> +		return udma_v2_udma_irq_handler(irq, data);
> +	}
> +
> +	if (udma_pop_from_ring(uc, &paddr) || !paddr)
> +		return IRQ_NONE;

[Severity: High]
Is there a risk of missing completion ring events if both the TR interrupt
and the completion ring interrupt are asserted simultaneously?

If the hardware asserts both interrupts, intr_status will have both bits set.
The handler processes the TR event and then returns early, completely
bypassing the udma_pop_from_ring() logic.

This could leave stale descriptors stuck in the completion ring indefinitely,
potentially causing ring slot leaks and channel stalls. Would it be safer to
process the TR event without returning immediately, allowing the completion
ring to be serviced as well?

[ ... ]
> +static int bcdma_v2_get_bchan(struct udma_chan *uc)
> +{
> +	struct udma_dev *ud = uc->ud;
> +	enum udma_tp_level tpl;
> +	int ret;
> +
> +	if (uc->bchan) {
> +		dev_dbg(ud->dev, "chan%d: already have bchan%d allocated\n",
> +			uc->id, uc->bchan->id);
> +		return 0;
> +	}
> +
> +	/*
> +	 * Use normal channels for peripherals, and highest TPL channel for
> +	 * mem2mem
> +	 */
> +	if (uc->config.tr_trigger_type)
> +		tpl = 0;
> +	else
> +		tpl = ud->bchan_tpl.levels - 1;
> +
> +	uc->bchan = __udma_reserve_bchan(ud, tpl, uc->id);

[Severity: High]
Could passing uc->id to __udma_reserve_bchan() lead to out-of-bounds array
accesses and allocation failures?

The driver creates a single logical pool of virtual channels. If a client
requests a DMA_MEM_TO_MEM channel and receives a virtual channel index
(uc->id) that is greater than or equal to the hardware block copy channel
count (e.g., 16), __udma_reserve_bchan() will use this out-of-bounds ID to
test and set bits in ud->bchan_map.

Should this use -1 to dynamically allocate a free hardware channel, as was
done in the V1 driver?

> +	if (IS_ERR(uc->bchan)) {
> +		ret = PTR_ERR(uc->bchan);
> +		uc->bchan = NULL;
> +		return ret;
> +	}
> +	uc->chan = uc->bchan;
> +	uc->tchan = uc->bchan;
> +
> +	return 0;
> +}
> +
> +static int bcdma_v2_alloc_bchan_resources(struct udma_chan *uc)
> +{
> +	struct k3_ring_cfg ring_cfg;
> +	struct udma_dev *ud = uc->ud;
> +	int ret;
> +
> +	ret = bcdma_v2_get_bchan(uc);
> +	if (ret)
> +		return ret;
> +
> +	ret = k3_ringacc_request_rings_pair(ud->ringacc, ud->match_data->chan_cnt + uc->id, -1,

[Severity: High]
Does calculating the ring IDs using uc->id instead of the hardware channel
ID (uc->bchan->id) violate the ring pool limits?

Since uc->id is the software virtual channel index, using it here may result
in requesting ring pairs outside the allocated hardware boundaries.

[ ... ]
> +static enum dma_status udma_v2_tx_status(struct dma_chan *chan,
> +					 dma_cookie_t cookie,
> +					 struct dma_tx_state *txstate)
> +{
[ ... ]
> +		if (uc->desc->dir == DMA_MEM_TO_DEV) {
> +			bcnt = udma_chanrt_read(uc, UDMA_CHAN_RT_SBCNT_REG);
> +
> +			if (uc->config.ep_type != PSIL_EP_NATIVE) {
> +				peer_bcnt = udma_chanrt_read(uc, 0x810);
> +
> +				if (bcnt > peer_bcnt)
> +					delay = bcnt - peer_bcnt;
> +			}
> +		} else if (uc->desc->dir == DMA_DEV_TO_MEM) {
> +			bcnt = udma_chanrt_read(uc, UDMA_CHAN_RT_BCNT_REG);
> +
> +			if (uc->config.ep_type != PSIL_EP_NATIVE) {
> +				peer_bcnt = udma_chanrt_read(uc, 0x810);

[Severity: Low]
Is there a reason this hardcodes the register offset 0x810 instead of using
the UDMA_CHAN_RT_PERIPH_BCNT_REG macro?

Using the predefined macro would improve code maintainability and keep it
consistent with other parts of the driver.

[ ... ]
> +static int udma_v2_pause(struct dma_chan *chan)
> +{
> +	struct udma_chan *uc = to_udma_chan(chan);
> +
> +	/* pause the channel */
> +	udma_chanrt_update_bits(uc, UDMA_CHAN_RT_CTL_REG,
> +				UDMA_CHAN_RT_CTL_PAUSE, UDMA_CHAN_RT_CTL_PAUSE);

[Severity: Medium]
Could this read-modify-write operation on the channel control register
silently corrupt state due to missing locks?

In udma_v2_pause() and udma_v2_resume(), udma_chanrt_update_bits() accesses
UDMA_CHAN_RT_CTL_REG without holding uc->vc.lock. Since the IRQ handler or
tasklet might hold the lock and concurrently invoke udma_v2_start() or
udma_v2_stop() (which also modify this register), it appears there is a
race condition.

Can a concurrent update cause the driver to drop channel enable or teardown
flags, potentially locking the channel state?

> +
> +	return 0;
> +}
> +
> +static int udma_v2_resume(struct dma_chan *chan)
> +{
> +	struct udma_chan *uc = to_udma_chan(chan);
> +
> +	/* resume the channel */
> +	udma_chanrt_update_bits(uc, UDMA_CHAN_RT_CTL_REG,
> +				UDMA_CHAN_RT_CTL_PAUSE, 0);

[Severity: Medium]
Could this register update face the same concurrent read-modify-write race
as mentioned above due to the absence of uc->vc.lock?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724102115.2854-1-s-adivi@ti.com?part=16