alloc: sof_heap: Fix ring buffer memory allocation #10402
Conversation
Update comp_trigger_local to return the error code returned by the schedule_task function. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
Handle ring buffer allocation failure by jumping to the new free_unlocked label that skips ll_unblock. Do not release the lock at this point because it is acquired later. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
…lloc Add missing support for the SOF_MEM_FLAG_USER_SHARED_BUFFER flag in the sof_heap_alloc function. Add a generic function is_heap_pointer that allows checking whether a given pointer belongs to a given heap. It is used in the sof_heap_free function to ensure that the freed pointer belongs to the specified heap, and if not, to free it using rfree. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
softwarecki
requested review from
abonislawski,
Copilot,
serhiy-katsyuba-intel,
tmleman and
wjablon1
softwarecki
requested review from
dabekjakub,
dbaluta,
jxstelter,
kv2019i,
lbetlej,
lgirdwood,
mmaka1,
plbossart and
ranj063
as code owners
There was a problem hiding this comment.
Pull request overview
This PR addresses a memory allocation bug in the ring buffer handling introduced by PR #10379. The core issue was that the sof_heap_alloc function did not properly handle the SOF_MEM_FLAG_USER_SHARED_BUFFER flag, breaking userspace support. The fix ensures that buffers with this flag are allocated using rballoc_align instead of the standard heap allocator.
Key changes:
- Added proper handling of
SOF_MEM_FLAG_USER_SHARED_BUFFERflag in allocation/deallocation logic - Implemented heap pointer validation to ensure correct memory freeing
- Fixed error handling and propagation in component connection and trigger functions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| zephyr/lib/alloc.c | Added is_heap_pointer helper function and updated sof_heap_alloc/sof_heap_free to properly handle shared buffer flags and validate heap ownership |
| src/ipc/ipc4/helper.c | Fixed error handling path for ring buffer creation failure to avoid unlocking when lock wasn't acquired |
| src/include/sof/audio/component_ext.h | Added early return on trigger error and propagated schedule_task return value |
| src/audio/module_adapter/module_adapter.c | Initialized heap metadata fields required by new is_heap_pointer validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case COMP_TRIGGER_START: | ||
| case COMP_TRIGGER_RELEASE: | ||
| schedule_task(dev->task, 0, dev->period); | ||
| ret = schedule_task(dev->task, 0, dev->period); |
There was a problem hiding this comment.
The return value of schedule_task is captured but never checked or returned. If schedule_task fails after a successful trigger operation, the error is silently ignored. Consider checking ret and handling the error appropriately.
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Btw, if you want to point to a specific commit you are fixing, you can use the "Fixes: commit-id ("desription")" notation. This is optional on SOF side, but can be useful to augment the commit text.
| void sof_heap_free(struct k_heap *heap, void *addr) | ||
| { | ||
| if (heap && addr) | ||
| if (heap && addr && is_heap_pointer(heap, addr)) |
There was a problem hiding this comment.
This seems a bit complex, but this would indeed seem to be a needed check, also for SOF_MEM_FLAG_LARGE_BUFFER .
| * Checks whether pointer is from a given heap memory. | ||
| * @param heap Pointer to a heap. | ||
| * @param ptr Pointer to memory being checked. | ||
| * @return True if pointer falls into heap memory region, false otherwise. |
There was a problem hiding this comment.
Might be worth to spell out in the comments that this API doesn't tell us if memory ptr is currently allocated or free.
|
@softwarecki @lgirdwood This broke testbench and other non-zephyr builds https://github.com/thesofproject/sof/actions/runs/19784196486/job/56688397772 |
6 participants
This PR fixes a memory allocation bug for the ring buffer introduced by the hastily merged ##10379 . That change modified the way the ring buffer is allocated, switching to the
sof_heap_allocfunction. However, it did not properly handle theSOF_MEM_FLAG_USER_SHARED_BUFFERflag, which broke userspace support.Additionally, the error handling for ring buffer creation was improved in the
ipc_comp_connectfunction, and error code propagation was fixed in thecomp_trigger_localfunction.