[PATCH 00/13] x86: cbfs: Various clean-ups to CBFS implementation

[PATCH 00/13] x86: cbfs: Various clean-ups to CBFS implementation

[Simon Glass]

This code is very old and has not had much of a clean-up since it was
written. This series aims to tidy it up to use error codes, avoid using
BSS when not necessary and to add a few more features.


Simon Glass (13):
  cbfs: Rename the result variable
  cbfs: Use ulong consistently
  cbfs: Use bool type for whether initialised
  cbfs: Adjust return value of file_cbfs_next_file()
  cbfs: Adjust file_cbfs_load_header() to use cbfs_priv
  cbfs: Adjust cbfs_load_header_ptr() to use cbfs_priv
  cbfs: Unify the two header loaders
  cbfs: Use void * for the position pointers
  cbfs: Record the start address in cbfs_priv
  cbfs: Return the error code from file_cbfs_init()
  cbfs: Change file_cbfs_find_uncached() to return an error
  cbfs: Allow reading a file from a CBFS given its base addr
  cbfs: Don't require the CBFS size with cbfs_init_mem()

 arch/x86/lib/fsp2/fsp_init.c |   3 +-
 cmd/cbfs.c                   |   3 +-
 fs/cbfs/cbfs.c               | 259 ++++++++++++++++++++++-------------
 include/cbfs.h               |  36 +++--
 4 files changed, 186 insertions(+), 115 deletions(-)

-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 01/13] cbfs: Rename the result variable

[Simon Glass]

At present the result variable in the cbfs_priv is called 'result' as is
the local variable in a few functions. Change the latter to 'ret' which is
more common in U-Boot and avoids confusion.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 fs/cbfs/cbfs.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 1aa6f8ee84..70440aa80b 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -145,18 +145,18 @@ static void file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,
 	priv->file_cache = NULL;
 
 	while (size >= align) {
-		int result;
+		int ret;
 		u32 used;
 
 		new_node = (struct cbfs_cachenode *)
 				malloc(sizeof(struct cbfs_cachenode));
-		result = file_cbfs_next_file(priv, start, size, align, new_node,
-					     &used);
+		ret = file_cbfs_next_file(priv, start, size, align, new_node,
+					  &used);
 
-		if (result < 0) {
+		if (ret < 0) {
 			free(new_node);
 			return;
-		} else if (result == 0) {
+		} else if (ret == 0) {
 			free(new_node);
 			break;
 		}
@@ -341,15 +341,15 @@ const struct cbfs_cachenode *file_cbfs_find_uncached(uintptr_t end_of_rom,
 	align = priv->header.align;
 
 	while (size >= align) {
-		int result;
+		int ret;
 		u32 used;
 
-		result = file_cbfs_next_file(priv, start, size, align, &node,
-					     &used);
+		ret = file_cbfs_next_file(priv, start, size, align, &node,
+					  &used);
 
-		if (result < 0)
+		if (ret < 0)
 			return NULL;
-		else if (result == 0)
+		else if (ret == 0)
 			break;
 
 		if (!strcmp(name, node.name))
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 02/13] cbfs: Use ulong consistently

[Simon Glass]

U-Boot uses ulong for addresses but there are a few places in this driver
that don't use it. Convert this driver over to follow this convention
fully.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 fs/cbfs/cbfs.c | 9 ++++-----
 include/cbfs.h | 4 ++--
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 70440aa80b..846102dce3 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -170,8 +170,7 @@ static void file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,
 }
 
 /* Get the CBFS header out of the ROM and do endian conversion. */
-static int file_cbfs_load_header(uintptr_t end_of_rom,
-				 struct cbfs_header *header)
+static int file_cbfs_load_header(ulong end_of_rom, struct cbfs_header *header)
 {
 	struct cbfs_header *header_in_rom;
 	int32_t offset = *(u32 *)(end_of_rom - 3);
@@ -204,7 +203,7 @@ static int cbfs_load_header_ptr(struct cbfs_priv *priv, ulong base,
 	return 0;
 }
 
-static void cbfs_init(struct cbfs_priv *priv, uintptr_t end_of_rom)
+static void cbfs_init(struct cbfs_priv *priv, ulong end_of_rom)
 {
 	u8 *start_of_rom;
 
@@ -221,7 +220,7 @@ static void cbfs_init(struct cbfs_priv *priv, uintptr_t end_of_rom)
 		priv->initialized = 1;
 }
 
-void file_cbfs_init(uintptr_t end_of_rom)
+void file_cbfs_init(ulong end_of_rom)
 {
 	cbfs_init(&cbfs_s, end_of_rom);
 }
@@ -324,7 +323,7 @@ const struct cbfs_cachenode *file_cbfs_find(const char *name)
 	return cbfs_find_file(&cbfs_s, name);
 }
 
-const struct cbfs_cachenode *file_cbfs_find_uncached(uintptr_t end_of_rom,
+const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom,
 						     const char *name)
 {
 	struct cbfs_priv *priv = &cbfs_s;
diff --git a/include/cbfs.h b/include/cbfs.h
index d915f9426d..10e951dccf 100644
--- a/include/cbfs.h
+++ b/include/cbfs.h
@@ -103,7 +103,7 @@ enum cbfs_result cbfs_get_result(void);
  * @end_of_rom: Points to the end of the ROM the CBFS should be read
  *                      from.
  */
-void file_cbfs_init(uintptr_t end_of_rom);
+void file_cbfs_init(ulong end_of_rom);
 
 /**
  * file_cbfs_get_header() - Get the header structure for the current CBFS.
@@ -172,7 +172,7 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp);
  *
  * @return A handle to the file, or NULL on error.
  */
-const struct cbfs_cachenode *file_cbfs_find_uncached(uintptr_t end_of_rom,
+const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom,
 						     const char *name);
 
 /**
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 03/13] cbfs: Use bool type for whether initialised

[Simon Glass]

At present this uses an int type. U-Boot now supports bool so use this
instead. Also use English spelling for initialised which we are here.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 fs/cbfs/cbfs.c | 28 ++++++++++++++--------------
 include/cbfs.h |  2 +-
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 846102dce3..322778d1c8 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -12,7 +12,7 @@ static const u32 good_magic = 0x4f524243;
 static const u8 good_file_magic[] = "LARCHIVE";
 
 struct cbfs_priv {
-	int initialized;
+	bool initialised;
 	struct cbfs_header header;
 	struct cbfs_cachenode *file_cache;
 	enum cbfs_result result;
@@ -25,8 +25,8 @@ const char *file_cbfs_error(void)
 	switch (cbfs_s.result) {
 	case CBFS_SUCCESS:
 		return "Success";
-	case CBFS_NOT_INITIALIZED:
-		return "CBFS not initialized";
+	case CBFS_NOT_INITIALISED:
+		return "CBFS not initialised";
 	case CBFS_BAD_HEADER:
 		return "Bad CBFS header";
 	case CBFS_BAD_FILE:
@@ -207,7 +207,7 @@ static void cbfs_init(struct cbfs_priv *priv, ulong end_of_rom)
 {
 	u8 *start_of_rom;
 
-	priv->initialized = 0;
+	priv->initialised = false;
 
 	if (file_cbfs_load_header(end_of_rom, &priv->header))
 		return;
@@ -217,7 +217,7 @@ static void cbfs_init(struct cbfs_priv *priv, ulong end_of_rom)
 	file_cbfs_fill_cache(priv, start_of_rom, priv->header.rom_size,
 			     priv->header.align);
 	if (priv->result == CBFS_SUCCESS)
-		priv->initialized = 1;
+		priv->initialised = true;
 }
 
 void file_cbfs_init(ulong end_of_rom)
@@ -244,7 +244,7 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp)
 	if (priv->result != CBFS_SUCCESS)
 		return -EINVAL;
 
-	priv->initialized = 1;
+	priv->initialised = true;
 	priv = malloc(sizeof(priv_s));
 	if (!priv)
 		return -ENOMEM;
@@ -258,11 +258,11 @@ const struct cbfs_header *file_cbfs_get_header(void)
 {
 	struct cbfs_priv *priv = &cbfs_s;
 
-	if (priv->initialized) {
+	if (priv->initialised) {
 		priv->result = CBFS_SUCCESS;
 		return &priv->header;
 	} else {
-		priv->result = CBFS_NOT_INITIALIZED;
+		priv->result = CBFS_NOT_INITIALISED;
 		return NULL;
 	}
 }
@@ -271,8 +271,8 @@ const struct cbfs_cachenode *file_cbfs_get_first(void)
 {
 	struct cbfs_priv *priv = &cbfs_s;
 
-	if (!priv->initialized) {
-		priv->result = CBFS_NOT_INITIALIZED;
+	if (!priv->initialised) {
+		priv->result = CBFS_NOT_INITIALISED;
 		return NULL;
 	} else {
 		priv->result = CBFS_SUCCESS;
@@ -284,8 +284,8 @@ void file_cbfs_get_next(const struct cbfs_cachenode **file)
 {
 	struct cbfs_priv *priv = &cbfs_s;
 
-	if (!priv->initialized) {
-		priv->result = CBFS_NOT_INITIALIZED;
+	if (!priv->initialised) {
+		priv->result = CBFS_NOT_INITIALISED;
 		*file = NULL;
 		return;
 	}
@@ -300,8 +300,8 @@ const struct cbfs_cachenode *cbfs_find_file(struct cbfs_priv *priv,
 {
 	struct cbfs_cachenode *cache_node = priv->file_cache;
 
-	if (!priv->initialized) {
-		priv->result = CBFS_NOT_INITIALIZED;
+	if (!priv->initialised) {
+		priv->result = CBFS_NOT_INITIALISED;
 		return NULL;
 	}
 
diff --git a/include/cbfs.h b/include/cbfs.h
index 10e951dccf..80ce539070 100644
--- a/include/cbfs.h
+++ b/include/cbfs.h
@@ -11,7 +11,7 @@
 
 enum cbfs_result {
 	CBFS_SUCCESS = 0,
-	CBFS_NOT_INITIALIZED,
+	CBFS_NOT_INITIALISED,
 	CBFS_BAD_HEADER,
 	CBFS_BAD_FILE,
 	CBFS_FILE_NOT_FOUND
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 04/13] cbfs: Adjust return value of file_cbfs_next_file()

[Simon Glass]

At present his uses a true return to indicate it found a file. Adjust it
to use 0 for this, so it is consistent with other functions.

Update its callers accordingling and add a check for malloc() failure in
file_cbfs_fill_cache().

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 fs/cbfs/cbfs.c | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 322778d1c8..1037d19225 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -77,11 +77,12 @@ static void swap_file_header(struct cbfs_fileheader *dest,
  * @param used		A pointer to the count of of bytes scanned through,
  *			including the file if one is found.
  *
- * @return 1 if a file is found, 0 if one isn't.
+ * @return 0 if a file is found, -ENOENT if one isn't, -EBADF if a bad header
+ *	is found.
  */
-static int file_cbfs_next_file(struct cbfs_priv *priv, u8 *start, u32 size,
-			       u32 align, struct cbfs_cachenode *new_node,
-			       u32 *used)
+static int file_cbfs_next_file(struct cbfs_priv *priv, u8 *start, int size,
+			       int align, struct cbfs_cachenode *new_node,
+			       int *used)
 {
 	struct cbfs_fileheader header;
 
@@ -105,7 +106,7 @@ static int file_cbfs_next_file(struct cbfs_priv *priv, u8 *start, u32 size,
 		swap_file_header(&header, file_header);
 		if (header.offset < sizeof(struct cbfs_fileheader)) {
 			priv->result = CBFS_BAD_FILE;
-			return -1;
+			return -EBADF;
 		}
 		new_node->next = NULL;
 		new_node->type = header.type;
@@ -122,14 +123,15 @@ static int file_cbfs_next_file(struct cbfs_priv *priv, u8 *start, u32 size,
 			step = step + align - step % align;
 
 		*used += step;
-		return 1;
+		return 0;
 	}
-	return 0;
+
+	return -ENOENT;
 }
 
 /* Look through a CBFS instance and copy file metadata into regular memory. */
-static void file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,
-				 u32 align)
+static int file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,
+				u32 align)
 {
 	struct cbfs_cachenode *cache_node;
 	struct cbfs_cachenode *new_node;
@@ -145,20 +147,21 @@ static void file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,
 	priv->file_cache = NULL;
 
 	while (size >= align) {
+		int used;
 		int ret;
-		u32 used;
 
 		new_node = (struct cbfs_cachenode *)
 				malloc(sizeof(struct cbfs_cachenode));
+		if (!new_node)
+			return -ENOMEM;
 		ret = file_cbfs_next_file(priv, start, size, align, new_node,
 					  &used);
 
 		if (ret < 0) {
 			free(new_node);
-			return;
-		} else if (ret == 0) {
-			free(new_node);
-			break;
+			if (ret == -ENOENT)
+				break;
+			return ret;
 		}
 		*cache_tail = new_node;
 		cache_tail = &new_node->next;
@@ -167,6 +170,8 @@ static void file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,
 		start += used;
 	}
 	priv->result = CBFS_SUCCESS;
+
+	return 0;
 }
 
 /* Get the CBFS header out of the ROM and do endian conversion. */
@@ -341,16 +346,14 @@ const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom,
 
 	while (size >= align) {
 		int ret;
-		u32 used;
+		int used;
 
 		ret = file_cbfs_next_file(priv, start, size, align, &node,
 					  &used);
-
-		if (ret < 0)
-			return NULL;
-		else if (ret == 0)
+		if (ret == -ENOENT)
 			break;
-
+		else if (ret)
+			return NULL;
 		if (!strcmp(name, node.name))
 			return &node;
 
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 05/13] cbfs: Adjust file_cbfs_load_header() to use cbfs_priv

[Simon Glass]

This function is strange at the moment in that it takes a header pointer
but then accesses the cbfs_s global. Currently clients have their own priv
pointer, so update the function to take that as a parameter instead.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 fs/cbfs/cbfs.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 1037d19225..765e078423 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -175,8 +175,9 @@ static int file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,
 }
 
 /* Get the CBFS header out of the ROM and do endian conversion. */
-static int file_cbfs_load_header(ulong end_of_rom, struct cbfs_header *header)
+static int file_cbfs_load_header(struct cbfs_priv *priv, ulong end_of_rom)
 {
+	struct cbfs_header *header = &priv->header;
 	struct cbfs_header *header_in_rom;
 	int32_t offset = *(u32 *)(end_of_rom - 3);
 
@@ -185,7 +186,7 @@ static int file_cbfs_load_header(ulong end_of_rom, struct cbfs_header *header)
 
 	if (header->magic != good_magic || header->offset >
 			header->rom_size - header->boot_block_size) {
-		cbfs_s.result = CBFS_BAD_HEADER;
+		priv->result = CBFS_BAD_HEADER;
 		return 1;
 	}
 	return 0;
@@ -214,7 +215,7 @@ static void cbfs_init(struct cbfs_priv *priv, ulong end_of_rom)
 
 	priv->initialised = false;
 
-	if (file_cbfs_load_header(end_of_rom, &priv->header))
+	if (file_cbfs_load_header(priv, end_of_rom))
 		return;
 
 	start_of_rom = (u8 *)(end_of_rom + 1 - priv->header.rom_size);
@@ -337,7 +338,7 @@ const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom,
 	u32 align;
 	static struct cbfs_cachenode node;
 
-	if (file_cbfs_load_header(end_of_rom, &priv->header))
+	if (file_cbfs_load_header(priv, end_of_rom))
 		return NULL;
 
 	start = (u8 *)(end_of_rom + 1 - priv->header.rom_size);
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 06/13] cbfs: Adjust cbfs_load_header_ptr() to use cbfs_priv

[Simon Glass]

This function is strange at the moment in that it takes a header pointer
but then accesses the cbfs_s global. Currently clients have their own priv
pointer, so update the function to take that as a parameter instead.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 fs/cbfs/cbfs.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 765e078423..05de58cf19 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -8,6 +8,9 @@
 #include <malloc.h>
 #include <asm/byteorder.h>
 
+/* Offset of master header from the start of a coreboot ROM */
+#define MASTER_HDR_OFFSET	0x38
+
 static const u32 good_magic = 0x4f524243;
 static const u8 good_file_magic[] = "LARCHIVE";
 
@@ -192,9 +195,9 @@ static int file_cbfs_load_header(struct cbfs_priv *priv, ulong end_of_rom)
 	return 0;
 }
 
-static int cbfs_load_header_ptr(struct cbfs_priv *priv, ulong base,
-				struct cbfs_header *header)
+static int cbfs_load_header_ptr(struct cbfs_priv *priv, ulong base)
 {
+	struct cbfs_header *header = &priv->header;
 	struct cbfs_header *header_in_rom;
 
 	header_in_rom = (struct cbfs_header *)base;
@@ -241,7 +244,7 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp)
 	 * valid. Assume that a master header appears at the start, at offset
 	 * 0x38.
 	 */
-	ret = cbfs_load_header_ptr(priv, base + 0x38, &priv->header);
+	ret = cbfs_load_header_ptr(priv, base + MASTER_HDR_OFFSET);
 	if (ret)
 		return ret;
 
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 07/13] cbfs: Unify the two header loaders

[Simon Glass]

These two functions have mostly the same code. Pull this out into a common
function.

Also make this function zero the private data so that callers don't have
to do it. Finally, update cbfs_load_header_ptr() to take the base of the
ROM as its parameter, which makes more sense than passing the address of
the header within the ROM.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 fs/cbfs/cbfs.c | 59 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 05de58cf19..b4e6b959d1 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -177,47 +177,63 @@ static int file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,
 	return 0;
 }
 
-/* Get the CBFS header out of the ROM and do endian conversion. */
-static int file_cbfs_load_header(struct cbfs_priv *priv, ulong end_of_rom)
+/**
+ * load_header() - Load the CBFS header
+ *
+ * Get the CBFS header out of the ROM and do endian conversion.
+ *
+ * @priv: Private data, which is inited by this function
+ * @addr: Address of CBFS header in memory-mapped SPI flash
+ * @return 0 if OK, -ENXIO if the header is bad
+ */
+static int load_header(struct cbfs_priv *priv, ulong addr)
 {
 	struct cbfs_header *header = &priv->header;
 	struct cbfs_header *header_in_rom;
-	int32_t offset = *(u32 *)(end_of_rom - 3);
 
-	header_in_rom = (struct cbfs_header *)(end_of_rom + offset + 1);
+	memset(priv, '\0', sizeof(*priv));
+	header_in_rom = (struct cbfs_header *)addr;
 	swap_header(header, header_in_rom);
 
 	if (header->magic != good_magic || header->offset >
 			header->rom_size - header->boot_block_size) {
 		priv->result = CBFS_BAD_HEADER;
-		return 1;
+		return -ENXIO;
 	}
+
 	return 0;
 }
 
-static int cbfs_load_header_ptr(struct cbfs_priv *priv, ulong base)
+/**
+ * file_cbfs_load_header() - Get the CBFS header out of the ROM, given the end
+ *
+ * @priv: Private data, which is inited by this function
+ * @addr: Address of the last byte of the ROM (typically 0xffffffff)
+ * @return 0 if OK, -ENXIO if the header is bad
+ */
+static int file_cbfs_load_header(struct cbfs_priv *priv, ulong end_of_rom)
 {
-	struct cbfs_header *header = &priv->header;
-	struct cbfs_header *header_in_rom;
-
-	header_in_rom = (struct cbfs_header *)base;
-	swap_header(header, header_in_rom);
+	int offset = *(u32 *)(end_of_rom - 3);
 
-	if (header->magic != good_magic || header->offset >
-			header->rom_size - header->boot_block_size) {
-		priv->result = CBFS_BAD_HEADER;
-		return -EFAULT;
-	}
+	return load_header(priv, end_of_rom + offset + 1);
+}
 
-	return 0;
+/**
+ * cbfs_load_header_ptr() - Get the CBFS header out of the ROM, given the base
+ *
+ * @priv: Private data, which is inited by this function
+ * @addr: Address of the first byte of the ROM (e.g. 0xff000000)
+ * @return 0 if OK, -ENXIO if the header is bad
+ */
+static int cbfs_load_header_ptr(struct cbfs_priv *priv, ulong base)
+{
+	return load_header(priv, base + MASTER_HDR_OFFSET);
 }
 
 static void cbfs_init(struct cbfs_priv *priv, ulong end_of_rom)
 {
 	u8 *start_of_rom;
 
-	priv->initialised = false;
-
 	if (file_cbfs_load_header(priv, end_of_rom))
 		return;
 
@@ -241,10 +257,9 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp)
 
 	/*
 	 * Use a local variable to start with until we know that the CBFS is
-	 * valid. Assume that a master header appears at the start, at offset
-	 * 0x38.
+	 * valid.
 	 */
-	ret = cbfs_load_header_ptr(priv, base + MASTER_HDR_OFFSET);
+	ret = cbfs_load_header_ptr(priv, base);
 	if (ret)
 		return ret;
 
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 08/13] cbfs: Use void * for the position pointers

[Simon Glass]

It doesn't make sense to use u8 * as the pointer type for accessing the
CBFS since we do not access it as bytes, but via structures. Change it to
void *, which allows us to avoid a cast.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 fs/cbfs/cbfs.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index b4e6b959d1..a4f9534724 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -83,7 +83,7 @@ static void swap_file_header(struct cbfs_fileheader *dest,
  * @return 0 if a file is found, -ENOENT if one isn't, -EBADF if a bad header
  *	is found.
  */
-static int file_cbfs_next_file(struct cbfs_priv *priv, u8 *start, int size,
+static int file_cbfs_next_file(struct cbfs_priv *priv, void *start, int size,
 			       int align, struct cbfs_cachenode *new_node,
 			       int *used)
 {
@@ -92,8 +92,7 @@ static int file_cbfs_next_file(struct cbfs_priv *priv, u8 *start, int size,
 	*used = 0;
 
 	while (size >= align) {
-		const struct cbfs_fileheader *file_header =
-			(const struct cbfs_fileheader *)start;
+		const struct cbfs_fileheader *file_header = start;
 		u32 name_len;
 		u32 step;
 
@@ -133,7 +132,7 @@ static int file_cbfs_next_file(struct cbfs_priv *priv, u8 *start, int size,
 }
 
 /* Look through a CBFS instance and copy file metadata into regular memory. */
-static int file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,
+static int file_cbfs_fill_cache(struct cbfs_priv *priv, void *start, u32 size,
 				u32 align)
 {
 	struct cbfs_cachenode *cache_node;
@@ -232,12 +231,12 @@ static int cbfs_load_header_ptr(struct cbfs_priv *priv, ulong base)
 
 static void cbfs_init(struct cbfs_priv *priv, ulong end_of_rom)
 {
-	u8 *start_of_rom;
+	void *start_of_rom;
 
 	if (file_cbfs_load_header(priv, end_of_rom))
 		return;
 
-	start_of_rom = (u8 *)(end_of_rom + 1 - priv->header.rom_size);
+	start_of_rom = (void *)(end_of_rom + 1 - priv->header.rom_size);
 
 	file_cbfs_fill_cache(priv, start_of_rom, priv->header.rom_size,
 			     priv->header.align);
@@ -263,7 +262,7 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp)
 	if (ret)
 		return ret;
 
-	file_cbfs_fill_cache(priv, (u8 *)base, priv->header.rom_size,
+	file_cbfs_fill_cache(priv, (void *)base, priv->header.rom_size,
 			     priv->header.align);
 	if (priv->result != CBFS_SUCCESS)
 		return -EINVAL;
@@ -351,7 +350,7 @@ const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom,
 						     const char *name)
 {
 	struct cbfs_priv *priv = &cbfs_s;
-	u8 *start;
+	void *start;
 	u32 size;
 	u32 align;
 	static struct cbfs_cachenode node;
@@ -359,7 +358,7 @@ const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom,
 	if (file_cbfs_load_header(priv, end_of_rom))
 		return NULL;
 
-	start = (u8 *)(end_of_rom + 1 - priv->header.rom_size);
+	start = (void *)(end_of_rom + 1 - priv->header.rom_size);
 	size = priv->header.rom_size;
 	align = priv->header.align;
 
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 09/13] cbfs: Record the start address in cbfs_priv

[Simon Glass]

The start address of the CBFS is used when scanning for files. It makes
sense to put this in our cbfs_priv struct and calculate it when we read
the header.

Update the code accordingly.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 fs/cbfs/cbfs.c | 44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index a4f9534724..58de258da3 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -14,8 +14,18 @@
 static const u32 good_magic = 0x4f524243;
 static const u8 good_file_magic[] = "LARCHIVE";
 
+/**
+ * struct cbfs_priv - Private data for this driver
+ *
+ * @initialised: true if this CBFS has been inited
+ * @start: Start position of CBFS in memory, typically memory-mapped SPI flash
+ * @header: Header read from the CBFS, byte-swapped so U-Boot can access it
+ * @file_cache: List of file headers read from CBFS
+ * @result: Success/error result
+ */
 struct cbfs_priv {
 	bool initialised;
+	void *start;
 	struct cbfs_header header;
 	struct cbfs_cachenode *file_cache;
 	enum cbfs_result result;
@@ -132,12 +142,12 @@ static int file_cbfs_next_file(struct cbfs_priv *priv, void *start, int size,
 }
 
 /* Look through a CBFS instance and copy file metadata into regular memory. */
-static int file_cbfs_fill_cache(struct cbfs_priv *priv, void *start, u32 size,
-				u32 align)
+static int file_cbfs_fill_cache(struct cbfs_priv *priv, int size, int align)
 {
 	struct cbfs_cachenode *cache_node;
 	struct cbfs_cachenode *new_node;
 	struct cbfs_cachenode **cache_tail = &priv->file_cache;
+	void *start;
 
 	/* Clear out old information. */
 	cache_node = priv->file_cache;
@@ -148,6 +158,7 @@ static int file_cbfs_fill_cache(struct cbfs_priv *priv, void *start, u32 size,
 	}
 	priv->file_cache = NULL;
 
+	start = priv->start;
 	while (size >= align) {
 		int used;
 		int ret;
@@ -213,8 +224,14 @@ static int load_header(struct cbfs_priv *priv, ulong addr)
 static int file_cbfs_load_header(struct cbfs_priv *priv, ulong end_of_rom)
 {
 	int offset = *(u32 *)(end_of_rom - 3);
+	int ret;
+
+	ret = load_header(priv, end_of_rom + offset + 1);
+	if (ret)
+		return ret;
+	priv->start = (void *)(end_of_rom + 1 - priv->header.rom_size);
 
-	return load_header(priv, end_of_rom + offset + 1);
+	return 0;
 }
 
 /**
@@ -226,20 +243,22 @@ static int file_cbfs_load_header(struct cbfs_priv *priv, ulong end_of_rom)
  */
 static int cbfs_load_header_ptr(struct cbfs_priv *priv, ulong base)
 {
-	return load_header(priv, base + MASTER_HDR_OFFSET);
+	int ret;
+
+	ret = load_header(priv, base + MASTER_HDR_OFFSET);
+	if (ret)
+		return ret;
+	priv->start = (void *)base;
+
+	return 0;
 }
 
 static void cbfs_init(struct cbfs_priv *priv, ulong end_of_rom)
 {
-	void *start_of_rom;
-
 	if (file_cbfs_load_header(priv, end_of_rom))
 		return;
 
-	start_of_rom = (void *)(end_of_rom + 1 - priv->header.rom_size);
-
-	file_cbfs_fill_cache(priv, start_of_rom, priv->header.rom_size,
-			     priv->header.align);
+	file_cbfs_fill_cache(priv, priv->header.rom_size, priv->header.align);
 	if (priv->result == CBFS_SUCCESS)
 		priv->initialised = true;
 }
@@ -262,8 +281,7 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp)
 	if (ret)
 		return ret;
 
-	file_cbfs_fill_cache(priv, (void *)base, priv->header.rom_size,
-			     priv->header.align);
+	file_cbfs_fill_cache(priv, priv->header.rom_size, priv->header.align);
 	if (priv->result != CBFS_SUCCESS)
 		return -EINVAL;
 
@@ -358,7 +376,7 @@ const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom,
 	if (file_cbfs_load_header(priv, end_of_rom))
 		return NULL;
 
-	start = (void *)(end_of_rom + 1 - priv->header.rom_size);
+	start = priv->start;
 	size = priv->header.rom_size;
 	align = priv->header.align;
 
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 10/13] cbfs: Return the error code from file_cbfs_init()

[Simon Glass]

We may as well return the error code and use it directly in the command
code. CBFS still uses its own error enum which we may be able to remove,
but leave it for now.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 cmd/cbfs.c     |  3 +--
 fs/cbfs/cbfs.c | 23 +++++++++++++++--------
 include/cbfs.h |  6 +++---
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/cmd/cbfs.c b/cmd/cbfs.c
index 98e652a4e7..83bdee6252 100644
--- a/cmd/cbfs.c
+++ b/cmd/cbfs.c
@@ -28,8 +28,7 @@ static int do_cbfs_init(cmd_tbl_t *cmdtp, int flag, int argc,
 			return 1;
 		}
 	}
-	file_cbfs_init(end_of_rom);
-	if (cbfs_get_result() != CBFS_SUCCESS) {
+	if (file_cbfs_init(end_of_rom)) {
 		printf("%s.\n", file_cbfs_error());
 		return 1;
 	}
diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 58de258da3..0db7cb9147 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -253,19 +253,26 @@ static int cbfs_load_header_ptr(struct cbfs_priv *priv, ulong base)
 	return 0;
 }
 
-static void cbfs_init(struct cbfs_priv *priv, ulong end_of_rom)
+static int cbfs_init(struct cbfs_priv *priv, ulong end_of_rom)
 {
-	if (file_cbfs_load_header(priv, end_of_rom))
-		return;
+	int ret;
 
-	file_cbfs_fill_cache(priv, priv->header.rom_size, priv->header.align);
-	if (priv->result == CBFS_SUCCESS)
-		priv->initialised = true;
+	ret = file_cbfs_load_header(priv, end_of_rom);
+	if (ret)
+		return ret;
+
+	ret = file_cbfs_fill_cache(priv, priv->header.rom_size,
+				   priv->header.align);
+	if (ret)
+		return ret;
+	priv->initialised = true;
+
+	return 0;
 }
 
-void file_cbfs_init(ulong end_of_rom)
+int file_cbfs_init(ulong end_of_rom)
 {
-	cbfs_init(&cbfs_s, end_of_rom);
+	return cbfs_init(&cbfs_s, end_of_rom);
 }
 
 int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp)
diff --git a/include/cbfs.h b/include/cbfs.h
index 80ce539070..5cc27d682d 100644
--- a/include/cbfs.h
+++ b/include/cbfs.h
@@ -100,10 +100,10 @@ enum cbfs_result cbfs_get_result(void);
 /**
  * file_cbfs_init() - Initialize the CBFS driver and load metadata into RAM.
  *
- * @end_of_rom: Points to the end of the ROM the CBFS should be read
- *                      from.
+ * @end_of_rom: Points to the end of the ROM the CBFS should be read from
+ * @return 0 if OK, -ve on error
  */
-void file_cbfs_init(ulong end_of_rom);
+int file_cbfs_init(ulong end_of_rom);
 
 /**
  * file_cbfs_get_header() - Get the header structure for the current CBFS.
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 11/13] cbfs: Change file_cbfs_find_uncached() to return an error

[Simon Glass]

This function currently returns a node pointer so there is no way to know
the error code. Also it uses data in BSS which seems unnecessary since the
caller might prefer to use a local variable.

Update the function and split its body out into a separate function so we
can use it later.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 fs/cbfs/cbfs.c | 48 +++++++++++++++++++++++++++---------------------
 include/cbfs.h | 16 +++++++---------
 2 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 0db7cb9147..76613fa871 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -371,40 +371,46 @@ const struct cbfs_cachenode *file_cbfs_find(const char *name)
 	return cbfs_find_file(&cbfs_s, name);
 }
 
-const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom,
-						     const char *name)
+static int find_uncached(struct cbfs_priv *priv, const char *name, u8 *start,
+			 struct cbfs_cachenode *node)
 {
-	struct cbfs_priv *priv = &cbfs_s;
-	void *start;
-	u32 size;
-	u32 align;
-	static struct cbfs_cachenode node;
-
-	if (file_cbfs_load_header(priv, end_of_rom))
-		return NULL;
-
-	start = priv->start;
-	size = priv->header.rom_size;
-	align = priv->header.align;
+	int size = priv->header.rom_size;
+	int align = priv->header.align;
 
 	while (size >= align) {
-		int ret;
 		int used;
+		int ret;
 
-		ret = file_cbfs_next_file(priv, start, size, align, &node,
+		ret = file_cbfs_next_file(priv, start, size, align, node,
 					  &used);
 		if (ret == -ENOENT)
 			break;
 		else if (ret)
-			return NULL;
-		if (!strcmp(name, node.name))
-			return &node;
+			return ret;
+		if (!strcmp(name, node->name))
+			return 0;
 
 		size -= used;
 		start += used;
 	}
-	cbfs_s.result = CBFS_FILE_NOT_FOUND;
-	return NULL;
+	priv->result = CBFS_FILE_NOT_FOUND;
+
+	return -ENOENT;
+}
+
+int file_cbfs_find_uncached(ulong end_of_rom, const char *name,
+			    struct cbfs_cachenode *node)
+{
+	struct cbfs_priv priv;
+	u8 *start;
+	int ret;
+
+	ret = file_cbfs_load_header(&priv, end_of_rom);
+	if (ret)
+		return ret;
+	start = (u8 *)(end_of_rom + 1 - priv.header.rom_size);
+
+	return find_uncached(&priv, name, start, node);
 }
 
 const char *file_cbfs_name(const struct cbfs_cachenode *file)
diff --git a/include/cbfs.h b/include/cbfs.h
index 5cc27d682d..4dd3c0795d 100644
--- a/include/cbfs.h
+++ b/include/cbfs.h
@@ -163,17 +163,15 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp);
 /***************************************************************************/
 
 /**
- * file_cbfs_find_uncached() - Find a file with a particular name in CBFS
- * without using the heap.
+ * file_cbfs_find_uncached() - Find a file in CBFS without using the heap
  *
- * @end_of_rom:		Points to the end of the ROM the CBFS should be read
- *                      from.
- * @name:		The name to search for.
- *
- * @return A handle to the file, or NULL on error.
+ * @end_of_rom: Points to the end of the ROM the CBFS should be read from
+ * @name: The name to search for
+ * @node: Returns the node if found
+ * @return 0 on success, -ENOENT if not found, -EFAULT on bad header
  */
-const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom,
-						     const char *name);
+int file_cbfs_find_uncached(ulong end_of_rom, const char *name,
+			    struct cbfs_cachenode *node);
 
 /**
  * file_cbfs_name() - Get the name of a file in CBFS.
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 12/13] cbfs: Allow reading a file from a CBFS given its base addr

[Simon Glass]

Currently we support reading a file from CBFS given the address of the end
of the ROM. Sometimes we only know the start of the CBFS. Add a function
to find a file given that.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 fs/cbfs/cbfs.c | 13 +++++++++++++
 include/cbfs.h | 11 +++++++++++
 2 files changed, 24 insertions(+)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 76613fa871..1603409a8f 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -413,6 +413,19 @@ int file_cbfs_find_uncached(ulong end_of_rom, const char *name,
 	return find_uncached(&priv, name, start, node);
 }
 
+int file_cbfs_find_uncached_base(ulong base, const char *name,
+				 struct cbfs_cachenode *node)
+{
+	struct cbfs_priv priv;
+	int ret;
+
+	ret = cbfs_load_header_ptr(&priv, base);
+	if (ret)
+		return ret;
+
+	return find_uncached(&priv, name, (u8 *)base, node);
+}
+
 const char *file_cbfs_name(const struct cbfs_cachenode *file)
 {
 	cbfs_s.result = CBFS_SUCCESS;
diff --git a/include/cbfs.h b/include/cbfs.h
index 4dd3c0795d..b1a8d2cad2 100644
--- a/include/cbfs.h
+++ b/include/cbfs.h
@@ -173,6 +173,17 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp);
 int file_cbfs_find_uncached(ulong end_of_rom, const char *name,
 			    struct cbfs_cachenode *node);
 
+/**
+ * file_cbfs_find_uncached() - Find a file in CBFS without using the heap
+ *
+ * @base: Points to the base of the CBFS
+ * @name: The name to search for
+ * @node: Returns the node if found
+ * @return 0 on success, -ENOENT if not found, -EFAULT on bad header
+ */
+int file_cbfs_find_uncached_base(ulong base, const char *name,
+				 struct cbfs_cachenode *node);
+
 /**
  * file_cbfs_name() - Get the name of a file in CBFS.
  *
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 13/13] cbfs: Don't require the CBFS size with cbfs_init_mem()

[Simon Glass]

The size is not actually used since it is present in the header. Drop this
parameter. Also tidy up error handling while we are here.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/lib/fsp2/fsp_init.c | 3 +--
 fs/cbfs/cbfs.c               | 9 +++++----
 include/cbfs.h               | 3 +--
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/x86/lib/fsp2/fsp_init.c b/arch/x86/lib/fsp2/fsp_init.c
index c7dc2ea257..668a4aa723 100644
--- a/arch/x86/lib/fsp2/fsp_init.c
+++ b/arch/x86/lib/fsp2/fsp_init.c
@@ -79,11 +79,10 @@ static int get_cbfs_fsp(enum fsp_type_t type, ulong map_base,
 	 * 'COREBOOT' (CBFS, size 1814528, offset 2117632).
 	 */
 	ulong cbfs_base = 0x205000;
-	ulong cbfs_size = 0x1bb000;
 	struct cbfs_priv *cbfs;
 	int ret;
 
-	ret = cbfs_init_mem(map_base + cbfs_base, cbfs_size, &cbfs);
+	ret = cbfs_init_mem(map_base + cbfs_base, &cbfs);
 	if (ret)
 		return ret;
 	if (!ret) {
diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 1603409a8f..30cd58f5ad 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -275,7 +275,7 @@ int file_cbfs_init(ulong end_of_rom)
 	return cbfs_init(&cbfs_s, end_of_rom);
 }
 
-int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp)
+int cbfs_init_mem(ulong base, struct cbfs_priv **privp)
 {
 	struct cbfs_priv priv_s, *priv = &priv_s;
 	int ret;
@@ -288,9 +288,10 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp)
 	if (ret)
 		return ret;
 
-	file_cbfs_fill_cache(priv, priv->header.rom_size, priv->header.align);
-	if (priv->result != CBFS_SUCCESS)
-		return -EINVAL;
+	ret = file_cbfs_fill_cache(priv, priv->header.rom_size,
+				   priv->header.align);
+	if (ret)
+		return log_msg_ret("fill", ret);
 
 	priv->initialised = true;
 	priv = malloc(sizeof(priv_s));
diff --git a/include/cbfs.h b/include/cbfs.h
index b1a8d2cad2..70f3b7a17b 100644
--- a/include/cbfs.h
+++ b/include/cbfs.h
@@ -151,11 +151,10 @@ const struct cbfs_cachenode *cbfs_find_file(struct cbfs_priv *cbfs,
  * cbfs_init_mem() - Set up a new CBFS
  *
  * @base: Base address of CBFS
- * @size: Size of CBFS in bytes
  * @cbfsp: Returns a pointer to CBFS on success
  * @return 0 if OK, -ve on error
  */
-int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp);
+int cbfs_init_mem(ulong base, struct cbfs_priv **privp);
 
 
 /***************************************************************************/
-- 
2.26.2.645.ge9eca65c58-goog

[PATCH 01/13] cbfs: Rename the result variable

[Bin Meng]

Hi Simon,

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> At present the result variable in the cbfs_priv is called 'result' as is
> the local variable in a few functions. Change the latter to 'ret' which is
> more common in U-Boot and avoids confusion.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  fs/cbfs/cbfs.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
> index 1aa6f8ee84..70440aa80b 100644
> --- a/fs/cbfs/cbfs.c
> +++ b/fs/cbfs/cbfs.c
> @@ -145,18 +145,18 @@ static void file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,
>         priv->file_cache = NULL;
>
>         while (size >= align) {
> -               int result;
> +               int ret;
>                 u32 used;
>
>                 new_node = (struct cbfs_cachenode *)
>                                 malloc(sizeof(struct cbfs_cachenode));
> -               result = file_cbfs_next_file(priv, start, size, align, new_node,
> -                                            &used);
> +               ret = file_cbfs_next_file(priv, start, size, align, new_node,
> +                                         &used);

Could you also fix the return value description in the comment block
of file_cbfs_next_file()? Now it only mentions 0 and 1, but it returns
-1 when something goes wrong.

>
> -               if (result < 0) {
> +               if (ret < 0) {
>                         free(new_node);
>                         return;
> -               } else if (result == 0) {
> +               } else if (ret == 0) {
>                         free(new_node);
>                         break;
>                 }
> @@ -341,15 +341,15 @@ const struct cbfs_cachenode *file_cbfs_find_uncached(uintptr_t end_of_rom,
>         align = priv->header.align;
>
>         while (size >= align) {
> -               int result;
> +               int ret;
>                 u32 used;
>
> -               result = file_cbfs_next_file(priv, start, size, align, &node,
> -                                            &used);
> +               ret = file_cbfs_next_file(priv, start, size, align, &node,
> +                                         &used);
>
> -               if (result < 0)
> +               if (ret < 0)
>                         return NULL;
> -               else if (result == 0)
> +               else if (ret == 0)
>                         break;
>
>                 if (!strcmp(name, node.name))
> --

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Regards,
Bin

[PATCH 02/13] cbfs: Use ulong consistently

[Bin Meng]

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> U-Boot uses ulong for addresses but there are a few places in this driver
> that don't use it. Convert this driver over to follow this convention
> fully.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  fs/cbfs/cbfs.c | 9 ++++-----
>  include/cbfs.h | 4 ++--
>  2 files changed, 6 insertions(+), 7 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 03/13] cbfs: Use bool type for whether initialised

[Bin Meng]

Hi Simon,

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> At present this uses an int type. U-Boot now supports bool so use this
> instead. Also use English spelling for initialised which we are here.

I don't understand what issue is with American English "initialized"?
I think they both are acceptable.

>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  fs/cbfs/cbfs.c | 28 ++++++++++++++--------------
>  include/cbfs.h |  2 +-
>  2 files changed, 15 insertions(+), 15 deletions(-)
>

Regards,
Bin

[PATCH 04/13] cbfs: Adjust return value of file_cbfs_next_file()

[Bin Meng]

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> At present his uses a true return to indicate it found a file. Adjust it

typo: this

> to use 0 for this, so it is consistent with other functions.
>
> Update its callers accordingling and add a check for malloc() failure in

accordingly

> file_cbfs_fill_cache().
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  fs/cbfs/cbfs.c | 43 +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 05/13] cbfs: Adjust file_cbfs_load_header() to use cbfs_priv

[Bin Meng]

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> This function is strange at the moment in that it takes a header pointer
> but then accesses the cbfs_s global. Currently clients have their own priv
> pointer, so update the function to take that as a parameter instead.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  fs/cbfs/cbfs.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 06/13] cbfs: Adjust cbfs_load_header_ptr() to use cbfs_priv

[Bin Meng]

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> This function is strange at the moment in that it takes a header pointer
> but then accesses the cbfs_s global. Currently clients have their own priv
> pointer, so update the function to take that as a parameter instead.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  fs/cbfs/cbfs.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 07/13] cbfs: Unify the two header loaders

[Bin Meng]

Hi Simon,

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> These two functions have mostly the same code. Pull this out into a common
> function.
>
> Also make this function zero the private data so that callers don't have
> to do it. Finally, update cbfs_load_header_ptr() to take the base of the
> ROM as its parameter, which makes more sense than passing the address of
> the header within the ROM.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  fs/cbfs/cbfs.c | 59 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
> index 05de58cf19..b4e6b959d1 100644
> --- a/fs/cbfs/cbfs.c
> +++ b/fs/cbfs/cbfs.c
> @@ -177,47 +177,63 @@ static int file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,
>         return 0;
>  }
>
> -/* Get the CBFS header out of the ROM and do endian conversion. */
> -static int file_cbfs_load_header(struct cbfs_priv *priv, ulong end_of_rom)
> +/**
> + * load_header() - Load the CBFS header
> + *
> + * Get the CBFS header out of the ROM and do endian conversion.
> + *
> + * @priv: Private data, which is inited by this function
> + * @addr: Address of CBFS header in memory-mapped SPI flash
> + * @return 0 if OK, -ENXIO if the header is bad
> + */
> +static int load_header(struct cbfs_priv *priv, ulong addr)
>  {
>         struct cbfs_header *header = &priv->header;
>         struct cbfs_header *header_in_rom;
> -       int32_t offset = *(u32 *)(end_of_rom - 3);
>
> -       header_in_rom = (struct cbfs_header *)(end_of_rom + offset + 1);
> +       memset(priv, '\0', sizeof(*priv));
> +       header_in_rom = (struct cbfs_header *)addr;
>         swap_header(header, header_in_rom);
>
>         if (header->magic != good_magic || header->offset >
>                         header->rom_size - header->boot_block_size) {
>                 priv->result = CBFS_BAD_HEADER;
> -               return 1;
> +               return -ENXIO;
>         }
> +
>         return 0;
>  }
>
> -static int cbfs_load_header_ptr(struct cbfs_priv *priv, ulong base)
> +/**
> + * file_cbfs_load_header() - Get the CBFS header out of the ROM, given the end
> + *
> + * @priv: Private data, which is inited by this function
> + * @addr: Address of the last byte of the ROM (typically 0xffffffff)

This should be end_of_rom

> + * @return 0 if OK, -ENXIO if the header is bad
> + */
> +static int file_cbfs_load_header(struct cbfs_priv *priv, ulong end_of_rom)
>  {
> -       struct cbfs_header *header = &priv->header;
> -       struct cbfs_header *header_in_rom;
> -
> -       header_in_rom = (struct cbfs_header *)base;
> -       swap_header(header, header_in_rom);
> +       int offset = *(u32 *)(end_of_rom - 3);
>
> -       if (header->magic != good_magic || header->offset >
> -                       header->rom_size - header->boot_block_size) {
> -               priv->result = CBFS_BAD_HEADER;
> -               return -EFAULT;
> -       }
> +       return load_header(priv, end_of_rom + offset + 1);
> +}
>
> -       return 0;
> +/**
> + * cbfs_load_header_ptr() - Get the CBFS header out of the ROM, given the base
> + *
> + * @priv: Private data, which is inited by this function
> + * @addr: Address of the first byte of the ROM (e.g. 0xff000000)

This should be base

> + * @return 0 if OK, -ENXIO if the header is bad
> + */
> +static int cbfs_load_header_ptr(struct cbfs_priv *priv, ulong base)
> +{
> +       return load_header(priv, base + MASTER_HDR_OFFSET);
>  }
>
>  static void cbfs_init(struct cbfs_priv *priv, ulong end_of_rom)
>  {
>         u8 *start_of_rom;
>
> -       priv->initialised = false;
> -
>         if (file_cbfs_load_header(priv, end_of_rom))
>                 return;
>
> @@ -241,10 +257,9 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp)
>
>         /*
>          * Use a local variable to start with until we know that the CBFS is
> -        * valid. Assume that a master header appears at the start, at offset
> -        * 0x38.
> +        * valid.
>          */
> -       ret = cbfs_load_header_ptr(priv, base + MASTER_HDR_OFFSET);
> +       ret = cbfs_load_header_ptr(priv, base);
>         if (ret)
>                 return ret;
>

Regards,
Bin

[PATCH 08/13] cbfs: Use void * for the position pointers

[Bin Meng]

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> It doesn't make sense to use u8 * as the pointer type for accessing the
> CBFS since we do not access it as bytes, but via structures. Change it to
> void *, which allows us to avoid a cast.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  fs/cbfs/cbfs.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 09/13] cbfs: Record the start address in cbfs_priv

[Bin Meng]

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> The start address of the CBFS is used when scanning for files. It makes
> sense to put this in our cbfs_priv struct and calculate it when we read
> the header.
>
> Update the code accordingly.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  fs/cbfs/cbfs.c | 44 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 13 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 10/13] cbfs: Return the error code from file_cbfs_init()

[Bin Meng]

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> We may as well return the error code and use it directly in the command
> code. CBFS still uses its own error enum which we may be able to remove,
> but leave it for now.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  cmd/cbfs.c     |  3 +--
>  fs/cbfs/cbfs.c | 23 +++++++++++++++--------
>  include/cbfs.h |  6 +++---
>  3 files changed, 19 insertions(+), 13 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 11/13] cbfs: Change file_cbfs_find_uncached() to return an error

[Bin Meng]

Hi Simon,

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> This function currently returns a node pointer so there is no way to know
> the error code. Also it uses data in BSS which seems unnecessary since the
> caller might prefer to use a local variable.
>
> Update the function and split its body out into a separate function so we
> can use it later.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  fs/cbfs/cbfs.c | 48 +++++++++++++++++++++++++++---------------------
>  include/cbfs.h | 16 +++++++---------
>  2 files changed, 34 insertions(+), 30 deletions(-)
>
> diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
> index 0db7cb9147..76613fa871 100644
> --- a/fs/cbfs/cbfs.c
> +++ b/fs/cbfs/cbfs.c
> @@ -371,40 +371,46 @@ const struct cbfs_cachenode *file_cbfs_find(const char *name)
>         return cbfs_find_file(&cbfs_s, name);
>  }
>
> -const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom,
> -                                                    const char *name)
> +static int find_uncached(struct cbfs_priv *priv, const char *name, u8 *start,

This should be void *start

> +                        struct cbfs_cachenode *node)
>  {
> -       struct cbfs_priv *priv = &cbfs_s;
> -       void *start;
> -       u32 size;
> -       u32 align;
> -       static struct cbfs_cachenode node;
> -
> -       if (file_cbfs_load_header(priv, end_of_rom))
> -               return NULL;
> -
> -       start = priv->start;
> -       size = priv->header.rom_size;
> -       align = priv->header.align;
> +       int size = priv->header.rom_size;
> +       int align = priv->header.align;
>
>         while (size >= align) {
> -               int ret;
>                 int used;
> +               int ret;
>
> -               ret = file_cbfs_next_file(priv, start, size, align, &node,
> +               ret = file_cbfs_next_file(priv, start, size, align, node,
>                                           &used);
>                 if (ret == -ENOENT)
>                         break;
>                 else if (ret)
> -                       return NULL;
> -               if (!strcmp(name, node.name))
> -                       return &node;
> +                       return ret;
> +               if (!strcmp(name, node->name))
> +                       return 0;
>
>                 size -= used;
>                 start += used;
>         }
> -       cbfs_s.result = CBFS_FILE_NOT_FOUND;
> -       return NULL;
> +       priv->result = CBFS_FILE_NOT_FOUND;
> +
> +       return -ENOENT;
> +}
> +
> +int file_cbfs_find_uncached(ulong end_of_rom, const char *name,
> +                           struct cbfs_cachenode *node)
> +{
> +       struct cbfs_priv priv;
> +       u8 *start;
> +       int ret;
> +
> +       ret = file_cbfs_load_header(&priv, end_of_rom);
> +       if (ret)
> +               return ret;
> +       start = (u8 *)(end_of_rom + 1 - priv.header.rom_size);

This should be: start = priv->start;

> +
> +       return find_uncached(&priv, name, start, node);
>  }
>
>  const char *file_cbfs_name(const struct cbfs_cachenode *file)
> diff --git a/include/cbfs.h b/include/cbfs.h
> index 5cc27d682d..4dd3c0795d 100644
> --- a/include/cbfs.h
> +++ b/include/cbfs.h
> @@ -163,17 +163,15 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp);
>  /***************************************************************************/
>
>  /**
> - * file_cbfs_find_uncached() - Find a file with a particular name in CBFS
> - * without using the heap.
> + * file_cbfs_find_uncached() - Find a file in CBFS without using the heap
>   *
> - * @end_of_rom:                Points to the end of the ROM the CBFS should be read
> - *                      from.
> - * @name:              The name to search for.
> - *
> - * @return A handle to the file, or NULL on error.
> + * @end_of_rom: Points to the end of the ROM the CBFS should be read from
> + * @name: The name to search for
> + * @node: Returns the node if found

This is misleading. Based on the comments it seems that we should
declare this to be:

struct cbfs_cachenode **node

> + * @return 0 on success, -ENOENT if not found, -EFAULT on bad header
>   */
> -const struct cbfs_cachenode *file_cbfs_find_uncached(ulong end_of_rom,
> -                                                    const char *name);
> +int file_cbfs_find_uncached(ulong end_of_rom, const char *name,
> +                           struct cbfs_cachenode *node);
>
>  /**
>   * file_cbfs_name() - Get the name of a file in CBFS.
> --

Regards,
Bin

[PATCH 12/13] cbfs: Allow reading a file from a CBFS given its base addr

[Bin Meng]

Hi Simon,

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> Currently we support reading a file from CBFS given the address of the end
> of the ROM. Sometimes we only know the start of the CBFS. Add a function
> to find a file given that.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  fs/cbfs/cbfs.c | 13 +++++++++++++
>  include/cbfs.h | 11 +++++++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
> index 76613fa871..1603409a8f 100644
> --- a/fs/cbfs/cbfs.c
> +++ b/fs/cbfs/cbfs.c
> @@ -413,6 +413,19 @@ int file_cbfs_find_uncached(ulong end_of_rom, const char *name,
>         return find_uncached(&priv, name, start, node);
>  }
>
> +int file_cbfs_find_uncached_base(ulong base, const char *name,
> +                                struct cbfs_cachenode *node)
> +{
> +       struct cbfs_priv priv;
> +       int ret;
> +
> +       ret = cbfs_load_header_ptr(&priv, base);
> +       if (ret)
> +               return ret;
> +
> +       return find_uncached(&priv, name, (u8 *)base, node);

(void *)base

> +}
> +
>  const char *file_cbfs_name(const struct cbfs_cachenode *file)
>  {
>         cbfs_s.result = CBFS_SUCCESS;
> diff --git a/include/cbfs.h b/include/cbfs.h
> index 4dd3c0795d..b1a8d2cad2 100644
> --- a/include/cbfs.h
> +++ b/include/cbfs.h
> @@ -173,6 +173,17 @@ int cbfs_init_mem(ulong base, ulong size, struct cbfs_priv **privp);
>  int file_cbfs_find_uncached(ulong end_of_rom, const char *name,
>                             struct cbfs_cachenode *node);
>
> +/**
> + * file_cbfs_find_uncached() - Find a file in CBFS without using the heap

file_cbfs_find_uncached_base(), and the description is wrong

> + *
> + * @base: Points to the base of the CBFS
> + * @name: The name to search for
> + * @node: Returns the node if found
> + * @return 0 on success, -ENOENT if not found, -EFAULT on bad header
> + */
> +int file_cbfs_find_uncached_base(ulong base, const char *name,
> +                                struct cbfs_cachenode *node);
> +
>  /**
>   * file_cbfs_name() - Get the name of a file in CBFS.
>   *

Regards,
Bin

[PATCH 13/13] cbfs: Don't require the CBFS size with cbfs_init_mem()

[Bin Meng]

On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
>
> The size is not actually used since it is present in the header. Drop this
> parameter. Also tidy up error handling while we are here.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/lib/fsp2/fsp_init.c | 3 +--
>  fs/cbfs/cbfs.c               | 9 +++++----
>  include/cbfs.h               | 3 +--
>  3 files changed, 7 insertions(+), 8 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 12/13] cbfs: Allow reading a file from a CBFS given its base addr

[Simon Glass]

Hi Bin,

On Wed, 20 May 2020 at 20:59, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Currently we support reading a file from CBFS given the address of the end
> > of the ROM. Sometimes we only know the start of the CBFS. Add a function
> > to find a file given that.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  fs/cbfs/cbfs.c | 13 +++++++++++++
> >  include/cbfs.h | 11 +++++++++++
> >  2 files changed, 24 insertions(+)
> >

Thanks for the comments. I had a change of plan at some point and the
series lost its consistency. Will send v2 soon.

Regards,
Simon

[PATCH 01/13] cbfs: Rename the result variable

[Simon Glass]

Hi Bin,

On Tue, 19 May 2020 at 07:53, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > At present the result variable in the cbfs_priv is called 'result' as is
> > the local variable in a few functions. Change the latter to 'ret' which is
> > more common in U-Boot and avoids confusion.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  fs/cbfs/cbfs.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
> > index 1aa6f8ee84..70440aa80b 100644
> > --- a/fs/cbfs/cbfs.c
> > +++ b/fs/cbfs/cbfs.c
> > @@ -145,18 +145,18 @@ static void file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,
> >         priv->file_cache = NULL;
> >
> >         while (size >= align) {
> > -               int result;
> > +               int ret;
> >                 u32 used;
> >
> >                 new_node = (struct cbfs_cachenode *)
> >                                 malloc(sizeof(struct cbfs_cachenode));
> > -               result = file_cbfs_next_file(priv, start, size, align, new_node,
> > -                                            &used);
> > +               ret = file_cbfs_next_file(priv, start, size, align, new_node,
> > +                                         &used);
>
> Could you also fix the return value description in the comment block
> of file_cbfs_next_file()? Now it only mentions 0 and 1, but it returns
> -1 when something goes wrong.

A later fixes the return values and the comments, as you may have seen.
[..]

Regards,
Simon

[PATCH 03/13] cbfs: Use bool type for whether initialised

[Simon Glass]

Hi Bin,

On Tue, 19 May 2020 at 07:53, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Wed, May 13, 2020 at 10:24 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > At present this uses an int type. U-Boot now supports bool so use this
> > instead. Also use English spelling for initialised which we are here.
>
> I don't understand what issue is with American English "initialized"?
> I think they both are acceptable.

Yes they are. Just trying to keep a balance :-)

Regards,
Simon