| Message ID | 20170216154457.19244.5369.stgit@tlendack-t1.amdoffice.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 16 Feb, at 09:44:57AM, Tom Lendacky wrote: > Update the efi_mem_type() to return EFI_RESERVED_TYPE instead of a > hardcoded 0. > > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/platform/efi/efi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c > index a15cf81..6407103 100644 > --- a/arch/x86/platform/efi/efi.c > +++ b/arch/x86/platform/efi/efi.c > @@ -1037,7 +1037,7 @@ u32 efi_mem_type(unsigned long phys_addr) > efi_memory_desc_t *md; > > if (!efi_enabled(EFI_MEMMAP)) > - return 0; > + return EFI_RESERVED_TYPE; > > for_each_efi_memory_desc(md) { > if ((md->phys_addr <= phys_addr) && > @@ -1045,7 +1045,7 @@ u32 efi_mem_type(unsigned long phys_addr) > (md->num_pages << EFI_PAGE_SHIFT)))) > return md->type; > } > - return 0; > + return EFI_RESERVED_TYPE; > } I see what you're getting at here, but arguably the return value in these cases never should have been zero to begin with (your change just makes that more obvious). Returning EFI_RESERVED_TYPE implies an EFI memmap entry exists for this address, which is misleading because it doesn't in the hunks you've modified above. Instead, could you look at returning a negative error value in the usual way we do in the Linux kernel, and update the function prototype to match? I don't think any callers actually require the return type to be u32.
On 2/21/2017 6:05 AM, Matt Fleming wrote: > On Thu, 16 Feb, at 09:44:57AM, Tom Lendacky wrote: >> Update the efi_mem_type() to return EFI_RESERVED_TYPE instead of a >> hardcoded 0. >> >> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >> --- >> arch/x86/platform/efi/efi.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c >> index a15cf81..6407103 100644 >> --- a/arch/x86/platform/efi/efi.c >> +++ b/arch/x86/platform/efi/efi.c >> @@ -1037,7 +1037,7 @@ u32 efi_mem_type(unsigned long phys_addr) >> efi_memory_desc_t *md; >> >> if (!efi_enabled(EFI_MEMMAP)) >> - return 0; >> + return EFI_RESERVED_TYPE; >> >> for_each_efi_memory_desc(md) { >> if ((md->phys_addr <= phys_addr) && >> @@ -1045,7 +1045,7 @@ u32 efi_mem_type(unsigned long phys_addr) >> (md->num_pages << EFI_PAGE_SHIFT)))) >> return md->type; >> } >> - return 0; >> + return EFI_RESERVED_TYPE; >> } > > I see what you're getting at here, but arguably the return value in > these cases never should have been zero to begin with (your change > just makes that more obvious). > > Returning EFI_RESERVED_TYPE implies an EFI memmap entry exists for > this address, which is misleading because it doesn't in the hunks > you've modified above. > > Instead, could you look at returning a negative error value in the > usual way we do in the Linux kernel, and update the function prototype > to match? I don't think any callers actually require the return type > to be u32. I can do that, I'll change the return type to an int. For the !efi_enabled I can return -ENOTSUPP and for when an entry isn't found I can return -EINVAL. Sound good? The ia64 arch is the only other arch that defines the function. It has just a single return 0 that I'll change to -EINVAL. Thanks, Tom >
On Thu, 23 Feb, at 11:27:55AM, Tom Lendacky wrote: > > I can do that, I'll change the return type to an int. For the > !efi_enabled I can return -ENOTSUPP and for when an entry isn't > found I can return -EINVAL. Sound good? Sounds good to me!
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index a15cf81..6407103 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -1037,7 +1037,7 @@ u32 efi_mem_type(unsigned long phys_addr) efi_memory_desc_t *md; if (!efi_enabled(EFI_MEMMAP)) - return 0; + return EFI_RESERVED_TYPE; for_each_efi_memory_desc(md) { if ((md->phys_addr <= phys_addr) && @@ -1045,7 +1045,7 @@ u32 efi_mem_type(unsigned long phys_addr) (md->num_pages << EFI_PAGE_SHIFT)))) return md->type; } - return 0; + return EFI_RESERVED_TYPE; } static int __init arch_parse_efi_cmdline(char *str)
Update the efi_mem_type() to return EFI_RESERVED_TYPE instead of a hardcoded 0. Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> --- arch/x86/platform/efi/efi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)