On Fri, 12 Oct 2018 10:44:53 +0100
Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx> wrote:
Add some trace-points to the driver to allow for debuging via the
trace pipe.
Signed-off-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>
---
Cc: Ingo Molnar <mingo@xxxxxxxxxx> (maintainer:TRACING)
Cc: Steven Rostedt <rostedt@xxxxxxxxxxx> (maintainer:TRACING)
---
drivers/dma/tegra20-apb-dma.c | 8 ++++
include/trace/events/tegra_apb_dma.h | 63
++++++++++++++++++++++++++++
2 files changed, 71 insertions(+)
create mode 100644 include/trace/events/tegra_apb_dma.h
diff --git a/drivers/dma/tegra20-apb-dma.c
b/drivers/dma/tegra20-apb-dma.c
index ce2888f67254..96095a3b7edd 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -38,6 +38,9 @@
#include "dmaengine.h"
+#define CREATE_TRACE_POINTS
+#include <trace/events/tegra_apb_dma.h>
+
#define TEGRA_APBDMA_GENERAL 0x0
#define TEGRA_APBDMA_GENERAL_ENABLE BIT(31)
@@ -672,6 +675,8 @@ static void tegra_dma_tasklet(unsigned long data)
dmaengine_desc_get_callback(&dma_desc->txd, &cb);
cb_count = dma_desc->cb_count;
dma_desc->cb_count = 0;
+ trace_tegra_dma_complete_cb(&tdc->dma_chan, cb_count,
+ cb.callback);
spin_unlock_irqrestore(&tdc->lock, flags);
while (cb_count--)
dmaengine_desc_callback_invoke(&cb, NULL);
@@ -688,6 +693,7 @@ static irqreturn_t tegra_dma_isr(int irq, void
*dev_id)
spin_lock_irqsave(&tdc->lock, flags);
+ trace_tegra_dma_isr(&tdc->dma_chan, irq);
status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
@@ -931,6 +937,8 @@ static enum dma_status tegra_dma_tx_status(struct
dma_chan *dc,
dma_set_residue(txstate, residual);
}
+ trace_tegra_dma_tx_status(&tdc->dma_chan, cookie,
+ txstate ? txstate->residue : -1);
Why just pass in txstate and put that logic into the trace event code?
See below.
spin_unlock_irqrestore(&tdc->lock, flags);
return ret;
}
diff --git a/include/trace/events/tegra_apb_dma.h
b/include/trace/events/tegra_apb_dma.h
new file mode 100644
index 000000000000..80d6f0cf4c36
--- /dev/null
+++ b/include/trace/events/tegra_apb_dma.h
@@ -0,0 +1,63 @@
+#if !defined(_TRACE_TEGRA_APB_DMA_H) ||
defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_TEGRA_APM_DMA_H
+
+#include <linux/tracepoint.h>
+#include <linux/dmaengine.h>
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM tegra_apb_dma
+
+TRACE_EVENT(tegra_dma_tx_status,
+ TP_PROTO(struct dma_chan *dc, s32 cookie, u32 residue),
TP_PROTO(struct dma_chan *dc, s32 cookie,
struct dma_tx_state *txstate),
+ TP_ARGS(dc, cookie, residue),
TP_ARGS(dc, cookie, txstate),
+ TP_STRUCT__entry(
+ __field(struct dma_chan *, dc)
+ __field(__s32, cookie)
+ __field(__u32, residue)
+ ),
+ TP_fast_assign(
+ __entry->dc = dc;
+ __entry->cookie = cookie;
+ __entry->residue = residue;
__entry->residue = txstate ? txstate->residue : -1;
+ ),
+ TP_printk("channel %s: dma cookie %d, residue %u",
+ dev_name(&__entry->dc->dev->device),
The dev_name must be done in the TP_fast_assign part (use __string).
What you have here can crash the system. That is, you saved the dc
pointer into the ring buffer. Now that dc pointer may be freed, and
then when you read the ring buffer, we are now dereferencing the stale
and freed dc pointer and BOOM!
+ __entry->cookie, __entry->residue)
+);
+
+TRACE_EVENT(tegra_dma_complete_cb,
+ TP_PROTO(struct dma_chan *dc, int count, void *ptr),
+ TP_ARGS(dc, count, ptr),
+ TP_STRUCT__entry(
+ __field(struct dma_chan *, dc)
+ __field(int, count)
+ __field(void *, ptr)
+ ),
+ TP_fast_assign(
+ __entry->dc = dc;
+ __entry->count = count;
+ __entry->ptr = ptr;
+ ),
+ TP_printk("channel %s: done %d, ptr %p",
+ dev_name(&__entry->dc->dev->device),
Same here.
+ __entry->count, __entry->ptr)
+);
+
+TRACE_EVENT(tegra_dma_isr,
+ TP_PROTO(struct dma_chan *dc, int irq),
+ TP_ARGS(dc, irq),
+ TP_STRUCT__entry(
+ __field(struct dma_chan *, dc)
+ __field(int, irq)
+ ),
+ TP_fast_assign(
+ __entry->dc = dc;
+ __entry->irq = irq;
+ ),
+ TP_printk("%s: irq %d\n", dev_name(&__entry->dc->dev->device),
And here.
