[RFC PATCH 04/31] lmb: remove local instances of the lmb structure variable

[Ilias Apalodimas]

On Wed, 12 Jun 2024 at 05:41, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Tom,
>
> On Tue, 11 Jun 2024 at 16:55, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Tue, Jun 11, 2024 at 04:08:56PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 11 Jun 2024 at 15:01, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Tue, Jun 11, 2024 at 12:52:22PM -0600, Simon Glass wrote:
> > > > > Hi Sughosh,
> > > > >
> > > > > On Fri, 7 Jun 2024 at 12:53, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > >
> > > > > > With the move of the LMB structure to a persistent state, there is no
> > > > > > need to declare the variable locally, and pass it as part of the LMB
> > > > > > API's. Remove all local variable instances and change the API's
> > > > > > correspondingly.
> > > > > >
> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > > ---
> > > > > >  arch/arc/lib/cache.c                 |   4 +-
> > > > > >  arch/arm/lib/stack.c                 |   4 +-
> > > > > >  arch/arm/mach-apple/board.c          |  17 ++-
> > > > > >  arch/arm/mach-snapdragon/board.c     |  17 ++-
> > > > > >  arch/arm/mach-stm32mp/dram_init.c    |   7 +-
> > > > > >  arch/arm/mach-stm32mp/stm32mp1/cpu.c |   6 +-
> > > > > >  arch/m68k/lib/bootm.c                |   7 +-
> > > > > >  arch/microblaze/lib/bootm.c          |   4 +-
> > > > > >  arch/mips/lib/bootm.c                |   9 +-
> > > > > >  arch/nios2/lib/bootm.c               |   4 +-
> > > > > >  arch/powerpc/cpu/mpc85xx/mp.c        |   4 +-
> > > > > >  arch/powerpc/include/asm/mp.h        |   4 +-
> > > > > >  arch/powerpc/lib/bootm.c             |  14 +-
> > > > > >  arch/riscv/lib/bootm.c               |   4 +-
> > > > > >  arch/sh/lib/bootm.c                  |   4 +-
> > > > > >  arch/x86/lib/bootm.c                 |   4 +-
> > > > > >  arch/xtensa/lib/bootm.c              |   4 +-
> > > > > >  board/xilinx/common/board.c          |   7 +-
> > > > > >  boot/bootm.c                         |  26 ++--
> > > > > >  boot/bootm_os.c                      |   5 +-
> > > > > >  boot/image-board.c                   |  32 ++---
> > > > > >  boot/image-fdt.c                     |  29 ++---
> > > > > >  cmd/bdinfo.c                         |   6 +-
> > > > > >  cmd/booti.c                          |   2 +-
> > > > > >  cmd/bootz.c                          |   2 +-
> > > > > >  cmd/load.c                           |   7 +-
> > > > > >  drivers/iommu/apple_dart.c           |   7 +-
> > > > > >  drivers/iommu/sandbox_iommu.c        |  15 +--
> > > > > >  fs/fs.c                              |   7 +-
> > > > > >  include/image.h                      |  22 +---
> > > > > >  include/lmb.h                        |  39 +++---
> > > > > >  lib/lmb.c                            |  81 ++++++------
> > > > > >  net/tftp.c                           |   5 +-
> > > > > >  net/wget.c                           |   5 +-
> > > > > >  test/cmd/bdinfo.c                    |   2 +-
> > > > > >  test/lib/lmb.c                       | 187 +++++++++++++--------------
> > > > > >  36 files changed, 270 insertions(+), 333 deletions(-)
> > > > >
> > > > > This isn't necessary...and it will make things harder. You can have a
> > > > > global 'lmb' while still allowing passing a different pointer when
> > > > > needed.
> > > >
> > > > There's only one reservation checking system and list of known
> > > > reservations, keep in mind.
> > >
> > > There is only one driver model, too, but we use a pointer. It makes
> > > tests much easier.
> > >
> > > In fact I see elsewhere in this series that it causes problems with
> > > tests. Best to use a pointer so it is easy to update.

I don't mind keeping it, but OTOH we are fundamentally changing how
lmb works, do we need to expose a ptr to make tests easier? We should
be aiming for code that is more readable and easier to maintain.
Isn't it better to just rewrite the tests replicating the existing
cases & add new potential ones?

> >
> > Maybe? I worry that will lead to thinking that we still, like today,
> > have many LMB lists, rather than a single LMB list that everyone must
> > use.
>
> Do people worry about that with driver model?
>
> Also IMO there is only really one LMB list today. We create it at the
> start of bootm and then it is done when we boot. The file-loading
> stuff is what makes all this confusing...and with bootstd that is
> under control as well.
>
> At lot of this effort seems to be about dealing with random scripts
> which load things. We want to make sure we complain if something
> overlaps. But we should be making the bootstd case work nicely and
> doing things within that framework. Also EFI sort-of has its own
> thing, which it is very-much in control of.
>

I agree that things should be nicely integrated in bootstd, but does
keeping a ptr as an argument help with that ?

> Overall I think this is a bit more subtle that just combining allocators.

Regards
/Ilias
>
> Regards,
> Simon