Re: [PATCH v8 5/7] misc: fw_loader: introduce FIP loader driver

[Christian Marangi <ansuelsmth@gmail.com>]

On Fri, Oct 23, 2026 at 09:12:32AM -0400, Simon Glass wrote:
> Hi Christian,
> 
> On 2026-10-22T19:40:35, Christian Marangi (Ansuel) <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:
> >
> > [...]
> >
> >  drivers/misc/Kconfig                |  23 ++
> >  drivers/misc/fw_loader/Makefile     |   1 +
> >  drivers/misc/fw_loader/fip_loader.c | 556 ++++++++++++++++++++++++++++++++++++
> >  drivers/misc/fw_loader/internal.h   |   2 +
> >  4 files changed, 582 insertions(+)
> 
> > diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> > @@ -0,0 +1,556 @@
> > +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)
> > +{
> > +	unsigned int offset = part_info->start + part_offset;
> 
> The parse path now uses lbaint_t, but this read path still truncates
> part_info->start to unsigned int, so on a large device the header is
> found but the data is read from the wrong block. Please can you use
> lbaint_t here too (and for blkcnt/blkstart, to match
> blk_parse_fip_firmware())?
> 
> > diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> > @@ -0,0 +1,556 @@
> > +	size = min(firmwarep->size, (size_t)ent->size - offset);
> > +	ret = ubi_volume_read(ubi_vol,
> > +			      (u8 *)firmwarep->data,
> > +			      ent->offset_address + offset,
> > +			      size);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return size - firmwarep->offset;
> 
> 'size' already has the offset taken off, so this subtracts it twice.
> With a non-zero offset the UBI path returns less than it actually
> read, whereas the block path returns the full 'read' count. I think
> this should just be 'return size;'
> 
> Also, ubi_volume_read() treats size == 0 as 'read the whole volume',
> which overruns the buffer. That happens if offset == ent->size (which
> the check in fw_get_fip_firmware() allows) or if the caller passes a
> zero-sized buffer. Please can you return early when size is 0?
> 

!!! No idea ubi_volume_read() 0 is read all. Thanks a lot for noticing
this.

-- 
	Ansuel