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

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

[Praveen Talari]

This series adds tracepoint support to the Qualcomm GPI DMA engine
driver, giving structured runtime visibility into GPI DMA behavior
without requiring invasive debug patches.

Patch 1 adds the qcom_gpi trace events header, covering command
dispatch, IRQ handling, channel/event control state transitions,
completion event processing, ring allocation, and TRE contents.

Patch 2 wires these tracepoints into the driver alongside the
existing dev_dbg() calls.

Patch 3 converts the remaining dev_dbg() call sites in the driver to
use the new tracepoints instead.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
Praveen Talari (3):
      dmaengine: qcom: gpi: trace: Add trace events header for Qualcomm GPI DMA
      dmaengine: qcom: gpi: Add trace event support
      dmaengine: qcom: gpi: Convert dev_dbg() calls to tracepoints

 drivers/dma/qcom/gpi.c          |  69 ++++-----
 include/trace/events/qcom_gpi.h | 313 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 340 insertions(+), 42 deletions(-)
---
base-commit: e30626823a406725ce29bc75cb8ec467d3e1e326
change-id: 20260831-add-trace-support-gpio-99eb54d42729

Best regards,
--  
Praveen Talari <praveen.talari@oss.qualcomm.com>

[PATCH 1/3] dmaengine: qcom: gpi: trace: Add trace events header for Qualcomm GPI DMA

[Praveen Talari]

Add a tracepoint header for the Qualcomm GPI DMA engine driver.

The events defined here cover channel/event command dispatch, IRQ
type handling, channel/event control state transitions, error IRQ
status, completion event processing (including the no-pending-
descriptor and transfer result paths), per-event processing in the
event ring, ring allocation/programming details, already-in-state
checks for pause/resume, and TRE queuing and contents. Together they
give full runtime visibility into GPI DMA behavior in place of the
driver's former dev_dbg() based debug logging.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 include/trace/events/qcom_gpi.h | 313 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 313 insertions(+)

diff --git a/include/trace/events/qcom_gpi.h b/include/trace/events/qcom_gpi.h
new file mode 100644
index 000000000000..5474e762f4b6
--- /dev/null
+++ b/include/trace/events/qcom_gpi.h
@@ -0,0 +1,313 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM qcom_gpi
+
+#if !defined(_TRACE_QCOM_GPI_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_QCOM_GPI_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(gpi_send_cmd,
+	    TP_PROTO(struct device *dev, u32 chid, u32 cmd, const char *cmd_str),
+	    TP_ARGS(dev, chid, cmd, cmd_str),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __string(cmd_str, cmd_str)
+			     __field(u32, chid)
+			     __field(u32, cmd)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __assign_str(cmd_str);
+			   __entry->chid = chid;
+			   __entry->cmd = cmd;
+	    ),
+
+	    TP_printk("%s: chid=%u cmd=%s(%u)",
+		      __get_str(name), __entry->chid, __get_str(cmd_str),
+		      __entry->cmd)
+);
+
+TRACE_EVENT(gpi_irq_status,
+	    TP_PROTO(struct device *dev, u32 gpii_id, u32 irq_type),
+	    TP_ARGS(dev, gpii_id, irq_type),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u32, gpii_id)
+			     __field(u32, irq_type)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->gpii_id = gpii_id;
+			   __entry->irq_type = irq_type;
+	    ),
+
+	    TP_printk("%s: gpii=%u irq_type=0x%08x",
+		      __get_str(name), __entry->gpii_id, __entry->irq_type)
+);
+
+TRACE_EVENT(gpi_ch_ctrl_irq,
+	    TP_PROTO(struct device *dev, u32 chid, u32 ch_state),
+	    TP_ARGS(dev, chid, ch_state),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u32, chid)
+			     __field(u32, ch_state)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->chid = chid;
+			   __entry->ch_state = ch_state;
+	    ),
+
+	    TP_printk("%s: chid=%u ch_state=%u",
+		      __get_str(name), __entry->chid, __entry->ch_state)
+);
+
+TRACE_EVENT(gpi_ev_process,
+	    TP_PROTO(struct device *dev, u32 chid, u32 ev_type, u8 code,
+		     u16 status, u32 length),
+	    TP_ARGS(dev, chid, ev_type, code, status, length),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u32, chid)
+			     __field(u32, ev_type)
+			     __field(u8, code)
+			     __field(u16, status)
+			     __field(u32, length)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->chid = chid;
+			   __entry->ev_type = ev_type;
+			   __entry->code = code;
+			   __entry->status = status;
+			   __entry->length = length;
+	    ),
+
+	    TP_printk("%s: chid=%u ev_type=0x%02x code=%u status=%u length=%u",
+		      __get_str(name), __entry->chid, __entry->ev_type,
+		      __entry->code, __entry->status, __entry->length)
+);
+
+TRACE_EVENT(gpi_queue_xfer,
+	    TP_PROTO(struct device *dev, u32 chid, u32 num_tre),
+	    TP_ARGS(dev, chid, num_tre),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u32, chid)
+			     __field(u32, num_tre)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->chid = chid;
+			   __entry->num_tre = num_tre;
+	    ),
+
+	    TP_printk("%s: chid=%u num_tre=%u",
+		      __get_str(name), __entry->chid, __entry->num_tre)
+);
+
+TRACE_EVENT(gpi_gen_err_irq,
+	    TP_PROTO(struct device *dev, u32 gpii_id, u32 irq_stts),
+	    TP_ARGS(dev, gpii_id, irq_stts),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u32, gpii_id)
+			     __field(u32, irq_stts)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->gpii_id = gpii_id;
+			   __entry->irq_stts = irq_stts;
+	    ),
+
+	    TP_printk("%s: gpii=%u irq_stts=0x%08x",
+		      __get_str(name), __entry->gpii_id, __entry->irq_stts)
+);
+
+TRACE_EVENT(gpi_ev_ctrl_irq,
+	    TP_PROTO(struct device *dev, u32 gpii_id, u32 ev_ch_irq, u32 ev_state),
+	    TP_ARGS(dev, gpii_id, ev_ch_irq, ev_state),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u32, gpii_id)
+			     __field(u32, ev_ch_irq)
+			     __field(u32, ev_state)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->gpii_id = gpii_id;
+			   __entry->ev_ch_irq = ev_ch_irq;
+			   __entry->ev_state = ev_state;
+	    ),
+
+	    TP_printk("%s: gpii=%u ev_ch_irq=0x%08x ev_state=%u",
+		      __get_str(name), __entry->gpii_id, __entry->ev_ch_irq,
+		      __entry->ev_state)
+);
+
+TRACE_EVENT(gpi_ev_no_desc,
+	    TP_PROTO(struct device *dev, u32 chid, const u32 *ev_dword,
+		     const u32 *tre_dword),
+	    TP_ARGS(dev, chid, ev_dword, tre_dword),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u32, chid)
+			     __array(u32, ev_dword, 4)
+			     __array(u32, tre_dword, 4)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->chid = chid;
+			   memcpy(__entry->ev_dword, ev_dword, sizeof(__entry->ev_dword));
+			   memcpy(__entry->tre_dword, tre_dword, sizeof(__entry->tre_dword));
+	    ),
+
+	    TP_printk("%s: chid=%u event=%08x:%08x:%08x:%08x pending_tre=%08x:%08x:%08x:%08x",
+		      __get_str(name), __entry->chid,
+		      __entry->ev_dword[0], __entry->ev_dword[1],
+		      __entry->ev_dword[2], __entry->ev_dword[3],
+		      __entry->tre_dword[0], __entry->tre_dword[1],
+		      __entry->tre_dword[2], __entry->tre_dword[3])
+);
+
+TRACE_EVENT(gpi_xfer_result,
+	    TP_PROTO(struct device *dev, u32 chid, int result, u32 residue),
+	    TP_ARGS(dev, chid, result, residue),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u32, chid)
+			     __field(int, result)
+			     __field(u32, residue)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->chid = chid;
+			   __entry->result = result;
+			   __entry->residue = residue;
+	    ),
+
+	    TP_printk("%s: chid=%u result=%d residue=%u",
+		      __get_str(name), __entry->chid, __entry->result,
+		      __entry->residue)
+);
+
+TRACE_EVENT(gpi_process_event,
+	    TP_PROTO(struct device *dev, u32 chid, u32 type, const u32 *dword),
+	    TP_ARGS(dev, chid, type, dword),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u32, chid)
+			     __field(u32, type)
+			     __array(u32, dword, 4)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->chid = chid;
+			   __entry->type = type;
+			   memcpy(__entry->dword, dword, sizeof(__entry->dword));
+	    ),
+
+	    TP_printk("%s: chid=%u type=0x%02x %08x:%08x:%08x:%08x",
+		      __get_str(name), __entry->chid, __entry->type,
+		      __entry->dword[0], __entry->dword[1],
+		      __entry->dword[2], __entry->dword[3])
+);
+
+TRACE_EVENT(gpi_alloc_ring,
+	    TP_PROTO(struct device *dev, u32 elements, u32 el_size,
+		     u32 req_len, u64 len, size_t alloc_size),
+	    TP_ARGS(dev, elements, el_size, req_len, len, alloc_size),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u32, elements)
+			     __field(u32, el_size)
+			     __field(u32, req_len)
+			     __field(u64, len)
+			     __field(size_t, alloc_size)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->elements = elements;
+			   __entry->el_size = el_size;
+			   __entry->req_len = req_len;
+			   __entry->len = len;
+			   __entry->alloc_size = alloc_size;
+	    ),
+
+	    TP_printk("%s: elements=%u el_size=%u req_len=%u len=%llu alloc_size=%zu",
+		      __get_str(name), __entry->elements, __entry->el_size,
+		      __entry->req_len, __entry->len, __entry->alloc_size)
+);
+
+TRACE_EVENT(gpi_ring_info,
+	    TP_PROTO(struct device *dev, dma_addr_t dma_handle, phys_addr_t phys_addr,
+		     u32 len, u32 el_size, u32 elements),
+	    TP_ARGS(dev, dma_handle, phys_addr, len, el_size, elements),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u64, dma_handle)
+			     __field(u64, phys_addr)
+			     __field(u32, len)
+			     __field(u32, el_size)
+			     __field(u32, elements)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->dma_handle = dma_handle;
+			   __entry->phys_addr = phys_addr;
+			   __entry->len = len;
+			   __entry->el_size = el_size;
+			   __entry->elements = elements;
+	    ),
+
+	    TP_printk("%s: dma_handle=%llx phys_addr=%llx len=%u el_size=%u elements=%u",
+		      __get_str(name), __entry->dma_handle, __entry->phys_addr,
+		      __entry->len, __entry->el_size, __entry->elements)
+);
+
+TRACE_EVENT(gpi_already_state,
+	    TP_PROTO(struct device *dev, u32 gpii_id, u32 pm_state),
+	    TP_ARGS(dev, gpii_id, pm_state),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u32, gpii_id)
+			     __field(u32, pm_state)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->gpii_id = gpii_id;
+			   __entry->pm_state = pm_state;
+	    ),
+
+	    TP_printk("%s: gpii=%u already in pm_state=%u",
+		      __get_str(name), __entry->gpii_id, __entry->pm_state)
+);
+
+TRACE_EVENT(gpi_tre,
+	    TP_PROTO(struct device *dev, u32 chid, u32 idx, const u32 *dword),
+	    TP_ARGS(dev, chid, idx, dword),
+
+	    TP_STRUCT__entry(__string(name, dev_name(dev))
+			     __field(u32, chid)
+			     __field(u32, idx)
+			     __array(u32, dword, 4)
+	    ),
+
+	    TP_fast_assign(__assign_str(name);
+			   __entry->chid = chid;
+			   __entry->idx = idx;
+			   memcpy(__entry->dword, dword, sizeof(__entry->dword));
+	    ),
+
+	    TP_printk("%s: chid=%u tre[%u]=%08x:%08x:%08x:%08x",
+		      __get_str(name), __entry->chid, __entry->idx,
+		      __entry->dword[0], __entry->dword[1],
+		      __entry->dword[2], __entry->dword[3])
+);
+
+#endif /* _TRACE_QCOM_GPI_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

-- 
2.34.1

Re: [PATCH 1/3] dmaengine: qcom: gpi: trace: Add trace events header for Qualcomm GPI DMA

[Steven Rostedt]

On Mon, 31 Aug 2026 22:06:35 +0530
Praveen Talari <praveen.talari@oss.qualcomm.com> wrote:


> +TRACE_EVENT(gpi_ev_process,
> +	    TP_PROTO(struct device *dev, u32 chid, u32 ev_type, u8 code,
> +		     u16 status, u32 length),
> +	    TP_ARGS(dev, chid, ev_type, code, status, length),
> +
> +	    TP_STRUCT__entry(__string(name, dev_name(dev))
> +			     __field(u32, chid)
> +			     __field(u32, ev_type)
> +			     __field(u8, code)
> +			     __field(u16, status)
> +			     __field(u32, length)

I would swap the above a little to have the u8 code at the end to remove
any holes. It may have a hole at the end of the event, but you don't want
holes in the middle.

			     __field(u32, chid)
			     __field(u32, ev_type)
			     __field(u32, length)
			     __field(u16, status)
			     __field(u8, code)



> +	    ),
> +
> +	    TP_fast_assign(__assign_str(name);
> +			   __entry->chid = chid;
> +			   __entry->ev_type = ev_type;
> +			   __entry->code = code;
> +			   __entry->status = status;
> +			   __entry->length = length;
> +	    ),
> +
> +	    TP_printk("%s: chid=%u ev_type=0x%02x code=%u status=%u length=%u",
> +		      __get_str(name), __entry->chid, __entry->ev_type,
> +		      __entry->code, __entry->status, __entry->length)
> +);
> +
>


> +TRACE_EVENT(gpi_alloc_ring,
> +	    TP_PROTO(struct device *dev, u32 elements, u32 el_size,
> +		     u32 req_len, u64 len, size_t alloc_size),
> +	    TP_ARGS(dev, elements, el_size, req_len, len, alloc_size),
> +
> +	    TP_STRUCT__entry(__string(name, dev_name(dev))
> +			     __field(u32, elements)
> +			     __field(u32, el_size)
> +			     __field(u32, req_len)
> +			     __field(u64, len)
> +			     __field(size_t, alloc_size)

Same here. I would move the 64 and size to the beginning and the other 4
byte fields to the end:

	    TP_STRUCT__entry(__field(u64, len)
			     __field(size_t, alloc_size)
			     __string(name, dev_name(dev))
			     __field(u32, elements)
			     __field(u32, el_size)
			     __field(u32, req_len)


And yes, the __string() is a 4 byte meta-data and should be treated as 4
bytes.

> +	    ),
> +
> +	    TP_fast_assign(__assign_str(name);
> +			   __entry->elements = elements;
> +			   __entry->el_size = el_size;
> +			   __entry->req_len = req_len;
> +			   __entry->len = len;
> +			   __entry->alloc_size = alloc_size;
> +	    ),
> +
> +	    TP_printk("%s: elements=%u el_size=%u req_len=%u len=%llu alloc_size=%zu",
> +		      __get_str(name), __entry->elements, __entry->el_size,
> +		      __entry->req_len, __entry->len, __entry->alloc_size)
> +);
> +
> +TRACE_EVENT(gpi_ring_info,
> +	    TP_PROTO(struct device *dev, dma_addr_t dma_handle, phys_addr_t phys_addr,
> +		     u32 len, u32 el_size, u32 elements),
> +	    TP_ARGS(dev, dma_handle, phys_addr, len, el_size, elements),
> +
> +	    TP_STRUCT__entry(__string(name, dev_name(dev))

Move the __string() to after phys_addr.

> +			     __field(u64, dma_handle)
> +			     __field(u64, phys_addr)
> +			     __field(u32, len)
> +			     __field(u32, el_size)
> +			     __field(u32, elements)
> +	    ),
> +
> +	    TP_fast_assign(__assign_str(name);
> +			   __entry->dma_handle = dma_handle;
> +			   __entry->phys_addr = phys_addr;
> +			   __entry->len = len;
> +			   __entry->el_size = el_size;
> +			   __entry->elements = elements;
> +	    ),
> +
> +	    TP_printk("%s: dma_handle=%llx phys_addr=%llx len=%u el_size=%u elements=%u",
> +		      __get_str(name), __entry->dma_handle, __entry->phys_addr,
> +		      __entry->len, __entry->el_size, __entry->elements)
> +);
> +

-- Steve

Re: [PATCH 1/3] dmaengine: qcom: gpi: trace: Add trace events header for Qualcomm GPI DMA

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Tracepoint names violate the `subsystem_event` naming convention.
--

commit 247e0c64dd13ba460fb185bdf6a4727ca8e574bd
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

dmaengine: qcom: gpi: trace: Add trace events header for Qualcomm GPI DMA

This commit adds a tracepoint header for the Qualcomm GPI DMA engine driver.
The events provide runtime visibility into the driver's behavior, replacing
former debug logging.

> diff --git a/include/trace/events/qcom_gpi.h b/include/trace/events/qcom_gpi.h
> new file mode 100644
> index 0000000000000..5474e762f4b64
> --- /dev/null
> +++ b/include/trace/events/qcom_gpi.h
> @@ -0,0 +1,313 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM qcom_gpi
> +
> +#if !defined(_TRACE_QCOM_GPI_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_QCOM_GPI_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(gpi_send_cmd,

[Severity: Low]
Should this tracepoint name be prefixed with the subsystem name to match the
subsystem_event naming convention?

Since TRACE_SYSTEM is defined as qcom_gpi above, defining the event as
gpi_send_cmd instead of qcom_gpi_send_cmd creates namespace inconsistencies in
tracing tools and debugfs. This naming pattern appears to be repeated for the
other TRACE_EVENT definitions in this file.

[ ... ]

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

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

[Praveen Talari]

Wire up the qcom_gpi tracepoints into the GPI DMA engine driver to
provide runtime visibility into driver behavior without requiring
invasive debug patches.

Tracepoints are added at the same points as their existing dev_dbg()
counterparts: channel/event command dispatch in gpi_send_cmd(),
top-level interrupt type in gpi_handle_irq(), channel state
transitions in gpi_process_ch_ctrl_irq(), completion event handling
in gpi_process_xfer_compl_event() and gpi_process_imed_data_event(),
and TRE queuing in gpi_issue_pending().

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/dma/qcom/gpi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index a5055a6273af..b09354a73b46 100644
--- a/drivers/dma/qcom/gpi.c
+++ b/drivers/dma/qcom/gpi.c
@@ -17,6 +17,9 @@
 #include "../dmaengine.h"
 #include "../virt-dma.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/qcom_gpi.h>
+
 #define TRE_TYPE_DMA		0x10
 #define TRE_TYPE_IMMEDIATE_DMA	0x11
 #define TRE_TYPE_GO		0x20
@@ -683,6 +686,7 @@ static int gpi_send_cmd(struct gpii *gpii, struct gchan *gchan,
 
 	dev_dbg(gpii->gpi_dev->dev,
 		"sending cmd: %s:%u\n", TO_GPI_CMD_STR(gpi_cmd), chid);
+	trace_gpi_send_cmd(gpii->gpi_dev->dev, chid, gpi_cmd, TO_GPI_CMD_STR(gpi_cmd));
 
 	/* send opcode and wait for completion */
 	reinit_completion(&gpii->cmd_completion);
@@ -773,6 +777,7 @@ static void gpi_process_ch_ctrl_irq(struct gpii *gpii)
 		if (gpii->gpi_cmd == GPI_CH_CMD_DE_ALLOC)
 			state = DEFAULT_CH_STATE;
 		gchan->ch_state = state;
+		trace_gpi_ch_ctrl_irq(gpii->gpi_dev->dev, chid, gchan->ch_state);
 
 		/*
 		 * Triggering complete all if ch_state is not a stop in process.
@@ -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 */
@@ -968,6 +974,8 @@ static void gpi_process_imed_data_event(struct gchan *gchan,
 	smp_wmb();
 
 	chid = imed_event->chid;
+	trace_gpi_ev_process(gpii->gpi_dev->dev, chid, IMMEDIATE_DATA_EV_TYPE,
+			     imed_event->code, imed_event->status, imed_event->length);
 	if (imed_event->code == MSM_GPI_TCE_EOT && gpii->ieob_set) {
 		if (chid == GPI_RX_CHAN)
 			goto gpi_free_desc;
@@ -1043,6 +1051,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;
@@ -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);

-- 
2.34.1

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

[sashiko-bot]

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

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

[Praveen Talari]

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.

Tracepoints are added at the same points as the dev_dbg() calls they
replace: channel/event command dispatch in gpi_send_cmd(), error and
top-level interrupt status in gpi_process_gen_err_irq() and
gpi_handle_irq(), event/channel control state transitions in the IRQ
handler and gpi_process_ch_ctrl_irq(), completion event handling
(including the no-pending-descriptor and transfer result paths) in
gpi_process_imed_data_event(), gpi_process_xfer_compl_event() and
gpi_process_events(), ring allocation details in gpi_alloc_ring(),
already-in-state checks in gpi_pause()/gpi_resume(), TRE contents in
gpi_create_i2c_tre()/gpi_create_spi_tre(), and TRE queuing in
gpi_issue_pending().

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/dma/qcom/gpi.c | 58 ++++++++++++++------------------------------------
 1 file changed, 16 insertions(+), 42 deletions(-)

diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index b09354a73b46..8e7d25a46147 100644
--- a/drivers/dma/qcom/gpi.c
+++ b/drivers/dma/qcom/gpi.c
@@ -684,8 +684,6 @@ static int gpi_send_cmd(struct gpii *gpii, struct gchan *gchan,
 	if (IS_CHAN_CMD(gpi_cmd))
 		chid = gchan->chid;
 
-	dev_dbg(gpii->gpi_dev->dev,
-		"sending cmd: %s:%u\n", TO_GPI_CMD_STR(gpi_cmd), chid);
 	trace_gpi_send_cmd(gpii->gpi_dev->dev, chid, gpi_cmd, TO_GPI_CMD_STR(gpi_cmd));
 
 	/* send opcode and wait for completion */
@@ -797,7 +795,7 @@ static void gpi_process_gen_err_irq(struct gpii *gpii)
 	u32 irq_stts = gpi_read_reg(gpii, gpii->regs + offset);
 
 	/* clear the status */
-	dev_dbg(gpii->gpi_dev->dev, "irq_stts:0x%x\n", irq_stts);
+	trace_gpi_gen_err_irq(gpii->gpi_dev->dev, gpii_id, irq_stts);
 
 	/* Clear the register */
 	offset = GPII_n_CNTXT_GPII_IRQ_CLR_OFFS(gpii_id);
@@ -866,8 +864,6 @@ static irqreturn_t gpi_handle_irq(int irq, void *data)
 			u32 ev_state;
 			u32 ev_ch_irq;
 
-			dev_dbg(gpii->gpi_dev->dev,
-				"processing EV CTRL interrupt\n");
 			offset = GPII_n_CNTXT_SRC_EV_CH_IRQ_OFFS(gpii_id);
 			ev_ch_irq = gpi_read_reg(gpii, gpii->regs + offset);
 
@@ -887,15 +883,14 @@ static irqreturn_t gpi_handle_irq(int irq, void *data)
 				ev_state = DEFAULT_EV_CH_STATE;
 
 			gpii->ev_state = ev_state;
-			dev_dbg(gpii->gpi_dev->dev, "setting EV state to %s\n",
-				TO_GPI_EV_STATE_STR(gpii->ev_state));
+			trace_gpi_ev_ctrl_irq(gpii->gpi_dev->dev, gpii_id, ev_ch_irq,
+					      gpii->ev_state);
 			complete_all(&gpii->cmd_completion);
 			type &= ~(GPII_n_CNTXT_TYPE_IRQ_MSK_EV_CTRL);
 		}
 
 		/* channel control irq */
 		if (type & GPII_n_CNTXT_TYPE_IRQ_MSK_CH_CTRL) {
-			dev_dbg(gpii->gpi_dev->dev, "process CH CTRL interrupts\n");
 			gpi_process_ch_ctrl_irq(gpii);
 			type &= ~(GPII_n_CNTXT_TYPE_IRQ_MSK_CH_CTRL);
 		}
@@ -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;
 	}
 	gpi_desc = to_gpi_desc(vd);
@@ -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);
@@ -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:
@@ -1113,7 +1097,6 @@ static void gpi_process_events(struct gpii *gpii)
 							     &gpi_event->xfer_compl_event);
 				break;
 			case STALE_EV_TYPE:
-				dev_dbg(gpii->gpi_dev->dev, "stale event, not processing\n");
 				break;
 			case IMMEDIATE_DATA_EV_TYPE:
 				gchan = &gpii->gchan[chid];
@@ -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);
 		}
@@ -1409,10 +1389,8 @@ static int gpi_alloc_ring(struct gpi_ring *ring, u32 elements,
 		bit++;
 	len = 1 << bit;
 	ring->alloc_size = (len + (len - 1));
-	dev_dbg(gpii->gpi_dev->dev,
-		"#el:%u el_size:%u len:%u actual_len:%llu alloc_size:%zu\n",
-		  elements, el_size, (elements * el_size), len,
-		  ring->alloc_size);
+	trace_gpi_alloc_ring(gpii->gpi_dev->dev, elements, el_size,
+			     (elements * el_size), len, ring->alloc_size);
 
 	ring->pre_aligned = dma_alloc_coherent(gpii->gpi_dev->dev,
 					       ring->alloc_size,
@@ -1437,10 +1415,8 @@ static int gpi_alloc_ring(struct gpi_ring *ring, u32 elements,
 	/* update to other cores */
 	smp_wmb();
 
-	dev_dbg(gpii->gpi_dev->dev,
-		"phy_pre:%pad phy_alig:%pa len:%u el_size:%u elements:%u\n",
-		&ring->dma_handle, &ring->phys_addr, ring->len,
-		ring->el_size, ring->elements);
+	trace_gpi_ring_info(gpii->gpi_dev->dev, ring->dma_handle, ring->phys_addr,
+			    ring->len, ring->el_size, ring->elements);
 
 	return 0;
 }
@@ -1542,7 +1518,7 @@ static int gpi_pause(struct dma_chan *chan)
 	 * client needs to call pause only once
 	 */
 	if (gpii->pm_state == PAUSE_STATE) {
-		dev_dbg(gpii->gpi_dev->dev, "channel is already paused\n");
+		trace_gpi_already_state(gpii->gpi_dev->dev, gpii->gpii_id, gpii->pm_state);
 		mutex_unlock(&gpii->ctrl_lock);
 		return 0;
 	}
@@ -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;
 	}
@@ -1703,8 +1679,7 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
 	}
 
 	for (i = 0; i < tre_idx; i++)
-		dev_dbg(dev, "TRE:%d %x:%x:%x:%x\n", i, desc->tre[i].dword[0],
-			desc->tre[i].dword[1], desc->tre[i].dword[2], desc->tre[i].dword[3]);
+		trace_gpi_tre(dev, chan->chid, i, desc->tre[i].dword);
 
 	return tre_idx;
 }
@@ -1797,8 +1772,7 @@ static int gpi_create_spi_tre(struct gchan *chan, struct gpi_desc *desc,
 					 TRE_FLAGS_IEOT);
 
 	for (i = 0; i < tre_idx; i++)
-		dev_dbg(dev, "TRE:%d %x:%x:%x:%x\n", i, desc->tre[i].dword[0],
-			desc->tre[i].dword[1], desc->tre[i].dword[2], desc->tre[i].dword[3]);
+		trace_gpi_tre(dev, chan->chid, i, desc->tre[i].dword);
 
 	return tre_idx;
 }

-- 
2.34.1

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

[sashiko-bot]

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