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 <[email protected]> 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-7b0fd0d0d...@oss.qualcomm.com?part=2
