Zephyr: fix build breakage with Zephyr "main" #9199
Conversation
lyakh
requested review from
tmleman,
kv2019i,
iuliana-prodan,
dbaluta and
abonislawski
as code owners
|
Thanks @lyakh! The timing is a bit unfortunate as we have this change before we managed to react to the previous one (so now if you want to update Zephyr, you'll need to touch multiple places in SOF). This needs to be merged together with a west update, but we can't do that before we have #9190 merged. |
|
Can you please add a short but actual commit message? At least the name of the functions changed. Or maybe even two commits to give enough room for something useful in commit titles. |
| #else | ||
| #define llext_manager_allocate_module(proc, ipc_config, ipc_specific_config) 0 | ||
| #define llext_manager_free_module(component_id) 0 | ||
| #define llext_unload(ext) 0 |
There was a problem hiding this comment.
Wow, this looks really weird.... I think it's not just "creative", unusual and harder to read...
-
I don't think having these being either functions or macros depending on Kconfig is a good idea. Debugging Kconfig and preprocessing issues is painful enough already, so having totally different error messages depending on CONFIG_LLEXT seems wrong. Please keep these functions always:
static inline uintptr_tllext_manager_free_module(component_id) { return 0; }. The generated code should be the same. -
Naive question sorry: why are these functions needed when CONFIG_LLEXT is false? I don't know all the details sorry but returning NULLs and zeros is an anti-pattern. Ideally, these shouldn't even be needed so a short comment/pointer explaining why would not hurt.
-
where is
llext_unload()defined when CONFIG_LLEXT is true?
There was a problem hiding this comment.
- Naive question sorry: why are these functions needed when CONFIG_LLEXT is false? I don't know all the details sorry but returning NULLs and zeros is an anti-pattern. Ideally, these shouldn't even be needed so a short comment/pointer explaining why would not hurt.
@marc-hb because they're called from lib_manager.c and that one is included in TGL builds too. No, I don't think it's needed there for the same reason as LLEXT - no memory mapping support.
- where is
llext_unload()defined when CONFIG_LLEXT is true?
in Zephyr
Fixes, needed for a recent Zephyr head plus a PR to fix CONFIG_LLEXT=n builds. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Not all configurations need LLEXT, don't include LLEXT support in such cases. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
cAVS platforms don't use LLEXT because memory mapping isn't supported on them. Select LLEXT per platform, so far only for ACE 1.5 and 2.0. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lyakh
requested review from
lgirdwood,
plbossart,
mmaka1 and
lbetlej
as code owners
|
ok, this one fixes the build on TGL which #9217 is still pending. Lets go with this as it gets things back to green. |
Two new build regressions have been introduced in Zephyr's main, fix them.