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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] gpi_issue_pending only queues the last submitted descriptor to hardware, abandoning earlier batched descriptors and causing data corruption/hangs.
- [High] The tasklet `gpi_ev_tasklet` acquires a read_lock without disabling interrupts, leading to a 3-way rwlock deadlock with hardirq and writers.
- [High] Tasklet is killed prematurely during channel teardown, allowing an in-flight IRQ to reschedule it after its resources are freed.
--

commit 174e7255a0bfd5a904a8fadea83aa5cb881dc7b3
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

dmaengine: qcom: gpi: Add trace event support

This patch wires up qcom_gpi tracepoints into the GPI DMA engine driver.
It supplements existing debug logs with tracepoints for better runtime
visibility into driver behavior.

> 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: High]
This is a pre-existing issue, but does gpi_ev_tasklet() risk a 3-way rwlock
deadlock with gpi_handle_irq() and writers?

gpi_ev_tasklet() acquires the rwlock in softirq without disabling local
interrupts:

	read_lock(&gpii->pm_lock);

If a writer calls a function like gpi_pause() and takes
write_lock_irq(&gpii->pm_lock), it spins and blocks new readers due to
qrwlock fairness.

If an interrupt then arrives on the tasklet's CPU, gpi_handle_irq() will run:

	read_lock_irqsave(&gpii->pm_lock, flags);

The hardirq spins waiting for the writer, the writer spins waiting for the
tasklet, and the tasklet is preempted by the hardirq, causing a total system
deadlock. Should gpi_ev_tasklet() use read_lock_irqsave() instead?

[Severity: High]
This isn't a bug introduced by this patch, but is the tasklet killed too
early in gpi_free_chan_resources(), allowing it to be rescheduled?

In gpi_free_chan_resources(), the tasklet is killed before disabling
interrupts:

	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);

If a final interrupt arrives (e.g., from the GPI_EV_CMD_DEALLOC command),
gpi_handle_irq() can process it and call tasklet_hi_schedule(&gpii->ev_task).

Meanwhile, gpi_free_chan_resources() frees and zeroes the event ring. The
rescheduled tasklet will then run gpi_process_events(), dereferencing the
zeroed ev_ring->rp pointer and causing a kernel panic. Should interrupts be
disabled before killing the tasklet?

[ ... ]
> @@ -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 isn't a bug introduced by this patch, but does gpi_issue_pending()
handle batched descriptors correctly?

If a client driver batches two or more descriptors using vchan_tx_submit()
before calling dma_async_issue_pending(), vchan_issue_pending() moves all
submitted descriptors (e.g., A and B) to desc_issued.

gpi_issue_pending() fetches only the last submitted descriptor:

	vd = list_last_entry(&gchan->vc.desc_issued,
			     struct virt_dma_desc, node);
	gpi_desc = to_gpi_desc(vd);
	trace_gpi_queue_xfer(gpii->gpi_dev->dev, gchan->chid,
			     gpi_desc->num_tre);

This queues B to the hardware, abandoning A.

When B completes, gpi_process_xfer_compl_event() completes the first
descriptor in the list:

	vd = vchan_next_desc(&gchan->vc);

Thus, A is erroneously reported as completed successfully (though it never
ran), and B remains in the list forever without completion, leading to silent
DMA data corruption and channel hangs. Can this logic be fixed to support
DMA batching properly?

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