Hi Heinrich, On Mon, 13 Jan 2025 at 12:50, Heinrich Schuchardt <[email protected]> wrote:
> On 13.01.25 15:33, Raymond Mao wrote: > > Hi Heinrich, > > > > On Fri, 10 Jan 2025 at 19:04, Heinrich Schuchardt <[email protected] > > <mailto:[email protected]>> wrote: > > > > Am 10. Januar 2025 22:56:33 MEZ schrieb Raymond Mao > > <[email protected] <mailto:[email protected]>>: > > >bloblist_find function only returns the pointer of blob data, > > >which is fine for those self-describing data like FDT. > > >But as a common scenario, an interface is needed to retrieve both > > >the pointer and the size of the blob data. > > > > > >Add a few ut test cases for the new api. > > > > > >Signed-off-by: Raymond Mao <[email protected] > > <mailto:[email protected]>> > > >Reviewed-by: Simon Glass <[email protected] <mailto: > [email protected]>> > > >Reviewed-by: Ilias Apalodimas <[email protected] > > <mailto:[email protected]>> > > >--- > > >Changes in v2 > > >- Rename function argument. > > >- Add ut tests. > > >Changes in v3 > > >- None. > > >Changes in v4 > > >- None. > > > > > > common/bloblist.c | 17 +++++++++++++++-- > > > include/bloblist.h | 18 ++++++++++++++++++ > > > test/common/bloblist.c | 4 ++++ > > > 3 files changed, 37 insertions(+), 2 deletions(-) > > > > > >diff --git a/common/bloblist.c b/common/bloblist.c > > >index 110bb9dc44..ab48a3cdb9 100644 > > >--- a/common/bloblist.c > > >+++ b/common/bloblist.c > > >@@ -222,14 +222,27 @@ static int bloblist_ensurerec(uint tag, > > struct bloblist_rec **recp, int size, > > > } > > > > > > void *bloblist_find(uint tag, int size) > > >+{ > > >+ void *blob = NULL; > > >+ int blob_size; > > >+ > > >+ blob = bloblist_get_blob(tag, &blob_size); > > >+ > > >+ if (size && size != blob_size) > > >+ return NULL; > > >+ > > >+ return blob; > > >+} > > >+ > > >+void *bloblist_get_blob(uint tag, int *sizep) > > > { > > > struct bloblist_rec *rec; > > > > > > rec = bloblist_findrec(tag); > > > if (!rec) > > > return NULL; > > >- if (size && size != rec->size) > > >- return NULL; > > >+ > > >+ *sizep = rec->size; > > > > > > return (void *)rec + rec_hdr_size(rec); > > > } > > >diff --git a/include/bloblist.h b/include/bloblist.h > > >index f999391f74..52ba0ddcf8 100644 > > >--- a/include/bloblist.h > > >+++ b/include/bloblist.h > > >@@ -250,6 +250,24 @@ static inline void > > *bloblist_check_magic(ulong addr) > > > return ptr; > > > } > > > > > >+#if CONFIG_IS_ENABLED(BLOBLIST) > > > > Why should we use an #ifdef here? > > Why would a caller invoke the function if the configuration is > disabled? > > > > Adding #ifdef here is to have the inline function so that we don't need > > to add the macro to all of its callers. > > There is just one. See patch 3, where you already check the CONFIG > albeit in the wrong place. > > Currently it is only one for TPM eventlog, but according to the Firmware Handoff spec we will use Transfer List (aka bloblist in U-Boot) to handover different information (blobs) from previous boot stage, so that we will need to call bloblist_get_blob in multiple subsystems in the near future. In patch #3, checking BLOBLIST is a temporary solution since we don't have a good idea at the moment on a required config to enable/disable "handoff from previous boot stage using bloblist" - which is separate from BLOBLIST itself. As discussed with Tom, I use BLOBLIST as a temporary solution but this will be replaced in the future. See my reply in patch #3 for more information. Regards, Raymond > > > > >+/** > > >+ * bloblist_get_blob() - Find a blob and get the size of it > > >+ * > > >+ * Searches the bloblist and returns the blob with the matching > tag > > >+ * > > >+ * @tag: Tag to search for (enum bloblist_tag_t) > > >+ * @sizep: Size of the blob found > > >+ * Return: pointer to bloblist if found, or NULL if not found > > >+ */ > > >+void *bloblist_get_blob(uint tag, int *sizep); > > > > If tag is expected to be a value from the enum, we should not use > > uint here. > > > > This is just following the original convention of existing bloblist > > functions which are all using uint for the tag. > > @Simon: Why aren't we using enum? > > Best regards > > Heinrich > > > > > Regards, > > Raymond > > > > > > >+#else > > >+static inline void *bloblist_get_blob(uint tag, int *sizep) > > >+{ > > >+ return NULL; > > >+} > > >+#endif > > >+ > > > /** > > > * bloblist_find() - Find a blob > > > * > > >diff --git a/test/common/bloblist.c b/test/common/bloblist.c > > >index 4bca62110a..324127596f 100644 > > >--- a/test/common/bloblist.c > > >+++ b/test/common/bloblist.c > > >@@ -98,10 +98,12 @@ static int bloblist_test_blob(struct > > unit_test_state *uts) > > > struct bloblist_hdr *hdr; > > > struct bloblist_rec *rec, *rec2; > > > char *data; > > >+ int size = 0; > > > > > > /* At the start there should be no records */ > > > hdr = clear_bloblist(); > > > ut_assertnull(bloblist_find(TEST_TAG, TEST_BLOBLIST_SIZE)); > > >+ ut_assertnull(bloblist_get_blob(TEST_TAG, &size)); > > > ut_assertok(bloblist_new(TEST_ADDR, TEST_BLOBLIST_SIZE, 0, > 0)); > > > ut_asserteq(sizeof(struct bloblist_hdr), > bloblist_get_size()); > > > ut_asserteq(TEST_BLOBLIST_SIZE, bloblist_get_total_size()); > > >@@ -114,6 +116,8 @@ static int bloblist_test_blob(struct > > unit_test_state *uts) > > > ut_asserteq_addr(rec + 1, data); > > > data = bloblist_find(TEST_TAG, TEST_SIZE); > > > ut_asserteq_addr(rec + 1, data); > > >+ ut_asserteq_addr(bloblist_get_blob(TEST_TAG, &size), data); > > >+ ut_asserteq(size, TEST_SIZE); > > > > > > /* Check the data is zeroed */ > > > ut_assertok(check_zero(data, TEST_SIZE)); > > > >
