Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New memory domain logic #5537
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
base: topic/sof-dev
Are you sure you want to change the base?
New memory domain logic #5537
Changes from all commits
b95629da24efcf6a1a8780922f6caef31162dec6319818189bb00e2537f470eFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
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.
The topology tokens were not really used for anything yet on the firmware side so its safe change the definitions.
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.
I suppose in theory these can overflow... We don't expect any realistic configurations to do that, but a malicious one could, especially once we start loading unsigned userspace modules
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.
I do not think adding any safeguards to the Linux driver side serves any purpose. The FW however should have some sanity checks about the values give to it through IPC.
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.
It looks like the Copilot does not have the slightest idea of what is going on here.
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.
Not just the copilot :) I assume pipeline object is built from scratch, so this reset logic is needed, but good to double-check. But I'm ok if you have checked for new pipelines we are starting from zero always (and we have no code-paths to refresh same widgets again).
Uh oh!
There was an error while loading. Please reload this page.
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.
nitpick: I'd do
if (!hdr) {kfree(payload); return 0;}and then continue below with the restThere 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.
I wanted to have the both sides of the if statement, because it may be that sof_ipc4_add_init_ext_dp_memory_data() did not add an object to the object array, and then we should not add the array at all, let alone mark the last element. There was an ext init payload even before my time and there are some bits defined there, so we may need to add the payload even if the array is not present. In that situation if (hdr) part remains, but the else branch should be removed. If I am to do something about this, then that would be adding the non array part always, e.g removing the the else branch.
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.
sorry, my proposal wasn't really addressing the meaning of the code, it was rather cosmetic with no change to functionality. What this your hunk does is organise code as follows:
which means that
a,b,canddare always either executed or not executed together, so making some of them under theifand others outside of it is a bit strange. And that's exactly equivalent towhich IMHO is more straight-forward. Also regarding your remark about removing the
elsepart - the above would make it even easier by removing the whole ofif.Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.