[U-Boot] [PATCH 1/4] x86: qemu: re-structure qemu_fwcfg_list_firmware()

[Bin Meng]

Hi Miao,

On Fri, Jan 15, 2016 at 11:12 AM, Miao Yan <yanmiaobest at gmail.com> wrote:
> Re-write the logic in qemu_fwcfg_list_firmware(), add a function
> qemu_cfg_read_firmware_list() to handle reading firmware list.

qemu_fwcfg_read_firmware_list()

>
> Signed-off-by: Miao Yan <yanmiaobest at gmail.com>
> ---
>  arch/x86/cpu/qemu/fw_cfg.c    | 60 +++++++++++++++++++++++++++++++++----------
>  arch/x86/include/asm/fw_cfg.h |  8 ++++++
>  2 files changed, 54 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/cpu/qemu/fw_cfg.c b/arch/x86/cpu/qemu/fw_cfg.c
> index 0599214..b22026c 100644
> --- a/arch/x86/cpu/qemu/fw_cfg.c
> +++ b/arch/x86/cpu/qemu/fw_cfg.c
> @@ -10,10 +10,13 @@
>  #include <malloc.h>
>  #include <asm/io.h>
>  #include <asm/fw_cfg.h>
> +#include <linux/list.h>
>
>  static bool fwcfg_present;
>  static bool fwcfg_dma_present;
>
> +static LIST_HEAD(fw_list);
> +
>  /* Read configuration item using fw_cfg PIO interface */
>  static void qemu_fwcfg_read_entry_pio(uint16_t entry,
>                 uint32_t size, void *address)
> @@ -162,29 +165,58 @@ static int qemu_fwcfg_setup_kernel(void *load_addr, void *initrd_addr)
>         return 0;
>  }
>
> -static int qemu_fwcfg_list_firmware(void)
> +static int qemu_fwcfg_read_firmware_list(void)
>  {
>         int i;
>         uint32_t count;
> -       struct fw_cfg_files *files;
> +       struct fw_file *file;
> +       struct list_head *entry;
> +
> +       /* don't read it twice */
> +       if (!list_empty(&fw_list))
> +               return 0;
>
>         qemu_fwcfg_read_entry(FW_CFG_FILE_DIR, 4, &count);
>         if (!count)
>                 return 0;
>
>         count = be32_to_cpu(count);
> -       files = malloc(count * sizeof(struct fw_cfg_file));
> -       if (!files)
> -               return -ENOMEM;
> -
> -       files->count = count;
> -       qemu_fwcfg_read_entry(FW_CFG_INVALID,
> -                             count * sizeof(struct fw_cfg_file),
> -                             files->files);
> -
> -       for (i = 0; i < files->count; i++)
> -               printf("%-56s\n", files->files[i].name);
> -       free(files);
> +       for (i = 0; i < count; i++) {
> +               file = malloc(sizeof(*file));
> +               if (!file) {
> +                       printf("error: allocating resource\n");
> +                       goto err;
> +               }
> +               qemu_fwcfg_read_entry(FW_CFG_INVALID,
> +                                     sizeof(struct fw_cfg_file), &file->cfg);
> +               file->addr = 0;
> +               list_add_tail(&file->list, &fw_list);
> +       }

nits: leave one blank line before return

> +       return 0;
> +
> +err:
> +       list_for_each(entry, &fw_list) {
> +               file = list_entry(entry, struct fw_file, list);
> +               free(file);
> +       }

nits: leave one blank line before return

> +       return -ENOMEM;
> +}
> +
> +static int qemu_fwcfg_list_firmware(void)
> +{
> +       int ret;
> +       struct list_head *entry;
> +       struct fw_file *file;
> +
> +       /* make sure fw_list is loaded */
> +       ret = qemu_fwcfg_read_firmware_list();
> +       if (ret)
> +               return ret;
> +
> +       list_for_each(entry, &fw_list) {
> +               file = list_entry(entry, struct fw_file, list);
> +               printf("%-56s\n", file->cfg.name);
> +       }

nits: leave one blank line before return

>         return 0;
>  }
>
> diff --git a/arch/x86/include/asm/fw_cfg.h b/arch/x86/include/asm/fw_cfg.h
> index fb110fa..285d805 100644
> --- a/arch/x86/include/asm/fw_cfg.h
> +++ b/arch/x86/include/asm/fw_cfg.h
> @@ -12,6 +12,8 @@
>  #define FW_DMA_PORT_LOW        0x514
>  #define FW_DMA_PORT_HIGH       0x518
>
> +#include <linux/list.h>
> +
>  enum qemu_fwcfg_items {
>         FW_CFG_SIGNATURE        = 0x00,
>         FW_CFG_ID               = 0x01,
> @@ -67,6 +69,12 @@ struct fw_cfg_file {
>         char name[FW_CFG_MAX_FILE_PATH];
>  };
>
> +struct fw_file {
> +       struct fw_cfg_file cfg;
> +       unsigned long addr;
> +       struct list_head list;
> +};

Can you please put a comment block above to describe what is each member for?

> +
>  struct fw_cfg_files {

Looks that this struct fw_cfg_files is no longer used by any codes.
Please remove this.

>         __be32 count;
>         struct fw_cfg_file files[];
> --

Regards,
Bin