Re: [PATCH 3/3] dmaengine: qcom: gpi: Convert dev_dbg() calls to tracepoints

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver invokes the DMA client callback while the transfer descriptor is still on the vchan `desc_issued` list, leading to a double-free and use-after-free list corruption if the channel is terminated concurrently.
- [High] Missing dma_rmb() when reading event ring elements allows speculative reads of stale DMA memory.
- [High] Missing dma_wmb() before writing to the channel doorbell register allows the device to fetch stale transfer ring elements (TREs).
--

commit 69c40ab9cf4118765a807f48c1fc907c907c1667
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

    dmaengine: qcom: gpi: Convert dev_dbg() calls to tracepoints

    Replace the remaining dev_dbg() based debug logging in the GPI DMA
    driver with the qcom_gpi tracepoints, providing structured runtime
    visibility into GPI DMA behavior without requiring invasive debug
    patches. dev_err() calls are left untouched.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..8e7d25a461475 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
[ ... ]
> @@ -1043,6 +1039,8 @@ static void gpi_process_xfer_compl_event(struct gchan *gchan,
>  	smp_wmb();
>  
>  	chid = compl_event->chid;
> +	trace_gpi_ev_process(gpii->gpi_dev->dev, chid, XFER_COMPLETE_EV_TYPE,
> +			     compl_event->code, compl_event->status, compl_event->length);
>  	if (compl_event->code == MSM_GPI_TCE_EOT && gpii->ieob_set) {
>  		if (chid == GPI_RX_CHAN)
>  			goto gpi_free_desc;
> @@ -1054,11 +1052,10 @@ static void gpi_process_xfer_compl_event(struct gchan *gchan,
>  		dev_err(gpii->gpi_dev->dev, "Error in Transaction\n");
>  		result.result = DMA_TRANS_ABORTED;
>  	} else {
> -		dev_dbg(gpii->gpi_dev->dev, "Transaction Success\n");
>  		result.result = DMA_TRANS_NOERROR;
>  	}
>  	result.residue = gpi_desc->len - compl_event->length;
> -	dev_dbg(gpii->gpi_dev->dev, "Residue %d\n", result.residue);
> +	trace_gpi_xfer_result(gpii->gpi_dev->dev, chid, result.result, result.residue);
>  
>  	dma_cookie_complete(&vd->tx);
>  	dmaengine_desc_get_callback_invoke(&vd->tx, &result);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does invoking the DMA client
callback here while the descriptor is still on the vchan desc_issued list
create a use-after-free risk?

If a client concurrently aborts the transfer (e.g., on a timeout) by calling
dmaengine_terminate_all(), gpi_terminate_all() will extract all descriptors
from the vchan lists (including this vd) and free them. When the tasklet
resumes, it drops the lock, executes this callback, and then hits the
gpi_free_desc path:

gpi_free_desc:
	spin_lock_irqsave(&gchan->vc.lock, flags);
	list_del(&vd->node);
	spin_unlock_irqrestore(&gchan->vc.lock, flags);
	kfree(gpi_desc);

Could this result in a double-free and list corruption? The identical
vulnerable pattern also exists in gpi_process_imed_data_event().

[ ... ]
> @@ -1090,11 +1087,8 @@ static void gpi_process_events(struct gpii *gpii)
>  			chid = gpi_event->xfer_compl_event.chid;
>  			type = gpi_event->xfer_compl_event.type;
>  
> -			dev_dbg(gpii->gpi_dev->dev,
> -				"Event: CHID:%u, type:%x %08x %08x %08x %08x\n",
> -				chid, type, gpi_event->gpi_ere.dword[0],
> -				gpi_event->gpi_ere.dword[1], gpi_event->gpi_ere.dword[2],
> -				gpi_event->gpi_ere.dword[3]);
> +			trace_gpi_process_event(gpii->gpi_dev->dev, chid, type,
> +						gpi_event->gpi_ere.dword);

[Severity: High]
This is a pre-existing issue, but does gpi_process_events() need a dma_rmb()
before accessing the event ring elements?

The hardware event ring read pointer is read using gpi_read_reg() which
wraps readl_relaxed(). Because readl_relaxed() doesn't provide memory
ordering guarantees, could the CPU speculatively read the DMA memory for
gpi_event before the register read completes, resulting in stale DMA memory
being read?

[ ... ]
> @@ -1887,6 +1871,7 @@ static void gpi_issue_pending(struct dma_chan *chan)
>  	}
>  
>  	gpi_desc = to_gpi_desc(vd);
> +	trace_gpi_queue_xfer(gpii->gpi_dev->dev, gchan->chid, gpi_desc->num_tre);
>  	for (i = 0; i < gpi_desc->num_tre; i++) {
>  		tre = &gpi_desc->tre[i];
>  		gpi_queue_xfer(gpii, gchan, tre, &wp);
>  	}

[Severity: High]
This is a pre-existing issue, but is a dma_wmb() barrier needed here after
copying the Transfer Ring Elements (TREs) and before writing to the channel
doorbell register?

Right after this loop, gpi_issue_pending() calls gpi_write_ch_db() which
wraps writel_relaxed(). Without a dma_wmb() enforcing memory ordering, could
the register write reach the device before the TRE memory writes are
completely visible, causing the device to fetch stale descriptors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-add-trace-support-gpio-v1-0-7b0fd0d0ddb3@oss.qualcomm.com?part=3