diff mbox series

[5/6] dma: tegra: add tracepoints to driver

Message ID 20181012094454.22841-6-ben.dooks@codethink.co.uk
State Superseded
Headers show
Series [1/6] dma: tegra: avoid overflow of byte tracking | expand

Commit Message

Ben Dooks Oct. 12, 2018, 9:44 a.m. UTC 
Add some trace-points to the driver to allow for debuging via the
trace pipe.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Cc: Ingo Molnar <mingo@redhat.com> (maintainer:TRACING)
Cc: Steven Rostedt <rostedt@goodmis.org> (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

Comments

Steven Rostedt Oct. 12, 2018, 5:01 p.m. UTC | #1 
On Fri, 12 Oct 2018 10:44:53 +0100
Ben Dooks <ben.dooks@codethink.co.uk> wrote:

> Add some trace-points to the driver to allow for debuging via the
> trace pipe.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> Cc: Ingo Molnar <mingo@redhat.com> (maintainer:TRACING)
> Cc: Steven Rostedt <rostedt@goodmis.org> (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.

-- Steve

> +		      __entry->irq));
> +
> +#endif /*  _TRACE_TEGRADMA_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
Ben Dooks Nov. 7, 2018, 10:02 a.m. UTC | #2 
On 2018-10-12 18:01, Steven Rostedt wrote:
> On Fri, 12 Oct 2018 10:44:53 +0100
> Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> 
>> Add some trace-points to the driver to allow for debuging via the
>> trace pipe.
>> 
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>> Cc: Ingo Molnar <mingo@redhat.com> (maintainer:TRACING)
>> Cc: Steven Rostedt <rostedt@goodmis.org> (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.

thank you for the review, v3 will hopefully have all these fixed.
diff mbox series

Patch

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);
 	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_ARGS(dc, cookie, residue),
+	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;
+	),
+	TP_printk("channel %s: dma cookie %d, residue %u",
+		  dev_name(&__entry->dc->dev->device),
+		  __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),
+		      __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),
+		      __entry->irq));
+
+#endif /*  _TRACE_TEGRADMA_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>