Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36334 )
Change subject: cbmem.h: Align comment with the reality of implementations ......................................................................
cbmem.h: Align comment with the reality of implementations
cbmem_top() should simply not be called before memory is initialed, in order for the implementation to return something meaningful.
Change-Id: I8fe32844af290626a0f91279143fda4d3442680f Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/include/cbmem.h 1 file changed, 2 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/36334/1
diff --git a/src/include/cbmem.h b/src/include/cbmem.h index f972ba6..4005fa2 100644 --- a/src/include/cbmem.h +++ b/src/include/cbmem.h @@ -71,9 +71,8 @@ /* Return the top address for dynamic cbmem. The address returned needs to * be consistent across romstage and ramstage, and it is required to be * below 4GiB for 32bit coreboot builds. On 64bit coreboot builds there's no - * upper limit. - * x86 boards or chipsets must return NULL before the cbmem backing store has - * been initialized. */ + * upper limit. This should not be called before memory is initialized. + */ void *cbmem_top(void);
/* Add a cbmem entry of a given size and id. These return NULL on failure. The
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36334 )
Change subject: cbmem.h: Align comment with the reality of implementations ......................................................................
Patch Set 1: Code-Review+2
This has no teeth for enforcement purposes, but it does capture the current reality.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36334 )
Change subject: cbmem.h: Align comment with the reality of implementations ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36334/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36334/1//COMMIT_MSG@10 PS1, Line 10: in order for the implementation I'd say this depends on the implementation, doesn't it?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36334 )
Change subject: cbmem.h: Align comment with the reality of implementations ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36334/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36334/1//COMMIT_MSG@10 PS1, Line 10: in order for the implementation
I'd say this depends on the implementation, doesn't it?
A generic API should not depend on the implementation. I don't think there is a current use case to check if cbmem_top return 0 to do something.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36334 )
Change subject: cbmem.h: Align comment with the reality of implementations ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36334/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36334/1//COMMIT_MSG@10 PS1, Line 10: in order for the implementation
I'd say this depends on the implementation, doesn't it? […]
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36334 )
Change subject: cbmem.h: Align comment with the reality of implementations ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/36334/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36334/1//COMMIT_MSG@10 PS1, Line 10: in order for the implementation
Done
I read this again now.... I fully agree ;)
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36334 )
Change subject: cbmem.h: Align comment with the reality of implementations ......................................................................
cbmem.h: Align comment with the reality of implementations
cbmem_top() should simply not be called before memory is initialed, in order for the implementation to return something meaningful.
Change-Id: I8fe32844af290626a0f91279143fda4d3442680f Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/36334 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Michael Niewöhner --- M src/include/cbmem.h 1 file changed, 2 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Michael Niewöhner: Looks good to me, but someone else must approve
diff --git a/src/include/cbmem.h b/src/include/cbmem.h index f972ba6..4005fa2 100644 --- a/src/include/cbmem.h +++ b/src/include/cbmem.h @@ -71,9 +71,8 @@ /* Return the top address for dynamic cbmem. The address returned needs to * be consistent across romstage and ramstage, and it is required to be * below 4GiB for 32bit coreboot builds. On 64bit coreboot builds there's no - * upper limit. - * x86 boards or chipsets must return NULL before the cbmem backing store has - * been initialized. */ + * upper limit. This should not be called before memory is initialized. + */ void *cbmem_top(void);
/* Add a cbmem entry of a given size and id. These return NULL on failure. The
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)