lib_manager: modules: Fix missing call to modules_free #8935

softwarecki · 2024-03-12T15:42:55Z

This PR addresses an issue with non-iadk loadable modules for which the modules_free function is not called.

For some of following commits I already prepared PRs - pending review.

modules: Remove unused code and variables #8897:
d37d689 modules: Remove unused module_entry_point from module_data
87f5e3e module_adapter: modules: Remove unused sys_service pointer
c951d57 module/base: modules: Remove module_adapter from module_data
c85d872 modules: Remove unused buffers
2b49f99 modules: Remove unnecessary functions from Processing Module Adapter

Actual PR content:
e27ea44 lib_manager: modules: move native lib support to lib_manager

lib_manager: Add lib_manager_get_module_manifest function #8974:
94b9976 lib_manager: Rename lib_manager_get_library_module_desc function
ca42dff lib_manager: Add lib_manager_get_module_manifest function
11d8a42 ipc4/helper: ipc4_get_comp_drv: Change parameter type to uint32_t
lib_manager: llext_manager: Add const to variables and function parameters cleanup #8898:
efd957d llext_manager: Remove unused desc parameter
7407ecd lib_manager: llext_manager: Add const to module allocate functions
66f35c9 FIX lib_manager: llext_manager: Add const modifier to module manifest pointers
61e877f lib_manager: llext_manager: Add const modifier to module manifest pointers
30fd9df lib_manager: llext_manager: Simplifying parameter list for free module
5c6c7c8 lib_manager: Simplifying parameter list for lib_manager_register_module
0e05cef lib_manager: Verify preload_page_count from module manifest
da99651 lib_manager: Add const to library manifest variable
component: module_adapter: Move module_interface pointer to comp_driver #8905:
3d34605 modules: lib_manager: Move declare_dynamic_module_adapter to lib_manager
6e80e8b component: module_adapter: Move module_interface pointer to comp_driver

lyakh

I like the end result, that modules.c is no longer needed for native modules. Need to test this with LLEXT (and add a CI LLEXT test...)

src/ipc/ipc4/helper.c

lyakh · 2024-03-13T09:01:28Z

src/library_manager/lib_manager.c

+	    build_info->api_version_number.full == SOF_MODULE_API_CURRENT_VERSION) {
+		/* Use lib_manager shim functions */
+		drv->ops.create = &lib_manager_module_create;
+		drv->ops.free = &lib_manager_module_free;


do I understand it correctly, that after this change modules.c will only be used for IADK? That would actually be good indeed.

By design IADK modules are inheriting interface from native loadable modules.

modules.c (part of the Processing Module Adapter) was supposed to be responsible only for calling C++ functions related to IADK modules. After this change it is no longer used by other module types.

lyakh

tested, unfortunately this breaks llext

src/audio/module_adapter/module/modules.c

softwarecki · 2024-03-21T15:03:29Z

@lyakh Can you check do it still breaks llext?

lyakh · 2024-03-22T08:27:07Z

@lyakh Can you check do it still breaks llext?

@softwarecki why do you think it's fixed now? What's been changed compared to that version? I compared and I don't see any changes that would fix it. buildinfo is still lost so the module entry point isn't identified and called correctly, AFAICS

softwarecki · 2024-03-22T11:27:18Z

@softwarecki why do you think it's fixed now? What's been changed compared to that version? I compared and I don't see any changes that would fix it. buildinfo is still lost so the module entry point isn't identified and called correctly, AFAICS

@lyakh The module entry is taken from a module manifest. This code path has not been changed (lib_manager_allocate_module -> llext_manager_allocate_module -> llext_manager_load_module).

lyakh · 2024-04-02T08:07:58Z

@softwarecki why do you think it's fixed now? What's been changed compared to that version? I compared and I don't see any changes that would fix it. buildinfo is still lost so the module entry point isn't identified and called correctly, AFAICS

@lyakh The module entry is taken from a module manifest. This code path has not been changed (lib_manager_allocate_module -> llext_manager_allocate_module -> llext_manager_load_module).

@softwarecki I'm talking about buildinfo not the manifest

lyakh · 2024-04-02T08:09:41Z

@softwarecki I'm talking about buildinfo not the manifest

@softwarecki ...and sorry for a delay - that comment was sitting in my browser for days, I must've forgotten to press "comment" :-(

softwarecki · 2024-04-02T08:33:30Z

@lyakh The buildinfo structure is not propagated up because there is no need for it. For llext modules it is retrieved in the llext_manager_link function and checked in llext_manager_allocate_module (see: https://github.com/thesofproject/sof/pull/8935/files#diff-105bb3747dadc76f4ae373fd79ca6aba210883d29b7caee205f4adda6f6d6dccR256) . For other modules types, obtaining and checking is done in lib_manager_register_module (https://github.com/thesofproject/sof/pull/8935/files#diff-957336c9ebdbe16dab94f6e49620e9b112d3b373d3de4d1035cdf289f745a00dR666).

lyakh

2 nitpicks - let's fix in a follow-up

src/ipc/ipc4/helper.c

lyakh · 2024-04-03T08:27:27Z

@lyakh The buildinfo structure is not propagated up because there is no need for it. For llext modules it is retrieved in the llext_manager_link function and checked in llext_manager_allocate_module (see: https://github.com/thesofproject/sof/pull/8935/files#diff-105bb3747dadc76f4ae373fd79ca6aba210883d29b7caee205f4adda6f6d6dccR256) . For other modules types, obtaining and checking is done in lib_manager_register_module (https://github.com/thesofproject/sof/pull/8935/files#diff-957336c9ebdbe16dab94f6e49620e9b112d3b373d3de4d1035cdf289f745a00dR666).

@softwarecki ah, you're right, now I get it! And tested - worked in a short test. It looks good in general, but a double-use test just has caused a DSP panic with this PR - investigating, removing the approval for now

a more thorough test caused a DSP panic, removing the approval temporarily

lyakh · 2024-04-03T10:07:02Z

@softwarecki please merge this diff with your "lib_manager: modules: move native lib support to lib_manager" (and also add a description, it certainly deserves one)
20240403-1-sof-ldmod.gitdiff.txt
then I'll be able to restore my approval

softwarecki · 2024-04-03T16:27:36Z

Rebased and applied @lyakh patch.

lyakh · 2024-04-12T13:51:53Z

@softwarecki @kv2019i @lgirdwood can we merge this one instead of #8974 - the first 3 commits are identical and the fourth one looks good to me too

kv2019i · 2024-04-12T13:56:58Z

Sorry @lyakh , I was too quick with #8974. @softwarecki can you update this and we can merge this as well?

lgirdwood

LGTM

Moved module type identification to lib_manager. Since llext modules place the buildinfo structure in a separate section, API version verification has been moved to llext_manager. Thanks to these changes, the Processing Module Adapter is used only by IADK modules as intended. This commit also fixes an issue with the modules_free function not being called for native modules. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lgirdwood · 2024-04-15T12:36:03Z

@softwarecki @wszypelt good to merge or real issue reported by CI ?

wszypelt · 2024-04-16T07:44:19Z

@lgirdwood I'm afraid that the PR may contain a sporadic issue, I'm taking this PR for full scope tests, I should have the results tomorrow :)

wszypelt · 2024-04-17T06:31:06Z

@lgirdwood @softwarecki After careful checking, I don't see any problems, CI also passed correctly

softwarecki requested review from lyakh, lgirdwood, abonislawski, kv2019i, marcinszkudlinski, jxstelter and pjdobrowolski March 12, 2024 15:42

softwarecki requested review from ranj063, plbossart, mmaka1, lbetlej and dbaluta as code owners March 12, 2024 15:42

softwarecki force-pushed the modules-unload branch from 12acc7a to 33b5408 Compare March 12, 2024 16:24

lyakh reviewed Mar 13, 2024

View reviewed changes

softwarecki force-pushed the modules-unload branch from 33b5408 to d37d689 Compare March 13, 2024 14:33

pjdobrowolski mentioned this pull request Mar 14, 2024

modules: restore behaviour for non-LLEXT modules #8933

Closed

lyakh requested changes Mar 14, 2024

View reviewed changes

src/audio/module_adapter/module/modules.c Show resolved Hide resolved

softwarecki force-pushed the modules-unload branch 2 times, most recently from 2b7ee8e to 37fa62d Compare March 15, 2024 16:27

softwarecki requested a review from lyakh March 15, 2024 16:44

softwarecki force-pushed the modules-unload branch from 37fa62d to a0c60a3 Compare March 21, 2024 14:59

softwarecki force-pushed the modules-unload branch from a0c60a3 to 68a9897 Compare March 22, 2024 10:10

lyakh previously approved these changes Apr 3, 2024

View reviewed changes

src/ipc/ipc4/helper.c Outdated Show resolved Hide resolved

src/ipc/ipc4/helper.c Outdated Show resolved Hide resolved

softwarecki force-pushed the modules-unload branch from 68a9897 to fa58e36 Compare April 3, 2024 16:26

lyakh approved these changes Apr 4, 2024

View reviewed changes

lgirdwood reviewed Apr 12, 2024

View reviewed changes

softwarecki force-pushed the modules-unload branch 2 times, most recently from e27ea44 to b96b913 Compare April 12, 2024 15:15

softwarecki force-pushed the modules-unload branch from b96b913 to 9ab785a Compare April 12, 2024 15:18

kv2019i approved these changes Apr 12, 2024

View reviewed changes

kv2019i merged commit 902b1e5 into thesofproject:main Apr 17, 2024
44 of 45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib_manager: modules: Fix missing call to modules_free #8935

lib_manager: modules: Fix missing call to modules_free #8935

softwarecki commented Mar 12, 2024 •

edited

Loading

lyakh left a comment

lyakh Mar 13, 2024

pjdobrowolski Mar 13, 2024

softwarecki Mar 13, 2024

lyakh left a comment

softwarecki commented Mar 21, 2024

lyakh commented Mar 22, 2024

softwarecki commented Mar 22, 2024

lyakh commented Apr 2, 2024

lyakh commented Apr 2, 2024

softwarecki commented Apr 2, 2024 •

edited

Loading

lyakh left a comment

lyakh commented Apr 3, 2024

lyakh commented Apr 3, 2024

softwarecki commented Apr 3, 2024

lyakh commented Apr 12, 2024

kv2019i commented Apr 12, 2024

lgirdwood left a comment

lgirdwood commented Apr 15, 2024

wszypelt commented Apr 16, 2024 •

edited

Loading

wszypelt commented Apr 17, 2024

lib_manager: modules: Fix missing call to modules_free #8935

lib_manager: modules: Fix missing call to modules_free #8935

Conversation

softwarecki commented Mar 12, 2024 • edited Loading

lyakh left a comment

Choose a reason for hiding this comment

lyakh Mar 13, 2024

Choose a reason for hiding this comment

pjdobrowolski Mar 13, 2024

Choose a reason for hiding this comment

softwarecki Mar 13, 2024

Choose a reason for hiding this comment

lyakh left a comment

Choose a reason for hiding this comment

softwarecki commented Mar 21, 2024

lyakh commented Mar 22, 2024

softwarecki commented Mar 22, 2024

lyakh commented Apr 2, 2024

lyakh commented Apr 2, 2024

softwarecki commented Apr 2, 2024 • edited Loading

lyakh left a comment

Choose a reason for hiding this comment

lyakh commented Apr 3, 2024

lyakh commented Apr 3, 2024

softwarecki commented Apr 3, 2024

lyakh commented Apr 12, 2024

kv2019i commented Apr 12, 2024

lgirdwood left a comment

Choose a reason for hiding this comment

lgirdwood commented Apr 15, 2024

wszypelt commented Apr 16, 2024 • edited Loading

wszypelt commented Apr 17, 2024

softwarecki commented Mar 12, 2024 •

edited

Loading

softwarecki commented Apr 2, 2024 •

edited

Loading

wszypelt commented Apr 16, 2024 •

edited

Loading