mbox series

[U-Boot,v2,0/7] efi_loader: Fix inconsistencies in efi_add_memory_map usage

Message ID 20190714222931.12713-1-pure.logic@nexus-software.ie
Headers show
Series efi_loader: Fix inconsistencies in efi_add_memory_map usage | expand

Message

Bryan O'Donoghue July 14, 2019, 10:29 p.m. UTC
V2:

Following on from a discussion with Heinrich Schuchardt, please find a
reworked set of patches updating efi_add_memory_map() to

- Return efi_status_t
- Return EFI_SUCCESS where appropriate
- Return EFI_NO_MAPPING in two cases where zero was returned to indicate an
  error
- Updating of users of efi_add_memory_map() to parse for
  EFI_SUCCESS/efi_status_t

I've opted to maintain other returned status codes propogated by functions
that call efi_add_memory_map(). For example efi_add_runtime_mmio()
continues return EFI_OUT_OF_RESOURCES instead of directly returning the
result code of efi_add_memory_map(). The idea being that other users of the
EFI layer such as Linux or grub would not be affected by this internal
u-boot change.

V1:

https://patchwork.ozlabs.org/patch/1129402/
https://patchwork.ozlabs.org/patch/1129403/

These two patches fix some inconsistent usage around efi_add_memory_map().

The first patch fixes the case where there is a mapping for an address
starting at 0 as is the case on RPI3. We should not print an error for
this. efi_add_memory_map(start = 0, ...) succeeds but
efi_carve_out_dt_rsv() does not properly parse the result code.

The second patch fixes the result code returned by efi_add_memory_map() in
two instances. Returing zero is the same as returning EFI_SUCCESS, we
should return one of the error codes from include/efi.h only, not zero to
indicate failure.

Bryan O'Donoghue (7):
  efi_loader: Change return type of efi_add_memory_map()
  efi_loader: Change efi_add_memory_map() to return EFI_SUCCESS
  efi_loader: Return non-zero for error in efi_add_memory_map()
  efi_loader: Update efi_allocate_pages() to new efi_add_memory_map()
  efi_loader: Update efi_free_pages() to new efi_add_memory_map()
  efi_loader: Treat the result of efi_add_memory_map as efi_status_t
  efi_loader: Capture efi_add_memory_map() result efi_add_runtime_mmio()

 cmd/bootefi.c                |  4 ++--
 include/efi_loader.h         |  4 ++--
 lib/efi_loader/efi_memory.c  | 24 +++++++++++-------------
 lib/efi_loader/efi_runtime.c |  6 +++---
 4 files changed, 18 insertions(+), 20 deletions(-)

Comments

Heinrich Schuchardt July 14, 2019, 11:21 p.m. UTC | #1
On 7/15/19 12:29 AM, Bryan O'Donoghue wrote:
> V2:

Thanks for updating your patch series.

Each single patch applied should end up in something that succeeds in
Travis CI. So a patch changing a return value should not be separated
from a patch adjusting the callers of the function or changing the
definition the function. I think all seven patches should be merged to one.

Best regards

Heinrich

>
> Following on from a discussion with Heinrich Schuchardt, please find a
> reworked set of patches updating efi_add_memory_map() to
>
> - Return efi_status_t
> - Return EFI_SUCCESS where appropriate
> - Return EFI_NO_MAPPING in two cases where zero was returned to indicate an
>    error
> - Updating of users of efi_add_memory_map() to parse for
>    EFI_SUCCESS/efi_status_t
>
> I've opted to maintain other returned status codes propogated by functions
> that call efi_add_memory_map(). For example efi_add_runtime_mmio()
> continues return EFI_OUT_OF_RESOURCES instead of directly returning the
> result code of efi_add_memory_map(). The idea being that other users of the
> EFI layer such as Linux or grub would not be affected by this internal
> u-boot change.
>
> V1:
>
> https://patchwork.ozlabs.org/patch/1129402/
> https://patchwork.ozlabs.org/patch/1129403/
>
> These two patches fix some inconsistent usage around efi_add_memory_map().
>
> The first patch fixes the case where there is a mapping for an address
> starting at 0 as is the case on RPI3. We should not print an error for
> this. efi_add_memory_map(start = 0, ...) succeeds but
> efi_carve_out_dt_rsv() does not properly parse the result code.
>
> The second patch fixes the result code returned by efi_add_memory_map() in
> two instances. Returing zero is the same as returning EFI_SUCCESS, we
> should return one of the error codes from include/efi.h only, not zero to
> indicate failure.
>
> Bryan O'Donoghue (7):
>    efi_loader: Change return type of efi_add_memory_map()
>    efi_loader: Change efi_add_memory_map() to return EFI_SUCCESS
>    efi_loader: Return non-zero for error in efi_add_memory_map()
>    efi_loader: Update efi_allocate_pages() to new efi_add_memory_map()
>    efi_loader: Update efi_free_pages() to new efi_add_memory_map()
>    efi_loader: Treat the result of efi_add_memory_map as efi_status_t
>    efi_loader: Capture efi_add_memory_map() result efi_add_runtime_mmio()
>
>   cmd/bootefi.c                |  4 ++--
>   include/efi_loader.h         |  4 ++--
>   lib/efi_loader/efi_memory.c  | 24 +++++++++++-------------
>   lib/efi_loader/efi_runtime.c |  6 +++---
>   4 files changed, 18 insertions(+), 20 deletions(-)
>