Re: [PATCH 07/12] intel-ipu3: css: firmware management

[Tomasz Figa <tfiga@chromium.org>]

Hi Yong,

Please see my comments inline.

On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi <yong.zhi@intel.com> wrote:
> Functions to load and install imgu FW blobs
>
> Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-abi.h    | 1572 ++++++++++++++++++++++++++++
>  drivers/media/pci/intel/ipu3/ipu3-css-fw.c |  272 +++++
>  drivers/media/pci/intel/ipu3/ipu3-css-fw.h |  215 ++++
>  drivers/media/pci/intel/ipu3/ipu3-css.h    |   54 +
>  4 files changed, 2113 insertions(+)
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-abi.h
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.c
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.h
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css.h
[snip]
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-fw.c b/drivers/media/pci/intel/ipu3/ipu3-css-fw.c
> new file mode 100644
> index 0000000..55edbb8
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-css-fw.c
> @@ -0,0 +1,272 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <asm/cacheflush.h>

Shouldn't need this.

> +#include <linux/device.h>
> +#include <linux/firmware.h>
> +#include <linux/slab.h>
> +
> +#include "ipu3-css.h"
> +#include "ipu3-css-fw.h"
> +
> +static void ipu3_css_fw_show_binary(struct device *dev,
> +                       struct imgu_fw_info *bi, const char *name)
> +{
> +       int i;
> +
> +       dev_dbg(dev, "found firmware binary type %i size %i name %s\n",
> +               bi->type, bi->blob.size, name);
> +       if (bi->type != IMGU_FW_ISP_FIRMWARE)
> +               return;
> +
> +       dev_dbg(dev, "    id %i mode %i bds 0x%x veceven %i/%i out_pins %i\n",
> +               bi->info.isp.sp.id, bi->info.isp.sp.pipeline.mode,
> +               bi->info.isp.sp.bds.supported_bds_factors,
> +               bi->info.isp.sp.enable.vf_veceven,
> +               bi->info.isp.sp.vf_dec.is_variable,
> +               bi->info.isp.num_output_pins);
> +
> +       dev_dbg(dev, "    input (%i,%i)-(%i,%i) formats %s%s%s\n",
> +               bi->info.isp.sp.input.min_width,
> +               bi->info.isp.sp.input.min_height,
> +               bi->info.isp.sp.input.max_width,
> +               bi->info.isp.sp.input.max_height,
> +               bi->info.isp.sp.enable.input_yuv ? "yuv420 " : "",
> +               bi->info.isp.sp.enable.input_feeder ||
> +               bi->info.isp.sp.enable.input_raw ? "raw8 raw10 " : "",
> +               bi->info.isp.sp.enable.input_raw ? "raw12" : "");
> +
> +       dev_dbg(dev, "    internal (%i,%i)\n",
> +               bi->info.isp.sp.internal.max_width,
> +               bi->info.isp.sp.internal.max_height);
> +
> +       dev_dbg(dev, "    output (%i,%i)-(%i,%i) formats",
> +               bi->info.isp.sp.output.min_width,
> +               bi->info.isp.sp.output.min_height,
> +               bi->info.isp.sp.output.max_width,
> +               bi->info.isp.sp.output.max_height);
> +       for (i = 0; i < bi->info.isp.num_output_formats; i++)
> +               dev_dbg(dev, " %i", bi->info.isp.output_formats[i]);
> +       dev_dbg(dev, " vf");
> +       for (i = 0; i < bi->info.isp.num_vf_formats; i++)
> +               dev_dbg(dev, " %i", bi->info.isp.vf_formats[i]);

When the function is called, neither num_output_formats nor
num_vf_formats is validated. It will cause an out of bound read here
if it isn't correct.

> +       dev_dbg(dev, "\n");
> +}
> +
> +const int ipu3_css_fw_obgrid_size(const struct imgu_fw_info *bi)
> +{
> +       unsigned int stripes = bi->info.isp.sp.iterator.num_stripes;
> +       unsigned int width, height, obgrid_size;
> +
> +       width = ALIGN(DIV_ROUND_UP(bi->info.isp.sp.internal.max_width,
> +               IMGU_OBGRID_TILE_SIZE * 2) + 1, IPU3_UAPI_ISP_VEC_ELEMS / 4);
> +       height = DIV_ROUND_UP(bi->info.isp.sp.internal.max_height,
> +               IMGU_OBGRID_TILE_SIZE * 2) + 1;
> +       obgrid_size = PAGE_ALIGN(width * height *
> +               sizeof(struct ipu3_uapi_obgrid_param)) * stripes;
> +
> +       return obgrid_size;
> +}
> +
> +void *ipu3_css_fw_pipeline_params(struct ipu3_css *css,
> +               enum imgu_abi_param_class c, enum imgu_abi_memories m,
> +               struct imgu_fw_isp_parameter *par, size_t par_size,
> +               void *binary_params)
> +{
> +       struct imgu_fw_info *bi = &css->fwp->binary_header[css->current_binary];
> +
> +       if (par->offset + par->size >
> +           bi->info.isp.sp.mem_initializers.params[c][m].size)
> +               return NULL;
> +
> +       if (par->size != par_size)
> +               pr_warn("parameter size doesn't match defined size\n");
> +
> +       if (par->size < par_size)
> +               return NULL;
> +
> +       return binary_params + par->offset;
> +}
> +
> +void ipu3_css_fw_cleanup(struct ipu3_css *css)
> +{
> +       if (css->binary) {
> +               int i;
> +
> +               for (i = 0; i < css->fwp->file_header.binary_nr; i++)
> +                       ipu3_css_dma_free(css->dev, &css->binary[i]);
> +               kfree(css->binary);
> +       }
> +       if (css->fw)
> +               release_firmware(css->fw);
> +
> +       css->binary = NULL;
> +       css->fw = NULL;
> +}
> +
> +int ipu3_css_fw_init(struct ipu3_css *css)
> +{
> +       static const u32 BLOCK_MAX = 65536;
> +       struct device *dev = css->dev;
> +       int binary_nr = 0;
> +       int i, j, r;
> +
> +       r = request_firmware(&css->fw, IMGU_FW_NAME, css->dev);
> +       if (r)
> +               return r;
> +
> +       /* Check and display fw header info */
> +
> +       css->fwp = (struct imgu_fw_header *)css->fw->data;
> +       if (css->fw->size < sizeof(struct imgu_fw_header *) ||
> +           css->fwp->file_header.h_size != sizeof(struct imgu_fw_bi_file_h))
> +               goto bad_fw;
> +       if (sizeof(struct imgu_fw_bi_file_h) +
> +           css->fwp->file_header.binary_nr * sizeof(struct imgu_fw_info) >
> +           css->fw->size)
> +               goto bad_fw;
> +
> +       dev_info(dev, "loaded firmware version %.64s, %u binaries, %u bytes\n",
> +                css->fwp->file_header.version, css->fwp->file_header.binary_nr,
> +                (int)css->fw->size);

nit: This cast is not needed. All you need is correct print format,
which is "%zu" in this case.

> +
> +       /* Validate and display info on fw binaries */
> +
> +       binary_nr = css->fwp->file_header.binary_nr;
> +
> +       css->fw_bl = css->fw_sp[0] = css->fw_sp[1] = -1;
> +       for (i = 0; i < binary_nr; i++) {
> +               struct imgu_fw_info *bi = &css->fwp->binary_header[i];
> +               const char *name = (void *)css->fwp + bi->blob.prog_name_offset;
> +               size_t len;
> +
> +               if (bi->blob.prog_name_offset >= css->fw->size)
> +                       goto bad_fw;
> +               len = strnlen(name, css->fw->size - bi->blob.prog_name_offset);
> +               if (len + 1 >= css->fw->size - bi->blob.prog_name_offset ||

Isn't this an off by one error? (css->fw->size -
bi->blob.prog_name_offset) would be size of the space from start of
the string to the end of the firmware. So I think we only need to
ensure that the string including zero (len + 1 bytes) fits there,
which would be also true when (len + 1 == css->fw->size -
bi->blob.prog_name_offset). Unless I'm missing something, the test
should be >, not >=.

> +                   len + 1 >= IMGU_ABI_MAX_BINARY_NAME)

I guess this one depends on what is the meaning of
IMGU_ABI_MAX_BINARY_NAME. If it doesn't include the terminating zero,
then this test is okay.

> +                       goto bad_fw;
> +
> +               ipu3_css_fw_show_binary(dev, bi, name);
> +
> +               if (bi->blob.size != bi->blob.text_size + bi->blob.icache_size
> +                   + bi->blob.data_size + bi->blob.padding_size)
> +                       goto bad_fw;
> +               if (bi->blob.offset + bi->blob.size > css->fw->size)
> +                       goto bad_fw;
> +
> +               if (bi->type == IMGU_FW_BOOTLOADER_FIRMWARE) {
> +                       css->fw_bl = i;
> +                       if (bi->info.bl.sw_state >= css->iomem_length ||
> +                           bi->info.bl.num_dma_cmds >= css->iomem_length
> +                           || bi->info.bl.dma_cmd_list >=
> +                           css->iomem_length)
> +                               goto bad_fw;
> +               }
> +               if (bi->type == IMGU_FW_SP_FIRMWARE ||
> +                   bi->type == IMGU_FW_SP1_FIRMWARE) {
> +                       css->fw_sp[bi->type == IMGU_FW_SP_FIRMWARE ? 0 : 1] = i;
> +                       if (bi->info.sp.per_frame_data >= css->iomem_length
> +                           || bi->info.sp.init_dmem_data >=
> +                           css->iomem_length
> +                           || bi->info.sp.host_sp_queue >=
> +                           css->iomem_length
> +                           || bi->info.sp.isp_started >= css->iomem_length
> +                           || bi->info.sp.sw_state >= css->iomem_length
> +                           || bi->info.sp.host_sp_queues_initialized >=
> +                           css->iomem_length
> +                           || bi->info.sp.sleep_mode >= css->iomem_length
> +                           || bi->info.sp.invalidate_tlb >=
> +                           css->iomem_length
> +                           || bi->info.sp.host_sp_com >= css->iomem_length
> +                           || bi->info.sp.output + 12 >=
> +                           css->iomem_length)
> +                               goto bad_fw;
> +               }
> +               if (bi->type != IMGU_FW_ISP_FIRMWARE)
> +                       continue;
> +
> +               if (bi->info.isp.sp.pipeline.mode >= IPU3_CSS_PIPE_ID_NUM)
> +                       goto bad_fw;
> +
> +               if (bi->info.isp.sp.iterator.num_stripes >
> +                   IPU3_UAPI_MAX_STRIPES)
> +                       goto bad_fw;
> +
> +               if (bi->info.isp.num_output_formats > IMGU_ABI_FRAME_FORMAT_NUM
> +                   || bi->info.isp.num_vf_formats > IMGU_ABI_FRAME_FORMAT_NUM)
> +                       goto bad_fw;
> +
> +               for (j = 0; j < bi->info.isp.num_output_formats; j++)
> +                       if (bi->info.isp.output_formats[j] < 0 ||
> +                           bi->info.isp.output_formats[j] >=
> +                           IMGU_ABI_FRAME_FORMAT_NUM)
> +                               goto bad_fw;
> +               for (j = 0; j < bi->info.isp.num_vf_formats; j++)
> +                       if (bi->info.isp.vf_formats[j] < 0 ||
> +                           bi->info.isp.vf_formats[j] >=
> +                           IMGU_ABI_FRAME_FORMAT_NUM)
> +                               goto bad_fw;

I think this is the first safe place where we can actually call
ipu3_css_fw_show_binary(). But for simplicity, I would just call it at
the end of the loop iteration after all the validations are done.

> +
> +               if (bi->info.isp.sp.block.block_width <= 0 ||
> +                   bi->info.isp.sp.block.block_width > BLOCK_MAX ||
> +                   bi->info.isp.sp.block.output_block_height <= 0 ||
> +                   bi->info.isp.sp.block.output_block_height > BLOCK_MAX)
> +                       goto bad_fw;
> +
> +               if (bi->blob.memory_offsets.offsets[IMGU_ABI_PARAM_CLASS_PARAM]
> +                   + sizeof(struct imgu_fw_param_memory_offsets)
> +                   > css->fw->size ||
> +                   bi->blob.memory_offsets.offsets[IMGU_ABI_PARAM_CLASS_CONFIG]
> +                   + sizeof(struct imgu_fw_config_memory_offsets)
> +                   > css->fw->size ||
> +                   bi->blob.memory_offsets.offsets[IMGU_ABI_PARAM_CLASS_STATE]
> +                   + sizeof(struct imgu_fw_state_memory_offsets)
> +                   > css->fw->size)
> +                       goto bad_fw;
> +       }
> +
> +       if (css->fw_bl == -1 || css->fw_sp[0] == -1 || css->fw_sp[1] == -1)
> +               goto bad_fw;
> +
> +       /* Allocate and map fw binaries into IMGU */
> +
> +       css->binary = kcalloc(binary_nr, sizeof(*css->binary), GFP_KERNEL);
> +       if (!css->binary) {
> +               r = -ENOMEM;
> +               goto error_out;
> +       }
> +
> +       for (i = 0; i < css->fwp->file_header.binary_nr; i++) {
> +               struct imgu_fw_info *bi = &css->fwp->binary_header[i];
> +               void *blob = (void *)css->fwp + bi->blob.offset;
> +               size_t size = bi->blob.size;
> +
> +               if (ipu3_css_dma_alloc(dev, &css->binary[i], size)) {
> +                       r = -ENOMEM;
> +                       goto error_out;
> +               }
> +               memcpy(css->binary[i].vaddr, blob, size);
> +               clflush_cache_range(css->binary[i].vaddr, size);

This cache flush should not be necessary, given that the memory
allocated by ipu3_css_dma_alloc() is coherent.

> +       }
> +
> +       return 0;
> +
> +bad_fw:
> +       dev_err(dev, "invalid firmware binary, size %u\n", (int)css->fw->size);
> +       r = -ENODEV;
> +
> +error_out:
> +       ipu3_css_fw_cleanup(css);
> +       return r;
> +}
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-fw.h b/drivers/media/pci/intel/ipu3/ipu3-css-fw.h
> new file mode 100644
> index 0000000..5a247e3
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-css-fw.h
> @@ -0,0 +1,215 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __IPU3_CSS_FW_H
> +#define __IPU3_CSS_FW_H
> +
> +/******************* Firmware file definitions *******************/
> +
> +#define IMGU_FW_NAME                   "ipu3-fw.bin"
> +
> +typedef u32 imgu_fw_ptr;
> +
> +enum imgu_fw_type {
> +       IMGU_FW_SP_FIRMWARE,    /* Firmware for the SP */
> +       IMGU_FW_SP1_FIRMWARE,   /* Firmware for the SP1 */
> +       IMGU_FW_ISP_FIRMWARE,   /* Firmware for the ISP */
> +       IMGU_FW_BOOTLOADER_FIRMWARE,    /* Firmware for the BootLoader */
> +       IMGU_FW_ACC_FIRMWARE    /* Firmware for accelerations */
> +};
> +
> +enum imgu_fw_acc_type {
> +       IMGU_FW_ACC_NONE,       /* Normal binary */
> +       IMGU_FW_ACC_OUTPUT,     /* Accelerator stage on output frame */
> +       IMGU_FW_ACC_VIEWFINDER, /* Accelerator stage on viewfinder frame */
> +       IMGU_FW_ACC_STANDALONE, /* Stand-alone acceleration */
> +};
> +
> +struct imgu_fw_isp_parameter {
> +       u32 offset;             /* Offset in isp_<mem>)parameters, etc. */

I suspect a typo in the comment above: "isp_<mem>)".

> +       u32 size;               /* Disabled if 0 */
> +};
> +
> +struct imgu_fw_param_memory_offsets {
> +       struct {
> +               struct imgu_fw_isp_parameter lin;       /* lin_vmem_params */
> +               struct imgu_fw_isp_parameter tnr3;      /* tnr3_vmem_params */
> +               struct imgu_fw_isp_parameter xnr3;      /* xnr3_vmem_params */
> +       } vmem;
> +       struct {
> +               struct imgu_fw_isp_parameter tnr;
> +               struct imgu_fw_isp_parameter tnr3;      /* tnr3_params */
> +               struct imgu_fw_isp_parameter xnr3;      /* xnr3_params */
> +               struct imgu_fw_isp_parameter plane_io_config;   /* 192 bytes */
> +               struct imgu_fw_isp_parameter rgbir;     /* rgbir_params */
> +       } dmem;
> +};
> +
> +struct imgu_fw_config_memory_offsets {
> +       struct {
> +               struct imgu_fw_isp_parameter iterator;
> +               struct imgu_fw_isp_parameter dvs;
> +               struct imgu_fw_isp_parameter output;
> +               struct imgu_fw_isp_parameter raw;
> +               struct imgu_fw_isp_parameter input_yuv;
> +               struct imgu_fw_isp_parameter tnr;
> +               struct imgu_fw_isp_parameter tnr3;
> +               struct imgu_fw_isp_parameter ref;
> +       } dmem;
> +};
> +
> +struct imgu_fw_state_memory_offsets {
> +       struct {
> +               struct imgu_fw_isp_parameter tnr;
> +               struct imgu_fw_isp_parameter tnr3;
> +               struct imgu_fw_isp_parameter ref;
> +       } dmem;
> +};
> +
> +union imgu_fw_all_memory_offsets {
> +       struct {
> +               struct imgu_fw_param_memory_offsets *param __aligned(8);
> +               struct imgu_fw_config_memory_offsets *config __aligned(8);
> +               struct imgu_fw_state_memory_offsets *state __aligned(8);

Pointers in a firmware blob? First of all, not only pointer is of
variable size, depending on whether the code is compiled in 32-bit or
64-bit mode, but also why is it even possible to have kernel pointers
as a part of a firmware blob coming from userspace?

> +       } offsets;
> +       struct {
> +               void *ptr __aligned(8);

Pointer in a firmware blob?

> +       } array[IMGU_ABI_PARAM_CLASS_NUM];
> +};
> +
> +struct imgu_fw_binary_xinfo {
> +       /* Part that is of interest to the SP. */
> +       struct imgu_abi_binary_info sp;
> +
> +       /* Rest of the binary info, only interesting to the host. */
> +       enum imgu_fw_acc_type type;
> +
> +       u32 num_output_formats __aligned(8);
> +       enum imgu_abi_frame_format output_formats[IMGU_ABI_FRAME_FORMAT_NUM];
> +
> +       /* number of supported vf formats */
> +       u32 num_vf_formats __aligned(8);
> +       /* types of supported vf formats */
> +       enum imgu_abi_frame_format vf_formats[IMGU_ABI_FRAME_FORMAT_NUM];
> +       u8 num_output_pins;
> +       imgu_fw_ptr xmem_addr;
> +
> +       const struct imgu_fw_blob_descr *blob __aligned(8);

Pointer in a firmware blob?

> +       u32 blob_index __aligned(8);
> +       union imgu_fw_all_memory_offsets mem_offsets __aligned(8);
> +       struct imgu_fw_binary_xinfo *next __aligned(8);

Pointer in a firmware blob?

> +};
> +
> +struct imgu_fw_sp_info {
> +       u32 init_dmem_data;     /* data sect config, stored to dmem */
> +       u32 per_frame_data;     /* Per frame data, stored to dmem */
> +       u32 group;              /* Per pipeline data, loaded by dma */
> +       u32 output;             /* SP output data, loaded by dmem */
> +       u32 host_sp_queue;      /* Host <-> SP queues */
> +       u32 host_sp_com;        /* Host <-> SP commands */
> +       u32 isp_started;        /* P'ed from sensor thread, csim only */
> +       u32 sw_state;           /* Polled from css */
> +#define IMGU_ABI_SP_SWSTATE_TERMINATED 0
> +#define IMGU_ABI_SP_SWSTATE_INITIALIZED        1
> +#define IMGU_ABI_SP_SWSTATE_CONNECTED  2
> +#define IMGU_ABI_SP_SWSTATE_RUNNING    3
> +       u32 host_sp_queues_initialized; /* Polled from the SP */
> +       u32 sleep_mode;         /* different mode to halt SP */
> +       u32 invalidate_tlb;     /* inform SP to invalidate mmu TLB */
> +       u32 debug_buffer_ddr_address;   /* inform SP the addr of DDR debug
> +                                        * queue
> +                                        */
> +       /* input system perf count array */
> +       u32 perf_counter_input_system_error;
> +       u32 threads_stack;      /* sp thread's stack pointers */
> +       u32 threads_stack_size; /* sp thread's stack sizes */
> +       u32 curr_binary_id;     /* current binary id */
> +       u32 raw_copy_line_count;        /* raw copy line counter */
> +       u32 ddr_parameter_address;      /* acc param ddrptr, sp dmem */
> +       u32 ddr_parameter_size; /* acc param size, sp dmem */
> +       /* Entry functions */
> +       u32 sp_entry;           /* The SP entry function */
> +       u32 tagger_frames_addr; /* Base address of tagger state */
> +};
> +
> +struct imgu_fw_bl_info {
> +       u32 num_dma_cmds;       /* Number of cmds sent by CSS */
> +       u32 dma_cmd_list;       /* Dma command list sent by CSS */
> +       u32 sw_state;           /* Polled from css */
> +#define IMGU_ABI_BL_SWSTATE_OK         0x100
> +#define IMGU_ABI_BL_SWSTATE_BUSY       (IMGU_ABI_BL_SWSTATE_OK + 1)
> +#define IMGU_ABI_BL_SWSTATE_ERR                (IMGU_ABI_BL_SWSTATE_OK + 2)
> +       /* Entry functions */
> +       u32 bl_entry;           /* The SP entry function */
> +};
> +
> +struct imgu_fw_acc_info {
> +       u32 per_frame_data;     /* Dummy for now */
> +};
> +
> +union imgu_fw_union {
> +       struct imgu_fw_binary_xinfo isp;        /* ISP info */
> +       struct imgu_fw_sp_info sp;      /* SP info */
> +       struct imgu_fw_sp_info sp1;     /* SP1 info */
> +       struct imgu_fw_bl_info bl;      /* Bootloader info */
> +       struct imgu_fw_acc_info acc;    /* Accelerator info */
> +};
> +
> +struct imgu_fw_info {
> +       size_t header_size;     /* size of fw header */
> +       enum imgu_fw_type type __aligned(8);
> +       union imgu_fw_union info;       /* Binary info */
> +       struct imgu_abi_blob_info blob; /* Blob info */
> +       /* Dynamic part */
> +       struct imgu_fw_info *next;

Pointer in a firmware blob?

> +
> +       u32 loaded __aligned(8);        /* Firmware has been loaded */
> +       const u8 *isp_code __aligned(8);        /* ISP pointer to code */

Pointer in a firmware blob?

> +       /* Firmware handle between user space and kernel */
> +       u32 handle __aligned(8);
> +       /* Sections to copy from/to ISP */
> +       struct imgu_abi_isp_param_segments mem_initializers;
> +       /* Initializer for local ISP memories */
> +};
> +
> +struct imgu_fw_blob_descr {
> +       const unsigned char *blob;
> +       struct imgu_fw_info header;
> +       const char *name;
> +       union imgu_fw_all_memory_offsets mem_offsets;
> +};
> +
> +struct imgu_fw_bi_file_h {
> +       char version[64];       /* branch tag + week day + time */
> +       int binary_nr;          /* Number of binaries */
> +       unsigned int h_size;    /* sizeof(struct imgu_fw_bi_file_h) */
> +};
> +
> +struct imgu_fw_header {
> +       struct imgu_fw_bi_file_h file_header;
> +       struct imgu_fw_info binary_header[1];   /* binary_nr items */

Since we don't know the number of items, shouldn't it be binary_header[0]?

Best regards,
Tomasz