Re: [PATCH 3/5] efi_loader: use correct type for AllocatePages, AllocatePool

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Heinrich,

On Wed, Aug 18, 2021 at 02:23:40PM +0900, AKASHI Takahiro wrote:
> On Wed, Aug 18, 2021 at 06:50:40AM +0200, Heinrich Schuchardt wrote:
> > Am 18. August 2021 03:45:28 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
> > >Heinrich,
> > >
> > >On Tue, Aug 17, 2021 at 06:02:23PM +0200, Heinrich Schuchardt wrote:
> > >> Use enum efi_memory_type and enum_allocate_type in the definitions of the
> > >> efi_allocate_pages(), efi_allocate_pool().
> > >> 
> > >> In the external UEFI API leave the type as int as the UEFI specification
> > >> explicitly requires that enums use a 32bit type.
> > >> 
> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > >> ---
> > >>  include/efi_loader.h        | 9 +++++----
> > >>  lib/efi_loader/efi_memory.c | 5 +++--
> > >>  2 files changed, 8 insertions(+), 6 deletions(-)
> > >> 
> > >> diff --git a/include/efi_loader.h b/include/efi_loader.h
> > >> index 32cb8d0f1e..c440962fe5 100644
> > >> --- a/include/efi_loader.h
> > >> +++ b/include/efi_loader.h
> > >> @@ -676,13 +676,14 @@ struct efi_device_path *efi_get_dp_from_boot(const efi_guid_t guid);
> > >>  /* Generic EFI memory allocator, call this to get memory */
> > >>  void *efi_alloc(uint64_t len, int memory_type);
> > >>  /* More specific EFI memory allocator, called by EFI payloads */
> > >> -efi_status_t efi_allocate_pages(int type, int memory_type, efi_uintn_t pages,
> > >> -				uint64_t *memory);
> > >> +efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> > >> +				enum efi_memory_type memory_type,
> > >> +				efi_uintn_t pages, uint64_t *memory);
> > >>  /* EFI memory free function. */
> > >>  efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages);
> > >>  /* EFI memory allocator for small allocations */
> > >> -efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size,
> > >> -			       void **buffer);
> > >> +efi_status_t efi_allocate_pool(enum efi_memory_type pool_type,
> > >> +			       efi_uintn_t size, void **buffer);
> > >>  /* EFI pool memory free function. */
> > >>  efi_status_t efi_free_pool(void *buffer);
> > >>  /* Returns the EFI memory map */
> > >> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > >> index be2f655dff..f4acbee4f9 100644
> > >> --- a/lib/efi_loader/efi_memory.c
> > >> +++ b/lib/efi_loader/efi_memory.c
> > >> @@ -454,7 +454,8 @@ static uint64_t efi_find_free_memory(uint64_t len, uint64_t max_addr)
> > >>   * @memory		allocated memory
> > >>   * @return		status code
> > >>   */
> > >> -efi_status_t efi_allocate_pages(int type, int memory_type,
> > >> +efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> > >> +				enum efi_memory_type memory_type,
> > >>  				efi_uintn_t pages, uint64_t *memory)
> > >>  {
> > >>  	u64 len = pages << EFI_PAGE_SHIFT;
> > >> @@ -556,7 +557,7 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
> > >>   * @buffer:	allocated memory
> > >>   * Return:	status code
> > >>   */
> > >> -efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
> > >> +efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer)
> > >
> > >Given the purpose of this patch series, I think that the second argument
> > >of this function should be renamed from "pool_type" to "memory_type"
> > >which is also used in efi_allocate_pages() to avoid any confusion.
> > >(and the description for @pool_type as well)
> > 
> > pool_type is the name used by the UEFI specification.
> 
> So what?
> 
> (for efi_allocate_pages())
> !/*
> ! * Allocate memory pages.
> ! *
> ! * @type                type of allocation to be performed
> ! * @memory_type         usage type of the allocated memory
> 
> !/**
> ! * efi_allocate_pool - allocate memory from pool
> ! *
> ! * @pool_type:  type of the pool from which memory is to be allocated
> 
> You give the same type of arguments different names and explanation.
> I think that could be confusing.
> It is worth noting that efi_allocate_pool() directly passes
> the "pool_type" variable to efi_allocate_pages().

Did you ignore my comment?

-Takahiro Akashi


> -Takahiro Akashi
> 
> > Best regards
> > 
> > Heinrich
> > 
> > >
> > >Otherwise, it looks good.
> > >
> > >-Takahiro Akashi
> > >
> > >
> > >>  {
> > >>  	efi_status_t r;
> > >>  	u64 addr;
> > >> -- 
> > >> 2.30.2
> > >> 
> >