Re: [PATCH v2 2/3] dmaengine: qcom: gpi: Add trace event support

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The `gpi_irq_status` tracepoint is only called before the interrupt processing loop, silently omitting back-to-back interrupts.

Pre-existing issues:
- [High] Tasklet Use-After-Free due to incorrect teardown order in gpi_free_chan_resources.
- [High] Incorrect channel cleanup in gpi_ch_init error path.
- [High] Leak of transfer ring DMA memory when gpi_ch_init fails.
- [High] Missing dma_wmb() barrier before writing to the channel doorbell register, which may cause the hardware to read stale descriptors.
- [High] Missing dma_rmb() barrier before reading event ring descriptors, risking processing of stale event data.
--

commit 4cf86dc6d78f8720092d0dce56c8bcfee50f39a4
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

    dmaengine: qcom: gpi: Add trace event support
    
    Wire up the qcom_gpi tracepoints into the GPI DMA engine driver to
    provide runtime visibility into driver behavior without requiring
    invasive debug patches.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..b09354a73b46e 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c

[ ... ]

> @@ -841,6 +846,7 @@ static irqreturn_t gpi_handle_irq(int irq, void *data)
>  
>  	offset = GPII_n_CNTXT_TYPE_IRQ_OFFS(gpii->gpii_id);
>  	type = gpi_read_reg(gpii, gpii->regs + offset);
> +	trace_gpi_irq_status(gpii->gpi_dev->dev, gpii_id, type);
>  
>  	do {
>  		/* global gpii error */

[Severity: Medium]
Will this tracepoint miss subsequent back-to-back interrupts?

In drivers/dma/qcom/gpi.c:gpi_handle_irq(), the hardware can trigger
back-to-back interrupts. The function reads the type register before the
loop, traces it, and then processes the interrupt. It reads the type
register again at the end of the loop and finds another interrupt, but
continues the loop without invoking trace_gpi_irq_status() for the newly
read type.

[ ... ]

> @@ -1887,6 +1897,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 there a missing dma_wmb() barrier
before writing to the channel doorbell register?

After the loop populates descriptors in
drivers/dma/qcom/gpi.c:gpi_issue_pending(), the doorbell is rung:

	gpi_desc->db = ch_ring->wp;
	gpi_write_ch_db(gchan, &gchan->ch_ring, gpi_desc->db);

Since gpi_write_ch_db() uses writel_relaxed(), without a preceding
dma_wmb(), could the CPU reorder the relaxed MMIO doorbell write before
the main memory TRE writes become globally visible, causing the hardware
to process uninitialized or stale descriptors?

[Severity: High]
This isn't a bug introduced by this patch, but does
gpi_free_chan_resources() in drivers/dma/qcom/gpi.c have a race
condition that could lead to a use-after-free?

	tasklet_kill(&gpii->ev_task);

	/* send command to de allocate event ring */
	if (cur_state == ACTIVE_STATE)
		gpi_send_cmd(gpii, NULL, GPI_EV_CMD_DEALLOC);

	gpi_free_ring(&gpii->ev_ring, gpii);

	/* disable interrupts */
	if (cur_state == ACTIVE_STATE)
		gpi_disable_interrupts(gpii);

The tasklet is killed and the ring is freed before the interrupt is
disabled. If a delayed or spurious hardware interrupt occurs in this
window, the interrupt handler will read pm_state as PREPARE_TERMINATE,
and could schedule the tasklet again. The tasklet would then run and
access the freed ev_ring.

[Severity: High]
This is a pre-existing issue, but does gpi_ch_init() in
drivers/dma/qcom/gpi.c have an incorrect channel cleanup in its
error path?

error_start_chan:
	for (i = i - 1; i >= 0; i--) {
		gpi_stop_chan(&gpii->gchan[i]);
		gpi_send_cmd(gpii, gchan, GPI_CH_CMD_RESET);
	}
	i = 2;
error_alloc_chan:
	for (i = i - 1; i >= 0; i--)
		gpi_reset_chan(gchan, GPI_CH_CMD_DE_ALLOC);

If initialization fails, it jumps to the error path. The loop iterates
over i, but it erroneously passes the single argument gchan to
gpi_stop_chan(), gpi_send_cmd(), and gpi_reset_chan() instead of
&gpii->gchan[i]. Could this repeatedly reset the same channel while
abandoning the other channel in a partially initialized state?

[Severity: High]
This isn't a bug introduced by this patch, but does
gpi_alloc_chan_resources() in drivers/dma/qcom/gpi.c leak the transfer
ring DMA memory when gpi_ch_init() fails?

	ret = gpi_alloc_ring(&gchan->ch_ring, CHAN_TRES,
			     sizeof(struct gpi_tre), gpii);
	if (ret)
		goto xfer_alloc_err;

	ret = gpi_ch_init(gchan);

	mutex_unlock(&gpii->ctrl_lock);

	return ret;

When gpi_ch_init() returns an error, the function exits without calling
gpi_free_ring() to release the locally allocated gchan->ch_ring.

[Severity: High]
This is a pre-existing issue, but is there a missing dma_rmb() barrier
before reading event ring descriptors in
drivers/dma/qcom/gpi.c:gpi_process_events()?

	cntxt_rp = gpi_read_reg(gpii, gpii->ev_ring_rp_lsb_reg);
	rp = to_virtual(ev_ring, cntxt_rp);

	do {
		while (rp != ev_ring->rp) {
			gpi_event = ev_ring->rp;

The CPU reads the hardware progress register via gpi_read_reg() which
uses readl_relaxed(). It then immediately reads the corresponding DMA
memory at ev_ring->rp. Without an intervening dma_rmb(), could the CPU
speculatively fetch the descriptors from memory before the progress
pointer read completes, reading stale events?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-add-trace-support-gpio-v2-0-01f81739bb88@oss.qualcomm.com?part=2