[PATCH v2] efi_loader: Trigger capsule updates with automatically generated boot options

[Heinrich Schuchardt]

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.

> 
>>
>>> @@ -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.

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
>>>
>>