[PATCH v5 5/6] misc: fw_loader: introduce FIP loader driver

[Simon Glass]

Hi Christian,

On 2026-04-03T13:51:57, Christian Marangi <ansuelsmth at gmail.com> wrote:
> misc: fw_loader: introduce FIP loader driver
>
> Introduce a variant of the FS loader driver to extract images from FIP
> image. These image can contain additional binary used to init Network
> accelerator or PHY firmware blob.
>
> The way FIP handle image type is with the usage of UUID.
>
> This FIP loader driver implement a simple FIP image parser that check
> every entry for a matching UUID.
>
> Similar to FS loader, this driver also support both UBI and Block
> devices.
>
> Also an additional property is added to handle special case with eMMC
> that doesn't have a GPT partition and require a global offset to
> reference the FIP partition.
>
> An example usage of this driver is the following:
>
> [...]

> diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> @@ -0,0 +1,578 @@
> +static int blk_read_fip_firmware(struct firmware *firmwarep,
> +                              struct blk_desc *desc,
> +                              struct disk_partition *part_info,
> +                              unsigned int part_offset,
> +                              const struct fip_toc_entry *ent)
> +{
> +     ...
> +     size_t size = ent->size;
> +     ...
> +     blkcnt = BLOCK_CNT(size + firmwarep->offset, desc);
> +     blkstart = ent->offset_address + firmwarep->offset;
> +     ...
> +     if (pos) {
> +             to_read = min(desc->blksz - pos, (unsigned long)size);
> +             ...
> +     }
> +
> +     /* Consume all the remaining block */
> +     for (i = 0; i < blkcnt && read < size; i++) {
> +             to_read = min(desc->blksz, (unsigned long)(size - read));

This reads the full ent->size bytes but starts at ent->offset_address
+ firmwarep->offset. Compare with ubi_read_fip_firmware() which
correctly reads (size - offset) bytes. The blk variant should also
adjust size by firmwarep->offset so the behaviour is consistent.

> diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> @@ -0,0 +1,578 @@
> +static int fw_get_fip_firmware_size(struct udevice *dev)
> +{
> +     ...
> +     return ent.size;
> +}

ent.size is u64 but the function returns int. This could truncate
large sizes. The same issue exists in fw_get_fip_firmware(). How about
using ulong which is the common type in U-Boot?

> diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> @@ -0,0 +1,578 @@
> +static int fw_get_fip_firmware(struct udevice *dev)
> +{
> +     ...
> +     ret = fw_parse_storage_info(dev, &info);
> +     if (ret)
> +             return ret;
> +
> +     struct firmware *firmwarep = dev_get_priv(dev);

Please can you move the firmwarep declaration to the top of the block.
The same pattern appears in fw_get_fip_firmware_size().

> diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> @@ -0,0 +1,578 @@
> +     if (ent.size + firmwarep->offset > firmwarep->size) {
> +             log_err("Not enough space to read firmware\n");
> +             return -ENOMEM;
> +     }

I suspect this check is wrong. If firmwarep->offset is an offset into
the FIP entry, you likely want to verify that (ent.size -
firmwarep->offset) fits in the buffer, not (ent.size +
firmwarep->offset).

Also -NOSPC might be better, since we are not actually out of memory.
When people see -ENOMEM they tend to want to increase the malloc size.

> diff --git a/drivers/misc/fw_loader/internal.h b/drivers/misc/fw_loader/internal.h
> @@ -25,11 +25,13 @@ struct phandle_part {
> +     int partoffset;

partoffset is declared as int but used as unsigned int in
fip_storage_info and read from DT with ofnode_read_u32(). Please can
you change this to u32 for consistency.

Regards,
Simon