sof icon indicating copy to clipboard operation
sof copied to clipboard

lib_manager: modules: Fix missing call to modules_free

Open softwarecki opened this issue 1 year ago • 3 comments

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.

  • [ ] https://github.com/thesofproject/sof/pull/8897: d37d68961 modules: Remove unused module_entry_point from module_data 87f5e3e69 module_adapter: modules: Remove unused sys_service pointer c951d57d6 module/base: modules: Remove module_adapter from module_data c85d87216 modules: Remove unused buffers 2b49f997e modules: Remove unnecessary functions from Processing Module Adapter

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

  • [ ] https://github.com/thesofproject/sof/pull/8974: 94b9976df lib_manager: Rename lib_manager_get_library_module_desc function ca42dff5a lib_manager: Add lib_manager_get_module_manifest function 11d8a423b ipc4/helper: ipc4_get_comp_drv: Change parameter type to uint32_t

  • [x] https://github.com/thesofproject/sof/pull/8898: efd957d51 llext_manager: Remove unused desc parameter 7407ecdda lib_manager: llext_manager: Add const to module allocate functions 66f35c92a FIX lib_manager: llext_manager: Add const modifier to module manifest pointers 61e877f15 lib_manager: llext_manager: Add const modifier to module manifest pointers 30fd9df1d lib_manager: llext_manager: Simplifying parameter list for free module 5c6c7c8c2 lib_manager: Simplifying parameter list for lib_manager_register_module 0e05cefe4 lib_manager: Verify preload_page_count from module manifest da99651a8 lib_manager: Add const to library manifest variable

  • [x] https://github.com/thesofproject/sof/pull/8905: 3d34605ab modules: lib_manager: Move declare_dynamic_module_adapter to lib_manager 6e80e8bff component: module_adapter: Move module_interface pointer to comp_driver

softwarecki avatar Mar 12 '24 15:03 softwarecki

@lyakh Can you check do it still breaks llext?

softwarecki avatar Mar 21 '24 15:03 softwarecki

@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

lyakh avatar Mar 22 '24 08:03 lyakh

@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 avatar Mar 22 '24 11:03 softwarecki

@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 avatar Apr 02 '24 08:04 lyakh

@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" :-(

lyakh avatar Apr 02 '24 08:04 lyakh

@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 avatar Apr 02 '24 08:04 softwarecki

@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

lyakh avatar Apr 03 '24 08:04 lyakh

@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

lyakh avatar Apr 03 '24 10:04 lyakh

Rebased and applied @lyakh patch.

softwarecki avatar Apr 03 '24 16:04 softwarecki

@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

lyakh avatar Apr 12 '24 13:04 lyakh

Sorry @lyakh , I was too quick with https://github.com/thesofproject/sof/pull/8974. @softwarecki can you update this and we can merge this as well?

kv2019i avatar Apr 12 '24 13:04 kv2019i

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

lgirdwood avatar Apr 15 '24 12:04 lgirdwood

@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 avatar Apr 16 '24 07:04 wszypelt

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

wszypelt avatar Apr 17 '24 06:04 wszypelt