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

[Christian Marangi <ansuelsmth@gmail.com>]

On Mon, Apr 06, 2026 at 11:14:43AM -0600, Simon Glass wrote:
> Hi Christian,
> 
> On 2026-04-03T13:51:57, Christian Marangi <ansuelsmth@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?
>

Can you better explain this? We need to return int to handle the negative
error.
 
> > 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.
> 

This might be confusing but I feel -ENOMEM is correct here. In
firmwarep->size we set the maximum size where we can store the firmware
data so the random guy might be correct to increase malloc size.

I posted v6 with all the other comments addressed, these 2 are the only one
left.

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

-- 
	Ansuel