[PATCH v2 2/3] efi: Convert device_path allocation to use malloc()

[Simon Glass]

Hi Ilias,

On Thu, 29 Aug 2024 at 06:39, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Thu, 29 Aug 2024 at 15:17, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 28 Aug 2024 at 01:40, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > [...]
> > >
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > (no changes since v1)
> > > > > > > >
> > > > > > > > What about https://lore.kernel.org/u-boot/CAFLszTjLAKaYK_jLXJ7Z571L-QMtoiqE-oxHCRs186dZ2Qok5g@mail.gmail.com/
> > > > > > > > ?
> > > > > > >
> > > > > > > Yes, I reordered the patches in this series.
> > > > > >
> > > > > > You don't need to reorder them. As Heinrich already pointed out some
> > > > > > of these functions are used in EFI protocols. e.g
> > > > > > duplicate_device_path() requires the memory to be allocated by EFI
> > > > > > memory services, you can't just replace it with malloc.
> > > > >
> > > > > The pointers returned can be freed with EFI services. I don't see any
> > > > > problem here. All the tests pass and all the spec rules are followed,
> > > > > so far as I can tell.
> > > >
> > > > Where exactly does that happen in the current patch?
> > > >
> > > > efi_alloc() calls efi_allocate_pool() and when we need to free the
> > > > device path we are calling efi_free_pool()
> > > >
> > >
> > > Looking at it again, this wasn't very clear. The previous patch converts
> > > efi_allocate_pool() to use malloc. So this patch is not needed at all
> >
> > That's right, but ultimately I'd like to drop efi_alloc() and the only
> > other place it is used is once in lib/efi_loader/efi_bootmgr.c
>
> Right, then the right thing to do here is to replace efi_alloc() with
> efi_allocate_pool() which will be calling malloc in the end. That way
> we are free to change the efi_allocate_pool implementation in the
> future without affecting the functions described on the spec

OK, that sounds good and it will make it clearer which things are
internal memory allocations and which are presented to the app as
things that it can free. The spec doesn't mention the pool, but I
assume that is implicit.

Regards,
Simon