Re: [PATCH 0/8] efi_loader: Complete the bootflow_efi() test

[Tom Rini <trini@konsulko.com>]

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

On Tue, Jan 07, 2025 at 06:57:50AM -0700, Simon Glass wrote:
> Hi Heinrich,
> 
> On Tue, 7 Jan 2025 at 06:11, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 07.01.25 13:15, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Mon, 6 Jan 2025 at 10:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >>
> > >> On 06.01.25 15:47, Simon Glass wrote:
> > >>> This test was hamstrung in code review so this series is an attempt to
> > >>> complete the intended functionality:
> > >>>
> > >>> - Check memory allocations look correct
> > >>> - Check that exit-boot-services removes active-DMA devices
> > >>> - Check that the bootflow is still present after testapp finishes
> > >>>
> > >>> The EFI functionality duplicates bootm_announce_and_cleanup() and still
> > >>> uses the defunct board_quiesce_devices() so a nice cleanup would be to
> > >>> call the bootm function instead, with suitable modifications. That would
> > >>> allow bootstage to work too.
> > >>>
> > >>> This series is based on sjg/master since the EFI logging was rejected so
> > >>> far.
> > >>
> > >> Yes, it was rejected because a solution at the lib/log.c level would be
> > >> more generic.
> > >
> > > As I mentioned, that idea isn't suitable for programmatic use.
> >
> > What can be done with show_addr("mem", rec->memory); that log_debug()
> > does not offer or which you could not do with a new log function in
> > lib/log.c that takes variadic arguments?
> 
> There are asserts in [1], for example. How do you propose to handle
> that? See [2] for my previous explanation, quoted here:
> 
> > CONFIG_LOG with a bloblist option would be a great idea, but it's hard
> > to programmatically scan text...plus only the external call sites are
> > actually logged.
> 
> Also see the discussion on the original patch [3]. There was also your
> reply at [4], but I think you missed that this is intended for use in
> unit tests (i.e. with ut_assert()).
> 
> You also requested that this be generalised, rather than being
> EFI-loader-specific. I have no objection to that, but don't have a use
> case for it yet, so have deferred that to later. It's a fairly simple
> change, if/when needed. If the series was not NAKed, I'd be happy to
> do it now.
> 
> > >
> > >>
> > >> Tom suggested not to send patches that are for private enjoyment to the
> > >> mailing list.
> > >
> > > My contributions to U-Boot are only ever about private enjoyment :-)
> > >
> > > Do you have any comments on the patches?
> 
> Regards,
> Simon
> 
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20250106144755.3054780-6-sjg@chromium.org/
> [2] https://lore.kernel.org/u-boot/CAFLszTjxOE_037+kR0jgdax80sBombYo_k0YgiuVnP=KZCOvuA@mail.gmail.com/
> [3] https://lore.kernel.org/u-boot/CAC_iWjKtaN54B98OKbkoXkC_GmKJ=x+M4=UY_O6roSOpZaDxag@mail.gmail.com/
> [4] https://lore.kernel.org/u-boot/D513D326-41A6-425E-B11F-85958065BCD2@gmx.de/

Looking at the logging portions of the original series again, especially
if this was made generic, we probably don't want to print to actual
console every time we're making a note of some memory allocation for
example, that would be unreadable outside of a debug context. The point
of this really seems to be "log things for verifying in tests later".
Does that end up being useful? I don't know. Heinrich or Ilias, do the
tests in [1] look generally useful?

-- 
Tom

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