Skip to content

ipc3: get_drv: Check comp_type also when driver is matched with UUID #7891

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

Closed
jsarha wants to merge 1 commit into from
Closed

ipc3: get_drv: Check comp_type also when driver is matched with UUID #7891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/audio/module_adapter/module/volume/volume.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1671,7 +1671,7 @@ static struct module_interface gain_interface = {
.free = volume_free
};

DECLARE_MODULE_ADAPTER(gain_interface, gain_uuid, gain_tr);
DECLARE_MODULE_ADAPTER_WITH_TYPE(gain_interface, gain_uuid, gain_tr, SOF_COMP_VOLUME);
SOF_MODULE_INIT(gain, sys_comp_module_gain_interface_init);
#endif
#endif
2 changes: 1 addition & 1 deletion src/audio/src/src.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,5 +1058,5 @@ static struct module_interface src_interface = {
.free = src_free,
};

DECLARE_MODULE_ADAPTER(src_interface, src_uuid, src_tr);
DECLARE_MODULE_ADAPTER_WITH_TYPE(src_interface, src_uuid, src_tr, SOF_COMP_SRC);
SOF_MODULE_INIT(src, sys_comp_module_src_interface_init);
7 changes: 5 additions & 2 deletions src/include/sof/audio/module_adapter/module/generic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
(value)); \
} while (0)

#define DECLARE_MODULE_ADAPTER(adapter, uuid, tr) \
#define DECLARE_MODULE_ADAPTER_WITH_TYPE(adapter, uuid, tr, comp_type) \
static struct comp_dev *module_##adapter##_shim_new(const struct comp_driver *drv, \
const struct comp_ipc_config *config, \
const void *spec) \
Expand All @@ -42,7 +42,7 @@ static struct comp_dev *module_##adapter##_shim_new(const struct comp_driver *dr
} \
\
static const struct comp_driver comp_##adapter##_module = { \
.type = SOF_COMP_MODULE_ADAPTER, \
.type = comp_type, \
Copy link
Collaborator

@lyakh lyakh Jul 14, 2023

Choose a reason for hiding this comment

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

this is a rather significant change, and I think it will break at least this check:

sof/src/audio/module_adapter/module_adapter.c

Line 135 in 7027228

if (drv->type == SOF_COMP_MODULE_ADAPTER) {
and possibly more. Whereas my proposal to change comp_specific_builder() would only affect IPC3. @ranj063 do you think this is a good idea?

Copy link
Contributor Author

@jsarha jsarha Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@lyakh , but how would I know in comp_specific_builder() what kind of messages the module - found with uuid - actually accepts, unless its somehow built in to the module? I can of course add an extra member to struct comp_driver, but I do not think this issue can not be solved without adding the accepted component message type to each module.

Copy link
Collaborator

@lyakh lyakh Jul 14, 2023

Choose a reason for hiding this comment

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

No, don't think it's possible to check the type generically in common code for all components with the current module-adapter API. That's why I'm not proposing that. Instead I'm just proposing to (1) check that the type value in IPC is meaningful - one of the values, that the firmware recognises, and error out otherwise, instead of silently ignoring it, and (2) in each individual component driver check that the type from the IPC is correct

Copy link
Contributor Author

@jsarha jsarha Jul 14, 2023

Choose a reason for hiding this comment

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

Ok, simple enough.

.uid = SOF_RT_UUID(uuid), \
.tctx = &(tr), \
.ops = { \
Expand Down Expand Up @@ -75,6 +75,9 @@ UT_STATIC void sys_comp_module_##adapter##_init(void) \
\
DECLARE_MODULE(sys_comp_module_##adapter##_init)

#define DECLARE_MODULE_ADAPTER(adapter, uuid, tr) \
DECLARE_MODULE_ADAPTER_WITH_TYPE(adapter, uuid, tr, SOF_COMP_MODULE_ADAPTER)

/**
* \enum module_state
* \brief Module-specific states
Expand Down
18 changes: 15 additions & 3 deletions src/ipc/ipc3/helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ static const struct comp_driver *get_drv(struct sof_ipc_comp *comp)
goto out;
}

if (comp->type > SOF_COMP_MODULE_ADAPTER) {
tr_err(&comp_tr, "Bad component type: %d\n", comp->type);
goto out;
Copy link
Collaborator

@lyakh lyakh Jul 5, 2023

Choose a reason for hiding this comment

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

goto out with drv == NULL is equivalent to return NULL really, but they do the goto everywhere in this function...

Copy link
Contributor Author

@jsarha jsarha Jul 5, 2023

Choose a reason for hiding this comment

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

Yes... maybe in the history lock have been kept trough out the function. No point for the label really now. I can drop it, unless we change the approach again...

}

/* search driver list with UUID */
key = k_spin_lock(&drivers->lock);

Expand All @@ -134,16 +139,23 @@ static const struct comp_driver *get_drv(struct sof_ipc_comp *comp)
}
}

if (!drv)
k_spin_unlock(&drivers->lock, key);

if (!drv) {
tr_err(&comp_tr,
"get_drv(): the provided UUID (%8x%8x%8x%8x) doesn't match to any driver!",
*(uint32_t *)(&comp_ext->uuid[0]),
*(uint32_t *)(&comp_ext->uuid[4]),
*(uint32_t *)(&comp_ext->uuid[8]),
*(uint32_t *)(&comp_ext->uuid[12]));
goto out;
}

k_spin_unlock(&drivers->lock, key);

if (drv->type != comp->type && drv->type != SOF_COMP_MODULE_ADAPTER) {
Copy link
Collaborator

@lyakh lyakh Jul 5, 2023

Choose a reason for hiding this comment

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

what exactly is this condition: drv->type == SOF_COMP_MODULE_ADAPTER? So it's possible to have a component in topology with UUID, matching SOF_COMP_MODULE_ADAPTER but with a different type? Or how is this possible?

Copy link
Contributor

@cujomalainey cujomalainey Jul 5, 2023

Choose a reason for hiding this comment

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

I see what you are doing here, but it doesn't validate that the comp type is the right type. E.g. this still leaves the possibility for a SRC UUID with a VOL ENUM.

@lyakh how terrible would it be if we just moved that busted switch case out to each component and just container_of our way back to the correct pointer for vol and src?

Copy link
Contributor Author

@jsarha jsarha Jul 5, 2023

Choose a reason for hiding this comment

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

For instance MUX components use module adapter. I could not figure out how dig out what is hidden behind the module adapter framework. The nature of IPC3 is still quite strange to me.

Copy link
Contributor

@cujomalainey cujomalainey Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

bleh, my container_of approach won't work since the data is parsed and copied, not aliased.

Honestly I am more and more inclined to just suggest we go back to #7830 (with an additional guard for vol) and move that check into a IPC3 only block. There is no easy way fix the protocol without making a major revision (deprecating the enum field) and fixing legacy component init payloads. Also if we ifdef it around IPC3 it will be very clear it can be deleted when IPC3 is removed from the repo.

Copy link
Contributor

@btian1 btian1 Jul 7, 2023

Choose a reason for hiding this comment

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

we can try to modify module_adapter.c to resolve this issue.

Copy link
Collaborator

@lyakh lyakh Jul 7, 2023

Choose a reason for hiding this comment

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

@cujomalainey @jsarha it seems to me that we just have to go with the "first principles" here: if we don't trust what we get over IPC, then we need to check everything. E.g. .type is checked in comp_specific_builder() but unrecognised type is just silently ignored and that doesn't seem right to me. Because if we do happen to have comp->ext_data_length != 0 then get_drv() might use UUID to get a matching driver and we'll continue with an invalid type.
Then we also get size from those topology IPCs. And in general it's valid for module-adapter components to have a size of 0. In that case no initialisation data is allocated. So individual drivers must check for that, yes.

tr_err(&comp_tr, "get_drv(): Found %pU driver and IPC type differ %d != %d",
drv->tctx->uuid_p, drv->type, comp->type);
drv = NULL;
}
out:
if (drv)
tr_dbg(&comp_tr, "get_drv(), found driver type %d, uuid %pU",
Expand Down