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";
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
