Skip to content

ipc4: Handle typing errors in component retrieval #7528

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
cujomalainey merged 2 commits into from
May 2, 2023
Merged

ipc4: Handle typing errors in component retrieval #7528

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78d8382
ipc4: Handle typing errors in component retrieval
andyross Apr 27, 2023
a94fa88
IPC4: Handle duplicate component IDs
andyross May 2, 2023
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
35 changes: 23 additions & 12 deletions src/ipc/ipc4/helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,16 @@ struct comp_dev *comp_new_ipc4(struct ipc4_module_init_instance *module_init)
return dev;
}

static struct ipc_comp_dev *get_comp(struct ipc *ipc, uint16_t type, uint32_t id)
{
struct ipc_comp_dev *c = ipc_get_comp_by_id(ipc, id);

if (c && c->type == type)
Copy link
Collaborator

@ranj063 ranj063 Apr 27, 2023

Choose a reason for hiding this comment

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

@andyross assuming there are 2 different types of components with the same ID, wouldnt this always fail if the first component in the list is not for the specified type even if the other one exists in the list?

Copy link
Contributor Author

@andyross andyross Apr 27, 2023

Choose a reason for hiding this comment

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

It would, but by my reading that would be a bug in the design? The list as currently used is already populated with polymorphic components, and already assuming that the first entry with a matching ID is the one desired. Are there circumstances where it would be possible to have duplicate IDs?

Copy link
Collaborator

@ranj063 ranj063 Apr 27, 2023

Choose a reason for hiding this comment

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

With IPC3, the kernel generated unique comp_id's for each component as the list of components is being parsed from topology. And I think IPC4 follows the same rule. So in real devices, this is unlikely.

Copy link
Contributor Author

@andyross andyross Apr 27, 2023

Choose a reason for hiding this comment

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

Ah... OK, so if the component ID is input to the protocol, this actually is something subject to injected bugs that we'll need to address, I think. I'll take a look, could be as simple as traversing the list and erroring out at creation time.

Copy link
Contributor Author

@andyross andyross Apr 27, 2023

Choose a reason for hiding this comment

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

Indeed, I see it now. You're absolutely right, the initial component ID is set by the kernel, not the firmware. So there needs to be protection in the firmware against duplicates, I'll see what I can work up and attach it to this PR, as it's related. The fix here is protecting us against the results of the duplication (and even if there aren't duplicates, this remains an externally triggerable crash which we have to fix), but there is a rooter-cause that needs to be addressed too.

return c;

return NULL;
}

struct ipc_comp_dev *ipc_get_comp_by_ppl_id(struct ipc *ipc, uint16_t type, uint32_t ppl_id)
{
struct ipc_comp_dev *icd;
Expand Down Expand Up @@ -168,10 +178,9 @@ static int ipc4_create_pipeline(struct ipc4_pipeline_create *pipe_desc)
struct ipc *ipc = ipc_get();

/* check whether pipeline id is already taken or in use */
ipc_pipe = ipc_get_comp_by_ppl_id(ipc, COMP_TYPE_PIPELINE,
pipe_desc->primary.r.instance_id);
ipc_pipe = ipc_get_comp_by_id(ipc, pipe_desc->primary.r.instance_id);
if (ipc_pipe) {
tr_err(&ipc_tr, "ipc: pipeline id is already taken, pipe_desc->instance_id = %u",
tr_err(&ipc_tr, "ipc: comp id is already taken, pipe_desc->instance_id = %u",
(uint32_t)pipe_desc->primary.r.instance_id);
return IPC4_INVALID_RESOURCE_ID;
}
Expand Down Expand Up @@ -283,7 +292,7 @@ int ipc_pipeline_free(struct ipc *ipc, uint32_t comp_id)
int ret;

/* check whether pipeline exists */
ipc_pipe = ipc_get_comp_by_id(ipc, comp_id);
ipc_pipe = get_comp(ipc, COMP_TYPE_PIPELINE, comp_id);
if (!ipc_pipe)
return IPC4_INVALID_RESOURCE_ID;

Expand Down Expand Up @@ -608,7 +617,7 @@ int ipc4_pipeline_complete(struct ipc *ipc, uint32_t comp_id)
struct ipc_comp_dev *ipc_pipe;
int ret;

ipc_pipe = ipc_get_comp_by_id(ipc, comp_id);
ipc_pipe = get_comp(ipc, COMP_TYPE_PIPELINE, comp_id);

/* Pass IPC to target core */
if (!cpu_is_me(ipc_pipe->core))
Expand Down Expand Up @@ -728,21 +737,23 @@ const struct comp_driver *ipc4_get_comp_drv(int module_id)

struct comp_dev *ipc4_get_comp_dev(uint32_t comp_id)
{
struct ipc *ipc = ipc_get();
struct ipc_comp_dev *icd;

icd = ipc_get_comp_by_id(ipc, comp_id);
if (!icd)
return NULL;
struct ipc_comp_dev *icd = get_comp(ipc_get(), COMP_TYPE_COMPONENT, comp_id);

return icd->cd;
return icd ? icd->cd : NULL;
}

int ipc4_add_comp_dev(struct comp_dev *dev)
{
struct ipc *ipc = ipc_get();
struct ipc_comp_dev *icd;

/* check id for duplicates */
icd = ipc_get_comp_by_id(ipc, dev->ipc_config.id);
if (icd) {
tr_err(&ipc_tr, "ipc: duplicate component ID");
return IPC4_INVALID_RESOURCE_ID;
}

/* allocate the IPC component container */
icd = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM,
sizeof(struct ipc_comp_dev));
Expand Down