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

[Ilias Apalodimas]

Hi Heinrich,

[...]



> >
> >   /**
> > - * device_is_present_and_system_part - check if a device exists
> > + * get_esp_handle - check if a device exists and contains an ESP
>
> The logic looks ok. Yet, the descriptions need rework.
>
> "check if a device exists and contains an ESP":
> This is not what this function does. It checks if a device has an ESP as
> immediate child.

Not only. If the provided DP is a pointing to a file in the ESP in
will exit before trying to probe any of the disk children.

>
> >    *
> >    * Check if a device pointed to by the device path, @dp, exists and is
> > - * located in UEFI system partition.
> > + * either an ESP or a disk containing an ESP.
>
> Nowhere do you check if the device is a disk. The end node of @dp could
> be anything.
>
> An NVMe drive may have many namespaces, a SCSI drive many LUNs.
>
> The partition is not the child of the drive but of the namespace or the LUN.
>
> An NVMe(NSID,EUI) node in a device-path is for a namespace not for a
> drive. Similarly an Scsi(PUN,LUN) node is for the LUN and not for the
> drive (PUN).
>
> Please, provide an accurate function description.

I am not sure I understand what you are expecting here.
We treat disks and partitions as a 'udevice'. What the function does
is look at the provided DP and get the associated handle for the
device to look at the device children.
I haven't looked closely into SCSI or NVMe device paths. Do you have
any suggestions?

>
> >    *
> >    * @dp              device path
> > - * Return:   true - yes, false - no
> > + * Return:   ESP handle or NULL
> >    */
> > -static bool device_is_present_and_system_part(struct efi_device_path *dp)
> > +efi_handle_t get_esp_handle(struct efi_device_path *dp)
> >   {
> > -     efi_handle_t handle;
> > +     efi_handle_t handle, dev_handle;
> > +     struct udevice *child_dev;
> >       struct efi_device_path *rem;
> > +     efi_status_t ret;
> >
> >       /* Check device exists */
> > -     handle = efi_dp_find_obj(dp, NULL, NULL);
> > -     if (!handle)
> > -             return false;
> > +     dev_handle = efi_dp_find_obj(dp, NULL, NULL);
> > +     if (!dev_handle)
> > +             return NULL;
> >
> >       /* Check device is on system partition */
>
> Should this be
> "Check if the device path points to an EFI system partition"?

This checks if the DP is on a partition with the ESP installed.
'device' is the udevice u-boot defines.
I don't mind changing it.

>
> >       handle = efi_dp_find_obj(dp, &efi_system_partition_guid, &rem);
> > -     if (!handle)
> > -             return false;
> > +     if (handle)
> > +             return handle;
> > +
> > +     /* If it's a disk check its children */
>
> The object that you check may be anything, not necessarily a disk.

efi_disk_create_raw() and efi_disk_create_part() are used to create
the EFI tags. What else can it be?
Shall I just remove the comment?

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>