* [RFC][PATCH v3 2/3] efi_pstore: Introducing workqueue updating sysfs entries
@ 2012-08-21 16:54 Seiji Aguchi
2012-08-21 18:00 ` Mike Waychison
0 siblings, 1 reply; 3+ messages in thread
From: Seiji Aguchi @ 2012-08-21 16:54 UTC (permalink / raw)
To: Mike Waychison, linux-kernel, Luck, Tony (tony.luck@intel.com),
Matthew Garrett (mjg@redhat.com),
dzickus
Cc: dle-develop, Satoru Moriya
[Problem]
efi_pstore creates sysfs entries ,which enable users to access to NVRAM,
in a write callback. If a kernel panic happens in interrupt contexts, pstore may
fail because it could sleep due to dynamic memory allocations during creating sysfs entries.
[Patch Description]
This patch removes sysfs operations from a write callback by introducing a workqueue
updating sysfs entries which is scheduled after the write callback is called.
efi_pstore will be robust against a kernel panic in an interrupt context with it.
Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>
---
drivers/firmware/efivars.c | 118 +++++++++++++++++++++++++++++++++++++++----
include/linux/efi.h | 3 +-
2 files changed, 109 insertions(+), 12 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index bd1df01..4698346 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -146,6 +146,13 @@ efivar_create_sysfs_entry(struct efivars *efivars,
efi_char16_t *variable_name,
efi_guid_t *vendor_guid);
+/*
+ * Prototype for workqueue functions updating sysfs entry
+ */
+
+static void efivar_update_sysfs_entry(struct work_struct *);
+static DECLARE_WORK(efivar_work, efivar_update_sysfs_entry);
+
/* Return the number of unicode characters in data */
static unsigned long
utf16_strnlen(efi_char16_t *s, size_t maxlength)
@@ -731,9 +738,6 @@ static int efi_pstore_write(enum pstore_type_id type,
0, NULL);
}
- if (found)
- list_del(&found->list);
-
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name[i] = name[i];
@@ -742,14 +746,7 @@ static int efi_pstore_write(enum pstore_type_id type,
spin_unlock_irqrestore(&efivars->lock, flags);
- if (found)
- efivar_unregister(found);
-
- if (size)
- ret = efivar_create_sysfs_entry(efivars,
- utf16_strsize(efi_name,
- DUMP_NAME_LEN * 2),
- efi_name, &vendor);
+ schedule_work(&efivar_work);
*id = part;
return ret;
@@ -1200,6 +1197,104 @@ EXPORT_SYMBOL_GPL(register_efivars);
static struct efivars __efivars;
static struct efivar_operations ops;
+static void delete_all_stale_sysfs_entries(void)
+{
+ struct efivars *efivars = &__efivars;
+ struct efivar_entry *entry, *n, *found;
+ efi_status_t status;
+ unsigned long flags;
+
+ while (1) {
+ found = NULL;
+ spin_lock_irqsave(&efivars->lock, flags);
+ list_for_each_entry_safe(entry, n, &efivars->list, list) {
+ status = get_var_data_locked(efivars, &entry->var);
+ if (status != EFI_SUCCESS) {
+ found = entry;
+ list_del(&entry->list);
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&efivars->lock, flags);
+ if (found)
+ efivar_unregister(entry);
+ else
+ break;
+ }
+}
+
+static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor)
+{
+ struct efivar_entry *entry, *n;
+ struct efivars *efivars = &__efivars;
+ unsigned long strsize1, strsize2;
+ bool found = false;
+
+ strsize1 = utf16_strsize(variable_name, 1024);
+ list_for_each_entry_safe(entry, n, &efivars->list, list) {
+ strsize2 = utf16_strsize(entry->var.VariableName, 1024);
+ if (strsize1 == strsize2 &&
+ !memcmp(variable_name, &(entry->var.VariableName),
+ strsize2) &&
+ !efi_guidcmp(entry->var.VendorGuid,
+ *vendor)) {
+ found = true;
+ break;
+ }
+ }
+ return found;
+}
+
+static void efivar_update_sysfs_entry(struct work_struct *work)
+{
+ struct efivars *efivars = &__efivars;
+ efi_guid_t vendor;
+ efi_char16_t *variable_name;
+ unsigned long variable_name_size = 1024, flags;
+ efi_status_t status = EFI_NOT_FOUND;
+ bool found;
+
+ /* Delete stale sysfs entries */
+ delete_all_stale_sysfs_entries();
+
+ /* Add new sysfs entries */
+ while (1) {
+ variable_name = kzalloc(variable_name_size, GFP_KERNEL);
+ if (!variable_name) {
+ pr_err("efivars: Memory allocation failed.\n");
+ return;
+ }
+
+ spin_lock_irqsave(&efivars->lock, flags);
+ found = false;
+ while (1) {
+ variable_name_size = 1024;
+ status = efivars->ops->get_next_variable(
+ &variable_name_size,
+ variable_name,
+ &vendor);
+ if (status != EFI_SUCCESS) {
+ break;
+ } else {
+ if (!variable_is_present(variable_name,
+ &vendor)) {
+ found = true;
+ break;
+ }
+ }
+ }
+ spin_unlock_irqrestore(&efivars->lock, flags);
+
+ if (!found) {
+ kfree(variable_name);
+ break;
+ } else
+ efivar_create_sysfs_entry(efivars,
+ variable_name_size,
+ variable_name, &vendor);
+ }
+}
+
/*
* For now we register the efi subsystem with the firmware subsystem
* and the vars subsystem with the efi subsystem. In the future, it
@@ -1255,6 +1350,7 @@ static void __exit
efivars_exit(void)
{
if (efi_enabled) {
+ cancel_work_sync(&efivar_work);
unregister_efivars(&__efivars);
kobject_put(efi_kobj);
}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 103adc6..cecdf58 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -641,7 +641,8 @@ struct efivars {
* 1) ->list - adds, removals, reads, writes
* 2) ops.[gs]et_variable() calls.
* It must not be held when creating sysfs entries or calling kmalloc.
- * ops.get_next_variable() is only called from register_efivars(),
+ * ops.get_next_variable() is only called from register_efivars()
+ * or efivar_update_sysfs_entry(),
* which is protected by the BKL, so that path is safe.
*/
spinlock_t lock;
-- 1.7.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC][PATCH v3 2/3] efi_pstore: Introducing workqueue updating sysfs entries
2012-08-21 16:54 [RFC][PATCH v3 2/3] efi_pstore: Introducing workqueue updating sysfs entries Seiji Aguchi
@ 2012-08-21 18:00 ` Mike Waychison
2012-08-21 19:24 ` Seiji Aguchi
0 siblings, 1 reply; 3+ messages in thread
From: Mike Waychison @ 2012-08-21 18:00 UTC (permalink / raw)
To: Seiji Aguchi
Cc: linux-kernel, Luck, Tony (tony.luck@intel.com),
Matthew Garrett (mjg@redhat.com),
dzickus, dle-develop, Satoru Moriya
On Tue, Aug 21, 2012 at 9:54 AM, Seiji Aguchi <seiji.aguchi@hds.com> wrote:
> [Problem]
> efi_pstore creates sysfs entries ,which enable users to access to NVRAM,
> in a write callback. If a kernel panic happens in interrupt contexts, pstore may
> fail because it could sleep due to dynamic memory allocations during creating sysfs entries.
>
> [Patch Description]
> This patch removes sysfs operations from a write callback by introducing a workqueue
> updating sysfs entries which is scheduled after the write callback is called.
> efi_pstore will be robust against a kernel panic in an interrupt context with it.
>
> Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com>
> ---
> drivers/firmware/efivars.c | 118 +++++++++++++++++++++++++++++++++++++++----
> include/linux/efi.h | 3 +-
> 2 files changed, 109 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index bd1df01..4698346 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -146,6 +146,13 @@ efivar_create_sysfs_entry(struct efivars *efivars,
> efi_char16_t *variable_name,
> efi_guid_t *vendor_guid);
>
> +/*
> + * Prototype for workqueue functions updating sysfs entry
> + */
> +
> +static void efivar_update_sysfs_entry(struct work_struct *);
> +static DECLARE_WORK(efivar_work, efivar_update_sysfs_entry);
> +
> /* Return the number of unicode characters in data */
> static unsigned long
> utf16_strnlen(efi_char16_t *s, size_t maxlength)
> @@ -731,9 +738,6 @@ static int efi_pstore_write(enum pstore_type_id type,
> 0, NULL);
> }
>
> - if (found)
> - list_del(&found->list);
> -
> for (i = 0; i < DUMP_NAME_LEN; i++)
> efi_name[i] = name[i];
>
> @@ -742,14 +746,7 @@ static int efi_pstore_write(enum pstore_type_id type,
>
> spin_unlock_irqrestore(&efivars->lock, flags);
>
> - if (found)
> - efivar_unregister(found);
> -
> - if (size)
> - ret = efivar_create_sysfs_entry(efivars,
> - utf16_strsize(efi_name,
> - DUMP_NAME_LEN * 2),
> - efi_name, &vendor);
> + schedule_work(&efivar_work);
>
> *id = part;
> return ret;
> @@ -1200,6 +1197,104 @@ EXPORT_SYMBOL_GPL(register_efivars);
> static struct efivars __efivars;
> static struct efivar_operations ops;
>
> +static void delete_all_stale_sysfs_entries(void)
> +{
> + struct efivars *efivars = &__efivars;
> + struct efivar_entry *entry, *n, *found;
> + efi_status_t status;
> + unsigned long flags;
> +
> + while (1) {
> + found = NULL;
> + spin_lock_irqsave(&efivars->lock, flags);
> + list_for_each_entry_safe(entry, n, &efivars->list, list) {
> + status = get_var_data_locked(efivars, &entry->var);
> + if (status != EFI_SUCCESS) {
> + found = entry;
> + list_del(&entry->list);
> + break;
> + }
> + }
> + spin_unlock_irqrestore(&efivars->lock, flags);
> + if (found)
> + efivar_unregister(entry);
> + else
> + break;
> + }
> +}
> +
> +static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor)
> +{
> + struct efivar_entry *entry, *n;
> + struct efivars *efivars = &__efivars;
> + unsigned long strsize1, strsize2;
> + bool found = false;
> +
> + strsize1 = utf16_strsize(variable_name, 1024);
> + list_for_each_entry_safe(entry, n, &efivars->list, list) {
> + strsize2 = utf16_strsize(entry->var.VariableName, 1024);
> + if (strsize1 == strsize2 &&
> + !memcmp(variable_name, &(entry->var.VariableName),
> + strsize2) &&
> + !efi_guidcmp(entry->var.VendorGuid,
> + *vendor)) {
> + found = true;
> + break;
> + }
> + }
> + return found;
> +}
> +
> +static void efivar_update_sysfs_entry(struct work_struct *work)
> +{
> + struct efivars *efivars = &__efivars;
> + efi_guid_t vendor;
> + efi_char16_t *variable_name;
> + unsigned long variable_name_size = 1024, flags;
> + efi_status_t status = EFI_NOT_FOUND;
> + bool found;
> +
> + /* Delete stale sysfs entries */
> + delete_all_stale_sysfs_entries();
> +
> + /* Add new sysfs entries */
> + while (1) {
> + variable_name = kzalloc(variable_name_size, GFP_KERNEL);
> + if (!variable_name) {
> + pr_err("efivars: Memory allocation failed.\n");
> + return;
> + }
> +
> + spin_lock_irqsave(&efivars->lock, flags);
> + found = false;
> + while (1) {
> + variable_name_size = 1024;
> + status = efivars->ops->get_next_variable(
> + &variable_name_size,
> + variable_name,
> + &vendor);
> + if (status != EFI_SUCCESS) {
> + break;
> + } else {
> + if (!variable_is_present(variable_name,
> + &vendor)) {
> + found = true;
> + break;
> + }
> + }
> + }
> + spin_unlock_irqrestore(&efivars->lock, flags);
> +
> + if (!found) {
> + kfree(variable_name);
> + break;
> + } else
> + efivar_create_sysfs_entry(efivars,
> + variable_name_size,
> + variable_name, &vendor);
> + }
> +}
> +
> /*
> * For now we register the efi subsystem with the firmware subsystem
> * and the vars subsystem with the efi subsystem. In the future, it
> @@ -1255,6 +1350,7 @@ static void __exit
> efivars_exit(void)
> {
> if (efi_enabled) {
> + cancel_work_sync(&efivar_work);
Please move this cancel_work_sync() to be before the efi_enabled test.
efi_enabled here means that we registered __efivars. There may be
another driver (gsmi) using the efivars code, so we should always be
cancelling this work.
> unregister_efivars(&__efivars);
> kobject_put(efi_kobj);
> }
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 103adc6..cecdf58 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -641,7 +641,8 @@ struct efivars {
> * 1) ->list - adds, removals, reads, writes
> * 2) ops.[gs]et_variable() calls.
> * It must not be held when creating sysfs entries or calling kmalloc.
> - * ops.get_next_variable() is only called from register_efivars(),
> + * ops.get_next_variable() is only called from register_efivars()
> + * or efivar_update_sysfs_entry(),
> * which is protected by the BKL, so that path is safe.
> */
> spinlock_t lock;
> -- 1.7.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [RFC][PATCH v3 2/3] efi_pstore: Introducing workqueue updating sysfs entries
2012-08-21 18:00 ` Mike Waychison
@ 2012-08-21 19:24 ` Seiji Aguchi
0 siblings, 0 replies; 3+ messages in thread
From: Seiji Aguchi @ 2012-08-21 19:24 UTC (permalink / raw)
To: Mike Waychison
Cc: linux-kernel, Luck, Tony (tony.luck@intel.com),
Matthew Garrett (mjg@redhat.com),
dzickus, dle-develop, Satoru Moriya
> > efivars_exit(void)
> > {
> > if (efi_enabled) {
> > + cancel_work_sync(&efivar_work);
>
> Please move this cancel_work_sync() to be before the efi_enabled test.
> efi_enabled here means that we registered __efivars. There may be another driver (gsmi) using the efivars code, so we should
> always be cancelling this work.
>
OK. I will fix it.
I confirmed that gsmi accually called register_efivars().
Thank you for letting me know about this.
Seiji
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-08-21 19:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-21 16:54 [RFC][PATCH v3 2/3] efi_pstore: Introducing workqueue updating sysfs entries Seiji Aguchi
2012-08-21 18:00 ` Mike Waychison
2012-08-21 19:24 ` Seiji Aguchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).