Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dma-trace: Fixes aimed to make the re-configuration robust and cleaner #4879

Closed
wants to merge 4 commits into from
Closed

dma-trace: Fixes aimed to make the re-configuration robust and cleaner #4879

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d0b9b08
dma-trace: Simply return with if dma_trace_buffer_init() fails
ujfalusi Oct 15, 2021
af0e962
dma-trace: Re-structure the buffer allocation and initialization
ujfalusi Oct 15, 2021
6652cab
dma-trace: Cancel the dtrace task before stopping the channel in reco…
ujfalusi Oct 15, 2021
81aff7b
dma-trace: Fix memory leak with buffer allocation on dtrace re-config
ujfalusi Oct 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 58 additions & 35 deletions src/trace/dma-trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,35 +218,69 @@ 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;

/*
* 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);
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;
}

Expand Down Expand Up @@ -284,6 +318,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,
Expand All @@ -298,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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how can a memory leak happen? Is start() called without a stop(), because the latter does free the buffer? Is stop() never called in such a case, just a repeated start()? Maybe worth mentioning this in the commit message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh, this PR kind of precedes the stop() from @ranj063.
But if you have old kernel that will not going to call it.
I think this is more like a safety net for the sake of completeness?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record: it is SOF_IPC_TRACE_DMA_FREE and the dma_trace_disable() for the trace stop which if it is called then stops/put the DMA channel and free up the buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me check w/o this series tomorrow against the SOF clients version. I might be able just close this PR.

} else {
err = dma_copy_set_stream_tag(&d->dc, d->stream_tag);
}
Expand Down Expand Up @@ -407,25 +445,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");
goto out;
}

/* 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);
/* Allocate and initialize the dma trace buffer if needed */
err = dma_trace_buffer_alloc(d);
if (err < 0)
return err;

#if CONFIG_DMA_GW
/*
Expand Down