Re: [PATCH v12 17/20] dmaengine: ti: k3-udma-v2: New driver for K3 BCDMA_V2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] dma: missing BCDMA V2 hardware stall workaround in cyclic transfers
- [High] dma: silent descriptor drop on AUTOPAIR timeout causes client hang
- [High] dmaengine: ti: k3-udma-v2: division by zero in udma_v2_tx_status() on zero-length transfers
- [Medium] dma: memory leak of RX flush DMA mappings on probe error paths
--

commit 5dc79a65acd74e4dd901364f6c663884a74b10f2
Author: Sai Sree Kartheek Adivi <s-adivi@ti.com>
dmaengine: ti: k3-udma-v2: New driver for K3 BCDMA_V2

This commit adds support for the K3 BCDMA_V2 DMA engine used in AM62L. It
introduces a new version enum for DMA match data and implements the V2-specific
static channel mapping, unified channel space, and direct IRQ functionality.

> --- a/drivers/dma/ti/k3-udma-common.c
> +++ b/drivers/dma/ti/k3-udma-common.c
[ ... ]
> @@ -958,7 +974,8 @@ udma_prep_dma_cyclic_tr(struct udma_chan *uc, dma_addr_t buf_addr,
>  	 * As we are in cyclic mode, we do not know which period might be the
>  	 * last one, so set the flag for each period.
>  	 */
> -	if (uc->config.ep_type == PSIL_EP_PDMA_XY &&
> +	if ((uc->config.ep_type == PSIL_EP_PDMA_XY ||
> +	    uc->config.ep_type == PSIL_EP_PDMA_XYMF) &&
>  	    uc->ud->match_data->type == DMA_TYPE_BCDMA) {
>  		period_csf = CPPI5_TR_CSF_EOP;
>  	}

[Severity: High]
Does this properly apply the hardware stall workaround for BCDMA V2 in cyclic
transfers? 

The commit introduces a hardware workaround in udma_prep_slave_sg_tr() to set
the CPPI5_TR_CSF_EOP flag on every TR, noting that BCDMA V2 hardware stalls
if a TR's ICNT0 is not a multiple of 16 bytes. 

However, for cyclic transfers where the period length is large and requires
multiple TRs, the first TR explicitly omits this flag. For example:

drivers/dma/ti/k3-udma-common.c:udma_prep_dma_cyclic_tr() {
    ...
    if (num_tr == 2) {
        cppi5_tr_csf_set(&tr_req[tr_idx].flags,
                         CPPI5_TR_CSF_SUPR_EVT);
        tr_idx++;
    ...
}

If the period length results in an ICNT0 that isn't a 16-byte multiple, will
the hardware stall here as well?

> --- /dev/null
> +++ b/drivers/dma/ti/k3-udma-v2.c
[ ... ]
> +static int udma_v2_start(struct udma_chan *uc)
> +{
[ ... ]
> +		/* Poll for autopair completion */
> +		read_poll_timeout_atomic(udma_v2_check_chan_autopair_completion,
> +					 status, status != 0, 100, 500, false, uc);
> +
> +		if (status <= 0) {
> +			udma_chanrt_write(uc, UDMA_CHAN_RT_CTL_REG, 0);
> +			/*
> +			 * udma_start_desc() above already pushed uc->desc to
> +			 * the hardware ring. Since the channel never actually
> +			 * started, no completion interrupt will ever arrive
> +			 * to free it - drain the ring and free it here to
> +			 * avoid leaking it.
> +			 */
> +			udma_reset_rings(uc);
> +			udma_desc_free(&uc->desc->vd);
> +			uc->desc = NULL;
> +			return -ETIMEDOUT;
> +		}

[Severity: High]
Could this error path leave virt-dma clients hanging indefinitely?

Because the descriptor was already popped from the virt-dma issued list via
vchan_next_desc() earlier in this function, freeing it manually without
invoking vchan_cookie_complete() skips the virt-dma completion notification. 

A DMA client that is waiting for the callback or blocking on dma_sync_wait()
will never be notified of the failure and could hang permanently.

> +static int udma_v2_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = udma_setup_rx_flush(ud);
> +	if (ret)
> +		return ret;
[ ... ]
> +	ret = dma_async_device_register(&ud->ddev);
> +	if (ret) {
> +		dev_err(dev, "failed to register slave DMA engine: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, ud);
> +
> +	ret = of_dma_controller_register(dev->of_node, udma_v2_of_xlate, ud);
> +	if (ret) {
> +		dev_err(dev, "failed to register of_dma controller\n");
> +		dma_async_device_unregister(&ud->ddev);
> +	}
> +
> +	return ret;
> +}

[Severity: Medium]
Are we leaking the RX flush mappings if probe fails here?

The call to udma_setup_rx_flush() creates persistent DMA mappings via
dma_map_single(). If a subsequent step like dma_async_device_register() or
of_dma_controller_register() fails, the driver returns without calling the
newly added udma_teardown_rx_flush(), permanently leaking the mappings and
bounce buffers.

Additionally, looking at udma_setup_rx_flush() in k3-udma-common.c, there is a
similar leak where if the second dma_map_single() fails, it returns -ENOMEM
without unmapping the first buffer.

> +static enum dma_status udma_v2_tx_status(struct dma_chan *chan,
> +					 dma_cookie_t cookie,
> +					 struct dma_tx_state *txstate)
> +{
[ ... ]
> +		if (bcnt && !(bcnt % uc->desc->residue))
> +			residue = 0;
> +		else
> +			residue -= bcnt % uc->desc->residue;

[Severity: High]
Can this cause a division by zero panic?

If a DMA transfer is prepared with a zero length, the transfer descriptor is
allocated with its residue set to 0. 

When the framework queries the status, bcnt && !(bcnt % uc->desc->residue)
short-circuits to false when bcnt is 0. The code then evaluates
bcnt % uc->desc->residue in the else branch, which will trigger a panic
because uc->desc->residue is 0.

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