-
Notifications
You must be signed in to change notification settings - Fork 318
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.
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
ipc3: override type field once comp_driver found #9496
Conversation
src/ipc/ipc3/helper.c
Outdated
/* Hack to fix mismatch between uuid and type as most downstream | ||
* systems match on the type but get_drv will match on uuid if present. | ||
**/ | ||
comp->type = drv->type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it be better to check and to error out in case of mismatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not possible to error out then due to usersapce, then it could be logged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can error out as well in get_drv. I figured this was the most likely way to preserve existing functionality rather than breaking it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like regardless of what way I do it, unit tests fail. I am curious what on device testing looks like @wszypelt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LNL test appear to pass but unit tests do not.
Here are my thoughts on the two solutions on their ups/downs
override | expect match | |
---|---|---|
upside | should always work regardless of state of host / topology | requires correctness |
downside | - we are overriding host information | 1.will break any topology that is not set correctly 2.may break with module adapter changes as enum changes with that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lgirdwood @lyakh would be great to get clarification on this, this is blocking the fuzzer currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lgirdwood @lyakh would be great to get clarification on this, this is blocking the fuzzer currently
@cujomalainey you mean a clarification of unit test failures? Sorry, I don't have an answer off the top of my head, I guess it just needs to be debugged and fixed. Supposedly unit tests use those fields in some different incompatible with our assumptions here way. @singalsu would you have an idea maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, which one is better given both have risks of topology mismatch, if we should do override or check enum + uuid. The double checks i listed above, just curious if there is any thoughts on the risks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cujomalainey I'm for correctness FWIW
A bad IPC can mismatch UUID and type, causing downstream processes to operate differently than the final component target. This is because get_drv will not reject the mismatch and given we don't know the state of topology we can't start now. Instead we can override the ipc with the proper type. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
80164bc
to
a4ccc5d
Compare
For the record, this was merged broken: https://github.com/thesofproject/sof/actions/runs/10964848537/job/30449371562
https://github.com/thesofproject/sof/actions/runs/10964848523/job/30449371607
(On the same day when unrelated #9475 (comment) was also merged broken) |
A bad IPC can mismatch UUID and type, causing downstream processes to operate differently than the final component target. This is because get_drv will not reject the mismatch and given we don't know the state of topology we can't start now. Instead we can override the ipc with the proper type.