Skip to content

Conversation

@softwarecki
Copy link
Collaborator

This pull request replaces specialized heap pointer checks with the generic is_heap_pointer utility for both shared and L3 heaps. The change removes the is_shared_buffer_heap_pointer and is_l3_heap_pointer functions, simplifying the memory handling path and reducing code duplication. By relying on a common utility, the implementation becomes more consistent and easier to maintain without altering existing behavior.

Replace the is_shared_buffer_heap_pointer function with the generic
is_heap_pointer for shared heap. This change simplifies the memory handling
path by relying on a common utility instead of a specialized function.

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the heap pointer validation logic by replacing specialized functions (is_shared_buffer_heap_pointer and is_l3_heap_pointer) with the generic is_heap_pointer utility. The change eliminates code duplication while maintaining identical functionality for memory deallocation checks in both shared and L3 heaps.

Key Changes:

  • Removed two specialized heap pointer check functions in favor of the generic is_heap_pointer
  • Updated rfree() to use is_heap_pointer for both L3 and shared buffer heap validation
  • Added initialization of init_mem and init_bytes fields in heap structures to support the generic pointer check

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Replace the is_l3_heap_pointer function with the generic is_heap_pointer
for L3 heap. This change simplifies the memory handling path by relying on
a common utility instead of a specialized function.

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
@softwarecki softwarecki added the P3 Low-impact bugs or features label Dec 12, 2025
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

good to use a single function of course, but relying on .init_mem and .init_bytes is a bit concerning, a comment in zephyr/include/zephyr/sys/sys_heap.h says about them:

Would be good to clean this up and put the two values somewhere else

But I guess if they do get removed, we'll store the values elsewhere

@softwarecki
Copy link
Collaborator Author

@lyakh

/* Note: the init_mem/bytes fields are for the static initializer to
 * have somewhere to put the arguments.  The actual heap metadata at
 * runtime lives in the heap memory itself and this struct simply
 * functions as an opaque pointer.  Would be good to clean this up and
 * put the two values somewhere else, though it would make
 * SYS_HEAP_DEFINE a little hairy to write.
 */

This comment was added more than 6 years ago (zephyrproject-rtos/zephyr@aa42277) and it seems unlikely that these fields will be replaced in the future. They are still used by heaps created with the SYS_HEAP_DEFINE macro. The author's concern was probably that these fields are only used during initialization and are unnecessary at runtime. If they start being used at runtime, the discomfort suggesting their removal will disappear.

Copy link
Contributor

@tmleman tmleman left a comment

Choose a reason for hiding this comment

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

LGTM

@kv2019i
Copy link
Collaborator

kv2019i commented Dec 12, 2025

Only known fails in CI, proceeding with merge.

@kv2019i kv2019i merged commit 06ab2ca into thesofproject:main Dec 12, 2025
35 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-impact bugs or features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants