On Tue, May 19, 2026 at 07:30:53PM -0400, Raymond Mao wrote: > Hi Tom, > > On Tue, May 19, 2026 at 12:23 PM Tom Rini <[email protected]> wrote: > > > > With bloblist, we need to both see if one already exists as well as > > create one if it does not. However, the current implementation leads to > > odd cases where we attempt to create a bloblist before this is possible > > and have things be overly complicated when we are given one to work > > with. > > > > This reworks things to instead have a bloblist_exists function, which as > > the name implies checks for an existing bloblist. This is used in > > the case of booting, to see if we have one and in turn if we have a > > device tree there as well as in the bloblist_init function to see if we > > need to do anything. > > > > In practical details, we move the logic from bloblist_init that was > > checking for a bloblist to the new bloblist_exists function and then can > > clarify the logic as it is much easier to state when we know we do not > > have one rather than all the ways we might have one. Then we have the > > locations that set gd->bloblist now also set the GD_FLG_BLOBLIST_READY > > flag. > > > > Signed-off-by: Tom Rini <[email protected]> > > --- > > Changes in v2: > > - As noted by Raymond, we need to still check if SPL is being passed a > > bloblist by TPL. > > --- > > common/bloblist.c | 144 +++++++++++++++++++++++---------------------- > > common/board_f.c | 4 +- > > include/bloblist.h | 32 +++++----- > > lib/fdtdec.c | 15 ++--- > > 4 files changed, 96 insertions(+), 99 deletions(-) > > > > diff --git a/common/bloblist.c b/common/bloblist.c > > index 09afdd1f96b4..51ae9cc50a5f 100644 > > --- a/common/bloblist.c > > +++ b/common/bloblist.c > > @@ -448,6 +448,7 @@ int bloblist_new(ulong addr, uint size, uint flags, > > uint align_log2) > > hdr->align_log2 = align_log2 ? align_log2 : > > BLOBLIST_BLOB_ALIGN_LOG2; > > hdr->chksum = 0; > > gd->bloblist = hdr; > > + gd->flags |= GD_FLG_BLOBLIST_READY; > > > > return 0; > > } > > @@ -475,6 +476,7 @@ int bloblist_check(ulong addr, uint size) > > return log_msg_ret("Bad checksum", -EIO); > > } > > gd->bloblist = hdr; > > + gd->flags |= GD_FLG_BLOBLIST_READY; > > > > return 0; > > } > > @@ -576,90 +578,88 @@ int __weak xferlist_from_boot_arg(ulong > > __always_unused *addr) > > return -ENOENT; > > } > > > > -int bloblist_init(void) > > +bool bloblist_exists(void) > > { > > - bool fixed = IS_ENABLED(CONFIG_BLOBLIST_FIXED); > > - int ret = 0; > > - ulong addr = 0, size; > > + int ret; > > + ulong addr = 0; > > > > /* Check if a valid transfer list passed in */ > > - if (!xferlist_from_boot_arg(&addr)) { > > - size = bloblist_get_total_size(); > > - } else { > > - /* > > - * If U-Boot is not in the first phase, an existing > > bloblist must > > - * be at a fixed address. > > - */ > > - bool from_addr = fixed && !xpl_is_first_phase(); > > - > > - /* > > - * If Firmware Handoff is mandatory but no transfer list is > > - * observed, report it as an error. > > - */ > > - if (IS_ENABLED(CONFIG_BLOBLIST_PASSAGE_MANDATORY)) > > - return -ENOENT; > > - > > - ret = -ENOENT; > > - > > - if (xpl_prev_phase() == PHASE_TPL && > > - !IS_ENABLED(CONFIG_TPL_BLOBLIST)) > > - from_addr = false; > > - if (fixed) > > - addr = IF_ENABLED_INT(CONFIG_BLOBLIST_FIXED, > > - CONFIG_BLOBLIST_ADDR); > > - size = CONFIG_BLOBLIST_SIZE; > > - > > - if (from_addr) > > - ret = bloblist_check(addr, size); > > + if (!xferlist_from_boot_arg(&addr)) > > + goto found; > > > > - if (ret) > > - log_debug("Bloblist at %lx not found (err=%d)\n", > > - addr, ret); > > - else > > - /* Get the real size */ > > - size = gd->bloblist->total_size; > > - } > > + /* > > + * If Firmware Handoff is mandatory but no transfer list is > > + * observed, report it as an error. > > + */ > > + if (IS_ENABLED(CONFIG_BLOBLIST_PASSAGE_MANDATORY)) > > + return false; > > > > - if (ret) { > > - /* > > - * If we don't have a bloblist from a fixed address, or the > > one > > - * in the fixed address is not valid. we must allocate the > > - * memory for it now. > > - */ > > - if (CONFIG_IS_ENABLED(BLOBLIST_ALLOC)) { > > - void *ptr = memalign(BLOBLIST_ALIGN, size); > > - > > - if (!ptr) > > - return log_msg_ret("alloc", -ENOMEM); > > - addr = map_to_sysmem(ptr); > > - } else if (!fixed) { > > - return log_msg_ret("BLOBLIST_FIXED is not enabled", > > - ret); > > - } > > - log_debug("Creating new bloblist size %lx at %lx\n", size, > > - addr); > > - ret = bloblist_new(addr, size, 0, 0); > > - } else { > > - log_debug("Found existing bloblist size %lx at %lx\n", size, > > - addr); > > - } > > + /* > > + * We have checked for a valid transfer list being passed. At this > > + * point, if we do not have a fixed address for the bloblist, we > > cannot > > + * be provided with one. > > + */ > > + if (xpl_is_first_phase() || !IS_ENABLED(CONFIG_BLOBLIST_FIXED)) > > + return false; > > > > - if (ret) > > - return log_msg_ret("ini", ret); > > - gd->flags |= GD_FLG_BLOBLIST_READY; > > + /* > > + * Check for a valid list as the configured address. > > + */ > > + addr = IF_ENABLED_INT(CONFIG_BLOBLIST_FIXED, > > + CONFIG_BLOBLIST_ADDR); > > + ret = bloblist_check(addr, CONFIG_BLOBLIST_SIZE); > > + if (!ret) > > + goto found; > > + > > + log_debug("Bloblist at %lx not found (err=%d)\n", addr, ret); > > + return false; > > > > +found: > > #ifdef DEBUG > > bloblist_show_stats(); > > bloblist_show_list(); > > #endif > > - > > - return 0; > > + return true; > > } > > > > -int bloblist_maybe_init(void) > > +int bloblist_init(void) > > { > > - if (CONFIG_IS_ENABLED(BLOBLIST) && !(gd->flags & > > GD_FLG_BLOBLIST_READY)) > > - return bloblist_init(); > > + int ret; > > + ulong addr = 0, size = CONFIG_BLOBLIST_SIZE; > > + > > + if (gd->flags & GD_FLG_BLOBLIST_READY) { > > + log_debug("Found existing bloblist size %x at %p\n", > > + gd->bloblist->total_size, gd->bloblist); > > + return 0; > > + } > > + > > This part is a bit difficult to read. > The patch seems to try to assume that '!(gd->flags & > GD_FLG_BLOBLIST_READY)' means 'no handoff from the previous stage',
Correct. > but I doubt this is a part of GD_FLG_BLOBLIST_READY's semantics. It is part of the semantics now, however with this patch. That said, I don't love the name GD_FLG_BLOBLIST_READY but that name was a side effect of the old behavior too and needing to bail out from "maybe_init". > Under the 'handoff from the previous stage' scenario, if > GD_FLG_BLOBLIST_READY is not set in time, the logic here breaks. Now that "exists" is not part of "init", we should have already called bloblist_check and set the flag before we can get here. -- Tom
signature.asc
Description: PGP signature
