[PATCH] x86, efi: Fix a build warning

[PATCH] x86, efi: Fix a build warning

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Fix this:

arch/x86/boot/compressed/eboot.c: In function ‘setup_efi_vars’:
arch/x86/boot/compressed/eboot.c:269:2: warning: passing argument 1 of ‘efi_call_phys’ makes pointer from integer without a cast [enabled by default]
In file included from arch/x86/boot/compressed/eboot.c:12:0:
/w/kernel/linux/arch/x86/include/asm/efi.h:8:33: note: expected ‘void *’ but argument is of type ‘long unsigned int’

after cc5a080c5d40 ("efi: Pass boot services variable info to runtime
code").

Cc: Matthew Garrett <matthew.garrett@nebula.com>
Cc: Matt Fleming <matt.fleming@intel.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/boot/compressed/eboot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 8615f7581820..41de115a55b7 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -266,7 +266,7 @@ static efi_status_t setup_efi_vars(struct boot_params *params)
 	while (data && data->next)
 		data = (struct setup_data *)(unsigned long)data->next;
 
-	status = efi_call_phys4(sys_table->runtime->query_variable_info,
+	status = efi_call_phys4((void *)sys_table->runtime->query_variable_info,
 				EFI_VARIABLE_NON_VOLATILE |
 				EFI_VARIABLE_BOOTSERVICE_ACCESS |
 				EFI_VARIABLE_RUNTIME_ACCESS, &store_size,
-- 
1.8.2.135.g7b592fa

Re: [PATCH] x86, efi: Fix a build warning

[Matt Fleming]

On 24/04/13 11:09, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> Fix this:
> 
> arch/x86/boot/compressed/eboot.c: In function ‘setup_efi_vars’:
> arch/x86/boot/compressed/eboot.c:269:2: warning: passing argument 1 of ‘efi_call_phys’ makes pointer from integer without a cast [enabled by default]
> In file included from arch/x86/boot/compressed/eboot.c:12:0:
> /w/kernel/linux/arch/x86/include/asm/efi.h:8:33: note: expected ‘void *’ but argument is of type ‘long unsigned int’
> 
> after cc5a080c5d40 ("efi: Pass boot services variable info to runtime
> code").
> 
> Cc: Matthew Garrett <matthew.garrett@nebula.com>
> Cc: Matt Fleming <matt.fleming@intel.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/boot/compressed/eboot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, applied!

Re: [PATCH] x86, efi: Fix a build warning

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> Fix this:
> 
> arch/x86/boot/compressed/eboot.c: In function ???setup_efi_vars???:
> arch/x86/boot/compressed/eboot.c:269:2: warning: passing argument 1 of ???efi_call_phys??? makes pointer from integer without a cast [enabled by default]
> In file included from arch/x86/boot/compressed/eboot.c:12:0:
> /w/kernel/linux/arch/x86/include/asm/efi.h:8:33: note: expected ???void *??? but argument is of type ???long unsigned int???
> 
> after cc5a080c5d40 ("efi: Pass boot services variable info to runtime
> code").
> 
> Cc: Matthew Garrett <matthew.garrett@nebula.com>
> Cc: Matt Fleming <matt.fleming@intel.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/boot/compressed/eboot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 8615f7581820..41de115a55b7 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -266,7 +266,7 @@ static efi_status_t setup_efi_vars(struct boot_params *params)
>  	while (data && data->next)
>  		data = (struct setup_data *)(unsigned long)data->next;
>  
> -	status = efi_call_phys4(sys_table->runtime->query_variable_info,
> +	status = efi_call_phys4((void *)sys_table->runtime->query_variable_info,
>  				EFI_VARIABLE_NON_VOLATILE |
>  				EFI_VARIABLE_BOOTSERVICE_ACCESS |
>  				EFI_VARIABLE_RUNTIME_ACCESS, &store_size,

Why isn't the query_variale_info field changed to void * instead? That way 
no cast would be needed.

Thanks,

	Ingo

Re: [PATCH] x86, efi: Fix a build warning

[Matt Fleming]

On 24/04/13 11:56, Ingo Molnar wrote:
> 
> * Borislav Petkov <bp@alien8.de> wrote:
> 
>> From: Borislav Petkov <bp@suse.de>
>>
>> Fix this:
>>
>> arch/x86/boot/compressed/eboot.c: In function ???setup_efi_vars???:
>> arch/x86/boot/compressed/eboot.c:269:2: warning: passing argument 1 of ???efi_call_phys??? makes pointer from integer without a cast [enabled by default]
>> In file included from arch/x86/boot/compressed/eboot.c:12:0:
>> /w/kernel/linux/arch/x86/include/asm/efi.h:8:33: note: expected ???void *??? but argument is of type ???long unsigned int???
>>
>> after cc5a080c5d40 ("efi: Pass boot services variable info to runtime
>> code").
>>
>> Cc: Matthew Garrett <matthew.garrett@nebula.com>
>> Cc: Matt Fleming <matt.fleming@intel.com>
>> Signed-off-by: Borislav Petkov <bp@suse.de>
>> ---
>>  arch/x86/boot/compressed/eboot.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
>> index 8615f7581820..41de115a55b7 100644
>> --- a/arch/x86/boot/compressed/eboot.c
>> +++ b/arch/x86/boot/compressed/eboot.c
>> @@ -266,7 +266,7 @@ static efi_status_t setup_efi_vars(struct boot_params *params)
>>  	while (data && data->next)
>>  		data = (struct setup_data *)(unsigned long)data->next;
>>  
>> -	status = efi_call_phys4(sys_table->runtime->query_variable_info,
>> +	status = efi_call_phys4((void *)sys_table->runtime->query_variable_info,
>>  				EFI_VARIABLE_NON_VOLATILE |
>>  				EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>  				EFI_VARIABLE_RUNTIME_ACCESS, &store_size,
> 
> Why isn't the query_variale_info field changed to void * instead? That way 
> no cast would be needed.

We could either change all fields in efi_system_table or the
efi_call_phys* prototypes. x86-64 already casts to (void *) when calling
efi_call0(), etc, though I'm not entirely sure why void * is needed.

Re: [PATCH] x86, efi: Fix a build warning

[Borislav Petkov]

On Wed, Apr 24, 2013 at 12:17:59PM +0100, Matt Fleming wrote:
> We could either change all fields in efi_system_table or the
> efi_call_phys* prototypes. x86-64 already casts to (void *) when
> calling efi_call0(), etc, though I'm not entirely sure why void * is
> needed.

Well, sizeof(void *) == sizeof(unsigned long) for both bitness so size
shouldn't be the reason.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86, efi: Fix a build warning

[Ingo Molnar]

* Matt Fleming <matt.fleming@intel.com> wrote:

> On 24/04/13 11:56, Ingo Molnar wrote:
> > 
> > * Borislav Petkov <bp@alien8.de> wrote:
> > 
> >> From: Borislav Petkov <bp@suse.de>
> >>
> >> Fix this:
> >>
> >> arch/x86/boot/compressed/eboot.c: In function ???setup_efi_vars???:
> >> arch/x86/boot/compressed/eboot.c:269:2: warning: passing argument 1 of ???efi_call_phys??? makes pointer from integer without a cast [enabled by default]
> >> In file included from arch/x86/boot/compressed/eboot.c:12:0:
> >> /w/kernel/linux/arch/x86/include/asm/efi.h:8:33: note: expected ???void *??? but argument is of type ???long unsigned int???
> >>
> >> after cc5a080c5d40 ("efi: Pass boot services variable info to runtime
> >> code").
> >>
> >> Cc: Matthew Garrett <matthew.garrett@nebula.com>
> >> Cc: Matt Fleming <matt.fleming@intel.com>
> >> Signed-off-by: Borislav Petkov <bp@suse.de>
> >> ---
> >>  arch/x86/boot/compressed/eboot.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> >> index 8615f7581820..41de115a55b7 100644
> >> --- a/arch/x86/boot/compressed/eboot.c
> >> +++ b/arch/x86/boot/compressed/eboot.c
> >> @@ -266,7 +266,7 @@ static efi_status_t setup_efi_vars(struct boot_params *params)
> >>  	while (data && data->next)
> >>  		data = (struct setup_data *)(unsigned long)data->next;
> >>  
> >> -	status = efi_call_phys4(sys_table->runtime->query_variable_info,
> >> +	status = efi_call_phys4((void *)sys_table->runtime->query_variable_info,
> >>  				EFI_VARIABLE_NON_VOLATILE |
> >>  				EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>  				EFI_VARIABLE_RUNTIME_ACCESS, &store_size,
> > 
> > Why isn't the query_variale_info field changed to void * instead? That way 
> > no cast would be needed.
> 
> We could either change all fields in efi_system_table or the
> efi_call_phys* prototypes. x86-64 already casts to (void *) when calling
> efi_call0(), etc, though I'm not entirely sure why void * is needed.

It's a basic cleanliness and robustness issue: we generally avoid type 
casts in the kernel, because type casts override compile-time type checks 
and are easy to get wrong. They are also ugly.

So in generaly we try to use the right type for the data structure, which 
matches its usage (and standardize functions/methods around that type) - 
then no cast is needed.

Thanks,

	Ingo

Re: [PATCH] x86, efi: Fix a build warning

[Matt Fleming]

On 25/04/13 07:55, Ingo Molnar wrote:
> It's a basic cleanliness and robustness issue: we generally avoid type 
> casts in the kernel, because type casts override compile-time type checks 
> and are easy to get wrong. They are also ugly.
> 
> So in generaly we try to use the right type for the data structure, which 
> matches its usage (and standardize functions/methods around that type) - 
> then no cast is needed.

Yeah, I'm not advocating using casts, I was just saying "Oh, x86-64
avoids requiring the caller of efi_call_phys* to perform the cast by
doing it in the definition of efi_call*. That's why this is only
affecting 32-bit."

Cleaning this up would be nice. I think at this point, I'll apply
Borislav's patch, and fix all this casting after v3.9 is released, since
instead of just changing query_variable_info, we might as well change
everything in efi_runtime_service_t so that it's consistent.

Re: [PATCH] x86, efi: Fix a build warning

[Ingo Molnar]

* Matt Fleming <matt.fleming@intel.com> wrote:

> On 25/04/13 07:55, Ingo Molnar wrote:
> > It's a basic cleanliness and robustness issue: we generally avoid type 
> > casts in the kernel, because type casts override compile-time type checks 
> > and are easy to get wrong. They are also ugly.
> > 
> > So in generaly we try to use the right type for the data structure, which 
> > matches its usage (and standardize functions/methods around that type) - 
> > then no cast is needed.
> 
> Yeah, I'm not advocating using casts, I was just saying "Oh, x86-64
> avoids requiring the caller of efi_call_phys* to perform the cast by
> doing it in the definition of efi_call*. That's why this is only
> affecting 32-bit."
> 
> Cleaning this up would be nice. I think at this point, I'll apply
> Borislav's patch, and fix all this casting after v3.9 is released, since
> instead of just changing query_variable_info, we might as well change
> everything in efi_runtime_service_t so that it's consistent.

Sounds fine to me.

Thanks,

	Ingo

Re: [PATCH] x86, efi: Fix a build warning

[Borislav Petkov]

On Thu, Apr 25, 2013 at 08:19:05AM +0100, Matt Fleming wrote:
> Yeah, I'm not advocating using casts, I was just saying "Oh, x86-64
> avoids requiring the caller of efi_call_phys* to perform the cast
> by doing it in the definition of efi_call*. That's why this is only
> affecting 32-bit."
>
> Cleaning this up would be nice. I think at this point, I'll apply
> Borislav's patch, and fix all this casting after v3.9 is released,
> since instead of just changing query_variable_info, we might as well
> change everything in efi_runtime_service_t so that it's consistent.

I wonder - if efi_runtime_service_t contained only void * like
efi_boot_services_t then we could drop all the casts too, right? Even in
the efi_call* macro calls too...

If you want me, I can take care of it after the merge window :)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86, efi: Fix a build warning

[Matt Fleming]

On 25/04/13 11:47, Borislav Petkov wrote:
> I wonder - if efi_runtime_service_t contained only void * like
> efi_boot_services_t then we could drop all the casts too, right? Even in
> the efi_call* macro calls too...

Yeah, that's what I figured.

> If you want me, I can take care of it after the merge window :)

That would be very helpful, thank you!