Skip to content
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.

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

modules: restore behaviour for non-LLEXT modules #8933

Closed
wants to merge 1 commit into from

Conversation

lyakh
Copy link
Collaborator

@lyakh lyakh commented Mar 12, 2024

An earlier commit inadvertantly potentially changed behaviour of non-llext modules by imposing too strict a condition for calling modules_new(). Restore the original behaviour for those modules.

Fixes: 6b9b4c2 ("modules: don't re-load on each restart")

An earlier commit inadvertantly potentially changed behaviour of
non-llext modules by imposing too strict a condition for calling
modules_new(). Restore the original behaviour for those modules.

Fixes: 6b9b4c2 ("modules: don't re-load on each restart")
Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Copy link
Contributor

@pjdobrowolski pjdobrowolski left a comment

Choose a reason for hiding this comment

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

We cannot allow register module with NULL pointer in md->ops or anything else. Untile module_adapter is not obsolete, llext need second separate condition.

@lyakh
Copy link
Collaborator Author

lyakh commented Mar 12, 2024

We cannot allow register module with NULL pointer in md->ops or anything else. Untile module_adapter is not obsolete, llext need second separate condition.

@pjdobrowolski could you please read the patch once again. It is not allowing any new NULL pointers. What it does for non-LLEXT modules is it extends the number of cases when modules_new() gets called, returning it to the state before my 6b9b4c2
If you check that commit, you'll see, that before it modules_new() was called always inside modules_init(). That commit changed it to call modules_new() only sometimes, namely the purpose of that change was to only call it when modules_init() was called for the first time. But this change should only affect LLEXT modules, non-LLEXT modules shouldn't be touched. So for non-LLEXT modules modules_new() should still be called every single time when modules_init() is entered. And this commit restores this behaviour. So I'm puzzled why this breaks loadable module QB tests.
In fact it isn't this PR that's breaking QB loadable module tests, please check #8932 QB result build 13704415 - you'll see the same failures

@pjdobrowolski
Copy link
Contributor

Ok, I see but why do you extends modules type? Are llext modules compatible with IADK?

@@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The module_data structure is unique for each module instance. You will not be able to tell whether this is the first loading of the module based on the value of any of its fields. I suggest following solution: #8939

@lyakh
Copy link
Collaborator Author

lyakh commented Mar 14, 2024

Ok, I see but why do you extends modules type? Are llext modules compatible with IADK?

@pjdobrowolski now sure what you mean by "extending the module type" - yes, LLEXT modules are different enough from other module types. No, they're in no way compatible with IADK.

@pjdobrowolski
Copy link
Contributor

Ok, I think that to prevent future misunderstanding we should integrate @softwarecki refactor #8935 and after that you will have separe path of development only for llext modules.

@lgirdwood
Copy link
Member

@lyakh @pjdobrowolski any update here ? I think some other PRs now applied, whats next ?

@lyakh lyakh closed this May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants