From d0b9b08981ebf75bfebe9597991c1751a9fbe71d Mon Sep 17 00:00:00 2001 From: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> Date: Fri, 15 Oct 2021 09:07:13 +0300 Subject: [PATCH 1/4] dma-trace: Simply return with if dma_trace_buffer_init() fails dma_trace_buffer_init() can only fail if the rballoc fails so there is no need to call dma_trace_buffer_free() as the buffer has not been allocated Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> --- src/trace/dma-trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/trace/dma-trace.c b/src/trace/dma-trace.c index 5bd09877d405..495f6c70214f 100644 --- a/src/trace/dma-trace.c +++ b/src/trace/dma-trace.c @@ -412,7 +412,7 @@ int dma_trace_enable(struct dma_trace_data *d) if (err < 0) { mtrace_printf(LOG_LEVEL_ERROR, "dma_trace_enable: buffer_init failed"); - goto out; + return err; } /* It should be the very first sent log for easy identification. */ From af0e962cb4b2abf6dcc0127fb637c4f2d2c53a9a Mon Sep 17 00:00:00 2001 From: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> Date: Fri, 15 Oct 2021 09:16:00 +0300 Subject: [PATCH 2/4] dma-trace: Re-structure the buffer allocation and initialization The current dma_trace_buffer_init() which allocates and initializes the dtrace buffer is re-tasked to simply (re-)initialize the dtrace buffer along with placing the markers at the same function. The allocation of the buffer is split out to dma_trace_buffer_alloc() which calls the dma_trace_buffer_init() to get the buffer initialized. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> --- src/trace/dma-trace.c | 76 ++++++++++++++++++++++++------------------- 1 file changed, 42 insertions(+), 34 deletions(-) diff --git a/src/trace/dma-trace.c b/src/trace/dma-trace.c index 495f6c70214f..cc98df6afd2b 100644 --- a/src/trace/dma-trace.c +++ b/src/trace/dma-trace.c @@ -218,35 +218,58 @@ int dma_trace_host_buffer(struct dma_trace_data *d, } #endif -static int dma_trace_buffer_init(struct dma_trace_data *d) +static void dma_trace_buffer_init(struct dma_trace_data *d, void *new_buffer) { struct dma_trace_buf *buffer = &d->dmatb; - void *buf; unsigned int flags; - /* allocate new buffer */ - buf = rballoc(0, SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA, - DMA_TRACE_LOCAL_SIZE); - if (!buf) { - tr_err(&dt_tr, "dma_trace_buffer_init(): alloc failed"); - return -ENOMEM; - } - - bzero(buf, DMA_TRACE_LOCAL_SIZE); - dcache_writeback_region(buf, DMA_TRACE_LOCAL_SIZE); - - /* initialise the DMA buffer, whole sequence in section */ spin_lock_irq(&d->lock, flags); - buffer->addr = buf; buffer->size = DMA_TRACE_LOCAL_SIZE; + if (new_buffer) { + buffer->addr = new_buffer; + buffer->end_addr = (char *)buffer->addr + buffer->size; + } buffer->w_ptr = buffer->addr; buffer->r_ptr = buffer->addr; - buffer->end_addr = (char *)buffer->addr + buffer->size; buffer->avail = 0; + bzero(buffer->addr, buffer->size); + dcache_writeback_region(buffer->addr, buffer->size); + + d->old_host_offset = 0; + d->posn.host_offset = 0; + spin_unlock_irq(&d->lock, flags); + /* It should be the very first sent log for easy identification. */ + mtrace_printf(LOG_LEVEL_INFO, + "SHM: FW ABI 0x%x DBG ABI 0x%x tag " SOF_GIT_TAG " src hash 0x%08x (ldc hash " + META_QUOTE(SOF_SRC_HASH) ")", + SOF_ABI_VERSION, SOF_ABI_DBG_VERSION, SOF_SRC_HASH); + + /* Use a different, DMA: prefix to ease identification of log files */ + tr_info(&dt_tr, + "DMA: FW ABI 0x%x DBG ABI 0x%x tag " SOF_GIT_TAG " src hash 0x%08x (ldc hash " + META_QUOTE(SOF_SRC_HASH) ")", + SOF_ABI_VERSION, SOF_ABI_DBG_VERSION, SOF_SRC_HASH); +} + +static int dma_trace_buffer_alloc(struct dma_trace_data *d) +{ + void *buf; + + /* allocate new buffer */ + buf = rballoc(0, SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA, + DMA_TRACE_LOCAL_SIZE); + if (!buf) { + mtrace_printf(LOG_LEVEL_ERROR, "dma_trace_buffer_alloc(): alloc failed"); + return -ENOMEM; + } + + /* Initialize the dma trace buffer */ + dma_trace_buffer_init(d, buf); + return 0; } @@ -407,25 +430,10 @@ int dma_trace_enable(struct dma_trace_data *d) { int err; - /* initialize dma trace buffer */ - err = dma_trace_buffer_init(d); - - if (err < 0) { - mtrace_printf(LOG_LEVEL_ERROR, "dma_trace_enable: buffer_init failed"); + /* Allocate and initialize the dma trace buffer */ + err = dma_trace_buffer_alloc(d); + if (err < 0) return err; - } - - /* It should be the very first sent log for easy identification. */ - mtrace_printf(LOG_LEVEL_INFO, - "SHM: FW ABI 0x%x DBG ABI 0x%x tag " SOF_GIT_TAG " src hash 0x%08x (ldc hash " - META_QUOTE(SOF_SRC_HASH) ")", - SOF_ABI_VERSION, SOF_ABI_DBG_VERSION, SOF_SRC_HASH); - - /* Use a different, DMA: prefix to ease identification of log files */ - tr_info(&dt_tr, - "DMA: FW ABI 0x%x DBG ABI 0x%x tag " SOF_GIT_TAG " src hash 0x%08x (ldc hash " - META_QUOTE(SOF_SRC_HASH) ")", - SOF_ABI_VERSION, SOF_ABI_DBG_VERSION, SOF_SRC_HASH); #if CONFIG_DMA_GW /* From 6652cab397d49245cbd62bdad8ea104c6890b2c5 Mon Sep 17 00:00:00 2001 From: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> Date: Fri, 15 Oct 2021 09:19:59 +0300 Subject: [PATCH 3/4] dma-trace: Cancel the dtrace task before stopping the channel in reconfig We should stop the task before the dtrace channel is stopped when we need to re-configure it. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> --- src/trace/dma-trace.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/trace/dma-trace.c b/src/trace/dma-trace.c index cc98df6afd2b..275d141a55c4 100644 --- a/src/trace/dma-trace.c +++ b/src/trace/dma-trace.c @@ -307,6 +307,7 @@ static int dma_trace_start(struct dma_trace_data *d) "dma_trace_start(): DMA reconfiguration (active stream_tag: %u)", d->active_stream_tag); + schedule_task_cancel(&d->dmat_work); err = dma_stop(d->dc.chan); if (err < 0) { mtrace_printf(LOG_LEVEL_ERROR, From 81aff7b8ceca3a2de0079b246f703d0c8219653c Mon Sep 17 00:00:00 2001 From: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> Date: Fri, 15 Oct 2021 09:23:43 +0300 Subject: [PATCH 4/4] dma-trace: Fix memory leak with buffer allocation on dtrace re-config Do not attempt to allocate the dtrace buffer again if it has been already allocated. This only possible if we are re-configuring the dtrace which implies that we also must have the DMA channel. In such case, skip the allocation and do a re-init of the buffer after the DMA channel has been stopped. Reported-by: Keyon Jie <yang.jie@linux.intel.com> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> --- src/trace/dma-trace.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/trace/dma-trace.c b/src/trace/dma-trace.c index 275d141a55c4..7da18d087b80 100644 --- a/src/trace/dma-trace.c +++ b/src/trace/dma-trace.c @@ -259,6 +259,17 @@ static int dma_trace_buffer_alloc(struct dma_trace_data *d) { void *buf; + /* + * Keep the existing dtrace buffer to avoid memory leak, unlikely to + * happen if host correctly using the dma_trace_disable(). + * + * The buffer can not be freed up here as it is likely in use. + * The (re-)initialization will happen in dma_trace_start() when it is + * safe to do (the DMA is stopped) + */ + if (dma_trace_initialized(d)) + return 0; + /* allocate new buffer */ buf = rballoc(0, SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA, DMA_TRACE_LOCAL_SIZE); @@ -322,6 +333,9 @@ static int dma_trace_start(struct dma_trace_data *d) d->dc.chan = NULL; err = dma_copy_set_stream_tag(&d->dc, d->stream_tag); } + + /* Re-initialize the dtrace buffer */ + dma_trace_buffer_init(d, NULL); } else { err = dma_copy_set_stream_tag(&d->dc, d->stream_tag); } @@ -431,7 +445,7 @@ int dma_trace_enable(struct dma_trace_data *d) { int err; - /* Allocate and initialize the dma trace buffer */ + /* Allocate and initialize the dma trace buffer if needed */ err = dma_trace_buffer_alloc(d); if (err < 0) return err;