[PATCH v2 0/7] efi_loader: Add support for logging to a buffer
It is a bit of a pain to log EFI boot-services calls at present. The output goes to the console so cannot easily be inspected later. Also it would be useful to be able to store the log and review it later, perhaps after something has gone wrong.
This series makes a start on implementing a log-to-buffer feature. It provides a simple 'efidebug log' command to inspect the buffer. For now, only memory allocations are logged.
This feature makes it possible to add tests to check which EFI calls are made by U-Boot itself. It may also make it easier to figure out what is needed for booting Windows.
Some patches to help with debugging sandbox memory-mapping are included, but for now EFI's use of mapping is not adjusted.
Changes in v2: - Rebase on efil series (memory address/pointers)
Simon Glass (7): efi_loader: Fix free in ..._media_device_boot_option() efi_loader: Add support for logging EFI calls efi_loader: Create the log on startup efi_loader: Add a command to show the EFI log test: efi_loader: Add a simple test for the EFI log efi_loader: Use the log with memory-related functions efi_loader: Add documentation for the EFI log
MAINTAINERS | 7 + cmd/efidebug.c | 53 ++++- configs/sandbox_defconfig | 3 + doc/develop/uefi/uefi.rst | 22 ++ doc/usage/cmd/efidebug.rst | 109 +++++++++ doc/usage/index.rst | 1 + include/bloblist.h | 1 + include/efi.h | 1 + include/efi_log.h | 316 +++++++++++++++++++++++++ lib/efi_loader/Kconfig | 19 ++ lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_bootmgr.c | 3 +- lib/efi_loader/efi_log.c | 444 +++++++++++++++++++++++++++++++++++ lib/efi_loader/efi_memory.c | 66 +++++- lib/efi_loader/efi_setup.c | 7 + test/lib/Makefile | 1 + test/lib/efi_log.c | 93 ++++++++ 17 files changed, 1131 insertions(+), 16 deletions(-) create mode 100644 doc/usage/cmd/efidebug.rst create mode 100644 include/efi_log.h create mode 100644 lib/efi_loader/efi_log.c create mode 100644 test/lib/efi_log.c
Freeing a NULL pointer is an error in EFI, so check the pointer first, before freeing it.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_bootmgr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index e3b8dfb6013..cb5ea0e4040 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -1186,7 +1186,8 @@ out: free(opt[i].lo); } free(opt); - efi_free_pool(handles); + if (handles) + efi_free_pool(handles);
if (ret == EFI_NOT_FOUND) return EFI_SUCCESS;
The current logging system suffers from some disadvantages, mainly that it writes its output to the console and cannot be easily reviewed.
Add a dedicated log, storing records in a binary format and including the result codes and any return values from each call. The log is built sequentially in memory and can be reviewed after any EFI operation. It could potentially be written to media for later review, but that is not implemented so far.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
MAINTAINERS | 6 + include/bloblist.h | 1 + include/efi.h | 1 + include/efi_log.h | 169 ++++++++++++++++++++++++++ lib/efi_loader/Kconfig | 19 +++ lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_log.c | 256 +++++++++++++++++++++++++++++++++++++++ 7 files changed, 453 insertions(+) create mode 100644 include/efi_log.h create mode 100644 lib/efi_loader/efi_log.c
diff --git a/MAINTAINERS b/MAINTAINERS index 8c6c0c2a4bc..890ca4290d3 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1075,6 +1075,12 @@ F: lib/efi/efi_app.c F: scripts/build-efi.sh F: test/dm/efi_media.c
+EFI LOGGING +M: Simon Glass sjg@chromium.org +S: Maintained +F: include/efi_log.h +F: lib/efi_loader/efi_log.c + EFI PAYLOAD M: Heinrich Schuchardt xypron.glpk@gmx.de M: Ilias Apalodimas ilias.apalodimas@linaro.org diff --git a/include/bloblist.h b/include/bloblist.h index ff32d3fecfd..1e1ca34aa92 100644 --- a/include/bloblist.h +++ b/include/bloblist.h @@ -153,6 +153,7 @@ enum bloblist_tag_t { BLOBLISTT_U_BOOT_SPL_HANDOFF = 0xfff000, /* Hand-off info from SPL */ BLOBLISTT_VBE = 0xfff001, /* VBE per-phase state */ BLOBLISTT_U_BOOT_VIDEO = 0xfff002, /* Video info from SPL */ + BLOBLISTT_EFI_LOG = 0xfff003, /* Log of EFI calls */ };
/** diff --git a/include/efi.h b/include/efi.h index 76feefe6c50..90823eb98fb 100644 --- a/include/efi.h +++ b/include/efi.h @@ -127,6 +127,7 @@ static inline void *guidcpy(void *dst, const void *src) #define EFI_COMPROMISED_DATA (EFI_ERROR_MASK | 33) #define EFI_IP_ADDRESS_CONFLICT (EFI_ERROR_MASK | 34) #define EFI_HTTP_ERROR (EFI_ERROR_MASK | 35) +#define EFI_ERROR_COUNT 36
#define EFI_WARN_UNKNOWN_GLYPH 1 #define EFI_WARN_DELETE_FAILURE 2 diff --git a/include/efi_log.h b/include/efi_log.h new file mode 100644 index 00000000000..1988e5f9df0 --- /dev/null +++ b/include/efi_log.h @@ -0,0 +1,169 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Logging (to memory) of calls from an EFI app + * + * Copyright 2024 Google LLC + * Written by Simon Glass sjg@chromium.org + */ + +#ifndef __EFI_LOG_H +#define __EFI_LOG_H + +#include <linux/types.h> +#include <efi.h> + +/** + * enum efil_tag - Types of logging records which can be created + */ +enum efil_tag { + EFILT_TESTING, + + EFILT_COUNT, +}; + +/** + * struct efil_rec_hdr - Header for each logging record + * + * @tag: Tag which indicates the type of the record + * @size: Size of the record in bytes + * @ended: true if record has been completed (i.e. the function returned), false + * if it is still pending + * @e_ret: Records the return function from the logged function + */ +struct efil_rec_hdr { + enum efil_tag tag; + int size; + bool ended; + efi_status_t e_ret; +}; + +/** + * struct efil_hdr - Holds the header for the log + * + * @upto: Offset at which to store the next log record + * @size: Total size of the log in bytes + */ +struct efil_hdr { + int upto; + int size; +}; + +enum efil_test_t { + EFI_LOG_TEST0, + EFI_LOG_TEST1, + + EFI_LOG_TEST_COUNT, +}; + +/** + * struct efil_testing - used for testing the log + */ +struct efil_testing { + enum efil_test_t enum_val; + efi_uintn_t int_val; + u64 *memory; + void **buffer; + u64 e_memory; + void *e_buffer; +}; + +/** + * struct efil_allocate_pages - holds info from efi_allocate_pages() call + * + * @e_memory: Contains the value of *@memory on return from the EFI function + */ +struct efil_allocate_pages { + enum efi_allocate_type type; + enum efi_memory_type memory_type; + efi_uintn_t pages; + u64 *memory; + u64 e_memory; +}; + +/* + * The functions below are in pairs, with a 'start' and 'end' call for each EFI + * function. The 'start' function (efi_logs_...) is called when the function is + * started. It records all the arguments. The 'end' function (efi_loge_...) is + * called when the function is ready to return. It records any output arguments + * as well as the return value. + * + * The start function returns the offset of the log record. This must be passed + * to the end function, so it can add the status code and any other useful + * information. It is not possible for the end functions to remember the offset + * from the associated start function, since EFI functions may be called in a + * nested way and there is no obvious way to determine the log record to which + * the end function refers. + * + * If the start function returns an error code (i.e. an offset < 0) then it is + * safe to pass that to the end function. It will simply ignore the operation. + * Common errors are -ENOENT if there is no log and -ENOSPC if the log is full + */ + +#if CONFIG_IS_ENABLED(EFI_LOG) + +/** + * efi_logs_testing() - Record a test call to an efi function + * + * @enum_val: enum value + * @int_val: integer value + * @buffer: place to write pointer address + * @memory: place to write memory address + * Return: log-offset of this new record, or -ve error code + */ +int efi_logs_testing(enum efil_test_t enum_val, efi_uintn_t int_value, + void *buffer, u64 *memory); + +/** + * efi_loge_testing() - Record a return from a test call + * + * This stores the value of the pointers also + * + * ofs: Offset of the record to end + * efi_ret: status code to record + */ +int efi_loge_testing(int ofs, efi_status_t efi_ret); + +#else /* !EFI_LOG */ + +static inline int efi_logs_testing(enum efil_test_t enum_val, + efi_uintn_t int_value, void *buffer, + u64 *memory) +{ + return -ENOSYS; +} + +static inline int efi_loge_testing(int ofs, efi_status_t efi_ret) +{ + return -ENOSYS; +} + +#endif /* EFI_LOG */ + +/* below are some general functions */ + +/** + * efi_log_show() - Show the EFI log + * + * Displays the log of EFI boot-services calls which are so-far enabled for + * logging + * + * Return: 0 on success, or -ve error code + */ +int efi_log_show(void); + +/** + * efi_log_reset() - Reset the log, erasing all records + * + * Return 0 if OK, -ENOENT if the log could not be found + + */ +int efi_log_reset(void); + +/** + * efi_log_init() - Create a log in the bloblist, then reset it + * + * Return 0 if OK, -ENOMEM if the bloblist is not large enough + */ +int efi_log_init(void); + +#endif /* __EFI_LOG_H */ diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index d93f28b8422..c42becfeef3 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -530,6 +530,25 @@ config EFI_SCROLL_ON_CLEAR_SCREEN to capture complete boot logs (except for interactive menus etc.) and can ease debugging related issues.
+config EFI_LOG + bool "Enable logging of EFI operations" + select BLOBLIST + help + This enables maintaining a log of EFI boot-time services, useful for + debugging. It keeps track of some of the calls which are made, so far + just those related to memory allocation. + +config EFI_LOG_SIZE + hex "Size of EFI log" + depends on EFI_LOG + default 0x4000 + help + Sets the size of the EFI log in bytes. The log is stored in the + bloblist so if its size is insufficient, U-Boot will raise an error. + + The amount of space needed depends on the EFI app being run. When + space runes out, further EFI calls will not be logged. + endmenu
menu "EFI bootmanager" diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 87131ab911d..7c8b2dd1ad6 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -39,6 +39,7 @@ obj-y += efi_file.o obj-$(CONFIG_EFI_LOADER_HII) += efi_hii.o obj-y += efi_image_loader.o obj-y += efi_load_options.o +obj-$(CONFIG_EFI_LOG) += efi_log.o obj-y += efi_memory.o obj-y += efi_root_node.o obj-y += efi_runtime.o diff --git a/lib/efi_loader/efi_log.c b/lib/efi_loader/efi_log.c new file mode 100644 index 00000000000..01e495d3995 --- /dev/null +++ b/lib/efi_loader/efi_log.c @@ -0,0 +1,256 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Logging (to memory) of calls from an EFI app + * + * Copyright 2024 Google LLC + * Written by Simon Glass sjg@chromium.org + */ + +#define LOG_CATEGORY LOGC_EFI + +#include <bloblist.h> +#include <efi_log.h> +#include <errno.h> +#include <log.h> + +/* names for enum efil_tag (abbreviated to keep output to a single line) */ +static const char *tag_name[EFILT_COUNT] = { + "testing", +}; + +/* names for error codes, trying to keep them short */ +static const char *error_name[EFI_ERROR_COUNT] = { + "OK", + "load", + "inval_param", + "unsupported", + "bad_buf_sz", + "buf_small", + "not_ready", + "device", + "write_prot", + "out_of_rsrc", + "vol_corrupt", + "vol_full", + "no_media", + "media_chg", + "not_found", + "no access", + "no_response", + "no_mapping", + "timeout", + "not_started", + "already", + "aborted", + "icmp", + "tftp", + "protocol", + "bad version", + "sec_violate", + "crc_error", + "end_media", + "end_file", + "inval_lang", + "compromised", + "ipaddr_busy", + "http", +}; + +static const char *test_enum_name[EFI_LOG_TEST_COUNT] = { + "test0", + "test1", +}; + +/** + * prep_rec() - prepare a new record in the log + * + * This creates a new record at the next available position, setting it up ready + * to hold data. The size and tag are set up. + * + * The log is updated so that the next record will start after this one + * + * @tag: tag of the EFI call to record + * @size: Number of bytes in the caller's struct + * @recp: Set to point to where the caller should add its data + * Return: Offset of this record (must be passed to finish_rec()) + */ +static int prep_rec(enum efil_tag tag, uint str_size, void **recp) +{ + struct efil_hdr *hdr = bloblist_find(BLOBLISTT_EFI_LOG, 0); + struct efil_rec_hdr *rec_hdr; + int ofs, size; + + if (!hdr) + return -ENOENT; + size = str_size + sizeof(struct efil_rec_hdr); + if (hdr->upto + size > hdr->size) + return -ENOSPC; + + rec_hdr = (void *)hdr + hdr->upto; + rec_hdr->size = size; + rec_hdr->tag = tag; + rec_hdr->ended = false; + *recp = rec_hdr + 1; + + ofs = hdr->upto; + hdr->upto += size; + + return ofs; +} + +/** + * finish_rec() - Finish a previously started record + * + * @ofs: Offset of record to finish + * @ret: Return code which is to be returned from the EFI function + * Return: Pointer to the structure where the caller should add its data + */ +static void *finish_rec(int ofs, efi_status_t ret) +{ + struct efil_hdr *hdr = bloblist_find(BLOBLISTT_EFI_LOG, 0); + struct efil_rec_hdr *rec_hdr; + + if (!hdr || ofs < 0) + return NULL; + rec_hdr = (void *)hdr + ofs; + rec_hdr->ended = true; + rec_hdr->e_ret = ret; + + return rec_hdr + 1; +} + +int efi_logs_testing(enum efil_test_t enum_val, efi_uintn_t int_val, + void *buffer, u64 *memory) +{ + struct efil_testing *rec; + int ret; + + ret = prep_rec(EFILT_TESTING, sizeof(*rec), (void **)&rec); + if (ret < 0) + return ret; + + rec->int_val = int_val; + rec->buffer = buffer; + rec->memory = memory; + rec->e_buffer = NULL; + rec->e_memory = 0; + + return ret; +} + +int efi_loge_testing(int ofs, efi_status_t efi_ret) +{ + struct efil_testing *rec; + + rec = finish_rec(ofs, efi_ret); + if (!rec) + return -ENOSPC; + rec->e_memory = *rec->memory; + rec->e_buffer = *rec->buffer; + + return 0; +} + +static void show_enum(const char *type_name[], int type) +{ + printf("%s ", type_name[type]); +} + +static void show_ulong(const char *prompt, ulong val) +{ + printf("%s %lx", prompt, val); + if (val >= 10) + printf("/%ld", val); + printf(" "); +} + +static void show_addr(const char *prompt, ulong addr) +{ + printf("%s %lx ", prompt, addr); +} + +static void show_ret(efi_status_t ret) +{ + int code; + + code = ret & ~EFI_ERROR_MASK; + if (code < ARRAY_SIZE(error_name)) + printf("ret %s", error_name[ret]); + else + printf("ret %lx", ret); +} + +void show_rec(int seq, struct efil_rec_hdr *rec_hdr) +{ + void *start = (void *)rec_hdr + sizeof(struct efil_rec_hdr); + + printf("%3d %12s ", seq, tag_name[rec_hdr->tag]); + switch (rec_hdr->tag) { + case EFILT_TESTING: { + struct efil_testing *rec = start; + + show_enum(test_enum_name, (int)rec->enum_val); + show_ulong("int", (ulong)rec->int_val); + show_addr("buf", map_to_sysmem(rec->buffer)); + show_addr("mem", map_to_sysmem(rec->memory)); + if (rec_hdr->ended) { + show_addr("*buf", + (ulong)map_to_sysmem((void *)rec->e_buffer)); + show_addr("*mem", + (ulong)rec->e_memory); + show_ret(rec_hdr->e_ret); + } + } + case EFILT_COUNT: + break; + } + printf("\n"); +} + +int efi_log_show(void) +{ + struct efil_hdr *hdr = bloblist_find(BLOBLISTT_EFI_LOG, 0); + struct efil_rec_hdr *rec_hdr; + int i; + + printf("EFI log (size %x)\n", hdr->upto); + if (!hdr) + return -ENOENT; + for (i = 0, rec_hdr = (void *)hdr + sizeof(*hdr); + (void *)rec_hdr - (void *)hdr < hdr->upto; + i++, rec_hdr = (void *)rec_hdr + rec_hdr->size) + show_rec(i, rec_hdr); + printf("%d records\n", i); + + return 0; +} + +int efi_log_reset(void) +{ + struct efil_hdr *hdr = bloblist_find(BLOBLISTT_EFI_LOG, 0); + + if (!hdr) + return -ENOENT; + hdr->upto = sizeof(struct efil_hdr); + hdr->size = CONFIG_EFI_LOG_SIZE; + + return 0; +} + +int efi_log_init(void) +{ + struct efil_hdr *hdr; + + hdr = bloblist_add(BLOBLISTT_EFI_LOG, CONFIG_EFI_LOG_SIZE, 0); + if (!hdr) { + /* + * Return -ENOMEM since we use -ENOSPC to mean that the log is + * full + */ + log_warning("Failed to setup EFI log\n"); + return log_msg_ret("eli", -ENOMEM); + } + efi_log_reset(); + + return 0; +}
Create an EFI log when the EFI subsystem is first touched. This happens after relocation in board_init_f()
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_setup.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index aa59bc7779d..468156db813 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -8,6 +8,7 @@ #define LOG_CATEGORY LOGC_EFI
#include <efi_loader.h> +#include <efi_log.h> #include <efi_variable.h> #include <log.h> #include <asm-generic/unaligned.h> @@ -186,6 +187,12 @@ int efi_init_early(void) /* Allow unaligned memory access */ allow_unaligned();
+ if (IS_ENABLED(CONFIG_EFI_LOG)) { + ret = efi_log_init(); + if (ret) + return -ENOSPC; + } + /* Initialize root node */ ret = efi_root_node_register(); if (ret != EFI_SUCCESS)
Add a simple command which lists the log. Avoid creating any new records when this command is invoked.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
cmd/efidebug.c | 53 +++++++++++++++--- doc/usage/cmd/efidebug.rst | 109 +++++++++++++++++++++++++++++++++++++ doc/usage/index.rst | 1 + 3 files changed, 155 insertions(+), 8 deletions(-) create mode 100644 doc/usage/cmd/efidebug.rst
diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 02f1e080e88..89bef0c9c1d 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -10,6 +10,7 @@ #include <dm/device.h> #include <efi_dt_fixup.h> #include <efi_load_initrd.h> +#include <efi_log.h> #include <efi_loader.h> #include <efi_rng.h> #include <efi_variable.h> @@ -532,6 +533,33 @@ static int do_efi_show_defaults(struct cmd_tbl *cmdtp, int flag, return CMD_RET_SUCCESS; }
+/** + * do_efi_show_log() - show UEFI log of boot-services calls + * + * @cmdtp: Command table + * @flag: Command flag + * @argc: Number of arguments + * @argv: Argument array + * Return: CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure + * + * Implement efidebug "log" sub-command. + * Show UEFI log records. + */ +static int do_efi_show_log(struct cmd_tbl *cmdtp, int flag, + int argc, char *const argv[]) +{ + if (!IS_ENABLED(CONFIG_EFI_LOG)) { + printf("Please enable CONFIG_EFI_LOG to use the log\n"); + return CMD_RET_FAILURE; + } + if (efi_log_show()) { + printf("Failed\n"); + return CMD_RET_FAILURE; + } + + return CMD_RET_SUCCESS; +} + static const char * const efi_mem_type_string[] = { [EFI_RESERVED_MEMORY_TYPE] = "RESERVED", [EFI_LOADER_CODE] = "LOADER CODE", @@ -1586,6 +1614,7 @@ static struct cmd_tbl cmd_efidebug_sub[] = { "", ""), U_BOOT_CMD_MKENT(images, CONFIG_SYS_MAXARGS, 1, do_efi_show_images, "", ""), + U_BOOT_CMD_MKENT(log, CONFIG_SYS_MAXARGS, 1, do_efi_show_log, "", ""), U_BOOT_CMD_MKENT(memmap, CONFIG_SYS_MAXARGS, 1, do_efi_show_memmap, "", ""), U_BOOT_CMD_MKENT(tables, CONFIG_SYS_MAXARGS, 1, do_efi_show_tables, @@ -1620,19 +1649,25 @@ static int do_efidebug(struct cmd_tbl *cmdtp, int flag,
argc--; argv++;
- /* Initialize UEFI drivers */ - r = efi_init_obj_list(); - if (r != EFI_SUCCESS) { - printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", - r & ~EFI_ERROR_MASK); - return CMD_RET_FAILURE; - } - cp = find_cmd_tbl(argv[0], cmd_efidebug_sub, ARRAY_SIZE(cmd_efidebug_sub)); if (!cp) return CMD_RET_USAGE;
+ /* + * Calling efi_init_obj_list() can add log records, so avoid it if just + * showing the log + */ + if (cp->cmd != do_efi_show_log) { + /* Initialize UEFI drivers */ + r = efi_init_obj_list(); + if (r != EFI_SUCCESS) { + printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", + r & ~EFI_ERROR_MASK); + return CMD_RET_FAILURE; + } + } + return cp->cmd(cmdtp, flag, argc, argv); }
@@ -1680,6 +1715,8 @@ U_BOOT_LONGHELP(efidebug, " - show default EFI filename and PXE architecture\n" "efidebug images\n" " - show loaded images\n" + "efidebug log\n" + " - show UEFI log\n" "efidebug memmap\n" " - show UEFI memory map\n" "efidebug tables\n" diff --git a/doc/usage/cmd/efidebug.rst b/doc/usage/cmd/efidebug.rst new file mode 100644 index 00000000000..5eca4079f82 --- /dev/null +++ b/doc/usage/cmd/efidebug.rst @@ -0,0 +1,109 @@ +.. SPDX-License-Identifier: GPL-2.0+ +.. Copyright 2024 Google LLC +.. Written by Simon Glass sjg@chromium.org + +.. index:: + single: efidebug (command) + +efidebug command +================ + +Synopsis +-------- + +:: + + efidebug log + +Description +----------- + +The *efidebug* command provides access to debugging features for the EFI-loader +subsystem. + +Only one of the subcommands are documented at present. + +efidebug log +~~~~~~~~~~~~ + +This shows a log of EFI boot-services calls which have been handled since U-Boot +started. This can be useful to see what the app is doing, or even what U-Boot +itself has called. + + +Example +------- + +This shows checking the log, then using 'efidebug tables' to fully set up the +EFI-loader subsystem, then checking the log again:: + + => efidebug log + EFI log (size 158) + 0 alloc_pool bt-data size 33/51 buf 7fffd8448ad0 *buf 7c20010 ret OK + 1 alloc_pages any-pages bt-data pgs 1 mem 7fffd8448a80 *mem 7c20000 ret OK + 2 alloc_pool bt-data size 60/96 buf 7fffd8448ac0 *buf 7c1f010 ret OK + 3 alloc_pages any-pages bt-data pgs 1 mem 7fffd8448a60 *mem 7c1f000 ret OK + 4 alloc_pool bt-data size 60/96 buf 7fffd8448ac0 *buf 7c1e010 ret OK + 5 alloc_pages any-pages bt-data pgs 1 mem 7fffd8448a60 *mem 7c1e000 ret OK + 6 records + => efidebug tables + efi_var_to_file() Cannot persist EFI variables without system partition + 0000000017bfc010 36122546-f7ef-4c8f-bd9b-eb8525b50c0b EFI Conformance Profiles Table + 0000000017bd4010 b122a263-3661-4f68-9929-78f8b0d62180 EFI System Resource Table + 0000000017bd8010 1e2ed096-30e2-4254-bd89-863bbef82325 TCG2 Final Events Table + 0000000017bd6010 eb66918a-7eef-402a-842e-931d21c38ae9 Runtime properties + 0000000008c49000 8868e871-e4f1-11d3-bc22-0080c73c8881 ACPI table + 0000000018c5b000 f2fd1544-9794-4a2c-992e-e5bbcf20e394 SMBIOS3 table + => efidebug log + EFI log (size a20) + 0 alloc_pool bt-data size 33/51 buf 7fffd8448ad0 *buf 7c20010 ret OK + 1 alloc_pages any-pages bt-data pgs 1 mem 7fffd8448a80 *mem 7c20000 ret OK + 2 alloc_pool bt-data size 60/96 buf 7fffd8448ac0 *buf 7c1f010 ret OK + 3 alloc_pages any-pages bt-data pgs 1 mem 7fffd8448a60 *mem 7c1f000 ret OK + 4 alloc_pool bt-data size 60/96 buf 7fffd8448ac0 *buf 7c1e010 ret OK + 5 alloc_pages any-pages bt-data pgs 1 mem 7fffd8448a60 *mem 7c1e000 ret OK + 6 alloc_pages any-pages rt-data pgs 20/32 mem 7fffd8448838 *mem 7bfe000 ret OK + 7 alloc_pool rt-data size 60/96 buf 7fffd84487e0 *buf 7bfd010 ret OK + 8 alloc_pages any-pages rt-data pgs 1 mem 7fffd8448780 *mem 7bfd000 ret OK + 9 alloc_pool rt-data size 180/384 buf 56f190ffd890 *buf 7bfc010 ret OK + 10 alloc_pages any-pages rt-data pgs 1 mem 7fffd8448800 *mem 7bfc000 ret OK + 11 alloc_pool bt-data size 4 buf 7fffd8448840 *buf 7bfb010 ret OK + 12 alloc_pages any-pages bt-data pgs 1 mem 7fffd84487f0 *mem 7bfb000 ret OK + 13 alloc_pool bt-data size 10/16 buf 7fffd8448728 *buf 7bfa010 ret OK + 14 alloc_pages any-pages bt-data pgs 1 mem 7fffd84486d0 *mem 7bfa000 ret OK + 15 alloc_pool bt-data size 60/96 buf 7fffd84487e0 *buf 7bf9010 ret OK + 16 alloc_pages any-pages bt-data pgs 1 mem 7fffd8448780 *mem 7bf9000 ret OK + 17 alloc_pool bt-data size 10000/65536 buf 56f19100fae0 *buf 7be8010 ret OK + 18 alloc_pages any-pages bt-data pgs 11/17 mem 7fffd84487d0 *mem 7be8000 ret OK + 19 alloc_pool acpi-nvs size 10000/65536 buf 56f19100fae8 *buf 7bd7010 ret OK + 20 alloc_pages any-pages acpi-nvs pgs 11/17 mem 7fffd84487d0 *mem 7bd7000 ret OK + 21 alloc_pool bt-data size 60/96 buf 7fffd84487d0 *buf 7bd6010 ret OK + 22 alloc_pages any-pages bt-data pgs 1 mem 7fffd8448770 *mem 7bd6000 ret OK + 23 alloc_pool rt-data size 8 buf 7fffd8448818 *buf 7bd5010 ret OK + 24 alloc_pages any-pages rt-data pgs 1 mem 7fffd84487c0 *mem 7bd5000 ret OK + 25 alloc_pool bt-data size 8 buf 7fffd8448360 *buf 7bd4010 ret OK + 26 alloc_pages any-pages bt-data pgs 1 mem 7fffd8448160 *mem 7bd4000 ret OK + 27 alloc_pool bt-data size f0/240 buf 7fffd8448378 *buf 7bd3010 ret OK + 28 alloc_pages any-pages bt-data pgs 1 mem 7fffd84482d0 *mem 7bd3000 ret OK + 29 free_pool buf 7bd3010 ret OK + 30 free_pages mem 7bd3000 pag 1 ret OK + 31 alloc_pool bt-data size 60/96 buf 7fffd84482d8 *buf 7bd3010 ret OK + 32 alloc_pages any-pages bt-data pgs 1 mem 7fffd8448280 *mem 7bd3000 ret OK + 33 free_pool buf 7bfa010 ret OK + 34 free_pages mem 7bfa000 pag 1 ret OK + 35 alloc_pool bt-data size f0/240 buf 7fffd8448380 *buf 7bfa010 ret OK + 36 alloc_pages any-pages bt-data pgs 1 mem 7fffd84482d0 *mem 7bfa000 ret OK + 37 free_pool buf 7bfa010 ret OK + 38 free_pages mem 7bfa000 pag 1 ret OK + 39 free_pool buf 7bd4010 ret OK + 40 free_pages mem 7bd4000 pag 1 ret OK + 41 alloc_pool bt-data size 61/97 buf 7fffd8448810 *buf 7bfa010 ret OK + 42 alloc_pages any-pages bt-data pgs 1 mem 7fffd84487c0 *mem 7bfa000 ret OK + 43 alloc_pool bt-data size 60/96 buf 7fffd8448800 *buf 7bd4010 ret OK + 44 alloc_pages any-pages bt-data pgs 1 mem 7fffd84487a0 *mem 7bd4000 ret OK + 45 alloc_pool bt-data size 60/96 buf 7fffd8448800 *buf 7bd2010 ret OK + 46 alloc_pages any-pages bt-data pgs 1 mem 7fffd84487a0 *mem 7bd2000 ret OK + 47 alloc_pool bt-data size 60/96 buf 7fffd8448810 *buf 7bd1010 ret OK + 48 alloc_pages any-pages bt-data pgs 1 mem 7fffd84487b0 *mem 7bd1000 ret OK + 49 records + => diff --git a/doc/usage/index.rst b/doc/usage/index.rst index cb7a23f1170..d070b9fba1f 100644 --- a/doc/usage/index.rst +++ b/doc/usage/index.rst @@ -60,6 +60,7 @@ Shell commands cmd/echo cmd/efi cmd/eficonfig + cmd/efidebug cmd/env cmd/event cmd/exception
Create a test which does some sample calls and checks the output.
Expand the size of the sandbox bloblist to accommodate the log.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
MAINTAINERS | 1 + configs/sandbox_defconfig | 3 +++ lib/efi_loader/efi_log.c | 7 +++--- test/lib/Makefile | 1 + test/lib/efi_log.c | 49 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 test/lib/efi_log.c
diff --git a/MAINTAINERS b/MAINTAINERS index 890ca4290d3..5e795ce9a4b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1080,6 +1080,7 @@ M: Simon Glass sjg@chromium.org S: Maintained F: include/efi_log.h F: lib/efi_loader/efi_log.c +F: test/lib/efi_log.c
EFI PAYLOAD M: Heinrich Schuchardt xypron.glpk@gmx.de diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 718e4a8283c..3c5954b6c7f 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -17,6 +17,7 @@ CONFIG_EFI_CAPSULE_ON_DISK=y CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y CONFIG_EFI_CAPSULE_AUTHENTICATE=y CONFIG_EFI_CAPSULE_CRT_FILE="board/sandbox/capsule_pub_key_good.crt" +CONFIG_EFI_LOG=y CONFIG_BUTTON_CMD=y CONFIG_FIT=y CONFIG_FIT_RSASSA_PSS=y @@ -50,6 +51,7 @@ CONFIG_LOG_DEFAULT_LEVEL=6 CONFIG_LOGF_FUNC=y CONFIG_DISPLAY_BOARDINFO_LATE=y CONFIG_STACKPROTECTOR=y +CONFIG_BLOBLIST_SIZE_RELOC=0x5000 CONFIG_CMD_CPU=y CONFIG_CMD_LICENSE=y CONFIG_CMD_SMBIOS=y @@ -71,6 +73,7 @@ CONFIG_CMD_NVEDIT_LOAD=y CONFIG_CMD_NVEDIT_SELECT=y CONFIG_LOOPW=y CONFIG_CMD_MD5SUM=y +CONFIG_CMD_MEMINFO=y CONFIG_CMD_MEM_SEARCH=y CONFIG_CMD_MX_CYCLIC=y CONFIG_CMD_MEMTEST=y diff --git a/lib/efi_loader/efi_log.c b/lib/efi_loader/efi_log.c index 01e495d3995..780860d8b2f 100644 --- a/lib/efi_loader/efi_log.c +++ b/lib/efi_loader/efi_log.c @@ -129,6 +129,7 @@ int efi_logs_testing(enum efil_test_t enum_val, efi_uintn_t int_val, if (ret < 0) return ret;
+ rec->enum_val = enum_val; rec->int_val = int_val; rec->buffer = buffer; rec->memory = memory; @@ -194,10 +195,8 @@ void show_rec(int seq, struct efil_rec_hdr *rec_hdr) show_addr("buf", map_to_sysmem(rec->buffer)); show_addr("mem", map_to_sysmem(rec->memory)); if (rec_hdr->ended) { - show_addr("*buf", - (ulong)map_to_sysmem((void *)rec->e_buffer)); - show_addr("*mem", - (ulong)rec->e_memory); + show_addr("*buf", (ulong)map_to_sysmem(rec->e_buffer)); + show_addr("*mem", (ulong)rec->e_memory); show_ret(rec_hdr->e_ret); } } diff --git a/test/lib/Makefile b/test/lib/Makefile index f516d001747..3556c4d199c 100644 --- a/test/lib/Makefile +++ b/test/lib/Makefile @@ -11,6 +11,7 @@ obj-y += abuf.o obj-y += alist.o obj-$(CONFIG_EFI_LOADER) += efi_device_path.o obj-$(CONFIG_EFI_SECURE_BOOT) += efi_image_region.o +obj-$(CONFIG_EFI_LOG) += efi_log.o obj-y += hexdump.o obj-$(CONFIG_SANDBOX) += kconfig.o obj-y += lmb.o diff --git a/test/lib/efi_log.c b/test/lib/efi_log.c new file mode 100644 index 00000000000..414b7081bd9 --- /dev/null +++ b/test/lib/efi_log.c @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright 2024 Google LLC + * Written by Simon Glass sjg@chromium.org + */ + +#include <efi_log.h> +#include <mapmem.h> +#include <test/lib.h> +#include <test/test.h> +#include <test/ut.h> + +/* basic test of logging */ +static int lib_test_efi_log_base(struct unit_test_state *uts) +{ + void **buf = map_sysmem(0x1000, 0); + u64 *addr = map_sysmem(0x1010, 0); + int ofs1, ofs2; + + ut_assertok(efi_log_reset()); + + ofs1 = efi_logs_testing(EFI_LOG_TEST0, 123, &buf[0], &addr[0]); + + ofs2 = efi_logs_testing(EFI_LOG_TEST1, 456, &buf[1], &addr[1]); + + /* simulate an EFI call setting the return values */ + addr[0] = 0x100; + buf[0] = map_sysmem(0x1100, 0); + addr[1] = 0x200; + buf[1] = map_sysmem(0x1200, 0); + + ut_assertok(efi_loge_testing(ofs2, EFI_LOAD_ERROR)); + ut_assertok(efi_loge_testing(ofs1, EFI_SUCCESS)); + + ut_assertok(efi_log_show()); + ut_assert_nextline("EFI log (size 98)"); + ut_assert_nextline( + " 0 testing test0 int 7b/123 buf 1000 mem 1010 *buf 1100 *mem 100 ret OK"); + ut_assert_nextline( + " 1 testing test1 int 1c8/456 buf 1008 mem 1018 *buf 1200 *mem 200 ret load"); + ut_assert_nextline("2 records"); + ut_assert_console_end(); + + unmap_sysmem(buf); + unmap_sysmem(addr); + + return 0; +} +LIB_TEST(lib_test_efi_log_base, UTF_CONSOLE);
Update a few memory functions to log their inputs and outputs. To avoid the use of 'goto', etc. the functions are turned into stubs, calling a separate function to do the actual operation.
Add a few tests.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
include/efi_log.h | 147 ++++++++++++++++++++++++++++ lib/efi_loader/efi_log.c | 189 ++++++++++++++++++++++++++++++++++++ lib/efi_loader/efi_memory.c | 66 +++++++++++-- test/lib/efi_log.c | 44 +++++++++ 4 files changed, 439 insertions(+), 7 deletions(-)
diff --git a/include/efi_log.h b/include/efi_log.h index 1988e5f9df0..a4b707e57c5 100644 --- a/include/efi_log.h +++ b/include/efi_log.h @@ -16,6 +16,11 @@ * enum efil_tag - Types of logging records which can be created */ enum efil_tag { + EFILT_ALLOCATE_PAGES, + EFILT_FREE_PAGES, + EFILT_ALLOCATE_POOL, + EFILT_FREE_POOL, + EFILT_TESTING,
EFILT_COUNT, @@ -80,6 +85,28 @@ struct efil_allocate_pages { u64 e_memory; };
+/** struct efil_free_pages - holds info from efi_free_pages() call */ +struct efil_free_pages { + u64 memory; + efi_uintn_t pages; +}; + +/** struct efil_allocate_pool - holds info from efi_allocate_pool() call + * + * @e_buffer: Contains the value of *@buffer on return from the EFI function + */ +struct efil_allocate_pool { + enum efi_memory_type pool_type; + efi_uintn_t size; + void **buffer; + void *e_buffer; +}; + +/** struct efil_free_pool - holds log-info from efi_free_pool() call */ +struct efil_free_pool { + void *buffer; +}; + /* * The functions below are in pairs, with a 'start' and 'end' call for each EFI * function. The 'start' function (efi_logs_...) is called when the function is @@ -123,8 +150,128 @@ int efi_logs_testing(enum efil_test_t enum_val, efi_uintn_t int_value, */ int efi_loge_testing(int ofs, efi_status_t efi_ret);
+/** + * efi_logs_allocate_pages() - Record a call to efi_allocate_pages() + * + * @type: type of allocation to be performed + * @memory_type: usage type of the allocated memory + * @pages: number of pages to be allocated + * @memory: place to write address of allocated memory + * Return: log-offset of this new record, or -ve error code + */ +int efi_logs_allocate_pages(enum efi_allocate_type type, + enum efi_memory_type memory_type, efi_uintn_t pages, + u64 *memory); + +/** + * efi_loge_allocate_pages() - Record a return from efi_allocate_pages() + * + * This stores the value of the memory pointer also + * + * ofs: Offset of the record to end + * efi_ret: status code to record + */ +int efi_loge_allocate_pages(int ofs, efi_status_t efi_ret); + +/** + * efi_logs_free_pages() - Record a call to efi_free_pages() + * + * @memory: start of the memory area to be freed + * @pages: number of pages to be freed + * Return: log-offset of this new record, or -ve error code + */ +int efi_logs_free_pages(u64 memory, efi_uintn_t pages); + +/** + * efi_loge_free_pages() - Record a return from efi_free_pages() + * + * ofs: Offset of the record to end + * efi_ret: status code to record + */ +int efi_loge_free_pages(int ofs, efi_status_t efi_ret); + +/** + * efi_logs_allocate_pool() - Record a call to efi_allocate_pool() + * + * @pool_type: type of the pool from which memory is to be allocated + * @size: number of bytes to be allocated + * @buffer: place to hold pointer to allocated memory + * Return: log-offset of this new record, or -ve error code + */ +int efi_logs_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, + void **buffer); + +/** + * efi_loge_allocate_pool() - Record a return from efi_allocate_pool() + * + * This stores the value of the buffer pointer also + * + * ofs: Offset of the record to end + * efi_ret: status code to record + */ +int efi_loge_allocate_pool(int ofs, efi_status_t efi_ret); + +/** + * efi_logs_free_pool() - Record a call to efi_free_pool() + * + * @buffer: start of memory to be freed + * Return: log-offset of this new record, or -ve error code + */ +int efi_logs_free_pool(void *buffer); + +/** + * efi_loge_free_pool() - Record a return from efi_free_pool() + * + * ofs: Offset of the record to end + * efi_ret: status code to record + */ +int efi_loge_free_pool(int ofs, efi_status_t efi_ret); + #else /* !EFI_LOG */
+static inline int efi_logs_allocate_pages(enum efi_allocate_type type, + enum efi_memory_type memory_type, + efi_uintn_t pages, u64 *memory) +{ + return -ENOSYS; +} + +static inline int efi_loge_allocate_pages(int ofs, efi_status_t efi_ret) +{ + return -ENOSYS; +} + +static inline int efi_logs_free_pages(u64 memory, efi_uintn_t pages) +{ + return -ENOSYS; +} + +static inline int efi_loge_free_pages(int ofs, efi_status_t efi_ret) +{ + return -ENOSYS; +} + +static inline int efi_logs_allocate_pool(enum efi_memory_type pool_type, + efi_uintn_t size, void **buffer) +{ + return -ENOSYS; +} + +static inline int efi_loge_allocate_pool(int ofs, efi_status_t efi_ret) +{ + return -ENOSYS; +} + +static inline int efi_logs_free_pool(void *buffer) +{ + return -ENOSYS; +} + +static inline int efi_loge_free_pool(int ofs, efi_status_t efi_ret) +{ + return -ENOSYS; +} + static inline int efi_logs_testing(enum efil_test_t enum_val, efi_uintn_t int_value, void *buffer, u64 *memory) diff --git a/lib/efi_loader/efi_log.c b/lib/efi_loader/efi_log.c index 780860d8b2f..4b9c6727fe7 100644 --- a/lib/efi_loader/efi_log.c +++ b/lib/efi_loader/efi_log.c @@ -15,9 +15,41 @@
/* names for enum efil_tag (abbreviated to keep output to a single line) */ static const char *tag_name[EFILT_COUNT] = { + "alloc_pages", + "free_pages", + "alloc_pool", + "free_pool", + "testing", };
+/* names for enum efi_allocate_type */ +static const char *allocate_type_name[EFI_MAX_ALLOCATE_TYPE] = { + "any-pages", + "max-addr", + "alloc-addr", +}; + +/* names for enum efi_memory_type */ +static const char *memory_type_name[EFI_MAX_MEMORY_TYPE] = { + "reserved", + "ldr-code", + "ldr-data", + "bt-code", + "bt-data", + "rt-code", + "rt-data", + "convent", + "unusable", + "acpi-rec", + "acpi-nvs", + "mmap-io", + "mmap-iop", + "pal-code", + "persist", + "unaccept", +}; + /* names for error codes, trying to keep them short */ static const char *error_name[EFI_ERROR_COUNT] = { "OK", @@ -152,6 +184,119 @@ int efi_loge_testing(int ofs, efi_status_t efi_ret) return 0; }
+int efi_logs_allocate_pages(enum efi_allocate_type type, + enum efi_memory_type memory_type, efi_uintn_t pages, + u64 *memory) +{ + struct efil_allocate_pages *rec; + int ret; + + ret = prep_rec(EFILT_ALLOCATE_PAGES, sizeof(*rec), (void **)&rec); + if (ret < 0) + return ret; + + rec->type = type; + rec->memory_type = memory_type; + rec->pages = pages; + rec->memory = memory; + rec->e_memory = 0; + + return ret; +} + +int efi_loge_allocate_pages(int ofs, efi_status_t efi_ret) +{ + struct efil_allocate_pages *rec; + + rec = finish_rec(ofs, efi_ret); + if (!rec) + return -ENOSPC; + rec->e_memory = *rec->memory; + + return 0; +} + +int efi_logs_free_pages(u64 memory, efi_uintn_t pages) +{ + struct efil_free_pages *rec; + int ret; + + ret = prep_rec(EFILT_FREE_PAGES, sizeof(*rec), (void **)&rec); + if (ret < 0) + return ret; + + rec->memory = memory; + rec->pages = pages; + + return ret; +} + +int efi_loge_free_pages(int ofs, efi_status_t efi_ret) +{ + struct efil_allocate_pages *rec; + + rec = finish_rec(ofs, efi_ret); + if (!rec) + return -ENOSPC; + + return 0; +} + +int efi_logs_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, + void **buffer) +{ + struct efil_allocate_pool *rec; + int ret; + + ret = prep_rec(EFILT_ALLOCATE_POOL, sizeof(*rec), (void **)&rec); + if (ret < 0) + return ret; + + rec->pool_type = pool_type; + rec->size = size; + rec->buffer = buffer; + rec->e_buffer = NULL; + + return ret; +} + +int efi_loge_allocate_pool(int ofs, efi_status_t efi_ret) +{ + struct efil_allocate_pool *rec; + + rec = finish_rec(ofs, efi_ret); + if (!rec) + return -ENOSPC; + rec->e_buffer = *rec->buffer; + + return 0; +} + +int efi_logs_free_pool(void *buffer) +{ + struct efil_free_pool *rec; + int ret; + + ret = prep_rec(EFILT_FREE_POOL, sizeof(*rec), (void **)&rec); + if (ret < 0) + return ret; + + rec->buffer = buffer; + + return ret; +} + +int efi_loge_free_pool(int ofs, efi_status_t efi_ret) +{ + struct efil_free_pool *rec; + + rec = finish_rec(ofs, efi_ret); + if (!rec) + return -ENOSPC; + + return 0; +} + static void show_enum(const char *type_name[], int type) { printf("%s ", type_name[type]); @@ -187,6 +332,50 @@ void show_rec(int seq, struct efil_rec_hdr *rec_hdr)
printf("%3d %12s ", seq, tag_name[rec_hdr->tag]); switch (rec_hdr->tag) { + case EFILT_ALLOCATE_PAGES: { + struct efil_allocate_pages *rec = start; + + show_enum(allocate_type_name, rec->type); + show_enum(memory_type_name, rec->memory_type); + show_ulong("pgs", (ulong)rec->pages); + show_addr("mem", (ulong)rec->memory); + if (rec_hdr->ended) { + show_addr("*mem", + (ulong)map_to_sysmem((void *)rec->e_memory)); + show_ret(rec_hdr->e_ret); + } + break; + } + case EFILT_FREE_PAGES: { + struct efil_free_pages *rec = start; + + show_addr("mem", map_to_sysmem((void *)rec->memory)); + show_ulong("pag", (ulong)rec->pages); + if (rec_hdr->ended) + show_ret(rec_hdr->e_ret); + break; + } + case EFILT_ALLOCATE_POOL: { + struct efil_allocate_pool *rec = start; + + show_enum(memory_type_name, rec->pool_type); + show_ulong("size", (ulong)rec->size); + show_addr("buf", (ulong)rec->buffer); + if (rec_hdr->ended) { + show_addr("*buf", + (ulong)map_to_sysmem((void *)rec->e_buffer)); + show_ret(rec_hdr->e_ret); + } + break; + } + case EFILT_FREE_POOL: { + struct efil_free_pool *rec = start; + + show_addr("buf", map_to_sysmem(rec->buffer)); + if (rec_hdr->ended) + show_ret(rec_hdr->e_ret); + break; + } case EFILT_TESTING: { struct efil_testing *rec = start;
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 5a6794652d1..d77243e54b2 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -8,6 +8,7 @@ #define LOG_CATEGORY LOGC_EFI
#include <efi_loader.h> +#include <efi_log.h> #include <init.h> #include <lmb.h> #include <log.h> @@ -429,9 +430,9 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) return EFI_NOT_FOUND; }
-efi_status_t efi_allocate_pages(enum efi_allocate_type type, - enum efi_memory_type mem_type, - efi_uintn_t pages, uint64_t *memoryp) +static efi_status_t efi_allocate_pages_(enum efi_allocate_type type, + enum efi_memory_type mem_type, + efi_uintn_t pages, uint64_t *memoryp) { u64 len; uint flags; @@ -491,7 +492,21 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, return EFI_SUCCESS; }
-efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) +efi_status_t efi_allocate_pages(enum efi_allocate_type type, + enum efi_memory_type memory_type, + efi_uintn_t pages, uint64_t *memory) +{ + efi_status_t ret; + int ofs; + + ofs = efi_logs_allocate_pages(type, memory_type, pages, memory); + ret = efi_allocate_pages_(type, memory_type, pages, memory); + efi_loge_allocate_pages(ofs, ret); + + return ret; +} + +static efi_status_t efi_free_pages_(uint64_t memory, efi_uintn_t pages) { u64 len; long status; @@ -516,6 +531,18 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) return ret; }
+efi_status_t efi_free_pages(u64 memory, efi_uintn_t pages) +{ + efi_status_t ret; + int ofs; + + ofs = efi_logs_free_pages(memory, pages); + ret = efi_free_pages_(memory, pages); + efi_loge_free_pages(ofs, ret); + + return ret; +} + void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, size_t align) { @@ -561,8 +588,8 @@ void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, return map_sysmem(aligned_mem, len); }
-efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, - void **buffer) +static efi_status_t efi_allocate_pool_(enum efi_memory_type mem_type, + efi_uintn_t size, void **buffer) { efi_status_t r; u64 addr; @@ -590,6 +617,19 @@ efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, return r; }
+efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, + void **buffer) +{ + efi_status_t ret; + int ofs; + + ofs = efi_logs_allocate_pool(pool_type, size, buffer); + ret = efi_allocate_pool_(pool_type, size, buffer); + efi_loge_allocate_pool(ofs, ret); + + return ret; +} + void *efi_alloc(size_t size) { void *buf; @@ -604,7 +644,7 @@ void *efi_alloc(size_t size) return buf; }
-efi_status_t efi_free_pool(void *buffer) +static efi_status_t efi_free_pool_(void *buffer) { efi_status_t ret; struct efi_pool_allocation *alloc; @@ -632,6 +672,18 @@ efi_status_t efi_free_pool(void *buffer) return ret; }
+efi_status_t efi_free_pool(void *buffer) +{ + efi_status_t ret; + int ofs; + + ofs = efi_logs_free_pool(buffer); + ret = efi_free_pool_(buffer); + efi_loge_free_pool(ofs, ret); + + return ret; +} + /** * efi_get_memory_map() - get map describing memory usage. * diff --git a/test/lib/efi_log.c b/test/lib/efi_log.c index 414b7081bd9..7c8f1cbcd06 100644 --- a/test/lib/efi_log.c +++ b/test/lib/efi_log.c @@ -47,3 +47,47 @@ static int lib_test_efi_log_base(struct unit_test_state *uts) return 0; } LIB_TEST(lib_test_efi_log_base, UTF_CONSOLE); + +/* test the memory-function logging */ +static int lib_test_efi_log_mem(struct unit_test_state *uts) +{ + void **buf = map_sysmem(0x1000, 0); + u64 *addr = map_sysmem(0x1010, 0); + int ofs1, ofs2; + + ut_assertok(efi_log_reset()); + + ofs1 = efi_logs_allocate_pool(EFI_BOOT_SERVICES_DATA, 100, buf); + ofs2 = efi_logs_allocate_pages(EFI_ALLOCATE_ANY_PAGES, + EFI_BOOT_SERVICES_CODE, 10, addr); + ut_assertok(efi_loge_allocate_pages(ofs2, EFI_LOAD_ERROR)); + ut_assertok(efi_loge_allocate_pool(ofs1, 0)); + + ofs1 = efi_logs_free_pool(*buf); + ut_assertok(efi_loge_free_pool(ofs1, EFI_INVALID_PARAMETER)); + + ofs2 = efi_logs_free_pages(*addr, 0); + ut_assertok(efi_loge_free_pool(ofs2, 0)); + + ut_assertok(efi_log_show()); + + ut_assert_nextline("EFI log (size c0)"); + + /* + * We end up with internal sandbox-addresses here since EFI_LOADER + * doesn't handle map_sysmem() correctly. So for now, only part of the + * string is matched. + */ + ut_assert_nextlinen(" 0 alloc_pool bt-data size 64/100 buf 10002000 *buf"); + ut_assert_nextlinen(" 1 alloc_pages any-pages bt-code pgs a/10 mem 10002010 *mem"); + ut_assert_nextlinen(" 2 free_pool buf"); + ut_assert_nextlinen(" 3 free_pages mem"); + + ut_assert_nextline("4 records"); + + unmap_sysmem(buf); + unmap_sysmem(addr); + + return 0; +} +LIB_TEST(lib_test_efi_log_mem, UTF_CONSOLE);
Add a note in the documentation about this feature.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Rebase on efil series (memory address/pointers)
doc/develop/uefi/uefi.rst | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index 48d6110b2ad..23d7dfc166d 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -1004,6 +1004,28 @@ This driver is only available if U-Boot is configured with:: CONFIG_BLK=y CONFIG_PARTITIONS=y
+Logging +------- + +The EFI subsystem supports logging of boot-services calls to a memory area. The +log can then be shown using the ``efidebug log`` command. Note that only a few +calls are logged at present. + +To enable this feature:: + + CONFIG_EFI_LOG=y + CONFIG_EFI_LOG_SIZE=0x4000 + CONFIG_BLOBLIST_SIZE_RELOC=0x5000 + +The bloblist size must be set to larger than the EFI size. For now the log size +is fixed at the start. + +Logging is performed within the internal U-Boot functions, not at the +boot-services interface. This makes it possible to log EFI calls made by U-Boot +itself, such as memory allocations. + +See the :doc:`/usage/cmd/efidebug` (*log* subcommand) for more information. + Miscellaneous -------------
Am 2. Dezember 2024 01:12:36 MEZ schrieb Simon Glass sjg@chromium.org:
It is a bit of a pain to log EFI boot-services calls at present. The output goes to the console so cannot easily be inspected later. Also it would be useful to be able to store the log and review it later, perhaps after something has gone wrong.
This series makes a start on implementing a log-to-buffer feature. It provides a simple 'efidebug log' command to inspect the buffer. For now, only memory allocations are logged.
In my previous review I suggested to move your work to the level of the logging library lib/log.c.
Why are you reiterating your series without exposing why an EFI specific solution. should be desireable?
Best regards
Heinrich
This feature makes it possible to add tests to check which EFI calls are made by U-Boot itself. It may also make it easier to figure out what is needed for booting Windows.
Some patches to help with debugging sandbox memory-mapping are included, but for now EFI's use of mapping is not adjusted.
Changes in v2:
- Rebase on efil series (memory address/pointers)
Simon Glass (7): efi_loader: Fix free in ..._media_device_boot_option() efi_loader: Add support for logging EFI calls efi_loader: Create the log on startup efi_loader: Add a command to show the EFI log test: efi_loader: Add a simple test for the EFI log efi_loader: Use the log with memory-related functions efi_loader: Add documentation for the EFI log
MAINTAINERS | 7 + cmd/efidebug.c | 53 ++++- configs/sandbox_defconfig | 3 + doc/develop/uefi/uefi.rst | 22 ++ doc/usage/cmd/efidebug.rst | 109 +++++++++ doc/usage/index.rst | 1 + include/bloblist.h | 1 + include/efi.h | 1 + include/efi_log.h | 316 +++++++++++++++++++++++++ lib/efi_loader/Kconfig | 19 ++ lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_bootmgr.c | 3 +- lib/efi_loader/efi_log.c | 444 +++++++++++++++++++++++++++++++++++ lib/efi_loader/efi_memory.c | 66 +++++- lib/efi_loader/efi_setup.c | 7 + test/lib/Makefile | 1 + test/lib/efi_log.c | 93 ++++++++ 17 files changed, 1131 insertions(+), 16 deletions(-) create mode 100644 doc/usage/cmd/efidebug.rst create mode 100644 include/efi_log.h create mode 100644 lib/efi_loader/efi_log.c create mode 100644 test/lib/efi_log.c
On Mon, Dec 02, 2024 at 01:24:27AM +0100, Heinrich Schuchardt wrote:
Am 2. Dezember 2024 01:12:36 MEZ schrieb Simon Glass sjg@chromium.org:
It is a bit of a pain to log EFI boot-services calls at present. The output goes to the console so cannot easily be inspected later. Also it would be useful to be able to store the log and review it later, perhaps after something has gone wrong.
This series makes a start on implementing a log-to-buffer feature. It provides a simple 'efidebug log' command to inspect the buffer. For now, only memory allocations are logged.
In my previous review I suggested to move your work to the level of the logging library lib/log.c.
Why are you reiterating your series without exposing why an EFI specific solution. should be desireable?
Even with https://lore.kernel.org/u-boot/CAFLszTgE_HZEbG7B09_oo=XfugvH07ix=_0eoSGaEQ+K... it is unclear why this patch series rather than either/both: - Expand CONFIG_LOG to have a "file" or "bloblist" option - Use CONFIG_TRACE
Do not match your needs. Especially the second option.
Hi Tom,
On Mon, 2 Dec 2024 at 07:16, Tom Rini trini@konsulko.com wrote:
On Mon, Dec 02, 2024 at 01:24:27AM +0100, Heinrich Schuchardt wrote:
Am 2. Dezember 2024 01:12:36 MEZ schrieb Simon Glass sjg@chromium.org:
It is a bit of a pain to log EFI boot-services calls at present. The output goes to the console so cannot easily be inspected later. Also it would be useful to be able to store the log and review it later, perhaps after something has gone wrong.
This series makes a start on implementing a log-to-buffer feature. It provides a simple 'efidebug log' command to inspect the buffer. For now, only memory allocations are logged.
In my previous review I suggested to move your work to the level of the logging library lib/log.c.
Why are you reiterating your series without exposing why an EFI specific solution. should be desireable?
Even with https://lore.kernel.org/u-boot/CAFLszTgE_HZEbG7B09_oo=XfugvH07ix=_0eoSGaEQ+K... it is unclear why this patch series rather than either/both:
- Expand CONFIG_LOG to have a "file" or "bloblist" option
- Use CONFIG_TRACE
Do not match your needs. Especially the second option.
From my side I'd like to change the conversation a little, to how to
land code, rather than why we should bother. "Code needs to land" should be the motto. If someone has taken the time to create something, our bias should be towards getting it in, with sufficient changes to make it fit the project. There are cases where something is just a bad idea, or should be done another way, but for people working on major features or changes, biasing towards not landing the code is just going to make them go elsewhere.
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. CONFIG_TRACE - ick, you want me to write a test which does a trace and then scans it to see what happened? The trace doesn't even include function arguments...
Regards, Simon
On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote:
From my side I'd like to change the conversation a little, to how to land code, rather than why we should bother. "Code needs to land" should be the motto. If someone has taken the time to create something, our bias should be towards getting it in, with sufficient changes to make it fit the project. There are cases where something is just a bad idea, or should be done another way, but for people working on major features or changes, biasing towards not landing the code is just going to make them go elsewhere.
This is the wrong approach I believe. The goal has always been and continues to be to have reviewed (whenever possible, our developer community is small) incremental change over time. Just because something has been posted a number of times does not mean it's ready to be merged.
Your challenge today is that on patchwork you have over 150 patches covering a wide variety of topics and of which many series have technically-merited feedback that needs to be addressed in a technical manner.
Hi Tom,
On Tue, 3 Dec 2024 at 18:29, Tom Rini trini@konsulko.com wrote:
On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote:
From my side I'd like to change the conversation a little, to how to land code, rather than why we should bother. "Code needs to land" should be the motto. If someone has taken the time to create something, our bias should be towards getting it in, with sufficient changes to make it fit the project. There are cases where something is just a bad idea, or should be done another way, but for people working on major features or changes, biasing towards not landing the code is just going to make them go elsewhere.
This is the wrong approach I believe. The goal has always been and continues to be to have reviewed (whenever possible, our developer community is small) incremental change over time.
Yes, I agree with that, but it isn't what I said.
Just because something has been posted a number of times does not mean it's ready to be merged.
I didn't say that either.
Your challenge today is that on patchwork you have over 150 patches covering a wide variety of topics and of which many series have technically-merited feedback that needs to be addressed in a technical manner.
By my count I have about 10 series in progress, with a small number of patches (< 10?) with pending feedback that isn't effectively just a NAK. It isn't a particularly large number, if you add up the patches I do in each cycle. It is in the nature of development and code review that things are often not right the first time, or someone else has another perspective, so I cannot see how this can be reduced.
Regards, Simon
On Wed, Dec 04, 2024 at 08:13:04AM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 3 Dec 2024 at 18:29, Tom Rini trini@konsulko.com wrote:
On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote:
From my side I'd like to change the conversation a little, to how to land code, rather than why we should bother. "Code needs to land" should be the motto. If someone has taken the time to create something, our bias should be towards getting it in, with sufficient changes to make it fit the project. There are cases where something is just a bad idea, or should be done another way, but for people working on major features or changes, biasing towards not landing the code is just going to make them go elsewhere.
This is the wrong approach I believe. The goal has always been and continues to be to have reviewed (whenever possible, our developer community is small) incremental change over time.
Yes, I agree with that, but it isn't what I said.
I don't know how else to interpret "Code needs to land". I'm not sure what reviewed patches we have that are outstanding, but also not fairly new. We do have patches outstanding, in general. Much of those fall in to the categories of: - Custodian is active, but also very busy (virtually everyone is a volunteer so I have a hard time getting forceful with people that have a large queue). - Custodian isn't active / no direct custodian and the code hasn't been reviewed by anyone else.
Just because something has been posted a number of times does not mean it's ready to be merged.
I didn't say that either.
It's an impression you give however when you repost a series less than a day after last posting, without addressing all of the feedback.
Your challenge today is that on patchwork you have over 150 patches covering a wide variety of topics and of which many series have technically-merited feedback that needs to be addressed in a technical manner.
By my count I have about 10 series in progress, with a small number of patches (< 10?) with pending feedback
I'm not sure about that less than 10 number, in part because it's hard to keep track of which feedback was applied, and which not. And part because some of those series are places where I told you I'm unsure about the core concept but you asked and I'm giving you leeway to show me the end result.
that isn't effectively just a NAK.
To be clear, "just a NAK" isn't the whole story. Those NAKs come with technical rationales and requests. But maybe they're just lost in the volume of emails you have in some cases. I know for example I mentioned a few times and places that we have tests today for booting the OS, when you mention that we need to add a test for booting the OS. We need to expand that test, yes.
It isn't a particularly large number, if you add up the patches I do in each cycle. It is in the nature of development and code review that things are often not right the first time, or someone else has another perspective, so I cannot see how this can be reduced.
The easy way to reduce it would be to go from 10 series in progress to 1 series in progress, and finish that before picking up series number 2, and so on. And then in the future, stop when you have more than 2 or 3 in progress at once.
But really, it feels like "Code needs to land" is another way of saying "Just a NAK should be ignored".
Hi Tom,
On Wed, 4 Dec 2024 at 09:27, Tom Rini trini@konsulko.com wrote:
On Wed, Dec 04, 2024 at 08:13:04AM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 3 Dec 2024 at 18:29, Tom Rini trini@konsulko.com wrote:
On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote:
From my side I'd like to change the conversation a little, to how to land code, rather than why we should bother. "Code needs to land" should be the motto. If someone has taken the time to create something, our bias should be towards getting it in, with sufficient changes to make it fit the project. There are cases where something is just a bad idea, or should be done another way, but for people working on major features or changes, biasing towards not landing the code is just going to make them go elsewhere.
This is the wrong approach I believe. The goal has always been and continues to be to have reviewed (whenever possible, our developer community is small) incremental change over time.
Yes, I agree with that, but it isn't what I said.
I don't know how else to interpret "Code needs to land". I'm not sure what reviewed patches we have that are outstanding, but also not fairly new. We do have patches outstanding, in general. Much of those fall in to the categories of:
- Custodian is active, but also very busy (virtually everyone is a volunteer so I have a hard time getting forceful with people that have a large queue).
- Custodian isn't active / no direct custodian and the code hasn't been reviewed by anyone else.
Here's my intention with 'code needs to land': to bias against people saying "I don't like this; I don't care about your use case; please go away"
Just because something has been posted a number of times does not mean it's ready to be merged.
I didn't say that either.
It's an impression you give however when you repost a series less than a day after last posting, without addressing all of the feedback.
I'm going to ignore that comment as it is not helpful and mischaracterises my contributions.
Your challenge today is that on patchwork you have over 150 patches covering a wide variety of topics and of which many series have technically-merited feedback that needs to be addressed in a technical manner.
By my count I have about 10 series in progress, with a small number of patches (< 10?) with pending feedback
I'm not sure about that less than 10 number, in part because it's hard to keep track of which feedback was applied, and which not. And part because some of those series are places where I told you I'm unsure about the core concept but you asked and I'm giving you leeway to show me the end result.
OK
that isn't effectively just a NAK.
To be clear, "just a NAK" isn't the whole story. Those NAKs come with technical rationales and requests. But maybe they're just lost in the volume of emails you have in some cases. I know for example I mentioned a few times and places that we have tests today for booting the OS, when you mention that we need to add a test for booting the OS. We need to expand that test, yes.
If you look at the OF_BLOBLIST thing, it is basically a NAK. I don't see any other way to interpret it,. So several boards in my lab have been broken for a year.
It isn't a particularly large number, if you add up the patches I do in each cycle. It is in the nature of development and code review that things are often not right the first time, or someone else has another perspective, so I cannot see how this can be reduced.
The easy way to reduce it would be to go from 10 series in progress to 1 series in progress, and finish that before picking up series number 2, and so on. And then in the future, stop when you have more than 2 or 3 in progress at once.
That's obviously not practical for the amount I am contributing and the length of time it takes us to get things in. I hope it isn't a serious suggestion!
But really, it feels like "Code needs to land" is another way of saying "Just a NAK should be ignored".
Yes, a bias more towards that would be more healthy IMO. A NAK needs to come with a full understanding of the use case, an alternative way to meet the use case, once that doesn't involve boiling the Pacific Ocean.
Regards, Simon
On Fri, Dec 06, 2024 at 12:19:38PM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 4 Dec 2024 at 09:27, Tom Rini trini@konsulko.com wrote:
On Wed, Dec 04, 2024 at 08:13:04AM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 3 Dec 2024 at 18:29, Tom Rini trini@konsulko.com wrote:
On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote:
From my side I'd like to change the conversation a little, to how to land code, rather than why we should bother. "Code needs to land" should be the motto. If someone has taken the time to create something, our bias should be towards getting it in, with sufficient changes to make it fit the project. There are cases where something is just a bad idea, or should be done another way, but for people working on major features or changes, biasing towards not landing the code is just going to make them go elsewhere.
This is the wrong approach I believe. The goal has always been and continues to be to have reviewed (whenever possible, our developer community is small) incremental change over time.
Yes, I agree with that, but it isn't what I said.
I don't know how else to interpret "Code needs to land". I'm not sure what reviewed patches we have that are outstanding, but also not fairly new. We do have patches outstanding, in general. Much of those fall in to the categories of:
- Custodian is active, but also very busy (virtually everyone is a volunteer so I have a hard time getting forceful with people that have a large queue).
- Custodian isn't active / no direct custodian and the code hasn't been reviewed by anyone else.
Here's my intention with 'code needs to land': to bias against people saying "I don't like this; I don't care about your use case; please go away"
To me that applies an arbitrary nature to patches being rejected, which I do not see as being the case.
Just because something has been posted a number of times does not mean it's ready to be merged.
I didn't say that either.
It's an impression you give however when you repost a series less than a day after last posting, without addressing all of the feedback.
I'm going to ignore that comment as it is not helpful and mischaracterises my contributions.
I stand by it, and will let others judge if that's the impression they have, or not.
Your challenge today is that on patchwork you have over 150 patches covering a wide variety of topics and of which many series have technically-merited feedback that needs to be addressed in a technical manner.
By my count I have about 10 series in progress, with a small number of patches (< 10?) with pending feedback
I'm not sure about that less than 10 number, in part because it's hard to keep track of which feedback was applied, and which not. And part because some of those series are places where I told you I'm unsure about the core concept but you asked and I'm giving you leeway to show me the end result.
OK
that isn't effectively just a NAK.
To be clear, "just a NAK" isn't the whole story. Those NAKs come with technical rationales and requests. But maybe they're just lost in the volume of emails you have in some cases. I know for example I mentioned a few times and places that we have tests today for booting the OS, when you mention that we need to add a test for booting the OS. We need to expand that test, yes.
If you look at the OF_BLOBLIST thing, it is basically a NAK. I don't see any other way to interpret it,. So several boards in my lab have been broken for a year.
To be clear, you want to revert: commit 70fe23859437ffe4efe0793423937d8b78ebf9d6 Author: Simon Glass sjg@chromium.org AuthorDate: Wed Jan 3 18:49:19 2024 -0700 Commit: Simon Glass sjg@chromium.org CommitDate: Sun Jan 7 13:45:07 2024 -0700
fdt: Allow the devicetree to come from a bloblist
Standard passage provides for a bloblist to be passed from one firmware phase to the next. That can be used to pass the devicetree along as well. Add an option to support this.
Tests for this will be added as part of the Universal Payload work.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Which means that you did not test your changes on your hardware at the time? And wish to (seemingly) replace that with an earlier version of the patch which was objected to on technical grounds at the time. Without explaining why the current code does not function on these platforms, only that it does not, for entirely unclear reasons.
So I do not think I've arbitrarily nak'd the revert you posted, I've been asking you to explain why the code doesn't work. And I still don't have an answer as to why after at run time we don't find a valid bloblist here the fall-back use cases aren't functioning. To me that seems like the bug to investigate and explain. Reverting code to find when functionality broke is great, I do it all the time. Saying we need to revert because something broke *without* saying why it broke is a problem.
It isn't a particularly large number, if you add up the patches I do in each cycle. It is in the nature of development and code review that things are often not right the first time, or someone else has another perspective, so I cannot see how this can be reduced.
The easy way to reduce it would be to go from 10 series in progress to 1 series in progress, and finish that before picking up series number 2, and so on. And then in the future, stop when you have more than 2 or 3 in progress at once.
That's obviously not practical for the amount I am contributing and the length of time it takes us to get things in. I hope it isn't a serious suggestion!
It's extremely serious. For example https://patchwork.ozlabs.org/project/uboot/list/?series=435516&state=* is a 46 part patch series that touches 3 or 4 different areas and the only type of review that's feasible is "Does this cause the binary to grow too much? Or in unexpected ways?" And that sets aside that I believe I did ask you to rework "bootm: Allow building bootm.c without CONFIG_SYS_BOOTM_LEN" to refactor things so that SYS_BOOTM_LEN is always defined. That's not even a hard change, I say as someone that had to do that as part of Kconfig migration of oddly (ab)used symbol names.
But in this case, your series that touch areas of code actively maintained by other people is even more relevant. Whereas areas without other active developers, or where you are the main developer, I can find some smaller limits on what must be reviewed all of your *EFI* has other active developers on it. Who are providing technical feedback, not arbitrary or stylistic feedback. Please be explicit about what you are suggesting in this case, so I do not mischaracterize your intention.
But really, it feels like "Code needs to land" is another way of saying "Just a NAK should be ignored".
Yes, a bias more towards that would be more healthy IMO. A NAK needs to come with a full understanding of the use case, an alternative way to meet the use case, once that doesn't involve boiling the Pacific Ocean.
As it stands, I do not like to, but sometimes feel I *have*to* break one board to unbreak another board and keep reminding both parties to resolve the issue so no board is broken. There has been, since I have taken over the lead of the project, at least once where I have over-ruled the relevant custodian and that has resulted in the custodian leaving. I both feel that was the right technical call, and regret it came to that, still. I do not want to adopt a more broad use of that policy. The default for absence of agreement must be to try again.
Hi Tom,
On Fri, 6 Dec 2024 at 13:12, Tom Rini trini@konsulko.com wrote:
On Fri, Dec 06, 2024 at 12:19:38PM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 4 Dec 2024 at 09:27, Tom Rini trini@konsulko.com wrote:
On Wed, Dec 04, 2024 at 08:13:04AM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 3 Dec 2024 at 18:29, Tom Rini trini@konsulko.com wrote:
On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote:
From my side I'd like to change the conversation a little, to how to land code, rather than why we should bother. "Code needs to land" should be the motto. If someone has taken the time to create something, our bias should be towards getting it in, with sufficient changes to make it fit the project. There are cases where something is just a bad idea, or should be done another way, but for people working on major features or changes, biasing towards not landing the code is just going to make them go elsewhere.
This is the wrong approach I believe. The goal has always been and continues to be to have reviewed (whenever possible, our developer community is small) incremental change over time.
Yes, I agree with that, but it isn't what I said.
I don't know how else to interpret "Code needs to land". I'm not sure what reviewed patches we have that are outstanding, but also not fairly new. We do have patches outstanding, in general. Much of those fall in to the categories of:
- Custodian is active, but also very busy (virtually everyone is a volunteer so I have a hard time getting forceful with people that have a large queue).
- Custodian isn't active / no direct custodian and the code hasn't been reviewed by anyone else.
Here's my intention with 'code needs to land': to bias against people saying "I don't like this; I don't care about your use case; please go away"
To me that applies an arbitrary nature to patches being rejected, which I do not see as being the case.
Just because something has been posted a number of times does not mean it's ready to be merged.
I didn't say that either.
It's an impression you give however when you repost a series less than a day after last posting, without addressing all of the feedback.
I'm going to ignore that comment as it is not helpful and mischaracterises my contributions.
I stand by it, and will let others judge if that's the impression they have, or not.
I'll put the relevant threads at [1], ditto.
Your challenge today is that on patchwork you have over 150 patches covering a wide variety of topics and of which many series have technically-merited feedback that needs to be addressed in a technical manner.
By my count I have about 10 series in progress, with a small number of patches (< 10?) with pending feedback
I'm not sure about that less than 10 number, in part because it's hard to keep track of which feedback was applied, and which not. And part because some of those series are places where I told you I'm unsure about the core concept but you asked and I'm giving you leeway to show me the end result.
OK
that isn't effectively just a NAK.
To be clear, "just a NAK" isn't the whole story. Those NAKs come with technical rationales and requests. But maybe they're just lost in the volume of emails you have in some cases. I know for example I mentioned a few times and places that we have tests today for booting the OS, when you mention that we need to add a test for booting the OS. We need to expand that test, yes.
If you look at the OF_BLOBLIST thing, it is basically a NAK. I don't see any other way to interpret it,. So several boards in my lab have been broken for a year.
To be clear, you want to revert: commit 70fe23859437ffe4efe0793423937d8b78ebf9d6 Author: Simon Glass sjg@chromium.org AuthorDate: Wed Jan 3 18:49:19 2024 -0700 Commit: Simon Glass sjg@chromium.org CommitDate: Sun Jan 7 13:45:07 2024 -0700
fdt: Allow the devicetree to come from a bloblist Standard passage provides for a bloblist to be passed from one firmware phase to the next. That can be used to pass the devicetree along as well. Add an option to support this. Tests for this will be added as part of the Universal Payload work. Signed-off-by: Simon Glass <sjg@chromium.org> Reviewed-by: Tom Rini <trini@konsulko.com> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>Which means that you did not test your changes on your hardware at the time? And wish to (seemingly) replace that with an earlier version of the patch which was objected to on technical grounds at the time. Without explaining why the current code does not function on these platforms, only that it does not, for entirely unclear reasons.
It never worked on my board, though. I spent 6 revisions trying to explain that. After number 6 [2] you got in touch and said that I needed to rework it, which I dutifully did, in version 7 [3]. I did that purely to unblock Raymond's series. I even put that in the patch:
"The discussion on this was not resolved and is now important due to the bloblist series from Raymond. So I am sending it again since I believe this is a better starting point than building on OF_BOARD"
Are you now saying I should have refused and argued for another few revisions? Patience does have its limits.
So I do not think I've arbitrarily nak'd the revert you posted, I've been asking you to explain why the code doesn't work. And I still don't have an answer as to why after at run time we don't find a valid bloblist here the fall-back use cases aren't functioning. To me that seems like the bug to investigate and explain. Reverting code to find when functionality broke is great, I do it all the time. Saying we need to revert because something broke *without* saying why it broke is a problem.
It isn't a particularly large number, if you add up the patches I do in each cycle. It is in the nature of development and code review that things are often not right the first time, or someone else has another perspective, so I cannot see how this can be reduced.
The easy way to reduce it would be to go from 10 series in progress to 1 series in progress, and finish that before picking up series number 2, and so on. And then in the future, stop when you have more than 2 or 3 in progress at once.
That's obviously not practical for the amount I am contributing and the length of time it takes us to get things in. I hope it isn't a serious suggestion!
It's extremely serious. For example https://patchwork.ozlabs.org/project/uboot/list/?series=435516&state=* is a 46 part patch series that touches 3 or 4 different areas and the only type of review that's feasible is "Does this cause the binary to grow too much? Or in unexpected ways?" And that sets aside that I believe I did ask you to rework "bootm: Allow building bootm.c without CONFIG_SYS_BOOTM_LEN" to refactor things so that SYS_BOOTM_LEN is always defined. That's not even a hard change, I say as someone that had to do that as part of Kconfig migration of oddly (ab)used symbol names.
But in this case, your series that touch areas of code actively maintained by other people is even more relevant. Whereas areas without other active developers, or where you are the main developer, I can find some smaller limits on what must be reviewed all of your *EFI* has other active developers on it. Who are providing technical feedback, not arbitrary or stylistic feedback. Please be explicit about what you are suggesting in this case, so I do not mischaracterize your intention.
But really, it feels like "Code needs to land" is another way of saying "Just a NAK should be ignored".
Yes, a bias more towards that would be more healthy IMO. A NAK needs to come with a full understanding of the use case, an alternative way to meet the use case, once that doesn't involve boiling the Pacific Ocean.
As it stands, I do not like to, but sometimes feel I *have*to* break one board to unbreak another board and keep reminding both parties to resolve the issue so no board is broken. There has been, since I have taken over the lead of the project, at least once where I have over-ruled the relevant custodian and that has resulted in the custodian leaving. I both feel that was the right technical call, and regret it came to that, still. I do not want to adopt a more broad use of that policy. The default for absence of agreement must be to try again.
I see that Peter sent a few indignant emails. You can't win.
Regards, Simon
-- Tom
[1] https://patchwork.ozlabs.org/project/uboot/patch/20230830180524.315916-31-sj... https://patchwork.ozlabs.org/project/uboot/patch/20230921015730.1511373-31-s... https://patchwork.ozlabs.org/project/uboot/patch/20230924192536.1812799-37-s... https://patchwork.ozlabs.org/project/uboot/patch/20230926141514.2101787-40-s... https://patchwork.ozlabs.org/project/uboot/patch/20231226094625.221671-1-sjg... https://patchwork.ozlabs.org/project/uboot/patch/20231228133654.2356023-1-sj... https://patchwork.ozlabs.org/project/uboot/patch/20231228194725.2482268-1-sj... https://patchwork.ozlabs.org/project/uboot/patch/20240104014919.413568-1-sjg... https://patchwork.ozlabs.org/project/uboot/patch/20241201144240.1664398-5-sj... https://patchwork.ozlabs.org/project/uboot/patch/20241201144240.1664398-6-sj...
[2] https://patchwork.ozlabs.org/project/uboot/patch/20231228194725.2482268-1-sj...
[3] https://patchwork.ozlabs.org/project/uboot/patch/20240104014919.413568-1-sjg...
On Fri, Dec 06, 2024 at 04:43:47PM -0700, Simon Glass wrote:
Hi Tom,
On Fri, 6 Dec 2024 at 13:12, Tom Rini trini@konsulko.com wrote:
On Fri, Dec 06, 2024 at 12:19:38PM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 4 Dec 2024 at 09:27, Tom Rini trini@konsulko.com wrote:
On Wed, Dec 04, 2024 at 08:13:04AM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 3 Dec 2024 at 18:29, Tom Rini trini@konsulko.com wrote:
On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote:
> From my side I'd like to change the conversation a little, to how to > land code, rather than why we should bother. "Code needs to land" > should be the motto. If someone has taken the time to create > something, our bias should be towards getting it in, with sufficient > changes to make it fit the project. There are cases where something is > just a bad idea, or should be done another way, but for people working > on major features or changes, biasing towards not landing the code is > just going to make them go elsewhere.
This is the wrong approach I believe. The goal has always been and continues to be to have reviewed (whenever possible, our developer community is small) incremental change over time.
Yes, I agree with that, but it isn't what I said.
I don't know how else to interpret "Code needs to land". I'm not sure what reviewed patches we have that are outstanding, but also not fairly new. We do have patches outstanding, in general. Much of those fall in to the categories of:
- Custodian is active, but also very busy (virtually everyone is a volunteer so I have a hard time getting forceful with people that have a large queue).
- Custodian isn't active / no direct custodian and the code hasn't been reviewed by anyone else.
Here's my intention with 'code needs to land': to bias against people saying "I don't like this; I don't care about your use case; please go away"
To me that applies an arbitrary nature to patches being rejected, which I do not see as being the case.
Just because something has been posted a number of times does not mean it's ready to be merged.
I didn't say that either.
It's an impression you give however when you repost a series less than a day after last posting, without addressing all of the feedback.
I'm going to ignore that comment as it is not helpful and mischaracterises my contributions.
I stand by it, and will let others judge if that's the impression they have, or not.
I'll put the relevant threads at [1], ditto.
Your challenge today is that on patchwork you have over 150 patches covering a wide variety of topics and of which many series have technically-merited feedback that needs to be addressed in a technical manner.
By my count I have about 10 series in progress, with a small number of patches (< 10?) with pending feedback
I'm not sure about that less than 10 number, in part because it's hard to keep track of which feedback was applied, and which not. And part because some of those series are places where I told you I'm unsure about the core concept but you asked and I'm giving you leeway to show me the end result.
OK
that isn't effectively just a NAK.
To be clear, "just a NAK" isn't the whole story. Those NAKs come with technical rationales and requests. But maybe they're just lost in the volume of emails you have in some cases. I know for example I mentioned a few times and places that we have tests today for booting the OS, when you mention that we need to add a test for booting the OS. We need to expand that test, yes.
If you look at the OF_BLOBLIST thing, it is basically a NAK. I don't see any other way to interpret it,. So several boards in my lab have been broken for a year.
To be clear, you want to revert: commit 70fe23859437ffe4efe0793423937d8b78ebf9d6 Author: Simon Glass sjg@chromium.org AuthorDate: Wed Jan 3 18:49:19 2024 -0700 Commit: Simon Glass sjg@chromium.org CommitDate: Sun Jan 7 13:45:07 2024 -0700
fdt: Allow the devicetree to come from a bloblist Standard passage provides for a bloblist to be passed from one firmware phase to the next. That can be used to pass the devicetree along as well. Add an option to support this. Tests for this will be added as part of the Universal Payload work. Signed-off-by: Simon Glass <sjg@chromium.org> Reviewed-by: Tom Rini <trini@konsulko.com> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>Which means that you did not test your changes on your hardware at the time? And wish to (seemingly) replace that with an earlier version of the patch which was objected to on technical grounds at the time. Without explaining why the current code does not function on these platforms, only that it does not, for entirely unclear reasons.
It never worked on my board, though. I spent 6 revisions trying to explain that. After number 6 [2] you got in touch and said that I needed to rework it, which I dutifully did, in version 7 [3]. I did that purely to unblock Raymond's series. I even put that in the patch:
"The discussion on this was not resolved and is now important due to the bloblist series from Raymond. So I am sending it again since I believe this is a better starting point than building on OF_BOARD"
Are you now saying I should have refused and argued for another few revisions? Patience does have its limits.
I'm saying that I assumed what you pushed functioned on your hardware, which is why I'm surprised now that it doesn't. I'll look through all of the links later, but I think you still haven't explained _why_ this solution doesn't function on your platforms (now or last year) and what your platforms need (as in, can't change) / want (you would need to rework some other part of the firmware stack to get them in agreement on that platform).
So I do not think I've arbitrarily nak'd the revert you posted, I've been asking you to explain why the code doesn't work. And I still don't have an answer as to why after at run time we don't find a valid bloblist here the fall-back use cases aren't functioning. To me that seems like the bug to investigate and explain. Reverting code to find when functionality broke is great, I do it all the time. Saying we need to revert because something broke *without* saying why it broke is a problem.
It isn't a particularly large number, if you add up the patches I do in each cycle. It is in the nature of development and code review that things are often not right the first time, or someone else has another perspective, so I cannot see how this can be reduced.
The easy way to reduce it would be to go from 10 series in progress to 1 series in progress, and finish that before picking up series number 2, and so on. And then in the future, stop when you have more than 2 or 3 in progress at once.
That's obviously not practical for the amount I am contributing and the length of time it takes us to get things in. I hope it isn't a serious suggestion!
It's extremely serious. For example https://patchwork.ozlabs.org/project/uboot/list/?series=435516&state=* is a 46 part patch series that touches 3 or 4 different areas and the only type of review that's feasible is "Does this cause the binary to grow too much? Or in unexpected ways?" And that sets aside that I believe I did ask you to rework "bootm: Allow building bootm.c without CONFIG_SYS_BOOTM_LEN" to refactor things so that SYS_BOOTM_LEN is always defined. That's not even a hard change, I say as someone that had to do that as part of Kconfig migration of oddly (ab)used symbol names.
But in this case, your series that touch areas of code actively maintained by other people is even more relevant. Whereas areas without other active developers, or where you are the main developer, I can find some smaller limits on what must be reviewed all of your *EFI* has other active developers on it. Who are providing technical feedback, not arbitrary or stylistic feedback. Please be explicit about what you are suggesting in this case, so I do not mischaracterize your intention.
But really, it feels like "Code needs to land" is another way of saying "Just a NAK should be ignored".
Yes, a bias more towards that would be more healthy IMO. A NAK needs to come with a full understanding of the use case, an alternative way to meet the use case, once that doesn't involve boiling the Pacific Ocean.
As it stands, I do not like to, but sometimes feel I *have*to* break one board to unbreak another board and keep reminding both parties to resolve the issue so no board is broken. There has been, since I have taken over the lead of the project, at least once where I have over-ruled the relevant custodian and that has resulted in the custodian leaving. I both feel that was the right technical call, and regret it came to that, still. I do not want to adopt a more broad use of that policy. The default for absence of agreement must be to try again.
I see that Peter sent a few indignant emails. You can't win.
I assume this is about the LED series, and I further assume you forgot / missed where he explained that removing the legacy code is a functional user visible usability regression on the pinephone.
Hi Tom,
On Fri, 6 Dec 2024 at 17:59, Tom Rini trini@konsulko.com wrote:
On Fri, Dec 06, 2024 at 04:43:47PM -0700, Simon Glass wrote:
Hi Tom,
On Fri, 6 Dec 2024 at 13:12, Tom Rini trini@konsulko.com wrote:
On Fri, Dec 06, 2024 at 12:19:38PM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 4 Dec 2024 at 09:27, Tom Rini trini@konsulko.com wrote:
On Wed, Dec 04, 2024 at 08:13:04AM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 3 Dec 2024 at 18:29, Tom Rini trini@konsulko.com wrote: > > On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote: > > > From my side I'd like to change the conversation a little, to how to > > land code, rather than why we should bother. "Code needs to land" > > should be the motto. If someone has taken the time to create > > something, our bias should be towards getting it in, with sufficient > > changes to make it fit the project. There are cases where something is > > just a bad idea, or should be done another way, but for people working > > on major features or changes, biasing towards not landing the code is > > just going to make them go elsewhere. > > This is the wrong approach I believe. The goal has always been and > continues to be to have reviewed (whenever possible, our developer > community is small) incremental change over time.
Yes, I agree with that, but it isn't what I said.
I don't know how else to interpret "Code needs to land". I'm not sure what reviewed patches we have that are outstanding, but also not fairly new. We do have patches outstanding, in general. Much of those fall in to the categories of:
- Custodian is active, but also very busy (virtually everyone is a volunteer so I have a hard time getting forceful with people that have a large queue).
- Custodian isn't active / no direct custodian and the code hasn't been reviewed by anyone else.
Here's my intention with 'code needs to land': to bias against people saying "I don't like this; I don't care about your use case; please go away"
To me that applies an arbitrary nature to patches being rejected, which I do not see as being the case.
> Just because something > has been posted a number of times does not mean it's ready to be merged.
I didn't say that either.
It's an impression you give however when you repost a series less than a day after last posting, without addressing all of the feedback.
I'm going to ignore that comment as it is not helpful and mischaracterises my contributions.
I stand by it, and will let others judge if that's the impression they have, or not.
I'll put the relevant threads at [1], ditto.
> Your challenge today is that on patchwork you have over 150 patches > covering a wide variety of topics and of which many series have > technically-merited feedback that needs to be addressed in a technical > manner.
By my count I have about 10 series in progress, with a small number of patches (< 10?) with pending feedback
I'm not sure about that less than 10 number, in part because it's hard to keep track of which feedback was applied, and which not. And part because some of those series are places where I told you I'm unsure about the core concept but you asked and I'm giving you leeway to show me the end result.
OK
that isn't effectively just a NAK.
To be clear, "just a NAK" isn't the whole story. Those NAKs come with technical rationales and requests. But maybe they're just lost in the volume of emails you have in some cases. I know for example I mentioned a few times and places that we have tests today for booting the OS, when you mention that we need to add a test for booting the OS. We need to expand that test, yes.
If you look at the OF_BLOBLIST thing, it is basically a NAK. I don't see any other way to interpret it,. So several boards in my lab have been broken for a year.
To be clear, you want to revert: commit 70fe23859437ffe4efe0793423937d8b78ebf9d6 Author: Simon Glass sjg@chromium.org AuthorDate: Wed Jan 3 18:49:19 2024 -0700 Commit: Simon Glass sjg@chromium.org CommitDate: Sun Jan 7 13:45:07 2024 -0700
fdt: Allow the devicetree to come from a bloblist Standard passage provides for a bloblist to be passed from one firmware phase to the next. That can be used to pass the devicetree along as well. Add an option to support this. Tests for this will be added as part of the Universal Payload work. Signed-off-by: Simon Glass <sjg@chromium.org> Reviewed-by: Tom Rini <trini@konsulko.com> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>Which means that you did not test your changes on your hardware at the time? And wish to (seemingly) replace that with an earlier version of the patch which was objected to on technical grounds at the time. Without explaining why the current code does not function on these platforms, only that it does not, for entirely unclear reasons.
It never worked on my board, though. I spent 6 revisions trying to explain that. After number 6 [2] you got in touch and said that I needed to rework it, which I dutifully did, in version 7 [3]. I did that purely to unblock Raymond's series. I even put that in the patch:
"The discussion on this was not resolved and is now important due to the bloblist series from Raymond. So I am sending it again since I believe this is a better starting point than building on OF_BOARD"
Are you now saying I should have refused and argued for another few revisions? Patience does have its limits.
I'm saying that I assumed what you pushed functioned on your hardware, which is why I'm surprised now that it doesn't.
How can you possibly assume that, or be 'surprised' about that?! I repeatedly said that the solution was no good for me and I was very clear that I only sent that patch because you insisted on it. You sent a 'thank you' afterwards, which rang pretty hollow after so much effort and still having broken boards.
From what I vaguely recall, in a discussion that went on till the cows
came home, it was tied up with U-Boot not being allowed to have its own devicetree. Another vague recollection was that I was stupid and I didn't understand...
I'll look through all of the links later, but I think you still haven't explained _why_ this solution doesn't function on your platforms (now or last year) and what your platforms need (as in, can't change) / want (you would need to rework some other part of the firmware stack to get them in agreement on that platform).
Actually I've explained that many, many times. They use bloblist but not for passing the FDT. This strange insistence that the bloblist must carry the FDT is what caused all these problems. I need a platform to be able to boot TPL, have a bloblist set up in SPL, pass it to U-Boot proper, all without having an FDT in it.
So I do not think I've arbitrarily nak'd the revert you posted, I've been asking you to explain why the code doesn't work. And I still don't have an answer as to why after at run time we don't find a valid bloblist here the fall-back use cases aren't functioning. To me that seems like the bug to investigate and explain. Reverting code to find when functionality broke is great, I do it all the time. Saying we need to revert because something broke *without* saying why it broke is a problem.
It isn't a particularly large number, if you add up the patches I do in each cycle. It is in the nature of development and code review that things are often not right the first time, or someone else has another perspective, so I cannot see how this can be reduced.
The easy way to reduce it would be to go from 10 series in progress to 1 series in progress, and finish that before picking up series number 2, and so on. And then in the future, stop when you have more than 2 or 3 in progress at once.
That's obviously not practical for the amount I am contributing and the length of time it takes us to get things in. I hope it isn't a serious suggestion!
It's extremely serious. For example https://patchwork.ozlabs.org/project/uboot/list/?series=435516&state=* is a 46 part patch series that touches 3 or 4 different areas and the only type of review that's feasible is "Does this cause the binary to grow too much? Or in unexpected ways?" And that sets aside that I believe I did ask you to rework "bootm: Allow building bootm.c without CONFIG_SYS_BOOTM_LEN" to refactor things so that SYS_BOOTM_LEN is always defined. That's not even a hard change, I say as someone that had to do that as part of Kconfig migration of oddly (ab)used symbol names.
But in this case, your series that touch areas of code actively maintained by other people is even more relevant. Whereas areas without other active developers, or where you are the main developer, I can find some smaller limits on what must be reviewed all of your *EFI* has other active developers on it. Who are providing technical feedback, not arbitrary or stylistic feedback. Please be explicit about what you are suggesting in this case, so I do not mischaracterize your intention.
But really, it feels like "Code needs to land" is another way of saying "Just a NAK should be ignored".
Yes, a bias more towards that would be more healthy IMO. A NAK needs to come with a full understanding of the use case, an alternative way to meet the use case, once that doesn't involve boiling the Pacific Ocean.
As it stands, I do not like to, but sometimes feel I *have*to* break one board to unbreak another board and keep reminding both parties to resolve the issue so no board is broken. There has been, since I have taken over the lead of the project, at least once where I have over-ruled the relevant custodian and that has resulted in the custodian leaving. I both feel that was the right technical call, and regret it came to that, still. I do not want to adopt a more broad use of that policy. The default for absence of agreement must be to try again.
I see that Peter sent a few indignant emails. You can't win.
I assume this is about the LED series, and I further assume you forgot / missed where he explained that removing the legacy code is a functional user visible usability regression on the pinephone.
It seems you are saying that we can break my boards completely, but Peter must have his flashing LEDs?
Regards, Simon
On Thu, Dec 12, 2024 at 07:09:12AM -0700, Simon Glass wrote:
Hi Tom,
On Fri, 6 Dec 2024 at 17:59, Tom Rini trini@konsulko.com wrote:
On Fri, Dec 06, 2024 at 04:43:47PM -0700, Simon Glass wrote:
Hi Tom,
On Fri, 6 Dec 2024 at 13:12, Tom Rini trini@konsulko.com wrote:
On Fri, Dec 06, 2024 at 12:19:38PM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 4 Dec 2024 at 09:27, Tom Rini trini@konsulko.com wrote:
On Wed, Dec 04, 2024 at 08:13:04AM -0700, Simon Glass wrote: > Hi Tom, > > On Tue, 3 Dec 2024 at 18:29, Tom Rini trini@konsulko.com wrote: > > > > On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote: > > > > > From my side I'd like to change the conversation a little, to how to > > > land code, rather than why we should bother. "Code needs to land" > > > should be the motto. If someone has taken the time to create > > > something, our bias should be towards getting it in, with sufficient > > > changes to make it fit the project. There are cases where something is > > > just a bad idea, or should be done another way, but for people working > > > on major features or changes, biasing towards not landing the code is > > > just going to make them go elsewhere. > > > > This is the wrong approach I believe. The goal has always been and > > continues to be to have reviewed (whenever possible, our developer > > community is small) incremental change over time. > > Yes, I agree with that, but it isn't what I said.
I don't know how else to interpret "Code needs to land". I'm not sure what reviewed patches we have that are outstanding, but also not fairly new. We do have patches outstanding, in general. Much of those fall in to the categories of:
- Custodian is active, but also very busy (virtually everyone is a volunteer so I have a hard time getting forceful with people that have a large queue).
- Custodian isn't active / no direct custodian and the code hasn't been reviewed by anyone else.
Here's my intention with 'code needs to land': to bias against people saying "I don't like this; I don't care about your use case; please go away"
To me that applies an arbitrary nature to patches being rejected, which I do not see as being the case.
> > Just because something > > has been posted a number of times does not mean it's ready to be merged. > > I didn't say that either.
It's an impression you give however when you repost a series less than a day after last posting, without addressing all of the feedback.
I'm going to ignore that comment as it is not helpful and mischaracterises my contributions.
I stand by it, and will let others judge if that's the impression they have, or not.
I'll put the relevant threads at [1], ditto.
> > Your challenge today is that on patchwork you have over 150 patches > > covering a wide variety of topics and of which many series have > > technically-merited feedback that needs to be addressed in a technical > > manner. > > By my count I have about 10 series in progress, with a small number of > patches (< 10?) with pending feedback
I'm not sure about that less than 10 number, in part because it's hard to keep track of which feedback was applied, and which not. And part because some of those series are places where I told you I'm unsure about the core concept but you asked and I'm giving you leeway to show me the end result.
OK
> that isn't effectively just a > NAK.
To be clear, "just a NAK" isn't the whole story. Those NAKs come with technical rationales and requests. But maybe they're just lost in the volume of emails you have in some cases. I know for example I mentioned a few times and places that we have tests today for booting the OS, when you mention that we need to add a test for booting the OS. We need to expand that test, yes.
If you look at the OF_BLOBLIST thing, it is basically a NAK. I don't see any other way to interpret it,. So several boards in my lab have been broken for a year.
To be clear, you want to revert: commit 70fe23859437ffe4efe0793423937d8b78ebf9d6 Author: Simon Glass sjg@chromium.org AuthorDate: Wed Jan 3 18:49:19 2024 -0700 Commit: Simon Glass sjg@chromium.org CommitDate: Sun Jan 7 13:45:07 2024 -0700
fdt: Allow the devicetree to come from a bloblist Standard passage provides for a bloblist to be passed from one firmware phase to the next. That can be used to pass the devicetree along as well. Add an option to support this. Tests for this will be added as part of the Universal Payload work. Signed-off-by: Simon Glass <sjg@chromium.org> Reviewed-by: Tom Rini <trini@konsulko.com> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>Which means that you did not test your changes on your hardware at the time? And wish to (seemingly) replace that with an earlier version of the patch which was objected to on technical grounds at the time. Without explaining why the current code does not function on these platforms, only that it does not, for entirely unclear reasons.
It never worked on my board, though. I spent 6 revisions trying to explain that. After number 6 [2] you got in touch and said that I needed to rework it, which I dutifully did, in version 7 [3]. I did that purely to unblock Raymond's series. I even put that in the patch:
"The discussion on this was not resolved and is now important due to the bloblist series from Raymond. So I am sending it again since I believe this is a better starting point than building on OF_BOARD"
Are you now saying I should have refused and argued for another few revisions? Patience does have its limits.
I'm saying that I assumed what you pushed functioned on your hardware, which is why I'm surprised now that it doesn't.
How can you possibly assume that, or be 'surprised' about that?! I repeatedly said that the solution was no good for me and I was very clear that I only sent that patch because you insisted on it. You sent a 'thank you' afterwards, which rang pretty hollow after so much effort and still having broken boards.
Well, I can and maybe should more often admit to missing a thing or two in very long threads, that's how. Doubly so in light of the patches I posted now (after you wrote the above, but before I read it) fixing your platforms.
From what I vaguely recall, in a discussion that went on till the cows came home, it was tied up with U-Boot not being allowed to have its own devicetree. Another vague recollection was that I was stupid and I didn't understand...
Yes, that's another thing that doesn't make any sense. You still insist that point while no one is making the point you insist someone is making.
I'll look through all of the links later, but I think you still haven't explained _why_ this solution doesn't function on your platforms (now or last year) and what your platforms need (as in, can't change) / want (you would need to rework some other part of the firmware stack to get them in agreement on that platform).
Actually I've explained that many, many times. They use bloblist but not for passing the FDT. This strange insistence that the bloblist must carry the FDT is what caused all these problems. I need a platform to be able to boot TPL, have a bloblist set up in SPL, pass it to U-Boot proper, all without having an FDT in it.
OK, but that doesn't seem to be how the platforms work? Bob and Kevin don't use TPL (but they use TF-A? I can't see in gitlab how we build for testing on HW and it's not clear if we're grabbing bl31 from somewhere), but it's SPL that initializes DRAM. And that means we can't look at the bloblist in DRAM before init'ing DRAM. Coral does have TPL, but does not enable TPL_BLOBLIST currently. But it looks like it also does DRAM initialization in SPL, so, overall the same class of problem.
And the insistence was that IF we have bloblist support THEN we must look at it. Which we can't if it's in DRAM and we're responsible for DRAM. Which these days is often not the case. And if you mentioned that problem in the many threads we had at the time, it was drowned out by other comments.
So I do not think I've arbitrarily nak'd the revert you posted, I've been asking you to explain why the code doesn't work. And I still don't have an answer as to why after at run time we don't find a valid bloblist here the fall-back use cases aren't functioning. To me that seems like the bug to investigate and explain. Reverting code to find when functionality broke is great, I do it all the time. Saying we need to revert because something broke *without* saying why it broke is a problem.
> It isn't a particularly large number, if you add up the patches I > do in each cycle. It is in the nature of development and code review > that things are often not right the first time, or someone else has > another perspective, so I cannot see how this can be reduced.
The easy way to reduce it would be to go from 10 series in progress to 1 series in progress, and finish that before picking up series number 2, and so on. And then in the future, stop when you have more than 2 or 3 in progress at once.
That's obviously not practical for the amount I am contributing and the length of time it takes us to get things in. I hope it isn't a serious suggestion!
It's extremely serious. For example https://patchwork.ozlabs.org/project/uboot/list/?series=435516&state=* is a 46 part patch series that touches 3 or 4 different areas and the only type of review that's feasible is "Does this cause the binary to grow too much? Or in unexpected ways?" And that sets aside that I believe I did ask you to rework "bootm: Allow building bootm.c without CONFIG_SYS_BOOTM_LEN" to refactor things so that SYS_BOOTM_LEN is always defined. That's not even a hard change, I say as someone that had to do that as part of Kconfig migration of oddly (ab)used symbol names.
But in this case, your series that touch areas of code actively maintained by other people is even more relevant. Whereas areas without other active developers, or where you are the main developer, I can find some smaller limits on what must be reviewed all of your *EFI* has other active developers on it. Who are providing technical feedback, not arbitrary or stylistic feedback. Please be explicit about what you are suggesting in this case, so I do not mischaracterize your intention.
But really, it feels like "Code needs to land" is another way of saying "Just a NAK should be ignored".
Yes, a bias more towards that would be more healthy IMO. A NAK needs to come with a full understanding of the use case, an alternative way to meet the use case, once that doesn't involve boiling the Pacific Ocean.
As it stands, I do not like to, but sometimes feel I *have*to* break one board to unbreak another board and keep reminding both parties to resolve the issue so no board is broken. There has been, since I have taken over the lead of the project, at least once where I have over-ruled the relevant custodian and that has resulted in the custodian leaving. I both feel that was the right technical call, and regret it came to that, still. I do not want to adopt a more broad use of that policy. The default for absence of agreement must be to try again.
I see that Peter sent a few indignant emails. You can't win.
I assume this is about the LED series, and I further assume you forgot / missed where he explained that removing the legacy code is a functional user visible usability regression on the pinephone.
It seems you are saying that we can break my boards completely, but Peter must have his flashing LEDs?
Well, as I've stated many times and in many threads, I do not like breaking one platform to fix another. And yes, I did think your three platforms were functional and you didn't like the solution we had come to. But also, pinephone, U-Boot is the firmware stack it ships with. Phones tend to not have a lot of easily usable user-visible feedback options. So yes, I really do not want to remove the early indications that the device is alive / working (or to report it's not working!) when we've also just recently added support for what they're missing (because another new device, the openwrt one, has similar user visible requirements). So to be clear, there's 5 platforms using the legacy code. 1 of which someone is actively trying to in their spare time port to the new codebase. I do not see the urgency in removing the legacy code right now. I also do not see the point in gating the removal after pinephone works (unless someone steps up for the other platforms, but it's also likely a working conversion will make it easier to see why the attempted quick conversion did NOT work, and so we redo the other 4 based on what we've learned and hope they do now work).
Hi Tom,
On Thu, 12 Dec 2024 at 09:51, Tom Rini trini@konsulko.com wrote:
On Thu, Dec 12, 2024 at 07:09:12AM -0700, Simon Glass wrote:
Hi Tom,
On Fri, 6 Dec 2024 at 17:59, Tom Rini trini@konsulko.com wrote:
On Fri, Dec 06, 2024 at 04:43:47PM -0700, Simon Glass wrote:
Hi Tom,
On Fri, 6 Dec 2024 at 13:12, Tom Rini trini@konsulko.com wrote:
On Fri, Dec 06, 2024 at 12:19:38PM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 4 Dec 2024 at 09:27, Tom Rini trini@konsulko.com wrote: > > On Wed, Dec 04, 2024 at 08:13:04AM -0700, Simon Glass wrote: > > Hi Tom, > > > > On Tue, 3 Dec 2024 at 18:29, Tom Rini trini@konsulko.com wrote: > > > > > > On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote: > > > > > > > From my side I'd like to change the conversation a little, to how to > > > > land code, rather than why we should bother. "Code needs to land" > > > > should be the motto. If someone has taken the time to create > > > > something, our bias should be towards getting it in, with sufficient > > > > changes to make it fit the project. There are cases where something is > > > > just a bad idea, or should be done another way, but for people working > > > > on major features or changes, biasing towards not landing the code is > > > > just going to make them go elsewhere. > > > > > > This is the wrong approach I believe. The goal has always been and > > > continues to be to have reviewed (whenever possible, our developer > > > community is small) incremental change over time. > > > > Yes, I agree with that, but it isn't what I said. > > I don't know how else to interpret "Code needs to land". I'm not sure > what reviewed patches we have that are outstanding, but also not fairly > new. We do have patches outstanding, in general. Much of those fall in > to the categories of: > - Custodian is active, but also very busy (virtually everyone is a > volunteer so I have a hard time getting forceful with people that have > a large queue). > - Custodian isn't active / no direct custodian and the code hasn't been > reviewed by anyone else.
Here's my intention with 'code needs to land': to bias against people saying "I don't like this; I don't care about your use case; please go away"
To me that applies an arbitrary nature to patches being rejected, which I do not see as being the case.
> > > Just because something > > > has been posted a number of times does not mean it's ready to be merged. > > > > I didn't say that either. > > It's an impression you give however when you repost a series less than a > day after last posting, without addressing all of the feedback.
I'm going to ignore that comment as it is not helpful and mischaracterises my contributions.
I stand by it, and will let others judge if that's the impression they have, or not.
I'll put the relevant threads at [1], ditto.
> > > Your challenge today is that on patchwork you have over 150 patches > > > covering a wide variety of topics and of which many series have > > > technically-merited feedback that needs to be addressed in a technical > > > manner. > > > > By my count I have about 10 series in progress, with a small number of > > patches (< 10?) with pending feedback > > I'm not sure about that less than 10 number, in part because it's hard > to keep track of which feedback was applied, and which not. And part > because some of those series are places where I told you I'm unsure > about the core concept but you asked and I'm giving you leeway to show > me the end result.
OK
> > > that isn't effectively just a > > NAK. > > To be clear, "just a NAK" isn't the whole story. Those NAKs come with > technical rationales and requests. But maybe they're just lost in the > volume of emails you have in some cases. I know for example I mentioned > a few times and places that we have tests today for booting the OS, when > you mention that we need to add a test for booting the OS. We need to > expand that test, yes.
If you look at the OF_BLOBLIST thing, it is basically a NAK. I don't see any other way to interpret it,. So several boards in my lab have been broken for a year.
To be clear, you want to revert: commit 70fe23859437ffe4efe0793423937d8b78ebf9d6 Author: Simon Glass sjg@chromium.org AuthorDate: Wed Jan 3 18:49:19 2024 -0700 Commit: Simon Glass sjg@chromium.org CommitDate: Sun Jan 7 13:45:07 2024 -0700
fdt: Allow the devicetree to come from a bloblist Standard passage provides for a bloblist to be passed from one firmware phase to the next. That can be used to pass the devicetree along as well. Add an option to support this. Tests for this will be added as part of the Universal Payload work. Signed-off-by: Simon Glass <sjg@chromium.org> Reviewed-by: Tom Rini <trini@konsulko.com> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>Which means that you did not test your changes on your hardware at the time? And wish to (seemingly) replace that with an earlier version of the patch which was objected to on technical grounds at the time. Without explaining why the current code does not function on these platforms, only that it does not, for entirely unclear reasons.
It never worked on my board, though. I spent 6 revisions trying to explain that. After number 6 [2] you got in touch and said that I needed to rework it, which I dutifully did, in version 7 [3]. I did that purely to unblock Raymond's series. I even put that in the patch:
"The discussion on this was not resolved and is now important due to the bloblist series from Raymond. So I am sending it again since I believe this is a better starting point than building on OF_BOARD"
Are you now saying I should have refused and argued for another few revisions? Patience does have its limits.
I'm saying that I assumed what you pushed functioned on your hardware, which is why I'm surprised now that it doesn't.
How can you possibly assume that, or be 'surprised' about that?! I repeatedly said that the solution was no good for me and I was very clear that I only sent that patch because you insisted on it. You sent a 'thank you' afterwards, which rang pretty hollow after so much effort and still having broken boards.
Well, I can and maybe should more often admit to missing a thing or two in very long threads, that's how. Doubly so in light of the patches I posted now (after you wrote the above, but before I read it) fixing your platforms.
From what I vaguely recall, in a discussion that went on till the cows came home, it was tied up with U-Boot not being allowed to have its own devicetree. Another vague recollection was that I was stupid and I didn't understand...
Yes, that's another thing that doesn't make any sense. You still insist that point while no one is making the point you insist someone is making.
Well, I did spend many happy hours arguing with someone about whether U-Boot was allowed to have its own devicetree, have its own properties in the devicetree, etc. I did not imagine it. I believe that OF_BLOBLIST was radioactive partly because of that.
I'll look through all of the links later, but I think you still haven't explained _why_ this solution doesn't function on your platforms (now or last year) and what your platforms need (as in, can't change) / want (you would need to rework some other part of the firmware stack to get them in agreement on that platform).
Actually I've explained that many, many times. They use bloblist but not for passing the FDT. This strange insistence that the bloblist must carry the FDT is what caused all these problems. I need a platform to be able to boot TPL, have a bloblist set up in SPL, pass it to U-Boot proper, all without having an FDT in it.
OK, but that doesn't seem to be how the platforms work? Bob and Kevin don't use TPL (but they use TF-A? I can't see in gitlab how we build for testing on HW and it's not clear if we're grabbing bl31 from somewhere), but it's SPL that initializes DRAM. And that means we can't look at the bloblist in DRAM before init'ing DRAM. Coral does have TPL, but does not enable TPL_BLOBLIST currently. But it looks like it also does DRAM initialization in SPL, so, overall the same class of problem.
Yes, that's right. See [1]
It would be nice to expose that information in gitlab. Not hard to do, just print out the parameters for the UBootProviderDriver. It would be even better to build bl31 from source and print out the source-hash used.
And the insistence was that IF we have bloblist support THEN we must look at it. Which we can't if it's in DRAM and we're responsible for DRAM. Which these days is often not the case. And if you mentioned that problem in the many threads we had at the time, it was drowned out by other comments.
My patch from a year ago fixed a regression and a bug. It should have been applied, but instead we all argued about it through 7 revisions and then it just died. That result is not on me, sorry.
So I do not think I've arbitrarily nak'd the revert you posted, I've been asking you to explain why the code doesn't work. And I still don't have an answer as to why after at run time we don't find a valid bloblist here the fall-back use cases aren't functioning. To me that seems like the bug to investigate and explain. Reverting code to find when functionality broke is great, I do it all the time. Saying we need to revert because something broke *without* saying why it broke is a problem.
> > It isn't a particularly large number, if you add up the patches I > > do in each cycle. It is in the nature of development and code review > > that things are often not right the first time, or someone else has > > another perspective, so I cannot see how this can be reduced. > > The easy way to reduce it would be to go from 10 series in progress to 1 > series in progress, and finish that before picking up series number 2, > and so on. And then in the future, stop when you have more than 2 or 3 > in progress at once.
That's obviously not practical for the amount I am contributing and the length of time it takes us to get things in. I hope it isn't a serious suggestion!
It's extremely serious. For example https://patchwork.ozlabs.org/project/uboot/list/?series=435516&state=* is a 46 part patch series that touches 3 or 4 different areas and the only type of review that's feasible is "Does this cause the binary to grow too much? Or in unexpected ways?" And that sets aside that I believe I did ask you to rework "bootm: Allow building bootm.c without CONFIG_SYS_BOOTM_LEN" to refactor things so that SYS_BOOTM_LEN is always defined. That's not even a hard change, I say as someone that had to do that as part of Kconfig migration of oddly (ab)used symbol names.
But in this case, your series that touch areas of code actively maintained by other people is even more relevant. Whereas areas without other active developers, or where you are the main developer, I can find some smaller limits on what must be reviewed all of your *EFI* has other active developers on it. Who are providing technical feedback, not arbitrary or stylistic feedback. Please be explicit about what you are suggesting in this case, so I do not mischaracterize your intention.
> But really, it feels like "Code needs to land" is another way of saying > "Just a NAK should be ignored".
Yes, a bias more towards that would be more healthy IMO. A NAK needs to come with a full understanding of the use case, an alternative way to meet the use case, once that doesn't involve boiling the Pacific Ocean.
As it stands, I do not like to, but sometimes feel I *have*to* break one board to unbreak another board and keep reminding both parties to resolve the issue so no board is broken. There has been, since I have taken over the lead of the project, at least once where I have over-ruled the relevant custodian and that has resulted in the custodian leaving. I both feel that was the right technical call, and regret it came to that, still. I do not want to adopt a more broad use of that policy. The default for absence of agreement must be to try again.
I see that Peter sent a few indignant emails. You can't win.
I assume this is about the LED series, and I further assume you forgot / missed where he explained that removing the legacy code is a functional user visible usability regression on the pinephone.
It seems you are saying that we can break my boards completely, but Peter must have his flashing LEDs?
Well, as I've stated many times and in many threads, I do not like breaking one platform to fix another. And yes, I did think your three platforms were functional and you didn't like the solution we had come to. But also, pinephone, U-Boot is the firmware stack it ships with. Phones tend to not have a lot of easily usable user-visible feedback options. So yes, I really do not want to remove the early indications that the device is alive / working (or to report it's not working!) when we've also just recently added support for what they're missing (because another new device, the openwrt one, has similar user visible requirements). So to be clear, there's 5 platforms using the legacy code. 1 of which someone is actively trying to in their spare time port to the new codebase. I do not see the urgency in removing the legacy code right now. I also do not see the point in gating the removal after pinephone works (unless someone steps up for the other platforms, but it's also likely a working conversion will make it easier to see why the attempted quick conversion did NOT work, and so we redo the other 4 based on what we've learned and hope they do now work).
I don't really disagree with any of that.
It might encourage bad behaviour though, e.g. a time-pressed maintainer pops his head up just long enough to cast doubt on patches, then heads off safe in the knowledge that nothing will happen for a few more months.
Regards, Simon
[1] https://github.com/labgrid-project/labgrid/pull/1411/commits/6d3b4a0cfc7e35e...
On Fri, Dec 13, 2024 at 05:37:11AM -0700, Simon Glass wrote:
Hi Tom,
On Thu, 12 Dec 2024 at 09:51, Tom Rini trini@konsulko.com wrote:
On Thu, Dec 12, 2024 at 07:09:12AM -0700, Simon Glass wrote:
Hi Tom,
On Fri, 6 Dec 2024 at 17:59, Tom Rini trini@konsulko.com wrote:
On Fri, Dec 06, 2024 at 04:43:47PM -0700, Simon Glass wrote:
Hi Tom,
On Fri, 6 Dec 2024 at 13:12, Tom Rini trini@konsulko.com wrote:
On Fri, Dec 06, 2024 at 12:19:38PM -0700, Simon Glass wrote: > Hi Tom, > > On Wed, 4 Dec 2024 at 09:27, Tom Rini trini@konsulko.com wrote: > > > > On Wed, Dec 04, 2024 at 08:13:04AM -0700, Simon Glass wrote: > > > Hi Tom, > > > > > > On Tue, 3 Dec 2024 at 18:29, Tom Rini trini@konsulko.com wrote: > > > > > > > > On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote: > > > > > > > > > From my side I'd like to change the conversation a little, to how to > > > > > land code, rather than why we should bother. "Code needs to land" > > > > > should be the motto. If someone has taken the time to create > > > > > something, our bias should be towards getting it in, with sufficient > > > > > changes to make it fit the project. There are cases where something is > > > > > just a bad idea, or should be done another way, but for people working > > > > > on major features or changes, biasing towards not landing the code is > > > > > just going to make them go elsewhere. > > > > > > > > This is the wrong approach I believe. The goal has always been and > > > > continues to be to have reviewed (whenever possible, our developer > > > > community is small) incremental change over time. > > > > > > Yes, I agree with that, but it isn't what I said. > > > > I don't know how else to interpret "Code needs to land". I'm not sure > > what reviewed patches we have that are outstanding, but also not fairly > > new. We do have patches outstanding, in general. Much of those fall in > > to the categories of: > > - Custodian is active, but also very busy (virtually everyone is a > > volunteer so I have a hard time getting forceful with people that have > > a large queue). > > - Custodian isn't active / no direct custodian and the code hasn't been > > reviewed by anyone else. > > Here's my intention with 'code needs to land': to bias against people > saying "I don't like this; I don't care about your use case; please go > away"
To me that applies an arbitrary nature to patches being rejected, which I do not see as being the case.
> > > > Just because something > > > > has been posted a number of times does not mean it's ready to be merged. > > > > > > I didn't say that either. > > > > It's an impression you give however when you repost a series less than a > > day after last posting, without addressing all of the feedback. > > I'm going to ignore that comment as it is not helpful and > mischaracterises my contributions.
I stand by it, and will let others judge if that's the impression they have, or not.
I'll put the relevant threads at [1], ditto.
> > > > Your challenge today is that on patchwork you have over 150 patches > > > > covering a wide variety of topics and of which many series have > > > > technically-merited feedback that needs to be addressed in a technical > > > > manner. > > > > > > By my count I have about 10 series in progress, with a small number of > > > patches (< 10?) with pending feedback > > > > I'm not sure about that less than 10 number, in part because it's hard > > to keep track of which feedback was applied, and which not. And part > > because some of those series are places where I told you I'm unsure > > about the core concept but you asked and I'm giving you leeway to show > > me the end result. > > OK > > > > > > that isn't effectively just a > > > NAK. > > > > To be clear, "just a NAK" isn't the whole story. Those NAKs come with > > technical rationales and requests. But maybe they're just lost in the > > volume of emails you have in some cases. I know for example I mentioned > > a few times and places that we have tests today for booting the OS, when > > you mention that we need to add a test for booting the OS. We need to > > expand that test, yes. > > If you look at the OF_BLOBLIST thing, it is basically a NAK. I don't > see any other way to interpret it,. So several boards in my lab have > been broken for a year.
To be clear, you want to revert: commit 70fe23859437ffe4efe0793423937d8b78ebf9d6 Author: Simon Glass sjg@chromium.org AuthorDate: Wed Jan 3 18:49:19 2024 -0700 Commit: Simon Glass sjg@chromium.org CommitDate: Sun Jan 7 13:45:07 2024 -0700
fdt: Allow the devicetree to come from a bloblist Standard passage provides for a bloblist to be passed from one firmware phase to the next. That can be used to pass the devicetree along as well. Add an option to support this. Tests for this will be added as part of the Universal Payload work. Signed-off-by: Simon Glass <sjg@chromium.org> Reviewed-by: Tom Rini <trini@konsulko.com> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>Which means that you did not test your changes on your hardware at the time? And wish to (seemingly) replace that with an earlier version of the patch which was objected to on technical grounds at the time. Without explaining why the current code does not function on these platforms, only that it does not, for entirely unclear reasons.
It never worked on my board, though. I spent 6 revisions trying to explain that. After number 6 [2] you got in touch and said that I needed to rework it, which I dutifully did, in version 7 [3]. I did that purely to unblock Raymond's series. I even put that in the patch:
"The discussion on this was not resolved and is now important due to the bloblist series from Raymond. So I am sending it again since I believe this is a better starting point than building on OF_BOARD"
Are you now saying I should have refused and argued for another few revisions? Patience does have its limits.
I'm saying that I assumed what you pushed functioned on your hardware, which is why I'm surprised now that it doesn't.
How can you possibly assume that, or be 'surprised' about that?! I repeatedly said that the solution was no good for me and I was very clear that I only sent that patch because you insisted on it. You sent a 'thank you' afterwards, which rang pretty hollow after so much effort and still having broken boards.
Well, I can and maybe should more often admit to missing a thing or two in very long threads, that's how. Doubly so in light of the patches I posted now (after you wrote the above, but before I read it) fixing your platforms.
From what I vaguely recall, in a discussion that went on till the cows came home, it was tied up with U-Boot not being allowed to have its own devicetree. Another vague recollection was that I was stupid and I didn't understand...
Yes, that's another thing that doesn't make any sense. You still insist that point while no one is making the point you insist someone is making.
Well, I did spend many happy hours arguing with someone about whether U-Boot was allowed to have its own devicetree, have its own properties in the devicetree, etc. I did not imagine it. I believe that OF_BLOBLIST was radioactive partly because of that.
This...
I'll look through all of the links later, but I think you still haven't explained _why_ this solution doesn't function on your platforms (now or last year) and what your platforms need (as in, can't change) / want (you would need to rework some other part of the firmware stack to get them in agreement on that platform).
Actually I've explained that many, many times. They use bloblist but not for passing the FDT. This strange insistence that the bloblist must carry the FDT is what caused all these problems. I need a platform to be able to boot TPL, have a bloblist set up in SPL, pass it to U-Boot proper, all without having an FDT in it.
OK, but that doesn't seem to be how the platforms work? Bob and Kevin don't use TPL (but they use TF-A? I can't see in gitlab how we build for testing on HW and it's not clear if we're grabbing bl31 from somewhere), but it's SPL that initializes DRAM. And that means we can't look at the bloblist in DRAM before init'ing DRAM. Coral does have TPL, but does not enable TPL_BLOBLIST currently. But it looks like it also does DRAM initialization in SPL, so, overall the same class of problem.
Yes, that's right. See [1]
It would be nice to expose that information in gitlab. Not hard to do, just print out the parameters for the UBootProviderDriver. It would be even better to build bl31 from source and print out the source-hash used.
And the insistence was that IF we have bloblist support THEN we must look at it. Which we can't if it's in DRAM and we're responsible for DRAM. Which these days is often not the case. And if you mentioned that problem in the many threads we had at the time, it was drowned out by other comments.
My patch from a year ago fixed a regression and a bug. It should have been applied, but instead we all argued about it through 7 revisions and then it just died. That result is not on me, sorry.
And this are both part of the same problem. You are unable to communicate the *why* of *what* you are doing in an effective manner. You need to figure out how to fix that problem, not go and write more code that still doesn't effectively communicate your design choices. "It works for me" doesn't scale beyond a solo project.
So I do not think I've arbitrarily nak'd the revert you posted, I've been asking you to explain why the code doesn't work. And I still don't have an answer as to why after at run time we don't find a valid bloblist here the fall-back use cases aren't functioning. To me that seems like the bug to investigate and explain. Reverting code to find when functionality broke is great, I do it all the time. Saying we need to revert because something broke *without* saying why it broke is a problem.
> > > It isn't a particularly large number, if you add up the patches I > > > do in each cycle. It is in the nature of development and code review > > > that things are often not right the first time, or someone else has > > > another perspective, so I cannot see how this can be reduced. > > > > The easy way to reduce it would be to go from 10 series in progress to 1 > > series in progress, and finish that before picking up series number 2, > > and so on. And then in the future, stop when you have more than 2 or 3 > > in progress at once. > > That's obviously not practical for the amount I am contributing and > the length of time it takes us to get things in. I hope it isn't a > serious suggestion!
It's extremely serious. For example https://patchwork.ozlabs.org/project/uboot/list/?series=435516&state=* is a 46 part patch series that touches 3 or 4 different areas and the only type of review that's feasible is "Does this cause the binary to grow too much? Or in unexpected ways?" And that sets aside that I believe I did ask you to rework "bootm: Allow building bootm.c without CONFIG_SYS_BOOTM_LEN" to refactor things so that SYS_BOOTM_LEN is always defined. That's not even a hard change, I say as someone that had to do that as part of Kconfig migration of oddly (ab)used symbol names.
But in this case, your series that touch areas of code actively maintained by other people is even more relevant. Whereas areas without other active developers, or where you are the main developer, I can find some smaller limits on what must be reviewed all of your *EFI* has other active developers on it. Who are providing technical feedback, not arbitrary or stylistic feedback. Please be explicit about what you are suggesting in this case, so I do not mischaracterize your intention.
> > But really, it feels like "Code needs to land" is another way of saying > > "Just a NAK should be ignored". > > Yes, a bias more towards that would be more healthy IMO. A NAK needs > to come with a full understanding of the use case, an alternative way > to meet the use case, once that doesn't involve boiling the Pacific > Ocean.
As it stands, I do not like to, but sometimes feel I *have*to* break one board to unbreak another board and keep reminding both parties to resolve the issue so no board is broken. There has been, since I have taken over the lead of the project, at least once where I have over-ruled the relevant custodian and that has resulted in the custodian leaving. I both feel that was the right technical call, and regret it came to that, still. I do not want to adopt a more broad use of that policy. The default for absence of agreement must be to try again.
I see that Peter sent a few indignant emails. You can't win.
I assume this is about the LED series, and I further assume you forgot / missed where he explained that removing the legacy code is a functional user visible usability regression on the pinephone.
It seems you are saying that we can break my boards completely, but Peter must have his flashing LEDs?
Well, as I've stated many times and in many threads, I do not like breaking one platform to fix another. And yes, I did think your three platforms were functional and you didn't like the solution we had come to. But also, pinephone, U-Boot is the firmware stack it ships with. Phones tend to not have a lot of easily usable user-visible feedback options. So yes, I really do not want to remove the early indications that the device is alive / working (or to report it's not working!) when we've also just recently added support for what they're missing (because another new device, the openwrt one, has similar user visible requirements). So to be clear, there's 5 platforms using the legacy code. 1 of which someone is actively trying to in their spare time port to the new codebase. I do not see the urgency in removing the legacy code right now. I also do not see the point in gating the removal after pinephone works (unless someone steps up for the other platforms, but it's also likely a working conversion will make it easier to see why the attempted quick conversion did NOT work, and so we redo the other 4 based on what we've learned and hope they do now work).
I don't really disagree with any of that.
I'm sure.
It might encourage bad behaviour though, e.g. a time-pressed maintainer pops his head up just long enough to cast doubt on patches, then heads off safe in the knowledge that nothing will happen for a few more months.
We can worry about that if it ever happens.
Hi Simon,
On Fri, 6 Dec 2024 at 21:19, Simon Glass sjg@chromium.org wrote:
Hi Tom,
On Wed, 4 Dec 2024 at 09:27, Tom Rini trini@konsulko.com wrote:
On Wed, Dec 04, 2024 at 08:13:04AM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 3 Dec 2024 at 18:29, Tom Rini trini@konsulko.com wrote:
On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote:
From my side I'd like to change the conversation a little, to how to land code, rather than why we should bother. "Code needs to land" should be the motto. If someone has taken the time to create something, our bias should be towards getting it in, with sufficient changes to make it fit the project. There are cases where something is just a bad idea, or should be done another way, but for people working on major features or changes, biasing towards not landing the code is just going to make them go elsewhere.
This is the wrong approach I believe. The goal has always been and continues to be to have reviewed (whenever possible, our developer community is small) incremental change over time.
Yes, I agree with that, but it isn't what I said.
I don't know how else to interpret "Code needs to land". I'm not sure what reviewed patches we have that are outstanding, but also not fairly new. We do have patches outstanding, in general. Much of those fall in to the categories of:
- Custodian is active, but also very busy (virtually everyone is a volunteer so I have a hard time getting forceful with people that have a large queue).
- Custodian isn't active / no direct custodian and the code hasn't been reviewed by anyone else.
Here's my intention with 'code needs to land': to bias against people saying "I don't like this; I don't care about your use case; please go away"
Just because something has been posted a number of times does not mean it's ready to be merged.
I didn't say that either.
It's an impression you give however when you repost a series less than a day after last posting, without addressing all of the feedback.
I'm going to ignore that comment as it is not helpful and mischaracterises my contributions.
I don't know how that mischaracterizes your contributions. No one is trying to debase your contributions or be unappreciative of your efforts.
But having 6-7 simultaneous patchsets of 30+ patches is simply impossible to track. So you do miss addressing feedback, but no one said it's done deliberately. I am pretty sure it's just natural due to the large volume of patches. It would be *way* easier for reviewers and you if you tried to send smaller patchesets that you could reliably track, discuss, and address.
On a personal note, it's frustrating for me because I constantly need to go back and forth between older conversations to figure out what comments you decided to address on revision X. It takes 3x of the usual time I need to review other patches and on top of that reviewing a series of 30+ patches takes time and takes more time to merge because it's simply a lot of code to review. So my suggestion is to spend some time and think how to make the *reviewers* life easier instead of yours.
Your challenge today is that on patchwork you have over 150 patches covering a wide variety of topics and of which many series have technically-merited feedback that needs to be addressed in a technical manner.
By my count I have about 10 series in progress, with a small number of patches (< 10?) with pending feedback
I'm not sure about that less than 10 number, in part because it's hard to keep track of which feedback was applied, and which not. And part because some of those series are places where I told you I'm unsure about the core concept but you asked and I'm giving you leeway to show me the end result.
OK
that isn't effectively just a NAK.
To be clear, "just a NAK" isn't the whole story. Those NAKs come with technical rationales and requests. But maybe they're just lost in the volume of emails you have in some cases. I know for example I mentioned a few times and places that we have tests today for booting the OS, when you mention that we need to add a test for booting the OS. We need to expand that test, yes.
If you look at the OF_BLOBLIST thing, it is basically a NAK. I don't see any other way to interpret it,. So several boards in my lab have been broken for a year.
I haven't had time to look into that series, but I will. The problem with reverts like that is - I get the feeling you need some special code for some special ancient chromebooks that no one cares or uses apart from you. That's problematic overall. We can't just make a project look weird because some hardware is special. It's the hardware that has to adapt, not the other way around - Reverts need to have a *very solid* explanation.
For example, this is your x86 revert patches https://lore.kernel.org/u-boot/20241112131830.576864-1-sjg@chromium.org/ and this is what actually got merged https://lore.kernel.org/u-boot/20241129170814.768438-1-ilias.apalodimas@lina... Which one would you rather read a year from now debugging a similar issue?
Another example is the fdt sutff here https://lore.kernel.org/u-boot/20241201144240.1664398-5-sjg@chromium.org/ Tom, Raymond and I spent a lot of our time explaining why we need this feature like this and what's the technical reason behind the decision. Your commit message basically says "I know better and you should all listen to my almighty ideas and btw this breaks 3 special ancient platforms, so I am reverting this". Now I don't know who Kevin and BoB are and I wish them luck, but that's hardly a readable commit message or will mean anything in a year from now.
The way I read this commit message is that you show zero respect for the effort all of the other people put into code explaining and documenting the feature, your 'justification' has no technical background whatsoever and then you come back complaining that people don't appreciate your efforts.
/Ilias
It isn't a particularly large number, if you add up the patches I do in each cycle. It is in the nature of development and code review that things are often not right the first time, or someone else has another perspective, so I cannot see how this can be reduced.
The easy way to reduce it would be to go from 10 series in progress to 1 series in progress, and finish that before picking up series number 2, and so on. And then in the future, stop when you have more than 2 or 3 in progress at once.
That's obviously not practical for the amount I am contributing and the length of time it takes us to get things in. I hope it isn't a serious suggestion!
But really, it feels like "Code needs to land" is another way of saying "Just a NAK should be ignored".
Yes, a bias more towards that would be more healthy IMO. A NAK needs to come with a full understanding of the use case, an alternative way to meet the use case, once that doesn't involve boiling the Pacific Ocean.
Regards, Simon
Hi Ilias,
On Sat, 7 Dec 2024 at 01:18, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Fri, 6 Dec 2024 at 21:19, Simon Glass sjg@chromium.org wrote:
Hi Tom,
On Wed, 4 Dec 2024 at 09:27, Tom Rini trini@konsulko.com wrote:
On Wed, Dec 04, 2024 at 08:13:04AM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 3 Dec 2024 at 18:29, Tom Rini trini@konsulko.com wrote:
On Mon, Dec 02, 2024 at 05:21:05PM -0700, Simon Glass wrote:
From my side I'd like to change the conversation a little, to how to land code, rather than why we should bother. "Code needs to land" should be the motto. If someone has taken the time to create something, our bias should be towards getting it in, with sufficient changes to make it fit the project. There are cases where something is just a bad idea, or should be done another way, but for people working on major features or changes, biasing towards not landing the code is just going to make them go elsewhere.
This is the wrong approach I believe. The goal has always been and continues to be to have reviewed (whenever possible, our developer community is small) incremental change over time.
Yes, I agree with that, but it isn't what I said.
I don't know how else to interpret "Code needs to land". I'm not sure what reviewed patches we have that are outstanding, but also not fairly new. We do have patches outstanding, in general. Much of those fall in to the categories of:
- Custodian is active, but also very busy (virtually everyone is a volunteer so I have a hard time getting forceful with people that have a large queue).
- Custodian isn't active / no direct custodian and the code hasn't been reviewed by anyone else.
Here's my intention with 'code needs to land': to bias against people saying "I don't like this; I don't care about your use case; please go away"
Just because something has been posted a number of times does not mean it's ready to be merged.
I didn't say that either.
It's an impression you give however when you repost a series less than a day after last posting, without addressing all of the feedback.
I'm going to ignore that comment as it is not helpful and mischaracterises my contributions.
I don't know how that mischaracterizes your contributions. No one is trying to debase your contributions or be unappreciative of your efforts.
But having 6-7 simultaneous patchsets of 30+ patches is simply impossible to track. So you do miss addressing feedback, but no one said it's done deliberately. I am pretty sure it's just natural due to the large volume of patches. It would be *way* easier for reviewers and you if you tried to send smaller patchesets that you could reliably track, discuss, and address.
On a personal note, it's frustrating for me because I constantly need to go back and forth between older conversations to figure out what comments you decided to address on revision X. It takes 3x of the usual time I need to review other patches and on top of that reviewing a series of 30+ patches takes time and takes more time to merge because it's simply a lot of code to review. So my suggestion is to spend some time and think how to make the *reviewers* life easier instead of yours.
I find it hard to review subsequent revisions, too. I tend to assume that people have made the changes and I use the per-patch change log to see that.
If this were the core issue, I believe I could improve it with better tooling, to keep track of lots of smaller series.
Your challenge today is that on patchwork you have over 150 patches covering a wide variety of topics and of which many series have technically-merited feedback that needs to be addressed in a technical manner.
By my count I have about 10 series in progress, with a small number of patches (< 10?) with pending feedback
I'm not sure about that less than 10 number, in part because it's hard to keep track of which feedback was applied, and which not. And part because some of those series are places where I told you I'm unsure about the core concept but you asked and I'm giving you leeway to show me the end result.
OK
that isn't effectively just a NAK.
To be clear, "just a NAK" isn't the whole story. Those NAKs come with technical rationales and requests. But maybe they're just lost in the volume of emails you have in some cases. I know for example I mentioned a few times and places that we have tests today for booting the OS, when you mention that we need to add a test for booting the OS. We need to expand that test, yes.
If you look at the OF_BLOBLIST thing, it is basically a NAK. I don't see any other way to interpret it,. So several boards in my lab have been broken for a year.
I haven't had time to look into that series, but I will. The problem with reverts like that is
- I get the feeling you need some special code for some special
ancient chromebooks that no one cares or uses apart from you. That's problematic overall. We can't just make a project look weird because some hardware is special. It's the hardware that has to adapt, not the other way around
- Reverts need to have a *very solid* explanation.
For example, this is your x86 revert patches https://lore.kernel.org/u-boot/20241112131830.576864-1-sjg@chromium.org/ and this is what actually got merged https://lore.kernel.org/u-boot/20241129170814.768438-1-ilias.apalodimas@lina... Which one would you rather read a year from now debugging a similar issue?
Reverts are done to fix regressions. We don't need to justify a revert that breaks something. It is just a revert. You can see how this should be done at [1]. Tom applied it within a day, without any fuss, as he should. No, this is not a core issue either.
Another example is the fdt sutff here https://lore.kernel.org/u-boot/20241201144240.1664398-5-sjg@chromium.org/ Tom, Raymond and I spent a lot of our time explaining why we need this feature like this and what's the technical reason behind the decision. Your commit message basically says "I know better and you should all listen to my almighty ideas and btw this breaks 3 special ancient platforms, so I am reverting this".
This is a deflection at best. WIth OF_BLOBLIST, you can just enable that option and get what you want. You can even make it the default in U-Boot. The issue is that you don't want to have the option. But without the ability to turn the option off, my special, ancient platforms, that no one cares about except me, break.
Now I don't know who Kevin and BoB are and I wish them luck, but that's hardly a readable commit message or will mean anything in a year from now.
chromebook_kevin and chromebook_bob are a rk3399-based Chromebooks
Again, the commit message is not a core issue. If you found yourself in a reflective mood, you might want to consider how I feel about 5-6 platforms which I care about a lot, being permanently broken in Tom's tree.
The way I read this commit message is that you show zero respect for the effort all of the other people put into code explaining and documenting the feature, your 'justification' has no technical background whatsoever and then you come back complaining that people don't appreciate your efforts.
Obviously you are entitled to your opinions, but I don't agree, sorry. I doubt anyone is particularly interested in my thoughts on this, so I'll refrain from commenting further.
Just to confirm, nothing in your email appears to address any of my concerns. If it was intended to, please point out what I missed.
/Ilias
It isn't a particularly large number, if you add up the patches I do in each cycle. It is in the nature of development and code review that things are often not right the first time, or someone else has another perspective, so I cannot see how this can be reduced.
The easy way to reduce it would be to go from 10 series in progress to 1 series in progress, and finish that before picking up series number 2, and so on. And then in the future, stop when you have more than 2 or 3 in progress at once.
That's obviously not practical for the amount I am contributing and the length of time it takes us to get things in. I hope it isn't a serious suggestion!
But really, it feels like "Code needs to land" is another way of saying "Just a NAK should be ignored".
Yes, a bias more towards that would be more healthy IMO. A NAK needs to come with a full understanding of the use case, an alternative way to meet the use case, once that doesn't involve boiling the Pacific Ocean.
- SImon
[1] https://patchwork.ozlabs.org/project/uboot/patch/20241201144240.1664398-8-sj...
Picking just a few points..
On Thu, Dec 12, 2024 at 07:04:54AM -0700, Simon Glass wrote:
Hi Ilias,
On Sat, 7 Dec 2024 at 01:18, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[snip]
I haven't had time to look into that series, but I will. The problem with reverts like that is
- I get the feeling you need some special code for some special
ancient chromebooks that no one cares or uses apart from you. That's problematic overall. We can't just make a project look weird because some hardware is special. It's the hardware that has to adapt, not the other way around
- Reverts need to have a *very solid* explanation.
For example, this is your x86 revert patches https://lore.kernel.org/u-boot/20241112131830.576864-1-sjg@chromium.org/ and this is what actually got merged https://lore.kernel.org/u-boot/20241129170814.768438-1-ilias.apalodimas@lina... Which one would you rather read a year from now debugging a similar issue?
Reverts are done to fix regressions. We don't need to justify a revert that breaks something. It is just a revert. You can see how this should be done at [1]. Tom applied it within a day, without any fuss, as he should. No, this is not a core issue either.
You most certainly do need to justify a revert. More often than not, reverting a change to fix one platform breaks another platform. See for example the whole discussion around SPI-NOR changes that may or may not remain in v2025.01 at this point because they enable some platforms and broke others, and only just about now *may* have fixed the others. The revert you note in [1] is another good example of how and who needs to justify a revert. You wrote 2e9313179a84 ("global_data: Drop spl_handoff") and so it's a lot easier for me to accept a revert from you. I assume that you tested all of the cases and applied it. I'm likely to apply https://patchwork.ozlabs.org/project/uboot/patch/20241209161434.6563-2-clamo... as soon as Svyatoslav says it's ready or asks because that's also their code. And if you want to get picky about it, I do wish your revert in [1] was more detailed about what was broken afterall, assuming a rework of the intended change is coming, so it's clear what was wrong, not just that it was wrong. A revert (and git bisect) are great for finding a problem, but only sometimes the *fix* for a problem.
[snip]
Now I don't know who Kevin and BoB are and I wish them luck, but that's hardly a readable commit message or will mean anything in a year from now.
chromebook_kevin and chromebook_bob are a rk3399-based Chromebooks
Again, the commit message is not a core issue. If you found yourself in a reflective mood, you might want to consider how I feel about 5-6 platforms which I care about a lot, being permanently broken in Tom's tree.
Commit message are a core issue, because they need to be clear (and not strictly concise) about what a change is for and what a platform requires. We're a community and the written word, including commit messages is how we tell everyone else things. Including ourselves a year down the line so we can see why we did what we did.
On Thu, Dec 12, 2024 at 07:04:54AM -0700, Simon Glass wrote:
Hi Ilias,
On Sat, 7 Dec 2024 at 01:18, Ilias Apalodimas
[snip]
Another example is the fdt sutff here https://lore.kernel.org/u-boot/20241201144240.1664398-5-sjg@chromium.org/ Tom, Raymond and I spent a lot of our time explaining why we need this feature like this and what's the technical reason behind the decision. Your commit message basically says "I know better and you should all listen to my almighty ideas and btw this breaks 3 special ancient platforms, so I am reverting this".
This is a deflection at best. WIth OF_BLOBLIST, you can just enable that option and get what you want. You can even make it the default in U-Boot. The issue is that you don't want to have the option. But without the ability to turn the option off, my special, ancient platforms, that no one cares about except me, break.
Now I don't know who Kevin and BoB are and I wish them luck, but that's hardly a readable commit message or will mean anything in a year from now.
chromebook_kevin and chromebook_bob are a rk3399-based Chromebooks
Again, the commit message is not a core issue. If you found yourself in a reflective mood, you might want to consider how I feel about 5-6 platforms which I care about a lot, being permanently broken in Tom's tree.
Having just this morning figured out the problem with those chromebooks (because they're available in CI, so thank you for that work), I think this is also a good example of one of the problems. My recollection of the threads from last year does not include you mentioning that on these three platforms the issue is that you put the fixed bloblist address in DRAM *and* that SPL initializes DRAM here (not TPL, not TF-A) that's the concern. My recollection of the threads is you had very different technical reasons for wanting what you proposed and those technical reasons had technical objections. I've posted https://patchwork.ozlabs.org/project/uboot/list/?series=436459&state=* now and that fixes those three platforms. And we could have done that a year ago if you had explained the problem on those platforms. I would have asked why not putting the bloblist in IRAM instead, at that point, and you may have had an answer, but that would have still left coral to sort out.
And how is this part of the problem? You cannot suggest architectural designs without explaining the technical details of what and why you want something. We all spent countless hours arguing about some feature of bloblist design which zero platforms actually need.
The way I read this commit message is that you show zero respect for the effort all of the other people put into code explaining and documenting the feature, your 'justification' has no technical background whatsoever and then you come back complaining that people don't appreciate your efforts.
Obviously you are entitled to your opinions, but I don't agree, sorry. I doubt anyone is particularly interested in my thoughts on this, so I'll refrain from commenting further.
Just to confirm, nothing in your email appears to address any of my concerns. If it was intended to, please point out what I missed.
No, this is another example of the problems. You've been given feedback here, which is that your communication skills are a problem, and your response is to say you don't agree.
Hi Heinrich,
On Sun, 1 Dec 2024 at 17:24, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 2. Dezember 2024 01:12:36 MEZ schrieb Simon Glass sjg@chromium.org:
It is a bit of a pain to log EFI boot-services calls at present. The output goes to the console so cannot easily be inspected later. Also it would be useful to be able to store the log and review it later, perhaps after something has gone wrong.
This series makes a start on implementing a log-to-buffer feature. It provides a simple 'efidebug log' command to inspect the buffer. For now, only memory allocations are logged.
In my previous review I suggested to move your work to the level of the logging library lib/log.c.
Well, we could do that, if we actually have a need for programmatic logging of other subsystems.
Why are you reiterating your series without exposing why an EFI specific solution. should be desireable?
I don't see that need yet, or at least, not for the purpose I am heading towards. In any case, we can easily refactor the code later if it is needed. For now I believe that this logging feature will help to tame some of the troubles we have.
[..]
Regards, Simon
Am 3. Dezember 2024 01:23:26 MEZ schrieb Simon Glass sjg@chromium.org:
Hi Heinrich,
On Sun, 1 Dec 2024 at 17:24, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 2. Dezember 2024 01:12:36 MEZ schrieb Simon Glass sjg@chromium.org:
It is a bit of a pain to log EFI boot-services calls at present. The output goes to the console so cannot easily be inspected later. Also it
You could use U-Boot's syslog driver. Or Linux tee command to solve the problem.
Both would allow post mortem analysis. Post mortem analysis is not possible when using a buffer in U-Boot.
So this series seems to be less then ideal.
would be useful to be able to store the log and review it later, perhaps after something has gone wrong.
This series makes a start on implementing a log-to-buffer feature. It provides a simple 'efidebug log' command to inspect the buffer. For now, only memory allocations are logged.
In my previous review I suggested to move your work to the level of the logging library lib/log.c.
Well, we could do that, if we actually have a need for programmatic logging of other subsystems.
Just do it right the first time.
Best regards
Heinrich
Why are you reiterating your series without exposing why an EFI specific solution. should be desireable?
I don't see that need yet, or at least, not for the purpose I am heading towards. In any case, we can easily refactor the code later if it is needed. For now I believe that this logging feature will help to tame some of the troubles we have.
[..]
Regards, Simon
participants (4)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Simon Glass -
Tom Rini