Hi Heinrich,
On Mon, 13 Jan 2025 at 12:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 13.01.25 15:33, Raymond Mao wrote:
Hi Heinrich,
On Fri, 10 Jan 2025 at 19:04, Heinrich Schuchardt <xypron.glpk@gmx.de mailto:xypron.glpk@gmx.de> wrote:
Am 10. Januar 2025 22:56:33 MEZ schrieb Raymond Mao <raymond.mao@linaro.org <mailto:raymond.mao@linaro.org>>: >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 <raymond.mao@linaro.org <mailto:raymond.mao@linaro.org>> >Reviewed-by: Simon Glass <sjg@chromium.org <mailto:sjg@chromium.org>>
>Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org <mailto:ilias.apalodimas@linaro.org>> >--- >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 isdisabled?
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 matchingtag
>+ * >+ * @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));