Skip to content

alloc: fix memory heap initialization#541

Merged
lgirdwood merged 1 commit intothesofproject:masterfrom
libinyang:init_heap_in_right_way
Nov 8, 2018
Merged

alloc: fix memory heap initialization#541
lgirdwood merged 1 commit intothesofproject:masterfrom
libinyang:init_heap_in_right_way

Conversation

@libinyang
Copy link
Copy Markdown
Contributor

@libinyang libinyang commented Nov 5, 2018

This patch fixes the bug of runtime & buffer memory heap initialization and
sets the correct base for each map.
This is a generic fix and It was verified to fix the issues below:
thesofproject/linux#253
#539 duplicate of linux #253

Signed-off-by: Libin Yang libin.yang@intel.com

@libinyang libinyang requested review from lgirdwood and tlauda November 5, 2018 15:06
@libinyang
Copy link
Copy Markdown
Contributor Author

@lgirdwood
@tlauda

Hi all,

This is a patch to fix the bug of memory initialization in FW.

@wenqingfu
Copy link
Copy Markdown

Is there a corresponding issue number of the "bug"? @libinyang

Copy link
Copy Markdown
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

Can you please explain the bug and the fix in detail in the commit message (otherwise it;s really hard to follow what you are fixing and why).

@lgirdwood lgirdwood requested review from lbetlej and mmaka1 November 6, 2018 14:10
@libinyang
Copy link
Copy Markdown
Contributor Author

@wenqingfu
@lgirdwood
@mmaka1
@tlauda

This patch is made for the bug #539. However, as the memory issue should be very general, I believe this patch may fix some other potential unstable issues because of the memory allocation.

This bug is the member base of memmap.runtime[] map and memmap.buffer[] map is set wrong.

@libinyang libinyang force-pushed the init_heap_in_right_way branch from 90a1086 to 394dda2 Compare November 7, 2018 06:06
@libinyang
Copy link
Copy Markdown
Contributor Author

Patch updated by adding more comments in the code.

Copy link
Copy Markdown
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

Please describe what is broken with existing code and how your changes fix this in the commit message otherwise this cant be applied (even if it's correct). I've spent 20 minutes working out what is wrong with the current code that could have been avoided if you had explained in the commit message.

Comment thread src/lib/alloc.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please follow coding style of one variable per line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

please follow coding style of one variable per line.

Thanks for point it out. I didn't realize we have such coding style in sof project.

Comment thread src/lib/alloc.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

keep the new line

Comment thread src/lib/alloc.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

keep new line

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK for this and upper review comments.

This patch fixes the bug of runtime & buffer memory heap initialization,
and sets the correct base address for each map.

Signed-off-by: Libin Yang <libin.yang@intel.com>
@plbossart
Copy link
Copy Markdown
Member

this change makes issue thesofproject/linux#253 go away, can we please prioritize reviews and integration.

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.

4 participants