| Message ID | 20240103184053.226203-1-gpiccoli@igalia.com |
|---|---|
| State | Accepted |
| Commit | a28655c330ab294862cabe66deadb0f85cd4f191 |
| Headers | show |
| Series | efi: pstore: Allow dynamic initialization based on module parameter | expand |
On 03/01/2024 15:40, Guilherme G. Piccoli wrote: > The efi-pstore module parameter "pstore_disable" warrants that users > are able to deactivate such backend. There is also a Kconfig option > for the default value of this parameter. It was originally added due > to some bad UEFI FW implementations that could break with many variables > written. > > Some distros (such as Arch Linux) set this in their config file still > nowadays. And once it is set, even being a writable module parameter, > there is effectively no way to make use of efi-pstore anymore. > If "pstore_disable" is set to true, the init function of the module exits > early and is never called again after the initcall processing. > > Let's switch this module parameter to have a callback and perform the > pstore backend registration again each time it's set from Y->N (and > vice-versa). With this, the writable nature of the parameter starts to > make sense, given that users now can switch back to using efi-pstore > or not during runtime by writing into it. Hi folks, a friendly ping - any comments on this one, suggestions? Thanks in advance, Guilherme
On Mon, Jan 15, 2024 at 06:20:45PM -0300, Guilherme G. Piccoli wrote: > On 03/01/2024 15:40, Guilherme G. Piccoli wrote: > > The efi-pstore module parameter "pstore_disable" warrants that users > > are able to deactivate such backend. There is also a Kconfig option > > for the default value of this parameter. It was originally added due > > to some bad UEFI FW implementations that could break with many variables > > written. > > > > Some distros (such as Arch Linux) set this in their config file still > > nowadays. And once it is set, even being a writable module parameter, > > there is effectively no way to make use of efi-pstore anymore. > > If "pstore_disable" is set to true, the init function of the module exits > > early and is never called again after the initcall processing. > > > > Let's switch this module parameter to have a callback and perform the > > pstore backend registration again each time it's set from Y->N (and > > vice-versa). With this, the writable nature of the parameter starts to > > make sense, given that users now can switch back to using efi-pstore > > or not during runtime by writing into it. > > Hi folks, a friendly ping - any comments on this one, suggestions? It's the middle of the merge window, not much anyone can do until after that is over to pick up new stuff. thanks, greg k-h
On 16/01/2024 05:46, Greg KH wrote: > On Mon, Jan 15, 2024 at 06:20:45PM -0300, Guilherme G. Piccoli wrote: >> On 03/01/2024 15:40, Guilherme G. Piccoli wrote: >>> The efi-pstore module parameter "pstore_disable" warrants that users >>> are able to deactivate such backend. There is also a Kconfig option >>> for the default value of this parameter. It was originally added due >>> to some bad UEFI FW implementations that could break with many variables >>> written. >>> >>> Some distros (such as Arch Linux) set this in their config file still >>> nowadays. And once it is set, even being a writable module parameter, >>> there is effectively no way to make use of efi-pstore anymore. >>> If "pstore_disable" is set to true, the init function of the module exits >>> early and is never called again after the initcall processing. >>> >>> Let's switch this module parameter to have a callback and perform the >>> pstore backend registration again each time it's set from Y->N (and >>> vice-versa). With this, the writable nature of the parameter starts to >>> make sense, given that users now can switch back to using efi-pstore >>> or not during runtime by writing into it. >> >> Hi folks, a friendly ping - any comments on this one, suggestions? > > It's the middle of the merge window, not much anyone can do until after > that is over to pick up new stuff. > > thanks, > > greg k-h > OK, that's fine! No hurries, and thanks for your response Greg :)
On Wed, 03 Jan 2024 15:40:32 -0300, Guilherme G. Piccoli wrote: > The efi-pstore module parameter "pstore_disable" warrants that users > are able to deactivate such backend. There is also a Kconfig option > for the default value of this parameter. It was originally added due > to some bad UEFI FW implementations that could break with many variables > written. > > Some distros (such as Arch Linux) set this in their config file still > nowadays. And once it is set, even being a writable module parameter, > there is effectively no way to make use of efi-pstore anymore. > If "pstore_disable" is set to true, the init function of the module exits > early and is never called again after the initcall processing. > > [...] Applied to for-next/pstore, thanks! [1/1] efi: pstore: Allow dynamic initialization based on module parameter https://git.kernel.org/kees/c/c3f849caf81b Take care,
diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c index e7b9ec6f8a86..833cbb995dd3 100644 --- a/drivers/firmware/efi/efi-pstore.c +++ b/drivers/firmware/efi/efi-pstore.c @@ -14,16 +14,43 @@ static unsigned int record_size = 1024; module_param(record_size, uint, 0444); MODULE_PARM_DESC(record_size, "size of each pstore UEFI var (in bytes, min/default=1024)"); -static bool efivars_pstore_disable = - IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE); - -module_param_named(pstore_disable, efivars_pstore_disable, bool, 0644); - #define PSTORE_EFI_ATTRIBUTES \ (EFI_VARIABLE_NON_VOLATILE | \ EFI_VARIABLE_BOOTSERVICE_ACCESS | \ EFI_VARIABLE_RUNTIME_ACCESS) +static bool pstore_disable = IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE); + +static int efivars_pstore_init(void); +static void efivars_pstore_exit(void); + +static int efi_pstore_disable_set(const char *val, const struct kernel_param *kp) +{ + int err; + bool old_pstore_disable = pstore_disable; + + err = param_set_bool(val, kp); + if (err) + return err; + + if (old_pstore_disable != pstore_disable) { + if (pstore_disable) + efivars_pstore_exit(); + else + efivars_pstore_init(); + } + + return 0; +} + +static const struct kernel_param_ops pstore_disable_ops = { + .set = efi_pstore_disable_set, + .get = param_get_bool, +}; + +module_param_cb(pstore_disable, &pstore_disable_ops, &pstore_disable, 0644); +__MODULE_PARM_TYPE(pstore_disable, "bool"); + static int efi_pstore_open(struct pstore_info *psi) { int err; @@ -218,12 +245,12 @@ static struct pstore_info efi_pstore_info = { .erase = efi_pstore_erase, }; -static __init int efivars_pstore_init(void) +static int efivars_pstore_init(void) { if (!efivar_supports_writes()) return 0; - if (efivars_pstore_disable) + if (pstore_disable) return 0; /* @@ -250,7 +277,7 @@ static __init int efivars_pstore_init(void) return 0; } -static __exit void efivars_pstore_exit(void) +static void efivars_pstore_exit(void) { if (!efi_pstore_info.bufsize) return;
The efi-pstore module parameter "pstore_disable" warrants that users are able to deactivate such backend. There is also a Kconfig option for the default value of this parameter. It was originally added due to some bad UEFI FW implementations that could break with many variables written. Some distros (such as Arch Linux) set this in their config file still nowadays. And once it is set, even being a writable module parameter, there is effectively no way to make use of efi-pstore anymore. If "pstore_disable" is set to true, the init function of the module exits early and is never called again after the initcall processing. Let's switch this module parameter to have a callback and perform the pstore backend registration again each time it's set from Y->N (and vice-versa). With this, the writable nature of the parameter starts to make sense, given that users now can switch back to using efi-pstore or not during runtime by writing into it. Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- drivers/firmware/efi/efi-pstore.c | 43 +++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 8 deletions(-)