module: Revert "loading module: fix native module interface register" #8923
Conversation
lyakh
requested review from
lgirdwood,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
|
The fail in https://sof-ci.01.org/sofpr/PR8923/build3238/devicetest/index.html (mtrace shows "ASSERTION FAIL [!z_smp_cpu_mobile()] @ ZEPHYR_BASE/subsys/pm/pm.c:133") is caused by #8908 |
| @@ -161,7 +163,8 @@ static int modules_init(struct processing_module *mod) | |||
|
|
|||
| /* Call module specific init function if exists. */ | |||
| if (mod->is_native_sof) { | |||
| const struct module_interface *mod_in = md->ops; | |||
| struct module_interface *mod_in = | |||
| (struct module_interface *)md->module_adapter; | |||
|
|
|||
There was a problem hiding this comment.
That changes break loadable modules #8667 which works on main. Please do not introduce changes breaking working flow.
There was a problem hiding this comment.
@pjdobrowolski I don't see how this revert can break anything. As you see from the hunk above around lines 90-110 the version before this commit stored a pointer to native loadable module interface, returned by native_system_agent_start() in md->ops and after this commit it's in md->module_adapter. The effect of this is that before this commit all framework calls to interface functions went directly to the loaded module functions, whereas after this commit they first call wrappers from module.c, which then call respective methods from the loaded module.
The difference is particularly visible for .free() and .init(). For the .free() case you have to call the wrapper, otherwise you miss calling lib_manager_free_module(). As for .init() this change probably has the effect, that if you don't reload the loaded module, then the second call to .init() would go directly to the loaded module, instead of modules_init(). Is this what is breaking your tests?
| @@ -136,7 +136,7 @@ static int modules_init(struct processing_module *mod) | |||
| } | |||
| comp_info(dev, "modules_init() start"); | |||
|
|
|||
| if (!md->module_adapter && md->ops == &interface) { | |||
| if (!md->llext || md->ops == &interface) { | |||
There was a problem hiding this comment.
I don't understand this change. Is it fix anything or it is only a "potentially" fix?
Ohh I see. Its some kind of hotfix for massive new changes added on March 6 to the PR from September 2023, merged quickly on March 8. Well done guys!
There was a problem hiding this comment.
@softwarecki to your question: this is restoring the original behaviour of non-llext loadable modules that wasn't caught by QB, that is in fact testing native loadable modules AFAIK. And I think the reason, why it couldn't be caught by it is, that .free() is currently not working anyway, and this affects only the second and following initialisations.
This reverts commit a082752. The commit prevents modules_free() from being called, which leads to leaking memory and a failure to call lib_manager_free_module(). For LLEXT modules this also leads to failures when using a module for the second and following times. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lyakh
changed the title
|
Let's split this into two. This one now only contains the revert, which raised objections, and the separate fix went into #8933 |
Currently loadable modules are broken because of a regression caused by a082752 This PR reverts that commit and fixes a mistake in another recent update.