Re: [U-Boot] [PATCH 3/6] cbfs: Move static variables into a struct

Hi Simon,

On Wed, Aug 14, 2019 at 11:09 AM Simon Glass sjg@chromium.org wrote:

At present there are a number of static variables in BSS. This cannot work with SPL, at least until BSS is available in board_init_r().

Move the variables into a struct, so it is possible to malloc() it and use it before BSS is available.

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

fs/cbfs/cbfs.c | 96 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 63 insertions(+), 33 deletions(-)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c index 2a9edcc9a0..f4298d7320 100644 --- a/fs/cbfs/cbfs.c +++ b/fs/cbfs/cbfs.c @@ -11,9 +11,14 @@ enum cbfs_result file_cbfs_result; static const u32 good_magic = 0x4f524243; static const u8 good_file_magic[] = "LARCHIVE"; -static int initialized; -static struct cbfs_header cbfs_header; -static struct cbfs_cachenode *file_cache;

+struct cbfs_priv {

•   int initialized;
  

•   struct cbfs_header header;
  

•   struct cbfs_cachenode *file_cache;
  

+};

Could we move this struct define to cbfs.h?

+static struct cbfs_priv cbfs_s;

const char *file_cbfs_error(void) { @@ -69,8 +74,9 @@ static void swap_file_header(struct cbfs_fileheader *dest,

• @return 1 if a file is found, 0 if one isn't.

*/ -static int file_cbfs_next_file(u8 *start, u32 size, u32 align,

•                          struct cbfs_cachenode *newNode, u32 *used)
  

+static int file_cbfs_next_file(struct cbfs_priv *priv, u8 *start, u32 size,

•                          u32 align, struct cbfs_cachenode *newNode,
  

•                          u32 *used)
  

{ struct cbfs_fileheader header;

@@ -117,20 +123,21 @@ static int file_cbfs_next_file(u8 *start, u32 size, u32 align, }

/* Look through a CBFS instance and copy file metadata into regular memory. */ -static void file_cbfs_fill_cache(u8 *start, u32 size, u32 align) +static void file_cbfs_fill_cache(struct cbfs_priv *priv, u8 *start, u32 size,

•                            u32 align)
  

{ struct cbfs_cachenode *cache_node; struct cbfs_cachenode *newNode;

•   struct cbfs_cachenode **cache_tail = &file_cache;
  

•   struct cbfs_cachenode **cache_tail = &priv->file_cache;
  
    /* Clear out old information. */
  

•   cache_node = file_cache;
  

•   cache_node = priv->file_cache;
    while (cache_node) {
            struct cbfs_cachenode *oldNode = cache_node;
            cache_node = cache_node->next;
            free(oldNode);
    }
  

•   file_cache = NULL;
  

•   priv->file_cache = NULL;
  
    while (size >= align) {
            int result;
  

@@ -138,8 +145,8 @@ static void file_cbfs_fill_cache(u8 *start, u32 size, u32 align)

            newNode = (struct cbfs_cachenode *)
                            malloc(sizeof(struct cbfs_cachenode));

•           result = file_cbfs_next_file(start, size, align,
  

•                   newNode, &used);
  

•           result = file_cbfs_next_file(priv, start, size, align, newNode,
  

•                                        &used);
  
            if (result < 0) {
                    free(newNode);
  

@@ -175,27 +182,35 @@ static int file_cbfs_load_header(uintptr_t end_of_rom, return 0; }

-void file_cbfs_init(uintptr_t end_of_rom) +void cbfs_init(struct cbfs_priv *priv, uintptr_t end_of_rom)

nits: this should be static

{ u8 *start_of_rom;

•   initialized = 0;
  
  

•   if (file_cbfs_load_header(end_of_rom, &cbfs_header))
  

•   priv->initialized = 0;
  

•   if (file_cbfs_load_header(end_of_rom, &priv->header))
            return;
  
  

•   start_of_rom = (u8 *)(end_of_rom + 1 - cbfs_header.rom_size);
  

•   start_of_rom = (u8 *)(end_of_rom + 1 - priv->header.rom_size);
  
  

•   file_cbfs_fill_cache(start_of_rom, cbfs_header.rom_size,
  

•                        cbfs_header.align);
  

•   file_cbfs_fill_cache(priv, start_of_rom, priv->header.rom_size,
  

•                        priv->header.align);
    if (file_cbfs_result == CBFS_SUCCESS)
  

•           initialized = 1;
  

•           priv->initialized = 1;
  

+}

+void file_cbfs_init(uintptr_t end_of_rom) +{

•   cbfs_init(&cbfs_s, end_of_rom);
  

}

const struct cbfs_header *file_cbfs_get_header(void) {

•   if (initialized) {
  

•   struct cbfs_priv *priv = &cbfs_s;
  

•   if (priv->initialized) {
            file_cbfs_result = CBFS_SUCCESS;
  

•           return &cbfs_header;
  

•           return &priv->header;
    } else {
            file_cbfs_result = CBFS_NOT_INITIALIZED;
            return NULL;
  

@@ -204,20 +219,24 @@ const struct cbfs_header *file_cbfs_get_header(void)

const struct cbfs_cachenode *file_cbfs_get_first(void) {

•   if (!initialized) {
  

•   struct cbfs_priv *priv = &cbfs_s;
  

•   if (!priv->initialized) {
            file_cbfs_result = CBFS_NOT_INITIALIZED;
            return NULL;
    } else {
            file_cbfs_result = CBFS_SUCCESS;
  

•           return file_cache;
  

•           return priv->file_cache;
    }
  

}

void file_cbfs_get_next(const struct cbfs_cachenode **file) {

•   if (!initialized) {
  

•   struct cbfs_priv *priv = &cbfs_s;
  

•   if (!priv->initialized) {
            file_cbfs_result = CBFS_NOT_INITIALIZED;
  

•           file = NULL;
  

•           *file = NULL;
            return;
    }
  
  

@@ -226,11 +245,12 @@ void file_cbfs_get_next(const struct cbfs_cachenode **file) file_cbfs_result = CBFS_SUCCESS; }

-const struct cbfs_cachenode *file_cbfs_find(const char *name) +const struct cbfs_cachenode *cbfs_find_file(struct cbfs_priv *priv,

•                                       const char *name)
  

{

•   struct cbfs_cachenode *cache_node = file_cache;
  

•   struct cbfs_cachenode *cache_node = priv->file_cache;
  
  

•   if (!initialized) {
  

•   if (!priv->initialized) {
            file_cbfs_result = CBFS_NOT_INITIALIZED;
            return NULL;
    }
  

@@ -248,26 +268,33 @@ const struct cbfs_cachenode *file_cbfs_find(const char *name) return cache_node; }

+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 char *name) {

•   struct cbfs_priv *priv = &cbfs_s;
    u8 *start;
    u32 size;
    u32 align;
    static struct cbfs_cachenode node;
  
  

•   if (file_cbfs_load_header(end_of_rom, &cbfs_header))
  

•   if (file_cbfs_load_header(end_of_rom, &priv->header))
            return NULL;
  
  

•   start = (u8 *)(end_of_rom + 1 - cbfs_header.rom_size);
  

•   size = cbfs_header.rom_size;
  

•   align = cbfs_header.align;
  

•   start = (u8 *)(end_of_rom + 1 - priv->header.rom_size);
  

•   size = priv->header.rom_size;
  

•   align = priv->header.align;
  
    while (size >= align) {
            int result;
            u32 used;
  
  

•           result = file_cbfs_next_file(start, size, align, &node, &used);
  

•           result = file_cbfs_next_file(priv, start, size, align, &node,
  

•                                        &used);
  
            if (result < 0)
                    return NULL;
  

@@ -287,18 +314,21 @@ const struct cbfs_cachenode *file_cbfs_find_uncached(uintptr_t end_of_rom, const char *file_cbfs_name(const struct cbfs_cachenode *file) { file_cbfs_result = CBFS_SUCCESS;

I would move these changes to the next patch that touch this function.

    return file->name;

}

u32 file_cbfs_size(const struct cbfs_cachenode *file) { file_cbfs_result = CBFS_SUCCESS;

ditto, please fix the following 2 as well

    return file->data_length;

}

u32 file_cbfs_type(const struct cbfs_cachenode *file) { file_cbfs_result = CBFS_SUCCESS;

•   return file->type;
  

}

@@ -312,7 +342,7 @@ long file_cbfs_read(const struct cbfs_cachenode *file, void *buffer, size = maxsize;

    memcpy(buffer, file->data, size);

•   file_cbfs_result = CBFS_SUCCESS;
  

•   return size;
  

}

Other than above, Reviewed-by: Bin Meng bmeng.cn@gmail.com

Regards, Bin