ipc3: override type field once comp_driver found#9496
ipc3: override type field once comp_driver found#9496lgirdwood merged 1 commit intothesofproject:mainfrom
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.
wouldn't it be better to check and to error out in case of mismatch?
There was a problem hiding this comment.
if not possible to error out then due to usersapce, then it could be logged.
There was a problem hiding this comment.
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.
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.
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.
@lgirdwood @lyakh would be great to get clarification on this, this is blocking the fuzzer currently
There was a problem hiding this comment.
@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.
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.
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.