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: 3073 bytes --]

On Tue, Jan 14, 2025 at 10:58:01AM -0600, Tom Rini wrote:
> 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.

So, digging in to this, what's going on is that the platforms I've noted
here (and a few others) don't actually enable any block devices. That's
why, today, with VBE on still, they discard all of the code still. Your
rework means that while there's still no block devices, we're now
including the code. What to do? Both of:
- These platforms *should* disable the assorted block device related
  library options they have enabled if / when possible (it might not be
  a neat and easy un-tangle).
- VBE needs to handle the case where there's not a block device enabled
  and not fail the build. Part of this requires the Kconfig rework I
  linked to earlier (so that we can have BLK off) to be complete, but
  having the VBE code check for BLK being enabled either in Makefile or
  C code should be the same regardless.

-- 
Tom

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