Skip to content

Revert trace enable commits, fix DEBUG_TRACE_PTR() macro so it can be used early #4760

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

Merged
lgirdwood merged 3 commits into from
Sep 15, 2021
Merged

Revert trace enable commits, fix DEBUG_TRACE_PTR() macro so it can be used early #4760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8b61686
alloc.c: fix DEBUG_TRACE_PTR() not to trace before trace is initialized
marc-hb Sep 14, 2021
583f852
Revert "trace: enable trace after it is ready"
marc-hb Sep 13, 2021
878bd7f
Revert "dma-trace: add check to avoid dereference from NULL"
marc-hb Sep 13, 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
5 changes: 2 additions & 3 deletions src/init/init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,10 @@ static int primary_core_init(int argc, char *argv[], struct sof *sof)
interrupt_init(sof);
#endif /* __ZEPHYR__ */

#if CONFIG_TRACE
trace_point(TRACE_BOOT_SYS_TRACES);
trace_init(sof);
#endif

trace_point(TRACE_BOOT_SYS_NOTIFIER);
init_system_notify(sof);
Expand All @@ -149,9 +151,6 @@ static int primary_core_init(int argc, char *argv[], struct sof *sof)

trace_point(TRACE_BOOT_PLATFORM);

/* now start the trace */
trace_on();

#if CONFIG_NO_SECONDARY_CORE_ROM
lp_sram_unpack();
#endif
Expand Down
8 changes: 0 additions & 8 deletions src/ipc/dma-copy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,6 @@ int dma_copy_to_host_nowait(struct dma_copy *dc, struct dma_sg_config *host_sg,
{
int ret;

/* return if DMA channel is not get yet */
if (!dc->chan)
return -EINVAL;

/* tell gateway to copy */
ret = dma_copy(dc->chan, size, 0);
if (ret < 0)
Expand All @@ -89,10 +85,6 @@ int dma_copy_to_host_nowait(struct dma_copy *dc, struct dma_sg_config *host_sg,
int32_t err;
int32_t offset = host_offset;

/* return if DMA channel is not get yet */
if (!dc->chan)
return -EINVAL;

if (size <= 0)
return 0;

Expand Down
6 changes: 3 additions & 3 deletions src/lib/alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,13 +646,13 @@ static void alloc_trace_heap(enum mem_zone zone, uint32_t caps, size_t bytes)
}

#define DEBUG_TRACE_PTR(ptr, bytes, zone, caps, flags) \
do { \
if (trace_get()) { \
Copy link
Contributor

@keyonjie keyonjie Sep 15, 2021

Choose a reason for hiding this comment

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

I thought about this, but it looks odd to add this check for each caller. Can we implement this inside the tr_xx()? Then we can fix the crash for any other potential callers e.g. the one you are demonstrating in src/init/init.c: 139

/* tr_err(&buffer_tr, "CRASHES (and requires a Linux reboot!)"); */

Copy link
Collaborator Author

@marc-hb marc-hb Sep 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That would be more complicated because it would have to be done in a few different places because of the number of layers and configurability of the code. It will also affect every tracing statement for something that basically never happens. If done at the macro level it would also grow the code size significantly.

I don't think it's worth it to guard against a crash that happened only once in years and only in your local workspace where you turned DEBUG_HEAP on and added one extra log statement in the source. The fewer changes, the better.

trace_init() is called extremely early so it's not very demanding not to use the trace before it.

odd to add this check for each caller.

So far we found two instances, one that I made up and the other one only in your workspace.

if (!ptr) { \
tr_err(&mem_tr, "failed to alloc 0x%x bytes zone 0x%x caps 0x%x flags 0x%x", \
bytes, zone, caps, flags); \
alloc_trace_heap(zone, caps, bytes); \
} \
} while (0)
}

#else
#define DEBUG_TRACE_PTR(ptr, bytes, zone, caps, flags)
#endif
Expand Down
4 changes: 1 addition & 3 deletions src/platform/intel/cavs/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,9 +524,7 @@ int platform_init(struct sof *sof)
#elif CONFIG_TRACE
/* Initialize DMA for Trace*/
trace_point(TRACE_BOOT_PLATFORM_DMA_TRACE);
ret = dma_trace_init_complete(sof->dmat);
if (ret < 0)
return ret;
dma_trace_init_complete(sof->dmat);
#endif

/* show heap status */
Expand Down
7 changes: 5 additions & 2 deletions src/trace/dma-trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ int dma_trace_init_complete(struct dma_trace_data *d)
SOF_TASK_PRI_MED, trace_work, d, 0, 0);

out:

return ret;
}

Expand Down Expand Up @@ -476,8 +477,9 @@ void dma_trace_on(void)
{
struct dma_trace_data *trace_data = dma_trace_data_get();

if (!trace_data || trace_data->enabled)
if (trace_data->enabled) {
return;
}

trace_data->enabled = 1;
schedule_task(&trace_data->dmat_work, DMA_TRACE_PERIOD,
Expand All @@ -489,8 +491,9 @@ void dma_trace_off(void)
{
struct dma_trace_data *trace_data = dma_trace_data_get();

if (!trace_data || !trace_data->enabled)
if (!trace_data->enabled) {
return;
}

schedule_task_cancel(&trace_data->dmat_work);
trace_data->enabled = 0;
Expand Down
7 changes: 5 additions & 2 deletions src/trace/trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,9 @@ void trace_log_unfiltered(bool send_atomic, const void *log_entry, const struct
struct trace *trace = trace_get();
va_list vl;

if (!trace || !trace->enable)
if (!trace->enable) {
return;
}

va_start(vl, arg_count);
vatrace_log(send_atomic, (uint32_t)log_entry, ctx, lvl, id_1, id_2, arg_count, vl);
Expand All @@ -289,8 +290,9 @@ void trace_log_filtered(bool send_atomic, const void *log_entry, const struct tr
uint64_t current_ts;
#endif /* CONFIG_TRACE_FILTERING_ADAPTIVE */

if (!trace || !trace->enable)
if (!trace->enable) {
return;
}

#if CONFIG_TRACE_FILTERING_VERBOSITY
if (!trace_filter_verbosity(lvl, ctx))
Expand Down Expand Up @@ -510,6 +512,7 @@ void trace_off(void)
void trace_init(struct sof *sof)
{
sof->trace = rzalloc(SOF_MEM_ZONE_SYS_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*sof->trace));
sof->trace->enable = 1;
sof->trace->pos = 0;
#if CONFIG_TRACE_FILTERING_ADAPTIVE
sof->trace->user_filter_override = false;
Expand Down