Re: [PATCH 01/15] vbe: Split out some VBE code into a common file

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3210 bytes --]

On Tue, Jan 14, 2025 at 05:58:04AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 13 Jan 2025 at 18:22, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Jan 09, 2025 at 05:29:56AM -0700, Simon Glass wrote:
> >
> > > Loading a FIT is useful for other VBE methods, such as ABrec, so start
> > > a new common file. Add functions for reading the version and nvdata.
> > > Also add a function to get the block device.
> >
> > This is not a good commit message when you're also introducing
> > functional changes (blk_dread -> blk_read).
> 
> Yes, I did not actually notice this size change at all, as mentioned,
> as I only built for a few boards (firefly rk3399/rk3288 and a few
> sandbox ones). The blk_dread() function is the old API for reading

You have much faster build machines available than I do, it would be
good to run a world build before/after (it might take an hour on
alexandra) before posting these big series.

> from a block. It didn't occur to me that blk_dread() would bypass the
> cache.

Yes, how is that happening? That's what doesn't make sense.

> 
> > Further, this functional
> > change seems to introduce some other code being pulled in very
> > unexpectedly, and that needs to be explained. For example,
> > phycore_am62x_r5_usbdfu is another case and that has
> > CONFIG_BLOCK_CACHE=y but CONFIG_SPL_BLOCK_CACHE=n, and yet the size
> > growth is in full U-Boot.
> 
> This board currently uses blk_dread(), but not blk_read(), so the
> addition of a blk_read() call in non-SPL causes the BLK cache to be
> pulled in.

But, how? I mean, this sounds like some underlying bug that needs to be
fixed. The platform sets CONFIG_BLK=y and CONFIG_BLOCK_CACHE=y and
drivers/block/blk-uclass.c has:
ulong blk_dread(struct blk_desc *desc, lbaint_t start, lbaint_t blkcnt,
                void *buffer)
{
        return blk_read(desc->bdev, start, blkcnt, buffer);
}

Or perhaps the answer / problem here is that I need to track down what's
going wrong here instead of asking you to track this down.

> > Just moving code should not result in
> > unexpected size change.
> 
> That's correct, so long as 'expected' size changes includes the normal
> function-call overhead and loss of intra-module optimisation.

Yes, but that's not what this series shows. There's (a) the above bug
and (b) you might be allocating buffers twice now instead of once?

> So, what to do here? I certainly don't want to use the old blk API.

Yes. I'm not 100% sure, especially after
https://patchwork.ozlabs.org/project/uboot/list/?series=437816&state=*
which I need to v2 what the non-SPL case is. And for new features, sure,
saying SPL_BLK is required too is fine. Heck, it *may* even be the case
that only TPL_BLK=n is required to be valid, all of that would require
other investigation and removals.

> But I can perhaps do a patch which changes VBE to use the new one,
> first, so the size growth is within that commit. I looked up
> BLOCK_CACHE and it is used by 90% of boards.

Yes, BLOCK_CACHE should be used by everyone at this point, it was one of
those cases of "this grows the code but it's a universal speedup".

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]