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

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(-)

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

Hi Bin,

On Wed, 14 Aug 2019 at 02:40, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> 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?

We could, but I would prefer to keep it opaque. I don't think outside
code needs to look at it.

[...]

Regards,
Simon