Skip to content

[user-space] part 0: preliminary trivial clean up patches #10291

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

Merged
lgirdwood merged 4 commits into from
Oct 13, 2025
Merged

[user-space] part 0: preliminary trivial clean up patches #10291

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
22b85f5
component: split a function into two
lyakh Aug 21, 2025
9b473d7
module: add a missing header
lyakh Oct 2, 2025
94bcb3a
probe: fix a variable size structure member
lyakh Oct 2, 2025
65bd156
library-manager: (cosmetic) make a function static
lyakh Oct 8, 2025
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
4 changes: 4 additions & 0 deletions src/include/module/module/base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@
#include "interface.h"
#include "../ipc4/base-config.h"

#ifdef SOF_MODULE_API_PRIVATE
#include <sof/list.h>
#endif

#define module_get_private_data(mod) ((mod)->priv.private)
#define module_set_private_data(mod, data) ((mod)->priv.private = data)

Expand Down
35 changes: 24 additions & 11 deletions src/include/sof/audio/component.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,24 @@ static inline enum sof_comp_type dev_comp_type(const struct comp_dev *dev)
return dev->ipc_config.type;
}

/**
* Initialize common part of a component device
* @param drv Parent component driver.
* @param dev Device.
* @param bytes Size of the component device in bytes.
*/
static inline void comp_init(const struct comp_driver *drv,
struct comp_dev *dev, size_t bytes)
{
dev->size = bytes;
dev->drv = drv;
dev->state = COMP_STATE_INIT;
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));
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
trace_comp_drv_get_tr_ctx(dev->drv), sizeof(struct tr_ctx));
trace_comp_drv_get_tr_ctx(dev->drv), sizeof(dev->tctx));

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@lyakh lyakh Oct 9, 2025

Choose a reason for hiding this comment

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

can someone get us a smarter copilot? :-/

Copy link
Collaborator

@ranj063 ranj063 Oct 9, 2025

Choose a reason for hiding this comment

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

come on, be nice, it's probably a highschool intern!

Copy link
Contributor

@ujfalusi ujfalusi Oct 10, 2025

Choose a reason for hiding this comment

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

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))

Copy link
Collaborator Author

@lyakh lyakh Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}

/**
* Allocates memory for the component device and initializes common part.
* @param drv Parent component driver.
Expand All @@ -857,23 +875,18 @@ static inline enum sof_comp_type dev_comp_type(const struct comp_dev *dev)
*/
static inline struct comp_dev *comp_alloc(const struct comp_driver *drv, size_t bytes)
{
struct comp_dev *dev = NULL;

/*
* Use uncached address everywhere to access components to rule out
* multi-core failures. TODO: verify if cached alias may be used in some cases
*/
dev = module_driver_heap_rzalloc(drv->user_heap, SOF_MEM_FLAG_USER | SOF_MEM_FLAG_COHERENT,
bytes);
struct comp_dev *dev = module_driver_heap_rzalloc(drv->user_heap,
SOF_MEM_FLAG_USER | SOF_MEM_FLAG_COHERENT,
bytes);

if (!dev)
return NULL;
dev->size = bytes;
dev->drv = drv;
dev->state = COMP_STATE_INIT;
list_init(&dev->bsink_list);
list_init(&dev->bsource_list);
memcpy_s(&dev->tctx, sizeof(struct tr_ctx),
trace_comp_drv_get_tr_ctx(dev->drv), sizeof(struct tr_ctx));

comp_init(drv, dev, bytes);

return dev;
}
Expand Down
6 changes: 3 additions & 3 deletions src/library_manager/lib_manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,8 +412,8 @@ int lib_manager_free_module(const uint32_t component_id)

#define PAGE_SZ 4096 /* equals to MAN_PAGE_SIZE used by rimage */

uintptr_t lib_manager_allocate_module(const struct comp_ipc_config *ipc_config,
const void *ipc_specific_config, const void **buildinfo)
static uintptr_t lib_manager_allocate_module(const struct comp_ipc_config *ipc_config,
const void *ipc_specific_config, const void **buildinfo)
{
tr_err(&lib_manager_tr, "Dynamic module allocation is not supported");
return 0;
Expand Down Expand Up @@ -578,7 +578,7 @@ static struct comp_dev *lib_manager_module_create(const struct comp_driver *drv,
const void *spec)
{
const struct sof_man_fw_desc *const desc = lib_manager_get_library_manifest(config->id);
const struct ipc_config_process *args = (struct ipc_config_process *)spec;
const struct ipc_config_process *args = (const struct ipc_config_process *)spec;
const uint32_t entry_index = LIB_MANAGER_GET_MODULE_INDEX(config->id);
const struct module_interface *ops = NULL;
const struct sof_man_module *mod;
Expand Down
2 changes: 1 addition & 1 deletion src/probe/probe.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,11 @@ struct probe_dma_ext {
* Probe main struct
*/
struct probe_pdata {
struct task dmap_work; /**< probe task */
struct probe_dma_ext ext_dma; /**< extraction DMA */
struct probe_dma_ext inject_dma[CONFIG_PROBE_DMA_MAX]; /**< injection DMA */
struct probe_point probe_points[CONFIG_PROBE_POINTS_MAX]; /**< probe points */
struct probe_data_packet header; /**< data packet header */
struct task dmap_work; /**< probe task */
};

/**
Expand Down
Loading