[PATCH v2] efi_loader: Trigger capsule updates with automatically generated boot options
Ilias Apalodimas
ilias.apalodimas at linaro.org
Mon Dec 15 11:28:57 CET 2025
On Sun, 14 Dec 2025 at 13:22, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 12/14/25 08:36, Ilias Apalodimas wrote:
> > Hi Heinrich
> >
> > [...]
> >
> >>> /*
> >>> * Determine if an MMC device is an SD card.
> >>> @@ -109,10 +122,11 @@ struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp)
> >>> */
> >>> static efi_handle_t find_handle(struct efi_device_path *dp,
> >>> const efi_guid_t *guid, bool short_path,
> >>> - struct efi_device_path **rem)
> >>> + struct efi_device_path **rem,
> >>> + enum match match)
> >>> {
> >>> efi_handle_t handle, best_handle = NULL;
> >>> - efi_uintn_t len, best_len = 0;
> >>> + efi_uintn_t len, match_len = 0, best_len = 0;
> >>>
> >>> len = efi_dp_instance_size(dp);
> >>>
> >>
> >> Where is the documentation update for find_handle()?
> >
> > The description is in efi_dp_find_obj() which is the only place
> > find_handle() is called. You want to add something in find_handle() as
> > well?
>
> I would like the function find_handle() to be documented in a way that
> would allow to review its functionality irrespective of where it is used.
Fair enough, I'll update both functions in v3
>
> >
> >>
> >>> @@ -137,22 +151,35 @@ static efi_handle_t find_handle(struct efi_device_path *dp,
> >>> if (!dp_current)
> >>> continue;
> >>> }
> >>> - len_current = efi_dp_instance_size(dp_current);
> >>> - if (rem) {
> >>
> >> Before this patch calling efi_dp_find_obj(rem == NULL) would only return
> >> exact matches.
> >>
> >> With the patch efi_dp_find_obj(rem == NULL) matches shorter paths.
> >>
> >> Your commit message does not describe that you want to change the logic
> >> of efi_fs_from_path().
> >
> > you mean efi_dp_find_obj()? I do mention the current functionality and
> > how it changes. There's a paragraph in the commit message that
> > explains that the function currently decides on the exact match based
> > on the validity of the 'rem ptr and that an an enum is added. If it's
> > unclear i'll reword it.
>
> I have created
>
> [PATCH 1/1] efi_selftest: enhance LoadImage test
> https://lore.kernel.org/u-boot/20251214110937.136425-1-heinrich.schuchardt@canonical.com/T/#u
>
> which demonstrates the unintended side effect which could not have been
> derived from the code comments or the commit message.
Ok thanks that helps.
The problem is not re-organizing efi_dp_find_obj() to search for
1. exact long path
2. exact short path
3. longest long path with dp as start
4. longest short path with dp as start
Instead of
1. exact long path
2. longest long path with dp as start
3. exact short path
4. longest short path with dp as start
In fact I think this change is correct and we should keep it. Exact
DPs should always take priority when searching for them.
The problem is that efi_dp_find_obj() will now do a best match even if
rem is NULL. IOW find_handle() is working correctly, but
efi_dp_find_obj() changes its behavior. Adding a rem check fixes the
issue
Something like this:
@@ -194,7 +194,7 @@ efi_handle_t efi_dp_find_obj(struct efi_device_path *dp,
handle = find_handle(dp, guid, false, rem, EXACT);
if (!handle)
handle = find_handle(dp, guid, true, rem, EXACT);
- if (handle)
+ if (handle || !rem)
return handle;
handle = find_handle(dp, guid, false, rem, SHORTER);
Earlier you proposed
#define LONGER ((void *)(uintptr_t) - 1)
#define SHORTER (NULL)
using *rem == LONGER for allowing longer DPs and *rem == SHORTER for
other cases to avoid the extra argument.
I can't make up my mind which is better.
Cheers
/Ilias
>
> Best regards
>
> Heinrich
>
> >
> >>
> >>> +
> >>> + match_len = len_current = efi_dp_instance_size(dp_current);
> >>> + switch (match) {
> >>> + case EXACT:
> >>> + if (len_current != len)
> >>> + continue;
> >>> + break;
> >>> + case SHORTER:
> >>> if (len_current > len)
> >>> continue;
> >>> - } else {
> >>> - if (len_current != len)
> >>> + break;
> >>> + case LONGER:
> >>> + if (len_current < len)
> >>> continue;
> >>
> >> So here the logic is:
> >>
> >> When search for \a\b;
> >>
> >> \a\b\c\d is a better match than \a\b\c
> >>
> >> Is this intended?
> >> I can't find it in the function description of find_handle().
> >
> > Yes it is, the description is in the efi_dp_find_obj(). You want me to
> > move it around? Or expand find_handle() as well?
> >
> >>
> >>> + match_len = len;
> >>> + break;
> >>> }
> >>> - if (memcmp(dp_current, dp, len_current))
> >>> +
> >>> + if (memcmp(dp_current, dp, match_len))
> >>> continue;
> >>> if (!rem)
> >>> return handle;
> >>> if (len_current > best_len) {
> >>> best_len = len_current;
> >>> best_handle = handle;
> >>> - *rem = (void*)((u8 *)dp + len_current);
> >>> + if (len_current <= len)
> >>> + *rem = (void *)((uintptr_t)dp + len_current);
> >>> + else
> >>> + *rem = (void *)((uintptr_t)dp_current + match_len);
> >>> }
> >>> }
> >>> return best_handle;
> >>> @@ -160,14 +187,25 @@ static efi_handle_t find_handle(struct efi_device_path *dp,
> >>>
> >>> efi_handle_t efi_dp_find_obj(struct efi_device_path *dp,
> >>> const efi_guid_t *guid,
> >>> - struct efi_device_path **rem)
> >>> + struct efi_device_path **rem, bool match_longer)
> >>> {
> >>> efi_handle_t handle;
> >>>
> >>> - handle = find_handle(dp, guid, false, rem);
> >>> + handle = find_handle(dp, guid, false, rem, EXACT);
> >>
> >> Here you are tweaking the logic a lot without really describing it:
> >>
> >> efi_dp_find(rem != NULL) used to match in sequence:
> >>
> >> 1) exact long path
> >> 2) longest long path with dp as start
> >> 3) exact short path
> >> 4) longest short path with dp as start
> >>
> >> now you have
> >>
> >> 1) exact long path
> >> 2) exact short path
> >> 3) longest long path with dp as start
> >> 4) longest short path with dp as start
> >>
> >> This might be intended. But it is neither described in the commit
> >> message nor in the function description.
> >
> > I'll reword the commit message.
> >
> >>
> >>> if (!handle)
> >>> - /* Match short form device path */
> >>> - handle = find_handle(dp, guid, true, rem);
> >>> + handle = find_handle(dp, guid, true, rem, EXACT);
> >>> + if (handle)
> >>> + return handle;
> >>> +
> >>
> >> Check the value of rem here (see comment above).
> >
> > Sure
> >
> >>
> >> Having both rem != NULL and match_longer == true wouldn't make much sense.
> >>
> >> Theorectically we could use *rem == LONGER for allowing longer DPs and
> >> *rem == SHORTER for other cases to avoid the extra argument.
> >>
> >> #define LONGER ((void *)(uintptr_t) - 1)
> >> #define SHORTER (NULL)
> >
> > Sure, but I think the enum is clearer. Unless you really want this
> > I'll keep the enum
> >
> > Thanks
> > /Ilias
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> + handle = find_handle(dp, guid, false, rem, SHORTER);
> >>> + if (!handle)
> >>> + handle = find_handle(dp, guid, true, rem, SHORTER);
> >>> +
> >>> + if (match_longer && !handle) {
> >>> + handle = find_handle(dp, guid, false, rem, LONGER);
> >>> + if (!handle)
> >>> + handle = find_handle(dp, guid, true, rem, LONGER);
> >>> + }
> >>>
> >>> return handle;
> >>> }
> >>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> >>> index 130c4db9606f..0d3d7c187d72 100644
> >>> --- a/lib/efi_loader/efi_disk.c
> >>> +++ b/lib/efi_loader/efi_disk.c
> >>> @@ -339,7 +339,7 @@ efi_fs_from_path(struct efi_device_path *full_path)
> >>> efi_free_pool(file_path);
> >>>
> >>> /* Get the EFI object for the partition */
> >>> - efiobj = efi_dp_find_obj(device_path, NULL, NULL);
> >>> + efiobj = efi_dp_find_obj(device_path, NULL, NULL, false);
> >>> efi_free_pool(device_path);
> >>> if (!efiobj)
> >>> return NULL;
> >>> --
> >>> 2.43.0
> >>>
> >>
>
More information about the U-Boot
mailing list