Trace - make clear that trace context is unused with Zephyr - #11102
Trace - make clear that trace context is unused with Zephyr#11102lyakh wants to merge 3 commits into
Conversation
Under Zephyr with CONFIG_ZEPHYR_LOG, tr_err()/tr_warn()/tr_info()/tr_dbg() expand directly to LOG_ERR()/LOG_WRN()/LOG_INF()/LOG_DBG() and completely ignore their first (tr_ctx) argument. A plain, otherwise-unreferenced DECLARE_TR_CTX() static is therefore dead and the linker discards it. However, a `struct comp_driver`/`struct dai_driver` instance whose `.tctx` field points at such a context keeps it alive, because the driver struct itself is always referenced (component/DAI driver registration). This adds a few bytes of otherwise-dead .data per affected driver and forces the firmware to retain values that can never be read on Zephyr. Guard the `.tctx = &foo_tr,` initializers (and the equivalent runtime `drv->tctx = &lib_manager_tr;` assignment) with #ifndef __ZEPHYR__ so the now fully-unreferenced trace contexts can be garbage-collected by the linker, same as any other unused DECLARE_TR_CTX(). Fix the shared DECLARE_MODULE_ADAPTER() macro once, which covers ~40 IPC4 module_adapter components: MODULE_ADAPTER_TCTX_INIT(tr) resolves to &(tr) normally and to NULL under Zephyr, so `.tctx = MODULE_ADAPTER_TCTX_INIT(tr),` still parses as a valid (harmless) initializer either way. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
pipeline_new() and buffer_new() unconditionally memcpy_s() the static pipe_tr/buffer_tr DECLARE_TR_CTX() objects into the new pipeline's/buffer's embedded tr_ctx field. On Zephyr this copy is pure overhead: comp_init() in component.h already skips the equivalent copy for comp_dev with #ifndef __ZEPHYR__, but these two sites were missed. buffer_new()'s copy was already conditional on \!CONFIG_SOF_USERSPACE_LL; simplify that to \!__ZEPHYR__ since CONFIG_SOF_USERSPACE_LL can only be defined in Zephyr builds. Also, since &pipe_tr/&buffer_tr were the only references to those objects, guarding the copy lets the linker garbage-collect them on Zephyr, same as ipc_tr and friends. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Under Zephyr with CONFIG_ZEPHYR_LOG, tr_err()/tr_warn()/tr_info()/tr_dbg() expand directly to LOG_ERR()/LOG_WRN()/LOG_INF()/LOG_DBG() and never look at their tr_ctx argument. None of these remaining DECLARE_TR_CTX() instances are referenced by anything other than tr_*() calls in the same file (verified: no .tctx/->tctx assignment picks up their address), so they are already dead code eliminated by the linker on Zephyr builds - this commit only documents that fact, it changes no generated code. (dai-legacy.c, host-legacy.c and the platform/library/schedule/*.c files are excluded: they are only ever built for non-Zephyr, non-hardware configurations - testbench/cmocka/legacy XTOS - so the comment would not apply to them.) Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
kv2019i
left a comment
There was a problem hiding this comment.
Thanks for tackling, but I think this needs iteration still...
| @@ -41,6 +41,7 @@ | |||
| LOG_MODULE_REGISTER(basefw, CONFIG_SOF_LOG_LEVEL); | |||
|
|
||
| static const struct comp_driver comp_basefw = { | ||
| .uid = SOF_RT_UUID(basefw_uuid), | ||
| #ifndef __ZEPHYR__ |
There was a problem hiding this comment.
Could this be reversed? I mean these are only needed in LIBRARY builds now (or are they even needed there)? Or maybe that's not accurate enough, but maybe "#ifdef CONFIG_TRACE" (i.e. a build really using sof-logger with dictionaries).
And for Zephyr, this doesn't cover the case where you build with Zephyr but disable CONFIG_ZEPHYR_LOG. I think these cases should be prevented somehow.
There was a problem hiding this comment.
all I want to achieve with these is that this code / data doesn't appear in "real" firmware builds made to be run on real hardware. What's the best way to achieve that - can be discussed. This one achieves the goal and passes all CI configurations, so I used it. If the PR in principle is fine, I'd take that and if needed I'd optimise it later - unless there are any real configurations that this is breaking
| @@ -35,6 +35,7 @@ LOG_MODULE_REGISTER(pipe, CONFIG_SOF_LOG_LEVEL); | |||
|
|
|||
| SOF_DEFINE_REG_UUID(pipe); | |||
There was a problem hiding this comment.
What's with the "!CONFIG_SOF_USERSPACE_LL" in the commit message?
There was a problem hiding this comment.
@kv2019i see the last hunk in this commit
|
|
||
| #else | ||
|
|
||
| /* unused with Zephyr, generates no output */ |
There was a problem hiding this comment.
unused if not CONFIG_TRACE, not really related to Zephyr...
There was a problem hiding this comment.
what does a Zephyr build with CONFIG_TRACE=y do? Does this make any sense? Is ever used? Maybe make CONFIG_TRACE depend on !ZEPHYR_SOF_MODULE?
There was a problem hiding this comment.
@lyakh sof-logger with Zephyr. We used that in the past, but no longer used on Intel platforms. If this is no longer needed, we can remove the whole sof-logger from tree. I think this should be a separate PR and get all stakeholders to review the removal.
There was a problem hiding this comment.
@kv2019i sof-logger needs CONFIG_TRACE?
There was a problem hiding this comment.
@lyakh It's right in the Kconfig description for CONFIG_TRACE:
config TRACE
» bool "Trace"
» default n if ZEPHYR_SOF_MODULE
» default y
» help
» Enable SOF DMA based traces compatible with the sof-logger tool.
This is the option to enable sof-logger compatible trace generation from FW.
There was a problem hiding this comment.
@lyakh It doesn't seem to be used by anyone anymore (at least with Zephyr), so maybe we can remove sof-logger (tools and FW-side infra). This should be a separate PR though:
work/sof$ grep -r CONFIG_TRACE app
app/prj.conf:CONFIG_TRACE=n
app/boards/acp_7_x_adsp.conf:CONFIG_TRACE=n
app/boards/intel_adsp_ace15_mtpm.conf.orig~:CONFIG_TRACE=n
app/boards/imx8mp_evk_mimx8ml8_adsp.conf:CONFIG_TRACE=n
app/boards/intel_adsp_ace40_nvls.conf:CONFIG_TRACE=n
app/boards/intel_adsp_ace40_nvl.conf:CONFIG_TRACE=n
app/boards/acp_7_0_adsp.conf:CONFIG_TRACE=n
app/prj.conf.orig:CONFIG_TRACE=n
In src/arch/xtensa/configs/ we still have usage (at least with @thesofproject/amd targets, most recent additions in Apr 2026). A separate PR (to remove sof-logger and CONFIG_TRACE) would be good so people get a chance to review.
|
This adds a lot of duplicated comments for relatively little value (and wasted tokens for future agents). Is it worth repeating the same comment everywhere? Could we add it once to the definition of DECLARE_TR_CTX instead? |
@abonislawski well, it's a trade-off. Ideally I'd just remove them all. This seemed the next best option to me, but I'm open to opinions. "Future tokens" don't bother me TBH - I more care about humans (e.g. about myself) and personally every time I stumble over one of those my fingers begin to itch to remove it. So, I hope this at least could help. |
lgirdwood
left a comment
There was a problem hiding this comment.
Legacy RTOS support can be removed after v2.15
|
|
||
| static const struct comp_driver comp_chain_dma = { | ||
| .uid = SOF_RT_UUID(chain_dma_uuid), | ||
| #ifndef __ZEPHYR__ |
There was a problem hiding this comment.
Zephyr is only RTOS now. We can remove support for legacy after v2.15
There was a problem hiding this comment.
@lgirdwood these preprocessor conditionals are still needed for cmocka, testbench, maybe some other builds too.
There was a problem hiding this comment.
Ack on @lyakh . @lgirdwood We have no legacy targets anymore, but we do still have test targets that are not Zephyr based. Testbench, ALSA SOF plugin, cmocka -- anything where SOF is built as a library to be integrated to some host application. It's unclear whether using a full Zephyr build for these uses is meaningful (but nobody has tried yet). Cmocka is transitioning to ztest, but testbench and ALSA SOF plugin are bigger questions marks. FYI @singalsu for testbench ties and @tmleman for cmocka/ztest.
@lgirdwood not critical, this only removes a couple of unused objects / fields from Zephyr builds. I'd also like to hear others' opinions on the comment made by @abonislawski - is adding these comments lines a good idea or not? |
We have a lot of code that uses trace context like
tr_dbg()/tr_err()calls. Changing all those locations would be a very big change. This PR takes a middle ground: it adds comments to trace context declarations where they are harmless and makes harmless those, that end up generating needless code under Zephyr. Yes, made with AI.