On 07.01.25 16:11, Tom Rini wrote:
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.g...@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.g...@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-...@chromium.org/
[2]
https://lore.kernel.org/u-boot/CAFLszTjxOE_037+kR0jgdax80sBombYo_k0YgiuVnP=kzco...@mail.gmail.com/
[3]
https://lore.kernel.org/u-boot/CAC_iWjKtaN54B98OKbkoXkC_GmKJ=x+M4=uy_o6rosopzad...@mail.gmail.com/
[4] https://lore.kernel.org/u-boot/d513d326-41a6-425e-b11f-85958065b...@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?
The tests in [1] are not documented, not even in the commit message. So
the reasoning behind the tests remains Simon's secret.
At first sight the tests in [1] don't make much sense. E.g. that only a
subset of memory types have been used does not tell that the right
memory type has been used for the right object.
Implementing a specific tracing functionality for EFI is definitively
the wrong way forward as it will lead to code duplication.
We already have function _log() which is variadic.
Simon could write a new log driver that parses the `format` parameter
and saves the binary data in an appropriate format for analysis by the
unit tests:
* For %s the driver should save the string and not the address of the
string.
* For %pD the driver should save the device path instead of the pointer.
* ...
Some changes to the log driver interface will be needed to pass the
variadic arguments instead of the formatted message.
Best regards
Heinrich
