Re: [PATCH v9 7/8] misc: fw_loader: introduce FIP loader driver

[Christian Marangi <ansuelsmth@gmail.com>]

On Mon, Oct 26, 2026 at 05:37:29AM -0700, Simon Glass wrote:
> Hi Christian,
> 
> On 2026-10-23T19:00:21, 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:
> >
> > [...]
> >
> >  drivers/misc/Kconfig                |  23 ++
> >  drivers/misc/fw_loader/Makefile     |   1 +
> >  drivers/misc/fw_loader/fip_loader.c | 558 ++++++++++++++++++++++++++++++++++++
> >  drivers/misc/fw_loader/internal.h   |   2 +
> >  4 files changed, 584 insertions(+)
> 
> > diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> > @@ -0,0 +1,558 @@
> > +	lbaint_t offset = part_info->start + part_offset;
> > +	lbaint_t pos, to_read, read = 0;
> > +	lbaint_t blkstart, blkcnt;
> > +	char *read_buf;
> > +	size_t size;
> > +	int i, ret;
> > +
> > +	read_buf = malloc(desc->blksz);
> > +	if (!read_buf)
> > +		return -ENOMEM;
> > +
> > +	size = min(firmwarep->size, (size_t)ent->size - firmwarep->offset);
> > +	blkcnt = BLOCK_CNT(size, desc);
> > +	blkstart = ent->offset_address + firmwarep->offset;
> > +	pos = do_div(blkstart, desc->blksz);
> 
> One thing I'd like to check: the move to lbaint_t in v9 breaks 32-bit
> builds. Without CONFIG_SYS_64BIT_LBA, lbaint_t is a ulong, so on
> 32-bit ARM blkstart is only 32 bits wide. do_div() needs a u64, so
> __div64_32() then writes 64 bits through a pointer to a 32-bit
> variable. Building Cubietruck_defconfig with CONFIG_FIP_LOADER=y
> gives:
> 
>     include/div64.h:220:32: warning: right shift count >= width of type
>     include/div64.h:224:36: warning: passing argument 1 of
> '__div64_32' from incompatible pointer type
> 
> blkstart is a byte offset here, not an LBA, so please can you make it a u64?
> 
> > diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> > @@ -0,0 +1,558 @@
> > +		to_read = min(desc->blksz,
> > +			      (lbaint_t)(sizeof(*hdr) - read));
> 
> The same build also gives 'comparison of distinct pointer types'
> warnings from min() here and in blk_read_fip_toc_entry(). That build
> has no CONFIG_SYS_64BIT_LBA, so lbaint_t matches blksz, but size_t is
> unsigned int on 32-bit ARM, so the two min() calls in
> blk_read_fip_firmware() that cast to size_t still warn. With
> CONFIG_SYS_64BIT_LBA=y, lbaint_t becomes uint64_t, so these two calls
> warn as well. Using min_t() for all four would avoid this. No in-tree
> board enables FIP_LOADER yet, so CI won't catch it. It would be worth
> building a 32-bit board with it enabled before v10.
> 
> With those two nits fixed:
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 

I compiled for qemu_arm_defconfig and used min_t(lbaint_t...

I think should be enough. (I also verified 64bit target...)

-- 
	Ansuel