Re: [PATCH 12/12] efi: make const array 'apple' static

[Ard Biesheuvel]

Hi Lukas,

On 9 March 2018 at 08:29, Lukas Wunner <lukas@xxxxxxxxx> wrote:
> On Fri, Mar 09, 2018 at 08:47:19AM +0100, Ingo Molnar wrote:
>> * Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>> > From: Colin Ian King <colin.king@xxxxxxxxxxxxx>
>> >
>> > Don't populate the const read-only array 'buf' on the stack but instead
>> > make it static. Makes the object code smaller by 64 bytes:
>> >
>> > Before:
>> >    text        data     bss     dec     hex filename
>> >    9264           1      16    9281    2441 arch/x86/boot/compressed/eboot.o
>> >
>> > After:
>> >    text        data     bss     dec     hex filename
>> >    9200           1      16    9217    2401 arch/x86/boot/compressed/eboot.o
>> >
>> > (gcc version 7.2.0 x86_64)
>> >
>> > Signed-off-by: Colin Ian King <colin.king@xxxxxxxxxxxxx>
>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> > ---
>> >  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 886a9115af62..f2251c1c9853 100644
>> > --- a/arch/x86/boot/compressed/eboot.c
>> > +++ b/arch/x86/boot/compressed/eboot.c
>> > @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
>> >
>> >  static void setup_quirks(struct boot_params *boot_params)
>> >  {
>> > -   efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>> > +   static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>> >     efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
>> >             efi_table_attr(efi_system_table, fw_vendor, sys_table);
>>
>> As a general policy, please don't put 'static' variables into the local
>> scope, use file scope instead - right before setup_quirks() would be fine.
>
> Well, I believe the end result is the same and the closer the declaration
> is to where it's used, the easier the code is to read and understand.
>
> I object to patches like this because they paper over a missing
> compiler optimization:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725
>
> I have told Colin before that it would be more useful to look into
> fixing the underlying compiler issue rather than polluting the kernel
> with "static" keywords, but he keeps sending these patches so I've
> given up responding:
> https://lkml.org/lkml/2017/8/25/636
>
>
>> Plus an unicode string literal initializer would be pretty descriptive
>> as well,  instead of the weird looking character array, i.e. something
>> like:
>>
>>   static efi_char16_t const apple_unicode_str[] = u"Apple";
>>
>> ... or so?
>
> Last time I checked this didn't work, I believe it's because it's C11
> and the kernel is compiled with -std=gnu89.  And I'm also not sure if
> the oldest gcc version that we support understands u"".
>

Indeed

arch/x86/platform/efi/quirks.c:78:46: error: 'u' undeclared here (not
in a function); did you mean 'up'?

 static const efi_char16_t efi_dummy_name[] = u"DUMMY";