[user-space] part 0: preliminary trivial clean up patches #10291
Conversation
Split comp_alloc() into two parts - allocation and initialisation to be able to re-use the initialisation code with a different allocation method. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The private part of struct struct processing_module contains a list head, add the respective header. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Variable size structure members are only allowed at the end. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Make lib_manager_allocate_module() static also for the CONFIG_MM_DRV=n case. Also don't drop the const qualifier needlessly. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lyakh
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR performs preliminary cleanup and preparation work for a larger user-space effort. The changes focus on code organization, consistency improvements, and proper function visibility.
Key changes include:
- Reordering struct members for better organization
- Adding proper static keyword for internal functions
- Extracting common initialization logic into a reusable helper function
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/probe/probe.c | Reorders struct member to group task-related field with other members |
| src/library_manager/lib_manager.c | Adds static keyword and const qualifier for better encapsulation |
| src/include/sof/audio/component.h | Extracts common initialization logic into reusable comp_init function |
| src/include/module/module/base.h | Adds conditional include for SOF_MODULE_API_PRIVATE builds |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| list_init(&dev->bsink_list); | ||
| list_init(&dev->bsource_list); | ||
| memcpy_s(&dev->tctx, sizeof(dev->tctx), | ||
| trace_comp_drv_get_tr_ctx(dev->drv), sizeof(struct tr_ctx)); |
There was a problem hiding this comment.
The memcpy_s call uses inconsistent sizing - first parameter uses sizeof(dev->tctx) but the last parameter uses sizeof(struct tr_ctx). For clarity and safety, both should use sizeof(dev->tctx) or both should use sizeof(struct tr_ctx) to ensure they match.
| trace_comp_drv_get_tr_ctx(dev->drv), sizeof(struct tr_ctx)); | |
| trace_comp_drv_get_tr_ctx(dev->drv), sizeof(dev->tctx)); |
There was a problem hiding this comment.
can someone get us a smarter copilot? :-/
There was a problem hiding this comment.
come on, be nice, it's probably a highschool intern!
There was a problem hiding this comment.
to be fair, the original code used two sizeof(struct tr_ctx):
eb96c62 ("trace: comp: Add trace context for each component instance")
Since both src and dst is the same struct with the same size why not use just memcpy(&dev->tctx, trace_comp_drv_get_tr_ctx(dev->drv), sizeof(dev->tctx));? (or s/sizeof(dev->tctx)/sizeof(struct tr_ctx))
There was a problem hiding this comment.
to be fair, the original code used two
sizeof(struct tr_ctx): eb96c62 ("trace: comp: Add trace context for each component instance")Since both src and dst is the same struct with the same size why not use just
memcpy(&dev->tctx, trace_comp_drv_get_tr_ctx(dev->drv), sizeof(dev->tctx));? (or s/sizeof(dev->tctx)/sizeof(struct tr_ctx))
@ujfalusi correct, it did use the same sizes before and I've corrected that. I'm used to using memcpy() myself and would be happy to continue using it. But certain powers in the Universe think, that memcpy_s() is more secure... So, yes, I'd just use memcpy(&dev->tctx, trace_comp_drv_get_tr_ctx(dev->drv), sizeof(dev->tctx)) but if we have to use memcpy_s() then let's use it correctly - which in accordance with its SOF implementation means, that the first size is the size of the destination buffer and the second size is the size that has to be copied.
5 participants
Some trivial preparation and clean up.