Skip to content

Conversation

@ranj063
Copy link
Collaborator

@ranj063 ranj063 commented Oct 5, 2021

This will be used to stop the trace DMA and free
its resources. This change is tagged for ABI 3.20.

kernel PR thesofproject/linux#3197

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also put the channel and set d->dc.chan to NULL?
SOF_IPC_TRACE_DMA_PARAMS_EXT will fail if we have a valid channel already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we also have #4850 along with this patch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#4850 was merged, so this is good right @ujfalusi ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would still put the channel for consistency:

if (d->dc.chan) {
	dma_stop(d->dc.chan); /* Should we do an error check? */
	dma_channel_put(d->dc.chan);
	d->dc.chan = NULL;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, ill release the channel but error checking would be pointless because trace work is already stopped right?

@lgirdwood lgirdwood added this to the ABI-3.20 milestone Oct 7, 2021
@lgirdwood lgirdwood added the ABI ABI change involved label Oct 7, 2021
Copy link
Collaborator

Choose a reason for hiding this comment

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

#4850 was merged, so this is good right @ujfalusi ?

@kv2019i kv2019i requested a review from ujfalusi October 13, 2021 08:44
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@ujfalusi pls approve if good for you.

@lgirdwood
Copy link
Member

@ranj063 looks like we have a crash

unknown ipc: unknown command type 0
unknown ipc: debug cmd 0x50000
AddressSanitizer:DEADLYSIGNAL
=================================================================
==22==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000005870d2 bp 0x7fffb3ce93f0 sp 0x7fffb3ce93b0 T0)
==22==The signal is caused by a READ memory access.
==22==Hint: address points to the zero page.
SCARINESS: 10 (null-deref)
    #0 0x5870d2 in dma_stop /src/sof/src/include/sof/lib/dma.h:316:21
    #1 0x5870d2 in dma_trace_disable /src/sof/src/trace/dma-trace.c:429:2
    #2 0x55f464 in ipc_dma_trace_free /src/sof/src/ipc/ipc3/handler.c:757:2
    #3 0x55f464 in ipc_glb_trace_message /src/sof/src/ipc/ipc3/handler.c:886:3
    #4 0x55f464 in ipc_cmd /src/sof/src/ipc/ipc3/handler.c:1571:9
    #5 0x55dc56 in LLVMFuzzerTestOneInput /src/sof/tools/oss-fuzz/fuzz_ipc.c:28:2
    #6 0x456b03 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) cxa_noexception.cpp
    #7 0x4562fd in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) cxa_noexception.cpp
    #8 0x457b8b in fuzzer::Fuzzer::MutateAndTestOne() cxa_noexception.cpp
    #9 0x458675 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) cxa_noexception.cpp
    #10 0x447d68 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) cxa_noexception.cpp
    #11 0x470bd2 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #12 0x7fc4ca4a40b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #13 0x41f68d in _start (build-out/fuzz_ipc+0x41f68d)

DEDUP_TOKEN: dma_stop--dma_trace_disable--ipc_dma_trace_free
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /src/sof/src/include/sof/lib/dma.h:316:21 in dma_stop
==22==ABORTING

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we call this as dma_trace_free() to be consistent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it balances with trace_dma_enable during ipc_dma_trace_params() no?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would still put the channel for consistency:

if (d->dc.chan) {
	dma_stop(d->dc.chan); /* Should we do an error check? */
	dma_channel_put(d->dc.chan);
	d->dc.chan = NULL;
}

This will be used to stop the trace DMA and free
its resources. This change is tagged for ABI 3.20.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@ranj063 ranj063 force-pushed the fix/trace_dma_free branch from 28b0dd2 to 390b0d1 Compare October 15, 2021 04:43
@ujfalusi ujfalusi mentioned this pull request Oct 15, 2021
Copy link
Contributor

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

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

@ranj063, thank you, looks great!

@lgirdwood
Copy link
Member

CI showing a presence detect failure on ADL. @marc-hb I've seen this several time today, looks like we may need to filter the cpufreq errors out ? https://sof-ci.01.org/sofpr/PR4849/build10723/devicetest/?model=ADLP_RVP_SDW&testcase=verify-kernel-boot-log

@lgirdwood lgirdwood merged commit bd918ad into thesofproject:main Oct 15, 2021
@ranj063 ranj063 deleted the fix/trace_dma_free branch October 15, 2021 16:21
@marc-hb
Copy link
Collaborator

marc-hb commented Oct 15, 2021

cpufreq - and more. All boot logs on ADK: thesofproject/sof-test#786

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ABI ABI change involved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants