[PATCH 2/6] image: fit: Add some helpers for getting data
Simon Glass
sjg at chromium.org
Mon Mar 28 08:35:16 CEST 2022
Hi Sean,
On Thu, 24 Mar 2022 at 12:23, Sean Anderson <sean.anderson at seco.com> wrote:
>
> Several different firmware users have repetitive code to extract the
> firmware data from a FIT. Add some helper functions to reduce the amount
> of repetition. fit_conf_get_prop_node (eventually) calls
> fdt_check_node_offset_, so we can avoid an explicit if. In general, this
> version avoids printing on error because the callers are typically
> library functions, and because the FIT code generally has (debug)
> prints of its own. One difference in these helpers is that they use
> fit_image_get_data_and_size instead of fit_image_get_data, as the former
> handles external data correctly.
>
> Signed-off-by: Sean Anderson <sean.anderson at seco.com>
> ---
>
> arch/arm/cpu/armv8/sec_firmware.c | 39 ++---------------------------
> boot/image-fit.c | 37 +++++++++++++++++++++++++++
> cmd/fpga.c | 24 +++++-------------
> drivers/net/fsl-mc/mc.c | 30 +++-------------------
> drivers/net/pfe_eth/pfe_firmware.c | 40 +-----------------------------
> include/image.h | 4 +++
> 6 files changed, 53 insertions(+), 121 deletions(-)
It feels like this patch should be split up into generic / armv8 / fsp things.
>
> diff --git a/arch/arm/cpu/armv8/sec_firmware.c b/arch/arm/cpu/armv8/sec_firmware.c
> index 41525a10d5..879eb6ec81 100644
> --- a/arch/arm/cpu/armv8/sec_firmware.c
> +++ b/arch/arm/cpu/armv8/sec_firmware.c
> @@ -42,43 +42,8 @@ phys_addr_t sec_firmware_addr;
> static int sec_firmware_get_data(const void *sec_firmware_img,
> const void **data, size_t *size)
> {
> - int conf_node_off, fw_node_off;
> - char *desc;
> - int ret;
> -
> - conf_node_off = fit_conf_get_node(sec_firmware_img, NULL);
> - if (conf_node_off < 0) {
> - puts("SEC Firmware: no config\n");
> - return -ENOENT;
> - }
> -
> - fw_node_off = fit_conf_get_prop_node(sec_firmware_img, conf_node_off,
> - SEC_FIRMWARE_FIT_IMAGE);
> - if (fw_node_off < 0) {
> - printf("SEC Firmware: No '%s' in config\n",
> - SEC_FIRMWARE_FIT_IMAGE);
> - return -ENOLINK;
> - }
> -
> - /* Verify secure firmware image */
> - if (!(fit_image_verify(sec_firmware_img, fw_node_off))) {
> - printf("SEC Firmware: Bad firmware image (bad CRC)\n");
> - return -EINVAL;
> - }
> -
> - if (fit_image_get_data(sec_firmware_img, fw_node_off, data, size)) {
> - printf("SEC Firmware: Can't get %s subimage data/size",
> - SEC_FIRMWARE_FIT_IMAGE);
> - return -ENOENT;
> - }
> -
> - ret = fit_get_desc(sec_firmware_img, fw_node_off, &desc);
> - if (ret)
> - printf("SEC Firmware: Can't get description\n");
> - else
> - printf("%s\n", desc);
> -
> - return ret;
> + return fit_get_data_conf_prop(sec_firmware_img, SEC_FIRMWARE_FIT_IMAGE,
> + data, size);
> }
>
> /*
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> index 6610035d0a..b87378cbf6 100644
> --- a/boot/image-fit.c
> +++ b/boot/image-fit.c
> @@ -1919,6 +1919,43 @@ int fit_conf_get_prop_node(const void *fit, int noffset,
> return fit_conf_get_prop_node_index(fit, noffset, prop_name, 0);
> }
>
> +static int fit_get_data_tail(const void *fit, int noffset,
> + const void **data, size_t *size)
> +{
> + char *desc;
> +
> + if (noffset < 0)
> + return noffset;
> +
> + if (!fit_image_verify(fit, noffset))
> + return -EINVAL;
> +
> + if (fit_image_get_data_and_size(fit, noffset, data, size))
> + return -ENOENT;
> +
> + if (!fit_get_desc(fit, noffset, &desc))
> + printf("%s\n", desc);
> +
> + return 0;
> +}
> +
> +int fit_get_data_node(const void *fit, const char *image_uname,
> + const void **data, size_t *size)
> +{
> + int noffset = fit_image_get_node(fit, image_uname);
> +
> + return fit_get_data_tail(fit, noffset, data, size);
> +}
> +
> +int fit_get_data_conf_prop(const void *fit, const char *prop_name,
> + const void **data, size_t *size)
> +{
> + int noffset = fit_conf_get_node(fit, NULL);
> +
> + noffset = fit_conf_get_prop_node(fit, noffset, prop_name);
> + return fit_get_data_tail(fit, noffset, data, size);
> +}
> +
> static int fit_image_select(const void *fit, int rd_noffset, int verify)
> {
> fit_image_print(fit, rd_noffset, " ");
> diff --git a/cmd/fpga.c b/cmd/fpga.c
> index 3fdd0b35e8..1102a84d76 100644
> --- a/cmd/fpga.c
> +++ b/cmd/fpga.c
> @@ -322,7 +322,7 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc,
> case IMAGE_FORMAT_FIT:
> {
> const void *fit_hdr = (const void *)fpga_data;
> - int noffset;
> + int err;
> const void *fit_data;
>
> if (!fit_uname) {
> @@ -335,23 +335,11 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc,
> return CMD_RET_FAILURE;
> }
>
> - /* get fpga component image node offset */
> - noffset = fit_image_get_node(fit_hdr, fit_uname);
> - if (noffset < 0) {
> - printf("Can't find '%s' FIT subimage\n", fit_uname);
> - return CMD_RET_FAILURE;
> - }
> -
> - /* verify integrity */
> - if (!fit_image_verify(fit_hdr, noffset)) {
> - puts("Bad Data Hash\n");
> - return CMD_RET_FAILURE;
> - }
> -
> - /* get fpga subimage/external data address and length */
> - if (fit_image_get_data_and_size(fit_hdr, noffset,
> - &fit_data, &data_size)) {
> - puts("Fpga subimage data not found\n");
> + err = fit_get_data_node(fit_hdr, fit_uname, &fit_data,
> + &data_size);
> + if (err) {
> + printf("Could not load '%s' subimage (err %d)\n",
> + fit_uname, err);
> return CMD_RET_FAILURE;
> }
>
> diff --git a/drivers/net/fsl-mc/mc.c b/drivers/net/fsl-mc/mc.c
> index bc1c31d467..68833f9ddd 100644
> --- a/drivers/net/fsl-mc/mc.c
> +++ b/drivers/net/fsl-mc/mc.c
> @@ -137,13 +137,7 @@ int parse_mc_firmware_fit_image(u64 mc_fw_addr,
> size_t *raw_image_size)
> {
> int format;
> - void *fit_hdr;
> - int node_offset;
> - const void *data;
> - size_t size;
> - const char *uname = "firmware";
> -
> - fit_hdr = (void *)mc_fw_addr;
> + void *fit_hdr = (void *)mc_fw_addr;
>
> /* Check if Image is in FIT format */
> format = genimg_get_format(fit_hdr);
> @@ -158,26 +152,8 @@ int parse_mc_firmware_fit_image(u64 mc_fw_addr,
> return -EINVAL;
> }
>
> - node_offset = fit_image_get_node(fit_hdr, uname);
> -
> - if (node_offset < 0) {
> - printf("fsl-mc: ERR: Bad firmware image (missing subimage)\n");
> - return -ENOENT;
> - }
> -
> - /* Verify MC firmware image */
> - if (!(fit_image_verify(fit_hdr, node_offset))) {
> - printf("fsl-mc: ERR: Bad firmware image (bad CRC)\n");
> - return -EINVAL;
> - }
> -
> - /* Get address and size of raw image */
> - fit_image_get_data(fit_hdr, node_offset, &data, &size);
> -
> - *raw_image_addr = data;
> - *raw_image_size = size;
> -
> - return 0;
> + return fit_get_data_node(fit_hdr, "firmware", raw_image_addr,
> + raw_image_size);
> }
> #endif
>
> diff --git a/drivers/net/pfe_eth/pfe_firmware.c b/drivers/net/pfe_eth/pfe_firmware.c
> index 6669048181..adaa139219 100644
> --- a/drivers/net/pfe_eth/pfe_firmware.c
> +++ b/drivers/net/pfe_eth/pfe_firmware.c
> @@ -104,45 +104,7 @@ err:
> static int pfe_get_fw(const void **data,
> size_t *size, char *fw_name)
> {
> - int conf_node_off, fw_node_off;
> - char *conf_node_name = NULL;
> - char *desc;
> - int ret = 0;
> -
> - conf_node_name = PFE_FIRMWARE_FIT_CNF_NAME;
> -
> - conf_node_off = fit_conf_get_node(pfe_fit_addr, conf_node_name);
> - if (conf_node_off < 0) {
> - printf("PFE Firmware: %s: no such config\n", conf_node_name);
> - return -ENOENT;
> - }
> -
> - fw_node_off = fit_conf_get_prop_node(pfe_fit_addr, conf_node_off,
> - fw_name);
> - if (fw_node_off < 0) {
> - printf("PFE Firmware: No '%s' in config\n",
> - fw_name);
> - return -ENOLINK;
> - }
> -
> - if (!(fit_image_verify(pfe_fit_addr, fw_node_off))) {
> - printf("PFE Firmware: Bad firmware image (bad CRC)\n");
> - return -EINVAL;
> - }
> -
> - if (fit_image_get_data(pfe_fit_addr, fw_node_off, data, size)) {
> - printf("PFE Firmware: Can't get %s subimage data/size",
> - fw_name);
> - return -ENOENT;
> - }
> -
> - ret = fit_get_desc(pfe_fit_addr, fw_node_off, &desc);
> - if (ret)
> - printf("PFE Firmware: Can't get description\n");
> - else
> - printf("%s\n", desc);
> -
> - return ret;
> + return fit_get_data_conf_prop(pfe_fit_addr, fw_name, data, size);
> }
>
> /*
> diff --git a/include/image.h b/include/image.h
> index 97e5f2eb24..ae1f015896 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1010,6 +1010,10 @@ int fit_image_get_data_size_unciphered(const void *fit, int noffset,
> size_t *data_size);
> int fit_image_get_data_and_size(const void *fit, int noffset,
> const void **data, size_t *size);
> +int fit_get_data_node(const void *fit, const char *image_uname,
> + const void **data, size_t *size);
> +int fit_get_data_conf_prop(const void *fit, const char *prop_name,
> + const void **data, size_t *size);
Please comment the functions.
>
> int fit_image_hash_get_algo(const void *fit, int noffset, const char **algo);
> int fit_image_hash_get_value(const void *fit, int noffset, uint8_t **value,
> --
> 2.25.1
>
Regards,
SImon
More information about the U-Boot
mailing list