| Message ID | 20240901222259.456932-2-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi: Start tidying up memory management | expand |
On 02.09.24 00:22, Simon Glass wrote: > From my inspection none of the users need the memory to be zeroed. It > is somewhat unexpected that it does so, since the name gives no clue to > this. Though the UEFI specification does not require it, EDK II uses AllocateZeroPool() to implement EFI_DEVICE_PATH_UTILITIES_PROTOCOL.CreateDeviceNode(). We should not deviate from it. > > Drop the memset() so that it effectively becomes a wrapper around the > normal EFI-pool allocator. > > Another option would be to drop this function and call > efi_allocate_pool() directly, but that increase code size a little. > > Move the function comment to the header file like most other exported > functions in U-Boot. Yes, we should move all non-static EFI function descriptions to headers. But it makes no sense to move the comments of a single function and leave the other functions unchanged. Have a look at doc/api/efi.rst. Best regards Heinrich > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > Changes in v3: > - Add new patch to drop the memset() from efi_alloc() > - Drop patch to convert device_path allocation to use malloc() > > include/efi_loader.h | 11 ++++++++++- > lib/efi_loader/efi_memory.c | 9 --------- > 2 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index f84852e384f..38971d01442 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -758,8 +758,17 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf, > * Return: size in pages > */ > #define efi_size_in_pages(size) (((size) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT) > -/* Allocate boot service data pool memory */ > + > +/** > + * efi_alloc() - allocate boot services data pool memory > + * > + * Allocate memory from pool with type EFI_BOOT_SERVICES_DATA > + * > + * @size: number of bytes to allocate > + * Return: pointer to allocated memory, or NULL if out of memory > + */ > void *efi_alloc(size_t len); > + > /* Allocate pages on the specified alignment */ > void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align); > /* More specific EFI memory allocator, called by EFI payloads */ > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c > index c6f1dd09456..50cb2f3898b 100644 > --- a/lib/efi_loader/efi_memory.c > +++ b/lib/efi_loader/efi_memory.c > @@ -664,14 +664,6 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, > return r; > } > > -/** > - * efi_alloc() - allocate boot services data pool memory > - * > - * Allocate memory from pool and zero it out. > - * > - * @size: number of bytes to allocate > - * Return: pointer to allocated memory or NULL > - */ > void *efi_alloc(size_t size) > { > void *buf; > @@ -681,7 +673,6 @@ void *efi_alloc(size_t size) > log_err("out of memory"); > return NULL; > } > - memset(buf, 0, size); > > return buf; > }
diff --git a/include/efi_loader.h b/include/efi_loader.h index f84852e384f..38971d01442 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -758,8 +758,17 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf, * Return: size in pages */ #define efi_size_in_pages(size) (((size) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT) -/* Allocate boot service data pool memory */ + +/** + * efi_alloc() - allocate boot services data pool memory + * + * Allocate memory from pool with type EFI_BOOT_SERVICES_DATA + * + * @size: number of bytes to allocate + * Return: pointer to allocated memory, or NULL if out of memory + */ void *efi_alloc(size_t len); + /* Allocate pages on the specified alignment */ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align); /* More specific EFI memory allocator, called by EFI payloads */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index c6f1dd09456..50cb2f3898b 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -664,14 +664,6 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, return r; } -/** - * efi_alloc() - allocate boot services data pool memory - * - * Allocate memory from pool and zero it out. - * - * @size: number of bytes to allocate - * Return: pointer to allocated memory or NULL - */ void *efi_alloc(size_t size) { void *buf; @@ -681,7 +673,6 @@ void *efi_alloc(size_t size) log_err("out of memory"); return NULL; } - memset(buf, 0, size); return buf; }
From my inspection none of the users need the memory to be zeroed. It is somewhat unexpected that it does so, since the name gives no clue to this. Drop the memset() so that it effectively becomes a wrapper around the normal EFI-pool allocator. Another option would be to drop this function and call efi_allocate_pool() directly, but that increase code size a little. Move the function comment to the header file like most other exported functions in U-Boot. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v3: - Add new patch to drop the memset() from efi_alloc() - Drop patch to convert device_path allocation to use malloc() include/efi_loader.h | 11 ++++++++++- lib/efi_loader/efi_memory.c | 9 --------- 2 files changed, 10 insertions(+), 10 deletions(-)