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

[Ilias Apalodimas]

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?

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

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