| Message ID | 20250110215640.892539-1-raymond.mao@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v4,1/3] bloblist: add api to get blob with size | expand |
Am 10. Januar 2025 22:56:33 MEZ schrieb Raymond Mao <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> >Reviewed-by: Simon Glass <sjg@chromium.org> >Reviewed-by: Ilias Apalodimas <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 is disabled? >+/** >+ * 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. Best regards Heinrich >+#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));
Hi Heinrich, On Fri, 10 Jan 2025 at 19:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > Am 10. Januar 2025 22:56:33 MEZ schrieb Raymond Mao < > 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> > >Reviewed-by: Simon Glass <sjg@chromium.org> > >Reviewed-by: Ilias Apalodimas <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 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. > >+/** > >+ * 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. 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)); > >
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 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. > > >+/** > >+ * 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)); >
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 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)); > > > >
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) +/** + * 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); +#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));