Skip to content

audio: fix overriding const attribute #10118

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 3 commits into from
Jul 22, 2025
Merged

audio: fix overriding const attribute #10118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
83506c8
audio: chain_dma: (cosmetic) remove function names from logs
lyakh Jul 15, 2025
cb8861a
audio: chain_dma: don't abuse driver list lock
lyakh Jul 15, 2025
200d822
audio: don't override the "const" attribute
lyakh Jul 15, 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
72 changes: 29 additions & 43 deletions src/audio/chain_dma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ static int chain_link_start(struct comp_dev *dev)
if (err < 0)
return err;

comp_info(dev, "chain_link_start(): dma_start() link chan_index = %u",
comp_info(dev, "dma_start() link chan_index = %u",
cd->chan_link->index);
return 0;
}
Expand All @@ -112,7 +112,7 @@ static int chain_link_stop(struct comp_dev *dev)
if (err < 0)
return err;

comp_info(dev, "chain_link_stop(): dma_stop() link chan_index = %u",
comp_info(dev, "dma_stop() link chan_index = %u",
cd->chan_link->index);

return 0;
Expand All @@ -127,7 +127,7 @@ static int chain_host_stop(struct comp_dev *dev)
if (err < 0)
return err;

comp_info(dev, "chain_host_stop(): dma_stop() host chan_index = %u",
comp_info(dev, "dma_stop() host chan_index = %u",
cd->chan_host->index);

return 0;
Expand Down Expand Up @@ -187,11 +187,11 @@ static enum task_state chain_task_run(void *data)
case 0:
break;
case -EPIPE:
tr_warn(&chain_dma_tr, "chain_task_run(): dma_get_status() link xrun occurred,"
tr_warn(&chain_dma_tr, "dma_get_status() link xrun occurred,"
" ret = %d", ret);
break;
default:
tr_err(&chain_dma_tr, "chain_task_run(): dma_get_status() error, ret = %d", ret);
tr_err(&chain_dma_tr, "dma_get_status() error, ret = %d", ret);
return SOF_TASK_STATE_COMPLETED;
}

Expand All @@ -202,7 +202,7 @@ static enum task_state chain_task_run(void *data)
/* Host DMA does not report xruns. All error values will be treated as critical. */
ret = chain_get_dma_status(cd, cd->chan_host, &stat);
if (ret < 0) {
tr_err(&chain_dma_tr, "chain_task_run(): dma_get_status() error, ret = %d", ret);
tr_err(&chain_dma_tr, "dma_get_status() error, ret = %d", ret);
return SOF_TASK_STATE_COMPLETED;
}

Expand All @@ -221,14 +221,14 @@ static enum task_state chain_task_run(void *data)
ret = dma_reload(cd->chan_host->dma->z_dev, cd->chan_host->index, 0, 0, increment);
if (ret < 0) {
tr_err(&chain_dma_tr,
"chain_task_run(): dma_reload() host error, ret = %d", ret);
"dma_reload() host error, ret = %d", ret);
return SOF_TASK_STATE_COMPLETED;
}

ret = dma_reload(cd->chan_link->dma->z_dev, cd->chan_link->index, 0, 0, increment);
if (ret < 0) {
tr_err(&chain_dma_tr,
"chain_task_run(): dma_reload() link error, ret = %d", ret);
"dma_reload() link error, ret = %d", ret);
return SOF_TASK_STATE_COMPLETED;
}
} else {
Expand All @@ -246,8 +246,7 @@ static enum task_state chain_task_run(void *data)
half_buff_size);
if (ret < 0) {
tr_err(&chain_dma_tr,
"chain_task_run(): dma_reload() link error, ret = %d",
ret);
"dma_reload() link error, ret = %d", ret);
return SOF_TASK_STATE_COMPLETED;
}
cd->first_data_received = true;
Expand All @@ -262,7 +261,7 @@ static enum task_state chain_task_run(void *data)
0, 0, transferred);
if (ret < 0) {
tr_err(&chain_dma_tr,
"chain_task_run(): dma_reload() host error, ret = %d", ret);
"dma_reload() host error, ret = %d", ret);
return SOF_TASK_STATE_COMPLETED;
}

Expand All @@ -271,8 +270,8 @@ static enum task_state chain_task_run(void *data)
ret = dma_reload(cd->chan_link->dma->z_dev, cd->chan_link->index,
0, 0, half_buff_size);
if (ret < 0) {
tr_err(&chain_dma_tr, "chain_task_run(): dma_reload() "
"link error, ret = %d", ret);
tr_err(&chain_dma_tr,
"dma_reload() link error, ret = %d", ret);
return SOF_TASK_STATE_COMPLETED;
}
}
Expand All @@ -283,17 +282,13 @@ static enum task_state chain_task_run(void *data)

static int chain_task_start(struct comp_dev *dev)
{
struct comp_driver_list *drivers = comp_drivers_get();
struct chain_dma_data *cd = comp_get_drvdata(dev);
k_spinlock_key_t key;
int ret;

comp_info(dev, "chain_task_start(), host_dma_id = 0x%08x", cd->host_connector_node_id.dw);
comp_info(dev, "host_dma_id = 0x%08x", cd->host_connector_node_id.dw);

key = k_spin_lock(&drivers->lock);
switch (cd->chain_task.state) {
case SOF_TASK_STATE_QUEUED:
k_spin_unlock(&drivers->lock, key);
Copy link
Contributor

@jsarha jsarha Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Even if the used lock is wrong, it does not automatically mean that no locking is needed. Even the wrong lock protects against interleaved function calls. I have no idea if those can happen, so this is just a worried comment.

Copy link
Member

@lgirdwood lgirdwood Jul 22, 2025

Choose a reason for hiding this comment

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

@lyakh I suppose you could followup with a comment declaring where the lock is held for the state check.

return 0;
case SOF_TASK_STATE_COMPLETED:
break;
Expand All @@ -302,70 +297,64 @@ static int chain_task_start(struct comp_dev *dev)
case SOF_TASK_STATE_FREE:
break;
default:
comp_err(dev, "chain_task_start(), bad state transition");
ret = -EINVAL;
goto error;
comp_err(dev, "bad state transition");
return -EINVAL;
}

if (cd->stream_direction == SOF_IPC_STREAM_PLAYBACK) {
ret = chain_host_start(dev);
if (ret)
goto error;
return ret;
ret = chain_link_start(dev);
if (ret) {
chain_host_stop(dev);
goto error;
return ret;
}
} else {
ret = chain_link_start(dev);
if (ret)
goto error;
return ret;
ret = chain_host_start(dev);
if (ret) {
chain_link_stop(dev);
goto error;
return ret;
}
}

ret = schedule_task_init_ll(&cd->chain_task, SOF_UUID(chain_dma_uuid),
SOF_SCHEDULE_LL_TIMER, SOF_TASK_PRI_HIGH,
chain_task_run, cd, 0, 0);
if (ret < 0) {
comp_err(dev, "chain_task_start(), ll task initialization failed");
comp_err(dev, "ll task initialization failed");
goto error_task;
}

ret = schedule_task(&cd->chain_task, 0, 0);
if (ret < 0) {
comp_err(dev, "chain_task_start(), ll schedule task failed");
comp_err(dev, "ll schedule task failed");
schedule_task_free(&cd->chain_task);
goto error_task;
}

pm_policy_state_lock_get(PM_STATE_RUNTIME_IDLE, PM_ALL_SUBSTATES);
k_spin_unlock(&drivers->lock, key);

return 0;

error_task:
chain_host_stop(dev);
chain_link_stop(dev);
error:
k_spin_unlock(&drivers->lock, key);

return ret;
}

static int chain_task_pause(struct comp_dev *dev)
{
struct comp_driver_list *drivers = comp_drivers_get();
struct chain_dma_data *cd = comp_get_drvdata(dev);
k_spinlock_key_t key;
int ret, ret2;

if (cd->chain_task.state == SOF_TASK_STATE_FREE)
return 0;

key = k_spin_lock(&drivers->lock);
cd->first_data_received = false;
if (cd->stream_direction == SOF_IPC_STREAM_PLAYBACK) {
ret = chain_host_stop(dev);
Expand All @@ -377,8 +366,6 @@ static int chain_task_pause(struct comp_dev *dev)
if (!ret)
ret = ret2;

k_spin_unlock(&drivers->lock, key);

schedule_task_free(&cd->chain_task);
pm_policy_state_lock_put(PM_STATE_RUNTIME_IDLE, PM_ALL_SUBSTATES);

Expand Down Expand Up @@ -491,23 +478,23 @@ __cold static int chain_init(struct comp_dev *dev, void *addr, size_t length)

err = dma_config(cd->dma_host->z_dev, cd->chan_host->index, dma_cfg_host);
if (err < 0) {
comp_err(dev, "chain_init(): dma_config() failed");
comp_err(dev, "dma_config() failed");
goto error_host;
}

/* get link DMA channel */
channel = cd->link_connector_node_id.f.v_index;
channel = dma_request_channel(cd->dma_link->z_dev, &channel);
if (channel < 0) {
comp_err(dev, "chain_init(): dma_request_channel() failed");
comp_err(dev, "dma_request_channel() failed");
goto error_host;
}

cd->chan_link = &cd->dma_link->chan[channel];

err = dma_config(cd->dma_link->z_dev, cd->chan_link->index, dma_cfg_link);
if (err < 0) {
comp_err(dev, "chain_init(): dma_config() failed");
comp_err(dev, "dma_config() failed");
goto error_link;
}
return 0;
Expand Down Expand Up @@ -559,7 +546,7 @@ __cold static int chain_task_init(struct comp_dev *dev, uint8_t host_dma_id, uin

cd->dma_host = sof_dma_get(dir, 0, SOF_DMA_DEV_HOST, SOF_DMA_ACCESS_SHARED);
if (!cd->dma_host) {
comp_err(dev, "chain_task_init(): dma_get() returned NULL");
comp_err(dev, "dma_get() returned NULL");
return -EINVAL;
}

Expand All @@ -569,7 +556,7 @@ __cold static int chain_task_init(struct comp_dev *dev, uint8_t host_dma_id, uin
cd->dma_link = sof_dma_get(dir, SOF_DMA_CAP_HDA, SOF_DMA_DEV_HDA, SOF_DMA_ACCESS_SHARED);
if (!cd->dma_link) {
sof_dma_put(cd->dma_host);
comp_err(dev, "chain_task_init(): dma_get() returned NULL");
comp_err(dev, "dma_get() returned NULL");
return -EINVAL;
}

Expand All @@ -578,8 +565,7 @@ __cold static int chain_task_init(struct comp_dev *dev, uint8_t host_dma_id, uin
&addr_align);
if (ret < 0) {
comp_err(dev,
"chain_task_init(): could not get dma buffer address alignment, err = %d",
ret);
"could not get dma buffer address alignment, err = %d", ret);
goto error;
}

Expand Down Expand Up @@ -609,7 +595,7 @@ __cold static int chain_task_init(struct comp_dev *dev, uint8_t host_dma_id, uin
addr_align, false);

if (!cd->dma_buffer) {
comp_err(dev, "chain_task_init(): failed to alloc dma buffer");
comp_err(dev, "failed to alloc dma buffer");
ret = -EINVAL;
goto error;
}
Expand Down
31 changes: 31 additions & 0 deletions src/audio/component.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <rtos/alloc.h>
#include <rtos/cache.h>
#include <sof/lib/memory.h> /* for SHARED_DATA */
#include <sof/lib/uuid.h>
#include <sof/list.h>
#include <rtos/sof.h>
#include <rtos/string.h>
Expand Down Expand Up @@ -63,6 +64,36 @@ void comp_unregister(struct comp_driver_info *drv)
k_spin_unlock(&drivers->lock, key);
}

int comp_set_adapter_ops(const struct comp_driver *drv, const struct module_interface *ops)
{
struct comp_driver_list *drivers = comp_drivers_get();
struct list_item *clist;

/* The list is only modified in IPC context, and we're in IPC context too */
list_for_item(clist, &drivers->list) {
struct comp_driver_info *info = container_of(clist, struct comp_driver_info, list);

if (!memcmp(info->drv->uid, drv->uid, UUID_SIZE)) {
/*
* This function should only be called for dynamically
* loaded component drivers and their driver info cannot
* be NULL. Do a sanity check.
*/
if (!info->adapter_ops) {
tr_err(&comp_tr, "NULL adapter ops ptr for %pU!",
info->drv->tctx->uuid_p);
return -EINVAL;
}

tr_dbg(&comp_tr, "update uuid %pU", info->drv->tctx->uuid_p);
*info->adapter_ops = ops;
return 0;
}
}

return -ENODEV;
}

/* NOTE: Keep the component state diagram up to date:
* sof-docs/developer_guides/firmware/components/images/comp-dev-states.pu
*/
Expand Down
14 changes: 12 additions & 2 deletions src/include/sof/audio/component.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,8 +594,9 @@ struct comp_driver {

/** \brief Holds constant pointer to component driver */
struct comp_driver_info {
const struct comp_driver *drv; /**< pointer to component driver */
struct list_item list; /**< list of component drivers */
const struct comp_driver *drv; /**< pointer to component driver */
const struct module_interface **adapter_ops; /**< pointer for updating ops */
struct list_item list; /**< list of component drivers */
};

#define COMP_PROCESSING_DOMAIN_LL 0
Expand Down Expand Up @@ -968,6 +969,15 @@ int comp_register(struct comp_driver_info *drv);
*/
void comp_unregister(struct comp_driver_info *drv);

/**
* Set adapter ops for a dynamically created driver.
*
* @param drv Component driver to update.
* @param ops Module interface operations.
* @return 0 or a negative error code
*/
int comp_set_adapter_ops(const struct comp_driver *drv, const struct module_interface *ops);

/** @}*/

/**
Expand Down
13 changes: 8 additions & 5 deletions src/library_manager/lib_manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,17 +518,19 @@ static struct comp_dev *lib_manager_module_create(const struct comp_driver *drv,
/* Intel modules expects DW size here */
mod_cfg.size = args->size >> 2;

((struct comp_driver *)drv)->adapter_ops = native_system_agent_start(module_entry_point,
module_id, instance_id,
0, log_handle,
&mod_cfg);
const struct module_interface *ops = native_system_agent_start(module_entry_point,
module_id, instance_id,
0, log_handle, &mod_cfg);

if (!drv->adapter_ops) {
if (!ops) {
lib_manager_free_module(module_id);
tr_err(&lib_manager_tr, "native_system_agent_start failed!");
return NULL;
}

if (comp_set_adapter_ops(drv, ops) < 0)
return NULL;

dev = module_adapter_new(drv, config, spec);
if (!dev)
lib_manager_free_module(module_id);
Expand Down Expand Up @@ -662,6 +664,7 @@ int lib_manager_register_module(const uint32_t component_id)

/* Fill the new_drv_info structure with already known parameters */
new_drv_info->drv = drv;
new_drv_info->adapter_ops = &drv->adapter_ops;

/* Register new driver in the list */
ret = comp_register(new_drv_info);
Expand Down
Loading