[PATCH v2 4/7] efi_loader: Add a version of efi_binary_run() with more parameters

[Simon Glass]

Hi Ilias,

On Thu, 12 Dec 2024 at 08:45, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Thu, 12 Dec 2024 at 15:45, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 12 Dec 2024 at 00:49, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > On Thu, 12 Dec 2024 at 00:38, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > This uses a few global variables at present. With the bootflow we have
> > > > the required parameters, so add a function which accepts these. Update
> > > > the existing function to call the new one with the globals.
> > > >
> > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  lib/efi_loader/efi_bootbin.c | 29 +++++++++++++++++++++++++----
> > > >  1 file changed, 25 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c
> > > > index 8febd325f34..8ebd48547cb 100644
> > > > --- a/lib/efi_loader/efi_bootbin.c
> > > > +++ b/lib/efi_loader/efi_bootbin.c
> > > > @@ -209,18 +209,22 @@ out:
> > > >  }
> > > >
> > > >  /**
> > > > - * efi_binary_run() - run loaded UEFI image
> > > > + * efi_binary_run_() - run loaded UEFI image
> > > >   *
> > > >   * @image_ptr: memory address of the UEFI image
> > > >   * @size:      size of the UEFI image
> > > >   * @fdt:       device-tree
> > > > + * @device:    EFI device-path
> > > > + * @image:     EFI image-path
> > > >   *
> > > >   * Execute an EFI binary image loaded at @image.
> > > >   * @size may be zero if the binary is loaded with U-Boot load command.
> > > >   *
> > > >   * Return:     status code
> > > >   */
> > > > -efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt)
> > > > +efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
> > > > +                            struct efi_device_path *device,
> > > > +                            struct efi_device_path *image)
> > > >  {
> > >
> > > This needs to be static here. Not the next patch. But I don't think we
> > > need this at all (look below)
> > >
> > > >         efi_status_t ret;
> > > >
> > > > @@ -236,6 +240,23 @@ efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt)
> > > >         if (ret != EFI_SUCCESS)
> > > >                 return ret;
> > > >
> > > > -       return efi_run_image(image_ptr, size, bootefi_device_path,
> > > > -                            bootefi_image_path);
> > > > +       return efi_run_image(image_ptr, size, device, image);
> > > > +}
> > > > +
> > > > +/**
> > > > + * efi_binary_run() - run loaded UEFI image
> > > > + *
> > > > + * @image:     memory address of the UEFI image
> > > > + * @size:      size of the UEFI image
> > > > + * @fdt:       device-tree
> > > > + *
> > > > + * Execute an EFI binary image loaded at @image.
> > > > + * @size may be zero if the binary is loaded with U-Boot load command.
> > > > + *
> > > > + * Return:     status code
> > > > + */
> > > > +efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
> > > > +{
> > > > +       return efi_binary_run_(image, size, fdt, bootefi_device_path,
> > > > +                              bootefi_image_path);
> > > >  }
> > >
> > > We don't need functions and wrappers for such a simple call. Just
> > > update all the callisites with the extra parameters and keep one
> > > function
> >
> > If you look at the two call sites, you'll see that they would need to
> > pass bootefi_device_path and bootefi_image_path in. but these are
> > static variables in this file.
>
> Ah fair enough, then please rename efi_binary_run_ -> _efi_binary_run
> ands make it static on this patch

I don't mind where the underscore goes. I changed my mind about that
when the dtc maintainer pointed out that C-library functions start
with underscore, so putting an underscore at the start of the function
is confusing things.

"The underscore prefix is reserved for functions and types used by the
compiler and standard library. The standard library can use these
names freely."[1]

But I don't mind. We don't even use the C library except for sandbox
and tools. So let me know what you think.

Regards,
SImon

[1] https://stackoverflow.com/questions/39625352/why-do-some-functions-in-c-have-an-underscore-prefix