From: Borislav Petkov <[email protected]>
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 <[email protected]>
Cc: Matt Fleming <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
---
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
On 24/04/13 11:09, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> 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 <[email protected]>
> Cc: Matt Fleming <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> arch/x86/boot/compressed/eboot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks, applied!
* Borislav Petkov <[email protected]> wrote:
> From: Borislav Petkov <[email protected]>
>
> 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 <[email protected]>
> Cc: Matt Fleming <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> 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
On 24/04/13 11:56, Ingo Molnar wrote:
>
> * Borislav Petkov <[email protected]> wrote:
>
>> From: Borislav Petkov <[email protected]>
>>
>> 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 <[email protected]>
>> Cc: Matt Fleming <[email protected]>
>> Signed-off-by: Borislav Petkov <[email protected]>
>> ---
>> 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.
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.
--
* Matt Fleming <[email protected]> wrote:
> On 24/04/13 11:56, Ingo Molnar wrote:
> >
> > * Borislav Petkov <[email protected]> wrote:
> >
> >> From: Borislav Petkov <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Cc: Matt Fleming <[email protected]>
> >> Signed-off-by: Borislav Petkov <[email protected]>
> >> ---
> >> 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
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.
* Matt Fleming <[email protected]> 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
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.
--
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!