Skip to content

Add lock to mod resources tracking #10960

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.

Sign up for GitHub

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

Open
jsarha wants to merge 4 commits into
base: main
Choose a base branch
from
+83 −49
Open

Add lock to mod resources tracking #10960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8ff1c69
module: generic: turn mod_data_blob_handler_new() into a syscall
Jun 25, 2026
bdcd011
module: generic: turn mod_balloc_align() into a syscall
Jun 26, 2026
336aa1c
module: generic: add mutex to protect module_resources
Jun 25, 2026
1504a2c
module: generic: remove MEM_API_CHECK_THREAD debug mechanism
Jun 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions src/audio/module_adapter/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,6 @@ menu "Processing modules"
containers to allocate at once is selected by this
config option.

config MODULE_MEMORY_API_DEBUG
bool "Turn on memory API thread safety checks"
default y if DEBUG
help
The Module Memory API structures are not protected
by locks. This is because the initialization,
allocation, and freeing of resources should always
be done in the same thread. This option adds an
assert to make sure no other thread makes such
operations.

config CADENCE_CODEC
bool "Cadence codec"
help
Expand Down
98 changes: 69 additions & 29 deletions src/audio/module_adapter/module/generic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
*/

#include <rtos/symbol.h>
#include <rtos/mutex.h>
#include <sof/compiler_attributes.h>
#include <sof/objpool.h>
#include <sof/audio/module_adapter/module/generic.h>
Expand All @@ -25,16 +26,6 @@
#include <ipc4/pipeline.h>
#endif

/* The __ZEPHYR__ condition is to keep cmocka tests working */
#if CONFIG_MODULE_MEMORY_API_DEBUG && defined(__ZEPHYR__)
#define MEM_API_CHECK_THREAD(res) do { \
if ((res)->rsrc_mngr != k_current_get()) \
LOG_WRN("mngr %p != cur %p", (res)->rsrc_mngr, k_current_get()); \
} while (0)
#else
#define MEM_API_CHECK_THREAD(res)
#endif

LOG_MODULE_DECLARE(module_adapter, CONFIG_SOF_LOG_LEVEL);

int module_load_config(struct comp_dev *dev, const void *cfg, size_t size)
Expand Down Expand Up @@ -88,6 +79,7 @@ void mod_resource_init(struct processing_module *mod)
struct module_resources *res = &mod->priv.resources;

/* Init memory list */
k_mutex_init(&res->lock);
list_init(&res->objpool.list);
res->objpool.heap = res->alloc->heap;
res->objpool.vreg = res->alloc->vreg;
Expand Down Expand Up @@ -122,9 +114,6 @@ int module_init(struct processing_module *mod)
return -EIO;
}

#if CONFIG_MODULE_MEMORY_API_DEBUG && defined(__ZEPHYR__)
mod->priv.resources.rsrc_mngr = k_current_get();
#endif
/* Now we can proceed with module specific initialization */
#if CONFIG_SOF_USERSPACE_APPLICATION
if (mod->dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP)
Expand Down Expand Up @@ -185,20 +174,23 @@ void mod_heap_info(struct processing_module *mod, size_t *size, uintptr_t *start
* unloaded. The back-end, rballoc(), always aligns the memory to
* PLATFORM_DCACHE_ALIGN at the minimum.
*/
void *mod_balloc_align(struct processing_module *mod, size_t size, size_t alignment)
void *z_impl_mod_balloc_align(struct processing_module *mod, size_t size, size_t alignment)
{
struct module_resources *res = &mod->priv.resources;
struct module_resource *container;

MEM_API_CHECK_THREAD(res);
k_mutex_lock(&res->lock, K_FOREVER);

container = container_get(mod);
if (!container)
if (!container) {
k_mutex_unlock(&res->lock);
return NULL;
}

if (!size) {
comp_err(mod->dev, "requested allocation of 0 bytes.");
container_put(mod, container);
k_mutex_unlock(&res->lock);
return NULL;
}

Expand All @@ -210,6 +202,7 @@ void *mod_balloc_align(struct processing_module *mod, size_t size, size_t alignm
comp_err(mod->dev, "Failed to alloc %zu bytes %zu alignment for comp %#x.",
size, alignment, dev_comp_id(mod->dev));
container_put(mod, container);
k_mutex_unlock(&res->lock);
return NULL;
}
/* Store reference to allocated memory */
Expand All @@ -221,9 +214,10 @@ void *mod_balloc_align(struct processing_module *mod, size_t size, size_t alignm
if (res->heap_usage > res->heap_high_water_mark)
res->heap_high_water_mark = res->heap_usage;

k_mutex_unlock(&res->lock);
return ptr;
}
EXPORT_SYMBOL(mod_balloc_align);
EXPORT_SYMBOL(z_impl_mod_balloc_align);

/**
* Allocates aligned memory block with flags for module.
Expand All @@ -241,15 +235,18 @@ void *z_impl_mod_alloc_ext(struct processing_module *mod, uint32_t flags, size_t
struct module_resources *res = &mod->priv.resources;
struct module_resource *container;

MEM_API_CHECK_THREAD(res);
k_mutex_lock(&res->lock, K_FOREVER);

container = container_get(mod);
if (!container)
if (!container) {
k_mutex_unlock(&res->lock);
return NULL;
}

if (!size) {
comp_err(mod->dev, "requested allocation of 0 bytes.");
container_put(mod, container);
k_mutex_unlock(&res->lock);
return NULL;
}

Expand All @@ -260,6 +257,7 @@ void *z_impl_mod_alloc_ext(struct processing_module *mod, uint32_t flags, size_t
comp_err(mod->dev, "Failed to alloc %zu bytes %zu alignment for comp %#x.",
size, alignment, dev_comp_id(mod->dev));
container_put(mod, container);
k_mutex_unlock(&res->lock);
return NULL;
}
/* Store reference to allocated memory */
Expand All @@ -271,6 +269,7 @@ void *z_impl_mod_alloc_ext(struct processing_module *mod, uint32_t flags, size_t
if (res->heap_usage > res->heap_high_water_mark)
res->heap_high_water_mark = res->heap_usage;

k_mutex_unlock(&res->lock);
return ptr;
}
EXPORT_SYMBOL(z_impl_mod_alloc_ext);
Expand All @@ -283,31 +282,35 @@ EXPORT_SYMBOL(z_impl_mod_alloc_ext);
* Like comp_data_blob_handler_new() but the handler is automatically freed.
*/
#if CONFIG_COMP_BLOB
struct comp_data_blob_handler *mod_data_blob_handler_new(struct processing_module *mod)
struct comp_data_blob_handler *z_impl_mod_data_blob_handler_new(struct processing_module *mod)
{
struct module_resources * __maybe_unused res = &mod->priv.resources;
struct module_resources *res = &mod->priv.resources;
struct comp_data_blob_handler *bhp;
struct module_resource *container;

MEM_API_CHECK_THREAD(res);
k_mutex_lock(&res->lock, K_FOREVER);

container = container_get(mod);
if (!container)
if (!container) {
k_mutex_unlock(&res->lock);
return NULL;
}

bhp = comp_data_blob_handler_new_ext(mod->dev, false, NULL, NULL);
if (!bhp) {
container_put(mod, container);
k_mutex_unlock(&res->lock);
return NULL;
}

container->bhp = bhp;
container->size = 0;
container->type = MOD_RES_BLOB_HANDLER;

k_mutex_unlock(&res->lock);
return bhp;
}
EXPORT_SYMBOL(mod_data_blob_handler_new);
EXPORT_SYMBOL(z_impl_mod_data_blob_handler_new);
#endif

/**
Expand All @@ -325,22 +328,26 @@ const void *z_impl_mod_fast_get(struct processing_module *mod, const void * cons
struct module_resource *container;
const void *ptr;

MEM_API_CHECK_THREAD(res);
k_mutex_lock(&res->lock, K_FOREVER);

container = container_get(mod);
if (!container)
if (!container) {
k_mutex_unlock(&res->lock);
return NULL;
}

ptr = fast_get(res->alloc, dram_ptr, size);
if (!ptr) {
container_put(mod, container);
k_mutex_unlock(&res->lock);
return NULL;
}

container->sram_ptr = ptr;
container->size = 0;
container->type = MOD_RES_FAST_GET;

k_mutex_unlock(&res->lock);
return ptr;
}
EXPORT_SYMBOL(z_impl_mod_fast_get);
Expand Down Expand Up @@ -412,14 +419,17 @@ int z_impl_mod_free(struct processing_module *mod, const void *ptr)
{
struct module_resources *res = &mod->priv.resources;

MEM_API_CHECK_THREAD(res);
if (!ptr)
return 0;

/* Find which container holds this memory */
struct mod_res_cb_arg cb_arg = {mod, ptr};

k_mutex_lock(&res->lock, K_FOREVER);
int ret = objpool_iterate(&res->objpool, mod_res_free, &cb_arg);

k_mutex_unlock(&res->lock);

if (ret < 0)
comp_err(mod->dev, "error: could not find memory pointed by %p", ptr);

Expand Down Expand Up @@ -463,6 +473,20 @@ void *z_vrfy_mod_alloc_ext(struct processing_module *mod, uint32_t flags, size_t
}
#include <zephyr/syscalls/mod_alloc_ext_mrsh.c>

void *z_vrfy_mod_balloc_align(struct processing_module *mod, size_t size, size_t alignment)
{
size_t h_size = 0;
uintptr_t h_start;

K_OOPS(K_SYSCALL_MEMORY_WRITE(mod, sizeof(*mod)));
mod_heap_info(mod, &h_size, &h_start);
if (h_size)
K_OOPS(K_SYSCALL_MEMORY_WRITE(h_start, h_size));

return z_impl_mod_balloc_align(mod, size, alignment);
}
#include <zephyr/syscalls/mod_balloc_align_mrsh.c>

int z_vrfy_mod_free(struct processing_module *mod, const void *ptr)
{
size_t h_size = 0;
Expand All @@ -476,6 +500,22 @@ int z_vrfy_mod_free(struct processing_module *mod, const void *ptr)
return z_impl_mod_free(mod, ptr);
}
#include <zephyr/syscalls/mod_free_mrsh.c>

#if CONFIG_COMP_BLOB
struct comp_data_blob_handler *z_vrfy_mod_data_blob_handler_new(struct processing_module *mod)
{
size_t h_size = 0;
uintptr_t h_start;

K_OOPS(K_SYSCALL_MEMORY_WRITE(mod, sizeof(*mod)));
mod_heap_info(mod, &h_size, &h_start);
if (h_size)
K_OOPS(K_SYSCALL_MEMORY_WRITE(h_start, h_size));

return z_impl_mod_data_blob_handler_new(mod);
}
#include <zephyr/syscalls/mod_data_blob_handler_new_mrsh.c>
#endif
#endif

#if CONFIG_COMP_BLOB
Expand Down Expand Up @@ -698,13 +738,13 @@ void mod_free_all(struct processing_module *mod)
{
struct module_resources *res = &mod->priv.resources;

MEM_API_CHECK_THREAD(res);

/* Free all contents found in used containers */
struct mod_res_cb_arg cb_arg = {mod, NULL};

k_mutex_lock(&res->lock, K_FOREVER);
objpool_iterate(&res->objpool, mod_res_free, &cb_arg);
objpool_prune(&res->objpool);
k_mutex_unlock(&res->lock);

/* Make sure resource lists and accounting are reset */
mod_resource_init(mod);

@lyakh lyakh Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd protect this too

Expand Down
23 changes: 14 additions & 9 deletions src/include/sof/audio/module_adapter/module/generic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,14 @@
#ifndef __SOF_AUDIO_MODULE_GENERIC__
#define __SOF_AUDIO_MODULE_GENERIC__

#include <rtos/mutex.h>
#include <sof/objpool.h>
#include <sof/ut.h>
#include <sof/audio/component.h>
#include <sof/audio/sink_api.h>
#include <sof/audio/source_api.h>
#include "module_interface.h"

/* The __ZEPHYR__ condition is to keep cmocka tests working */
#if CONFIG_MODULE_MEMORY_API_DEBUG && defined(__ZEPHYR__)
#include <zephyr/kernel/thread.h>
#endif
#include <sof/compiler_attributes.h>

/*
Expand Down Expand Up @@ -129,13 +126,11 @@ struct module_param {
* when the module unloads.
*/
struct module_resources {
struct k_mutex lock;
struct objpool_head objpool;
size_t heap_usage;
size_t heap_high_water_mark;
struct mod_alloc_ctx *alloc;
#if CONFIG_MODULE_MEMORY_API_DEBUG && defined(__ZEPHYR__)
k_tid_t rsrc_mngr;
#endif
};

enum mod_resource_type {
Expand Down Expand Up @@ -191,7 +186,12 @@ struct module_processing_data {
/*****************************************************************************/
int module_load_config(struct comp_dev *dev, const void *cfg, size_t size);
int module_init(struct processing_module *mod);
void *mod_balloc_align(struct processing_module *mod, size_t size, size_t alignment);
#if defined(__ZEPHYR__) && defined(CONFIG_SOF_FULL_ZEPHYR_APPLICATION)
__syscall void *mod_balloc_align(struct processing_module *mod, size_t size, size_t alignment);
#else
void *z_impl_mod_balloc_align(struct processing_module *mod, size_t size, size_t alignment);
#define mod_balloc_align z_impl_mod_balloc_align
#endif
void mod_resource_init(struct processing_module *mod);
void mod_heap_info(struct processing_module *mod, size_t *size, uintptr_t *start);
#if defined(__ZEPHYR__) && defined(CONFIG_SOF_FULL_ZEPHYR_APPLICATION)
Expand Down Expand Up @@ -241,7 +241,12 @@ static inline void *mod_zalloc(struct processing_module *mod, size_t size)
}

#if CONFIG_COMP_BLOB
struct comp_data_blob_handler *mod_data_blob_handler_new(struct processing_module *mod);
#if defined(__ZEPHYR__) && defined(CONFIG_SOF_FULL_ZEPHYR_APPLICATION)

@lyakh lyakh Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

not for this PR, but maybe we can drop globally the first part from these checks - I think we have a few of them across the code base

__syscall struct comp_data_blob_handler *mod_data_blob_handler_new(struct processing_module *mod);
#else
struct comp_data_blob_handler *z_impl_mod_data_blob_handler_new(struct processing_module *mod);
#define mod_data_blob_handler_new z_impl_mod_data_blob_handler_new
#endif
void mod_data_blob_handler_free(struct processing_module *mod, struct comp_data_blob_handler *dbh);
#endif
#if CONFIG_FAST_GET
Expand Down
Loading