Change in coreboot[master]: mrc_cache: Add data_size parameter to mrc_cache_load_current

[Shelley Chen (Code Review)]

Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46110 )

Change subject: mrc_cache: Add data_size parameter to mrc_cache_load_current
......................................................................

mrc_cache: Add data_size parameter to mrc_cache_load_current

Add parameter so that caller will know what the actual size of the
data returned is.  This is needed for ARM devices like trogdor, which
need to know the size of the training data when populating the QcLib
interface table.

BUG=b:150502246
BRANCH=None
TEST=./util/abuild/abuild -p none -t GOOGLE_NAMI -x -a

Change-Id: Ia314717ad2a7d5232b37a19951c1aecd7f843c27
Signed-off-by: Shelley Chen 
---
M src/drivers/mrc_cache/mrc_cache.c
M src/include/mrc_cache.h
2 files changed, 7 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/46110/1

diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c
index d2991ac..ded3181 100644
--- a/src/drivers/mrc_cache/mrc_cache.c
+++ b/src/drivers/mrc_cache/mrc_cache.c
@@ -261,23 +261,23 @@
 }

int mrc_cache_load_current(int type, uint32_t version, void *buffer,
-			   size_t buffer_size)
+			   size_t buffer_size, size_t *data_size)
 {
    struct region_device rdev;
    struct mrc_metadata md;
-	size_t data_size;
+	size_t dsize;

if (mrc_cache_find_current(type, version, &rdev, &md) < 0)
    	return -1;

-	data_size = region_device_sz(&rdev);
-	if (buffer_size < data_size)
+	*data_size = dsize = region_device_sz(&rdev);
+	if (buffer_size < dsize)
    	return -1;

-	if (rdev_readat(&rdev, buffer, 0, data_size) != data_size)
+	if (rdev_readat(&rdev, buffer, 0, dsize) != dsize)
    	return -1;

-	if (mrc_data_valid(&md, buffer, data_size) < 0)
+	if (mrc_data_valid(&md, buffer, dsize) < 0)
    	return -1;

return 0;
diff --git a/src/include/mrc_cache.h b/src/include/mrc_cache.h
index da2bf79..cb713a1 100644
--- a/src/include/mrc_cache.h
+++ b/src/include/mrc_cache.h
@@ -31,7 +31,7 @@
  * success.
  */
 int mrc_cache_load_current(int type, uint32_t version, void *buffer,
-			   size_t buffer_size);
+			   size_t buffer_size, size_t *data_size);
 /**
  * mrc_cache_mmap_leak
  *

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46110 )

Change subject: mrc_cache: Add data_size parameter to mrc_cache_load_current
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46110/1/src/include/mrc_cache.h 
File src/include/mrc_cache.h:

https://review.coreboot.org/c/coreboot/+/46110/1/src/include/mrc_cache.h@33 
PS1, Line 33: int
Can we reuse the return value to return data_size in case of success and < 0 on error by changing to ssize_t?

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46110 )

Change subject: mrc_cache: Add data_size parameter to mrc_cache_load_current
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46110/1/src/include/mrc_cache.h 
File src/include/mrc_cache.h:

https://review.coreboot.org/c/coreboot/+/46110/1/src/include/mrc_cache.h@33 
PS1, Line 33: int
> Can we reuse the return value to return data_size in case of success and < 0 on error by changing to […]

i guess i thought that using it for two things would be too confusing.  But, if it's used like that elsewhere then I can change it.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46110 )

Change subject: mrc_cache: Add data_size parameter to mrc_cache_load_current
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46110/1/src/include/mrc_cache.h 
File src/include/mrc_cache.h:

https://review.coreboot.org/c/coreboot/+/46110/1/src/include/mrc_cache.h@33 
PS1, Line 33: int
> i guess i thought that using it for two things would be too confusing. […]

I don't see a problem with changing that. You can also use size_t and simply return the actual data size that was loaded. The caller would be expected to treat a size of 0 as nothing loaded because of some error.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46110 )

Change subject: mrc_cache: Add data_size parameter to mrc_cache_load_current
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/46110/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46110/1//COMMIT_MSG@10 
PS1, Line 10: This is needed for ARM devices like trogdor, which
            : need to know the size of the training data when populating the QcLib
            : interface table.
You know, thinking about this some more, I think it is more appropriate to specify the full size of the available memory buffer after all. That's because it is an in/out buffer and I guess it's theoretically possible that the new training data it wants to write is larger than the previous one. QcLib uses the size value to determine how much it is allowed to write back, and that's probably the more important point. For the passed-in buffer the size is probably encoded somewhere inside the data anyway.

So, I don't think it's wrong to expand the API to allow returning this info in general, but we probably don't need it for Trogdor after all.

https://review.coreboot.org/c/coreboot/+/46110/1/src/drivers/mrc_cache/mrc_c... 
File src/drivers/mrc_cache/mrc_cache.c:

https://review.coreboot.org/c/coreboot/+/46110/1/src/drivers/mrc_cache/mrc_c... 
PS1, Line 273: *data_size =
Let's make this optional, e.g.

if (data_size)
   *data_size = dsize;

so callers that don't care can just pass NULL. (Or go with Furquan's suggestion which is fine too.)

[Shelley Chen (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/46110

to look at the new patch set (#2).

Change subject: mrc_cache: Add data_size parameter to mrc_cache_load_current
......................................................................

mrc_cache: Add data_size parameter to mrc_cache_load_current

Add parameter so that caller will know what the actual size of the
data returned is.  This is needed for ARM devices like trogdor, which
need to know the size of the training data when populating the QcLib
interface table.

BUG=b:150502246
BRANCH=None
TEST=./util/abuild/abuild -p none -t GOOGLE_NAMI -x -a

Change-Id: Ia314717ad2a7d5232b37a19951c1aecd7f843c27
Signed-off-by: Shelley Chen 
---
M src/drivers/mrc_cache/mrc_cache.c
M src/include/mrc_cache.h
2 files changed, 7 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/46110/2

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46110 )

Change subject: mrc_cache: Add data_size parameter to mrc_cache_load_current
......................................................................

Patch Set 2:

(3 comments)

s

https://review.coreboot.org/c/coreboot/+/46110/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46110/1//COMMIT_MSG@10 
PS1, Line 10: This is needed for ARM devices like trogdor, which
            : need to know the size of the training data when populating the QcLib
            : interface table.
> You know, thinking about this some more, I think it is more appropriate to specify the full size of  […]

Changed so return value is data size.

https://review.coreboot.org/c/coreboot/+/46110/1/src/drivers/mrc_cache/mrc_c... 
File src/drivers/mrc_cache/mrc_cache.c:

https://review.coreboot.org/c/coreboot/+/46110/1/src/drivers/mrc_cache/mrc_c... 
PS1, Line 273: *data_size =
> Let's make this optional, e.g. […]

Changed so return value is data size.

https://review.coreboot.org/c/coreboot/+/46110/1/src/include/mrc_cache.h 
File src/include/mrc_cache.h:

https://review.coreboot.org/c/coreboot/+/46110/1/src/include/mrc_cache.h@33 
PS1, Line 33: int
> I don't see a problem with changing that. […]

Done

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46110 )

Change subject: mrc_cache: Add data_size parameter to mrc_cache_load_current
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46110/2/src/drivers/mrc_cache/mrc_c... 
File src/drivers/mrc_cache/mrc_cache.c:

https://review.coreboot.org/c/coreboot/+/46110/2/src/drivers/mrc_cache/mrc_c... 
PS2, Line 263: size_t
It has to be ssize_t if you want to be able to return -1. Or you'll need to change it to use 0 to denote errors.

[Shelley Chen (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/46110

to look at the new patch set (#3).

Change subject: mrc_cache: Add data_size parameter to mrc_cache_load_current
......................................................................

mrc_cache: Add data_size parameter to mrc_cache_load_current

Add parameter so that caller will know what the actual size of the
data returned is.  This is needed for ARM devices like trogdor, which
need to know the size of the training data when populating the QcLib
interface table.

BUG=b:150502246
BRANCH=None
TEST=./util/abuild/abuild -p none -t GOOGLE_NAMI -x -a

Change-Id: Ia314717ad2a7d5232b37a19951c1aecd7f843c27
Signed-off-by: Shelley Chen 
---
M src/drivers/mrc_cache/mrc_cache.c
M src/include/mrc_cache.h
2 files changed, 8 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/46110/3

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46110 )

Change subject: mrc_cache: Add data_size parameter to mrc_cache_load_current
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46110/2/src/drivers/mrc_cache/mrc_c... 
File src/drivers/mrc_cache/mrc_cache.c:

https://review.coreboot.org/c/coreboot/+/46110/2/src/drivers/mrc_cache/mrc_c... 
PS2, Line 263: size_t
> It has to be ssize_t if you want to be able to return -1. […]

Done

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46110 )

Change subject: mrc_cache: Add data_size parameter to mrc_cache_load_current
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: mrc_cache: Change mrc_cache_load_current to return data_size of entry

[Shelley Chen (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/46110

to look at the new patch set (#4).

Change subject: mrc_cache: Change mrc_cache_load_current to return data_size of entry
......................................................................

mrc_cache: Change mrc_cache_load_current to return data_size of entry

Modify mrc_cache_load current to return the data_size of the mrc_cache
entry so that caller will know what the actual size of the data
returned is.  This is needed for ARM devices like trogdor, which need
to know the size of the training data when populating the QcLib
interface table.

BUG=b:150502246
BRANCH=None
TEST=./util/abuild/abuild -p none -t GOOGLE_NAMI -x -a

Change-Id: Ia314717ad2a7d5232b37a19951c1aecd7f843c27
Signed-off-by: Shelley Chen 
---
M src/drivers/mrc_cache/mrc_cache.c
M src/include/mrc_cache.h
2 files changed, 8 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/46110/4

Change in coreboot[master]: mrc_cache: Change mrc_cache_load_current to return size of entry

[Shelley Chen (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/46110

to look at the new patch set (#5).

Change subject: mrc_cache: Change mrc_cache_load_current to return size of entry
......................................................................

mrc_cache: Change mrc_cache_load_current to return size of entry

Modify mrc_cache_load current to return the size of the mrc_cache
entry so that caller will know what the actual size of the data
returned is.  This is needed for ARM devices like trogdor, which need
to know the size of the training data when populating the QcLib
interface table.

BUG=b:150502246
BRANCH=None
TEST=./util/abuild/abuild -p none -t GOOGLE_NAMI -x -a

Change-Id: Ia314717ad2a7d5232b37a19951c1aecd7f843c27
Signed-off-by: Shelley Chen 
---
M src/drivers/mrc_cache/mrc_cache.c
M src/include/mrc_cache.h
2 files changed, 8 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/46110/5

Change in coreboot[master]: mrc_cache: Change mrc_cache_load_current to return size of entry

[Shelley Chen (Code Review)]

Shelley Chen has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46110 )

Change subject: mrc_cache: Change mrc_cache_load_current to return size of entry
......................................................................

mrc_cache: Change mrc_cache_load_current to return size of entry

Modify mrc_cache_load current to return the size of the mrc_cache
entry so that caller will know what the actual size of the data
returned is.  This is needed for ARM devices like trogdor, which need
to know the size of the training data when populating the QcLib
interface table.

BUG=b:150502246
BRANCH=None
TEST=./util/abuild/abuild -p none -t GOOGLE_NAMI -x -a

Change-Id: Ia314717ad2a7d5232b37a19951c1aecd7f843c27
Signed-off-by: Shelley Chen 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46110
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
---
M src/drivers/mrc_cache/mrc_cache.c
M src/include/mrc_cache.h
2 files changed, 8 insertions(+), 8 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c
index d2991ac..a083655 100644
--- a/src/drivers/mrc_cache/mrc_cache.c
+++ b/src/drivers/mrc_cache/mrc_cache.c
@@ -260,12 +260,12 @@
    return rdev_chain(rdev, rdev, md_size, data_size);
 }

-int mrc_cache_load_current(int type, uint32_t version, void *buffer,
-			   size_t buffer_size)
+ssize_t mrc_cache_load_current(int type, uint32_t version, void *buffer,
+			      size_t buffer_size)
 {
    struct region_device rdev;
    struct mrc_metadata md;
-	size_t data_size;
+	ssize_t data_size;

if (mrc_cache_find_current(type, version, &rdev, &md) < 0)
    	return -1;
@@ -280,7 +280,7 @@
    if (mrc_data_valid(&md, buffer, data_size) < 0)
    	return -1;

-	return 0;
+	return data_size;
 }

void *mrc_cache_current_mmap_leak(int type, uint32_t version,
diff --git a/src/include/mrc_cache.h b/src/include/mrc_cache.h
index da2bf79..f1e6b52 100644
--- a/src/include/mrc_cache.h
+++ b/src/include/mrc_cache.h
@@ -27,11 +27,11 @@
  * mrc_cache_load_current
  *
  * Fill in the buffer with the latest slot data.  This will be a
- * common entry point for ARM platforms.  Returns < 0 on error, 0 on
- * success.
+ * common entry point for ARM platforms.  Returns < 0 on error, size
+ * of the returned data on success.
  */
-int mrc_cache_load_current(int type, uint32_t version, void *buffer,
-			   size_t buffer_size);
+ssize_t mrc_cache_load_current(int type, uint32_t version, void *buffer,
+			       size_t buffer_size);
 /**
  * mrc_cache_mmap_leak
  *