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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unvalidated hardware event 'chid' used as an array index.
- [High] Unvalidated hardware event 'tre_index' leads to out-of-bounds memory read.
- [High] The driver drops gchan->vc.lock while a DMA descriptor remains on the virt-dma issued list, leading to a race with channel termination and a use-after-free.
- [High] Error path in gpi_resume() leaves the hardware IRQ enabled while the software state remains paused, leading to unbalanced IRQ enables upon retry.
- [High] The driver does not validate the physical address (`ptr`) returned in a DMA completion event, allowing untrusted input to corrupt ring buffer pointers.
- [High] Missing dma_rmb() before reading the DMA event ring buffer.
- [High] Missing memory barrier before ringing the event doorbell in gpi_process_events().
- [High] Missing dma_wmb() before writing to the channel doorbell in gpi_issue_pending().
--

commit 1bf9fce9a4cdb676de685f3966a35631e5323808
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 b09354a73b46e..8e7d25a461475 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -945,17 +940,10 @@ static void gpi_process_imed_data_event(struct gchan *gchan,
>  		struct gpi_tre *gpi_tre;
>  
>  		spin_unlock_irqrestore(&gchan->vc.lock, flags);
> -		dev_dbg(gpii->gpi_dev->dev, "event without a pending descriptor!\n");
>  		gpi_ere = (struct gpi_ere *)imed_event;
> -		dev_dbg(gpii->gpi_dev->dev,
> -			"Event: %08x %08x %08x %08x\n",
> -			gpi_ere->dword[0], gpi_ere->dword[1],
> -			gpi_ere->dword[2], gpi_ere->dword[3]);
>  		gpi_tre = tre;
> -		dev_dbg(gpii->gpi_dev->dev,
> -			"Pending TRE: %08x %08x %08x %08x\n",
> -			gpi_tre->dword[0], gpi_tre->dword[1],
> -			gpi_tre->dword[2], gpi_tre->dword[3]);
> +		trace_gpi_ev_no_desc(gpii->gpi_dev->dev, imed_event->chid,
> +				     gpi_ere->dword, gpi_tre->dword);
>  		return;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does this code lead to an
out-of-bounds memory read?

drivers/dma/qcom/gpi.c:gpi_process_imed_data_event() {
    ...
    void *tre = ch_ring->base + (ch_ring->el_size * imed_event->tre_index);
    ...
    gpi_tre = tre;
    trace_gpi_ev_no_desc(gpii->gpi_dev->dev, imed_event->chid,
                         gpi_ere->dword, gpi_tre->dword);
}

The tre_index is read from imed_event->tre_index without a bounds check against
ch_ring->elements. If it exceeds the ring size, will the tracepoint dereference
out-of-bounds memory?

[ ... ]

> @@ -1064,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 is a pre-existing issue, but does this driver validate the physical address
returned in a DMA completion event?

drivers/dma/qcom/gpi.c:gpi_process_xfer_compl_event() {
    ...
    void *ev_rp = to_virtual(ch_ring, compl_event->ptr);
    ...
}

The compl_event->ptr is a physical address provided by the hardware DMA event,
and to_virtual() converts it without bounds checking. If the hardware yields an
invalid ptr outside the physical bounds of the ring buffer, can this corrupt the
ring pointer tracking by assigning a bogus virtual pointer to ch_ring->rp?

[Severity: High]
This isn't a bug introduced by this patch, but can this code race with channel
termination and cause a use-after-free?

drivers/dma/qcom/gpi.c:gpi_process_xfer_compl_event() {
    ...
gpi_free_desc:
    spin_lock_irqsave(&gchan->vc.lock, flags);
    list_del(&vd->node);
    spin_unlock_irqrestore(&gchan->vc.lock, flags);
    kfree(gpi_desc);
}

The interrupt handler drops gchan->vc.lock earlier after getting the descriptor
via vchan_next_desc(), without removing it from the desc_issued list.

If a concurrent vchan_terminate_all() acquires the lock, removes, and frees the
descriptor, the IRQ handler will then resume, modify the freed descriptor
(dma_cookie_complete), and call list_del(&vd->node) on freed memory.

This also applies to gpi_process_imed_data_event() where the same pattern exists.

[ ... ]

> @@ -1100,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);
>  
>  			switch (type) {
>  			case XFER_COMPLETE_EV_TYPE:
>  				gchan = &gpii->gchan[chid];

[Severity: High]
This isn't a bug introduced by this patch, but is a dma_rmb() missing before
reading the DMA event ring buffer?

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;
    ...
}

Because gpi_read_reg() wraps readl_relaxed(), it provides no ordering guarantees
against normal memory reads. Can the CPU speculatively fetch from the DMA-mapped
event ring before the register read completes and confirms there is new data?

[Severity: High]
This is a pre-existing issue, but does this code use an unvalidated hardware
event chid as an array index?

drivers/dma/qcom/gpi.c:gpi_process_events() {
    ...
    chid = gpi_event->xfer_compl_event.chid;
    type = gpi_event->xfer_compl_event.type;

    trace_gpi_process_event(gpii->gpi_dev->dev, chid, type,
                            gpi_event->gpi_ere.dword);

    switch (type) {
    case XFER_COMPLETE_EV_TYPE:
        gchan = &gpii->gchan[chid];
    ...
}

If the hardware is compromised or fails and sends a chid value greater than or
equal to MAX_CHANNELS_PER_GPII, indexing gpii->gchan[chid] will read out of
bounds. Can this corrupt adjacent memory or crash the kernel?

[ ... ]

> @@ -1121,11 +1104,8 @@ static void gpi_process_events(struct gpii *gpii)
>  							    &gpi_event->immediate_data_event);
>  				break;
>  			case QUP_NOTIF_EV_TYPE:
> -				dev_dbg(gpii->gpi_dev->dev, "QUP_NOTIF_EV_TYPE\n");
>  				break;
>  			default:
> -				dev_dbg(gpii->gpi_dev->dev,
> -					"not supported event type:0x%x\n", type);
>  			}
>  			gpi_ring_recycle_ev_element(ev_ring);
>  		}

[Severity: High]
This is a pre-existing issue, but is a memory barrier missing before ringing
the event doorbell?

drivers/dma/qcom/gpi.c:gpi_process_events() {
    ...
            gpi_ring_recycle_ev_element(ev_ring);
        }
        gpi_write_ev_db(gpii, ev_ring, ev_ring->wp);
    ...
}

The gpi_write_ev_db() wraps writel_relaxed(). The smp_wmb() inside
gpi_ring_recycle_ev_element() only orders normal memory writes, not prior memory
reads against subsequent MMIO writes. Does this mean the hardware might
overwrite the DMA event ring descriptor before the CPU actually finishes reading
it? A full mb() or rmb() is typically required before the doorbell.

[ ... ]

> @@ -1578,7 +1554,7 @@ static int gpi_resume(struct dma_chan *chan)
>  
>  	mutex_lock(&gpii->ctrl_lock);
>  	if (gpii->pm_state == ACTIVE_STATE) {
> -		dev_dbg(gpii->gpi_dev->dev, "channel is already active\n");
> +		trace_gpi_already_state(gpii->gpi_dev->dev, gpii->gpii_id, gpii->pm_state);
>  		mutex_unlock(&gpii->ctrl_lock);
>  		return 0;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leave the
hardware IRQ enabled while the software state remains paused?

drivers/dma/qcom/gpi.c:gpi_resume() {
    ...
    enable_irq(gpii->irq);

    for (i = 0; i < MAX_CHANNELS_PER_GPII; i++) {
        ret = gpi_send_cmd(gpii, &gpii->gchan[i], GPI_CH_CMD_START);
        if (ret) {
            dev_err(gpii->gpi_dev->dev, "Error starting chan, ret:%d\n", ret);
            mutex_unlock(&gpii->ctrl_lock);
            return ret;
        }
    ...
}

If gpi_send_cmd() fails, it returns without disabling the IRQ, and pm_state
remains PAUSE_STATE. On the next resume attempt, will it bypass the active state
check and call enable_irq() a second time, resulting in an unbalanced IRQ enable
and a kernel warning?

[Severity: High]
This is a pre-existing issue, but is a dma_wmb() missing before writing to the
channel doorbell?

drivers/dma/qcom/gpi.c:gpi_issue_pending() {
    for (i = 0; i < gpi_desc->num_tre; i++) {
        tre = &gpi_desc->tre[i];
        gpi_queue_xfer(gpii, gchan, tre, &wp);
    }

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

Descriptors are written to the DMA ring using memcpy() in gpi_queue_xfer(), and
then the doorbell is rung via gpi_write_ch_db(), which wraps writel_relaxed().
Since writel_relaxed() does not order against prior normal memory writes, can
the doorbell write reach the device before the memcpy() is visible in RAM?

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