diff mbox

[1/3] arm64: efistub: drop __init annotation from handle_kernel_image()

Message ID 1453979254-25374-2-git-send-email-ard.biesheuvel@linaro.org
State Superseded
Headers show

Commit Message

Ard Biesheuvel Jan. 28, 2016, 11:07 a.m. UTC 
After moving arm64-stub.c to libstub/, all of its sections are emitted
as .init.xxx sections automatically, and the __init annotation of
handle_kernel_image() causes it to end up in .init.init.text, which is
not recognized as an __init section by the linker scripts. So drop the
annotation.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 drivers/firmware/efi/libstub/arm64-stub.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

-- 
2.5.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Comments

Will Deacon Jan. 28, 2016, 3:56 p.m. UTC | #1 
On Thu, Jan 28, 2016 at 12:07:32PM +0100, Ard Biesheuvel wrote:
> After moving arm64-stub.c to libstub/, all of its sections are emitted

> as .init.xxx sections automatically, and the __init annotation of

> handle_kernel_image() causes it to end up in .init.init.text, which is

> not recognized as an __init section by the linker scripts. So drop the

> annotation.

> 

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  drivers/firmware/efi/libstub/arm64-stub.c | 14 +++++++-------

>  1 file changed, 7 insertions(+), 7 deletions(-)


Acked-by: Will Deacon <will.deacon@arm.com>


Will

> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c

> index 78dfbd34b6bf..9e0342745e4f 100644

> --- a/drivers/firmware/efi/libstub/arm64-stub.c

> +++ b/drivers/firmware/efi/libstub/arm64-stub.c

> @@ -13,13 +13,13 @@

>  #include <asm/efi.h>

>  #include <asm/sections.h>

>  

> -efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,

> -					unsigned long *image_addr,

> -					unsigned long *image_size,

> -					unsigned long *reserve_addr,

> -					unsigned long *reserve_size,

> -					unsigned long dram_base,

> -					efi_loaded_image_t *image)

> +efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,

> +				 unsigned long *image_addr,

> +				 unsigned long *image_size,

> +				 unsigned long *reserve_addr,

> +				 unsigned long *reserve_size,

> +				 unsigned long dram_base,

> +				 efi_loaded_image_t *image)

>  {

>  	efi_status_t status;

>  	unsigned long kernel_size, kernel_memsize = 0;

> -- 

> 2.5.0

> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Ard Biesheuvel Jan. 29, 2016, 9:36 a.m. UTC | #2 
On 28 January 2016 at 23:58, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Thu, 28 Jan, at 12:07:32PM, Ard Biesheuvel wrote:

>> After moving arm64-stub.c to libstub/, all of its sections are emitted

>> as .init.xxx sections automatically, and the __init annotation of

>> handle_kernel_image() causes it to end up in .init.init.text, which is

>> not recognized as an __init section by the linker scripts. So drop the

>> annotation.

>>

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>  drivers/firmware/efi/libstub/arm64-stub.c | 14 +++++++-------

>>  1 file changed, 7 insertions(+), 7 deletions(-)

>>

>> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c

>> index 78dfbd34b6bf..9e0342745e4f 100644

>> --- a/drivers/firmware/efi/libstub/arm64-stub.c

>> +++ b/drivers/firmware/efi/libstub/arm64-stub.c

>> @@ -13,13 +13,13 @@

>>  #include <asm/efi.h>

>>  #include <asm/sections.h>

>>

>> -efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,

>> -                                     unsigned long *image_addr,

>> -                                     unsigned long *image_size,

>> -                                     unsigned long *reserve_addr,

>> -                                     unsigned long *reserve_size,

>> -                                     unsigned long dram_base,

>> -                                     efi_loaded_image_t *image)

>> +efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,

>> +                              unsigned long *image_addr,

>> +                              unsigned long *image_size,

>> +                              unsigned long *reserve_addr,

>> +                              unsigned long *reserve_size,

>> +                              unsigned long dram_base,

>> +                              efi_loaded_image_t *image)

>>  {

>>       efi_status_t status;

>>       unsigned long kernel_size, kernel_memsize = 0;

>> --

>> 2.5.0

>>

>

> Would it make more sense to #undef __init in one of the arm64 efistub

> header files? I'm thinking of the case where some poor unsuspecting

> developer writes a new function and marks it as __init, and we miss it

> during review.

>


Yes, I can add it to efistub.h, and make sure it gets included in all the files

Should we #undef it and #define it to a string that is easily grep'ed
for, so it is easy to find the explanatory comment that goes along
with it?
E.g.,

#define __init __init_not_supported_in_efi_stub

> At least if we do the #undef we can document why __init makes no sense

> in the EFI stub, and at the same time automatically fix things up.

>

> An alternative would be to write some Kbuild magic that complains if

> it sees __init, but that seems like more work than is reasonable.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Ard Biesheuvel Jan. 29, 2016, 4:03 p.m. UTC | #3 
On 29 January 2016 at 17:00, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 29 Jan, at 10:36:03AM, Ard Biesheuvel wrote:
>> On 28 January 2016 at 23:58, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> >
>> > Would it make more sense to #undef __init in one of the arm64 efistub
>> > header files? I'm thinking of the case where some poor unsuspecting
>> > developer writes a new function and marks it as __init, and we miss it
>> > during review.
>> >
>>
>> Yes, I can add it to efistub.h, and make sure it gets included in all the files
>>
>> Should we #undef it and #define it to a string that is easily grep'ed
>> for, so it is easy to find the explanatory comment that goes along
>> with it?
>> E.g.,
>>
>> #define __init __init_not_supported_in_efi_stub
>
> This would produce a compilation failure if someone tags something as
> __init right? Looks fine to me.

Yes, but it makes the error message difficult to decipher, since
__init is printed and not what it resolves to. Locally, I tried this,
which seems to work:

#undef __init
#define __init __attribute__((__init_not_supported_in_efi_stub))
#pragma GCC diagnostic error "-Wattributes"

which produces

  CC      drivers/firmware/efi/libstub/arm-stub.o
/home/ard/linux-2.6/drivers/firmware/efi/libstub/arm-stub.c:24:1:
error: ‘__init_not_supported_in_efi_stub’ attribute directive ignored
[-Werror=attributes]
 {
 ^
cc1: some warnings being treated as errors

which is slightly dodgy, but at least puts the string right in the error message
Ard Biesheuvel Feb. 2, 2016, 11:09 a.m. UTC | #4 
On 2 February 2016 at 12:08, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 29 Jan, at 05:03:16PM, Ard Biesheuvel wrote:
>> On 29 January 2016 at 17:00, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> > On Fri, 29 Jan, at 10:36:03AM, Ard Biesheuvel wrote:
>> >> On 28 January 2016 at 23:58, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> >> >
>> >> > Would it make more sense to #undef __init in one of the arm64 efistub
>> >> > header files? I'm thinking of the case where some poor unsuspecting
>> >> > developer writes a new function and marks it as __init, and we miss it
>> >> > during review.
>> >> >
>> >>
>> >> Yes, I can add it to efistub.h, and make sure it gets included in all the files
>> >>
>> >> Should we #undef it and #define it to a string that is easily grep'ed
>> >> for, so it is easy to find the explanatory comment that goes along
>> >> with it?
>> >> E.g.,
>> >>
>> >> #define __init __init_not_supported_in_efi_stub
>> >
>> > This would produce a compilation failure if someone tags something as
>> > __init right? Looks fine to me.
>>
>> Yes, but it makes the error message difficult to decipher, since
>> __init is printed and not what it resolves to. Locally, I tried this,
>> which seems to work:
>>
>> #undef __init
>> #define __init __attribute__((__init_not_supported_in_efi_stub))
>> #pragma GCC diagnostic error "-Wattributes"
>>
>> which produces
>>
>>   CC      drivers/firmware/efi/libstub/arm-stub.o
>> /home/ard/linux-2.6/drivers/firmware/efi/libstub/arm-stub.c:24:1:
>> error: ‘__init_not_supported_in_efi_stub’ attribute directive ignored
>> [-Werror=attributes]
>>  {
>>  ^
>> cc1: some warnings being treated as errors
>>
>> which is slightly dodgy, but at least puts the string right in the error message
>
> What about,
>
> #define __init __compiletime_error("__init not supported in EFI boot stub")
>

That only works for invocations, i.e., it needs to be used in header
files, and will trigger the error if a call to the function remains
after optimization. We want it at function definition time instead.
Ard Biesheuvel Feb. 3, 2016, 3:21 p.m. UTC | #5 
On 3 February 2016 at 16:19, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Tue, 02 Feb, at 12:09:41PM, Ard Biesheuvel wrote:

>> On 2 February 2016 at 12:08, Matt Fleming <matt@codeblueprint.co.uk> wrote:

>> >

>> > What about,

>> >

>> > #define __init __compiletime_error("__init not supported in EFI boot stub")

>> >

>>

>> That only works for invocations, i.e., it needs to be used in header

>> files, and will trigger the error if a call to the function remains

>> after optimization. We want it at function definition time instead.

>

> Good point.

>

> OK, how about we just do the #undef, call it good, and I add the task

> of printing some helpful error message to my growing TODO list?


I take it you didn't like my #pragma then? :-)

In any case, I don't care deeply about this, so just #undef'ing it is fine by me

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox

Patch

diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
index 78dfbd34b6bf..9e0342745e4f 100644
--- a/drivers/firmware/efi/libstub/arm64-stub.c
+++ b/drivers/firmware/efi/libstub/arm64-stub.c
@@ -13,13 +13,13 @@ 
 #include <asm/efi.h>
 #include <asm/sections.h>
 
-efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,
-					unsigned long *image_addr,
-					unsigned long *image_size,
-					unsigned long *reserve_addr,
-					unsigned long *reserve_size,
-					unsigned long dram_base,
-					efi_loaded_image_t *image)
+efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,
+				 unsigned long *image_addr,
+				 unsigned long *image_size,
+				 unsigned long *reserve_addr,
+				 unsigned long *reserve_size,
+				 unsigned long dram_base,
+				 efi_loaded_image_t *image)
 {
 	efi_status_t status;
 	unsigned long kernel_size, kernel_memsize = 0;