| Message ID | 1461952128-2135409-1-git-send-email-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
On Fri, 29 Apr, at 07:48:31PM, Arnd Bergmann wrote: > gcc complains about a newly added file for the EFI Bootloader Control: > > drivers/firmware/efi/efibc.c: In function 'efibc_set_variable': > drivers/firmware/efi/efibc.c:53:1: error: the frame size of 2272 bytes is larger than 1024 bytes [-Werror=frame-larger-than=] > > The problem is the declaration of a local variable of type > struct efivar_entry, which is by itself larger than the warning > limit of 1024 bytes. > > We know that the reboot notifiers are not called from a deep stack, > so this is not an actual bug, but we should still try to rework > the code to avoid the warning. We also know that reboot notifiers > are never run concurrently on multiple CPUs, so there is no problem > in just making the variable 'static'. I assumed reboot notifiers were guaranteed to be non-concurrent too but having dug into the callers of kernel_reboot(), I couldn't find any kind of mutual exclusion. How/where is this guaranteed?
On Saturday 30 April 2016 21:14:49 Matt Fleming wrote: > On Fri, 29 Apr, at 07:48:31PM, Arnd Bergmann wrote: > > gcc complains about a newly added file for the EFI Bootloader Control: > > > > drivers/firmware/efi/efibc.c: In function 'efibc_set_variable': > > drivers/firmware/efi/efibc.c:53:1: error: the frame size of 2272 bytes is larger than 1024 bytes [-Werror=frame-larger-than=] > > > > The problem is the declaration of a local variable of type > > struct efivar_entry, which is by itself larger than the warning > > limit of 1024 bytes. > > > > We know that the reboot notifiers are not called from a deep stack, > > so this is not an actual bug, but we should still try to rework > > the code to avoid the warning. We also know that reboot notifiers > > are never run concurrently on multiple CPUs, so there is no problem > > in just making the variable 'static'. > > I assumed reboot notifiers were guaranteed to be non-concurrent too > but having dug into the callers of kernel_reboot(), I couldn't find > any kind of mutual exclusion. > > How/where is this guaranteed? The sys_restart() system call takes a mutex before calling kernel_restart() or kernel_poweroff(). I've had a closer look now and found that there are a few other callers of kernel_restart, so I guess if you restart using sysctl at the exact same time as calling /sbin/reboot, things may break. It's not something we'd have to worry about in practice, but it does make my patch incorrect. Should we come up with a different way to do it? Arnd
On Sun, 01 May, at 12:34:29AM, Arnd Bergmann wrote: > > The sys_restart() system call takes a mutex before calling kernel_restart() > or kernel_poweroff(). > > I've had a closer look now and found that there are a few other > callers of kernel_restart, so I guess if you restart using sysctl > at the exact same time as calling /sbin/reboot, things may break. Right. Or if the dm-verify-target driver saw an error. > It's not something we'd have to worry about in practice, but it does > make my patch incorrect. Should we come up with a different way to > do it? Jeremy proposed a patch to dynamically allocate the memory, which I think is the correct way to go given that our (reasonable) assumptions about reboot notifier concurrency are not guaranteed, https://lkml.kernel.org/r/87h9eked24.fsf@jcompost-MOBL1.tl.intel.com
On Saturday 30 April 2016 23:46:41 Matt Fleming wrote: > > > It's not something we'd have to worry about in practice, but it does > > make my patch incorrect. Should we come up with a different way to > > do it? > > Jeremy proposed a patch to dynamically allocate the memory, which I > think is the correct way to go given that our (reasonable) assumptions > about reboot notifier concurrency are not guaranteed, > > https://lkml.kernel.org/r/87h9eked24.fsf@jcompost-MOBL1.tl.intel.com Sure, that works. I considered doing it that way but it seemed more complicated. Please use that patch instead of mine. Arnd
On Sun, 01 May, at 01:25:12AM, Arnd Bergmann wrote: > On Saturday 30 April 2016 23:46:41 Matt Fleming wrote: > > > > > It's not something we'd have to worry about in practice, but it does > > > make my patch incorrect. Should we come up with a different way to > > > do it? > > > > Jeremy proposed a patch to dynamically allocate the memory, which I > > think is the correct way to go given that our (reasonable) assumptions > > about reboot notifier concurrency are not guaranteed, > > > > https://lkml.kernel.org/r/87h9eked24.fsf@jcompost-MOBL1.tl.intel.com > > Sure, that works. I considered doing it that way but it seemed more > complicated. Please use that patch instead of mine. Thanks Ard!
diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c index 2e0c7ccd9d9e..83ac90efa03f 100644 --- a/drivers/firmware/efi/efibc.c +++ b/drivers/firmware/efi/efibc.c @@ -31,8 +31,9 @@ static void efibc_str_to_str16(const char *str, efi_char16_t *str16) static void efibc_set_variable(const char *name, const char *value) { int ret; - efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID; - struct efivar_entry entry; + static struct efivar_entry entry = { + .var.VendorGuid = LINUX_EFI_LOADER_ENTRY_GUID, + }; size_t size = (strlen(value) + 1) * sizeof(efi_char16_t); if (size > sizeof(entry.var.Data)) @@ -40,7 +41,6 @@ static void efibc_set_variable(const char *name, const char *value) efibc_str_to_str16(name, entry.var.VariableName); efibc_str_to_str16(value, (efi_char16_t *)entry.var.Data); - memcpy(&entry.var.VendorGuid, &guid, sizeof(guid)); ret = efivar_entry_set(&entry, EFI_VARIABLE_NON_VOLATILE
gcc complains about a newly added file for the EFI Bootloader Control: drivers/firmware/efi/efibc.c: In function 'efibc_set_variable': drivers/firmware/efi/efibc.c:53:1: error: the frame size of 2272 bytes is larger than 1024 bytes [-Werror=frame-larger-than=] The problem is the declaration of a local variable of type struct efivar_entry, which is by itself larger than the warning limit of 1024 bytes. We know that the reboot notifiers are not called from a deep stack, so this is not an actual bug, but we should still try to rework the code to avoid the warning. We also know that reboot notifiers are never run concurrently on multiple CPUs, so there is no problem in just making the variable 'static'. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Fixes: 06f7d4a1618d ("efibc: Add EFI Bootloader Control module") --- drivers/firmware/efi/efibc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.7.0