Skip to content
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

zephyr: lib: alloc: k_panic is not required in failure scenario #8853

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

jxstelter
Copy link
Contributor

For rballoc_allign() call when caps are not correct it is enough to return error. k_panic() call is not required here. Previous change exposed this issue: #8832, but it is sufficient to log error and return NULL at this point.

Signed-off-by: Jaroslaw Stelter Jaroslaw.Stelter@intel.com

For rballoc_allign() call when caps are not correct it is
enough to return error. k_panic() call is not required here.
Previous change exposed this issue:
thesofproject#8832,
but it is sufficient to log error and return NULL at this point.

Signed-off-by: Jaroslaw Stelter <Jaroslaw.Stelter@intel.com>
@jxstelter jxstelter force-pushed the prv-jstelter-rballoc-fix branch from 5f267da to 6fc162f Compare February 12, 2024 10:42
@@ -388,7 +388,8 @@ void *rballoc_align(uint32_t flags, uint32_t caps, size_t bytes,
heap = &l3_heap;
return (__sparse_force void *)l3_heap_alloc_aligned(heap, align, bytes);
#else
k_panic();
tr_err(&zephyr_tr, "L3_HEAP not available.");
return NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

will the propagate all the way up to the host?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default result of this call was to return NULL on allocation failure. I added k_panic() when L3_HEAP support was added to rballoc_allign(). Although this change exposed a bug in the IPC3 handling code, it seems that returning an allocation error is sufficient here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, not what I meant, I skipped the word "error". I mean't will the host properly be notified of this new allocation error path should it be hit.

@jxstelter jxstelter marked this pull request as ready for review February 13, 2024 10:46
@marc-hb
Copy link
Collaborator

marc-hb commented Feb 13, 2024

I just spotted this only by chance while looking for something else. I could have lost a fair amount of time trying to do something else in the same area. Please mention this PR in the bug: #8832

@cujomalainey
Copy link
Contributor

I just spotted this only by chance while looking for something else. I could have lost a fair amount of time trying to do something else in the same area. Please mention this PR in the bug: #8832

@marc-hb it is mentioned in the bug, it was crosslinked when @jxstelter wrote the bug number in the first comment, it just doesn't generate a notification
image

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 13, 2024

it just doesn't generate a notification

My point exactly. I know a lot about this issue right now, the context is still fresh in my mind so I wouldn't need to read that bug again.

@lgirdwood lgirdwood merged commit 438b11b into thesofproject:main Feb 14, 2024
40 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants