Hi Ilias, On Thu, 29 Aug 2024 at 08:14, Simon Glass <s...@chromium.org> wrote: > > Hi Ilias, > > On Thu, 29 Aug 2024 at 06:39, Ilias Apalodimas > <ilias.apalodi...@linaro.org> wrote: > > > > On Thu, 29 Aug 2024 at 15:17, Simon Glass <s...@chromium.org> wrote: > > > > > > Hi Ilias, > > > > > > On Wed, 28 Aug 2024 at 01:40, Ilias Apalodimas > > > <ilias.apalodi...@linaro.org> wrote: > > > > > > > > Hi Simon, > > > > > > > > [...] > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > (no changes since v1) > > > > > > > > > > > > > > > > > > What about > > > > > > > > > https://lore.kernel.org/u-boot/caflsztjlakayk_jlxj7z571l-qmtoiqe-oxhcrs186dz2qo...@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.
OK, so it turns out that this increases code size, since the EFI call has more parameters. So I think the best thing to do is just leave efi_alloc() alone (or perhaps use it more consistently). For now I can drop the memset() in there, which seems to serve no purpose and is confusing. I'll send a v3 of this series. Regards, Simon
