[PATCH v2 0/3] Oops capture via EFI

[PATCH v2 0/3] Oops capture via EFI

[Matthew Garrett]

v2 just has Tony's review comments addressed. I've re-added the hunk I
accidentally deleted in 2/3, and most of the efivars code is now under
CONFIG_PSTORE in efivars.

[PATCH v2 1/3] pstore: Extend API

[Matthew Garrett]

Some pstore implementations may not have a static context, so extend the
API to pass the pstore_info struct to all calls and allow for a context
pointer.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/apei/erst.c |   18 +++++++++++++-----
 fs/pstore/inode.c        |   10 +++++-----
 fs/pstore/internal.h     |    2 +-
 fs/pstore/platform.c     |   13 +++++++------
 include/linux/pstore.h   |    8 +++++---
 5 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index e6cef8e..de3ae92 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -932,8 +932,10 @@ static int erst_check_table(struct acpi_table_erst *erst_tab)
 static int erst_open_pstore(struct pstore_info *psi);
 static int erst_close_pstore(struct pstore_info *psi);
 static ssize_t erst_reader(u64 *id, enum pstore_type_id *type,
-		       struct timespec *time);
-static u64 erst_writer(enum pstore_type_id type, size_t size);
+			   struct timespec *time, struct pstore_info *psi);
+static u64 erst_writer(enum pstore_type_id type, size_t size,
+		       struct pstore_info *psi);
+static int erst_clearer(u64 id, struct pstore_info *psi);
 
 static struct pstore_info erst_info = {
 	.owner		= THIS_MODULE,
@@ -942,7 +944,7 @@ static struct pstore_info erst_info = {
 	.close		= erst_close_pstore,
 	.read		= erst_reader,
 	.write		= erst_writer,
-	.erase		= erst_clear
+	.erase		= erst_clearer
 };
 
 #define CPER_CREATOR_PSTORE						\
@@ -983,7 +985,7 @@ static int erst_close_pstore(struct pstore_info *psi)
 }
 
 static ssize_t erst_reader(u64 *id, enum pstore_type_id *type,
-		       struct timespec *time)
+			   struct timespec *time, struct pstore_info *psi)
 {
 	int rc;
 	ssize_t len = 0;
@@ -1037,7 +1039,8 @@ out:
 	return (rc < 0) ? rc : (len - sizeof(*rcd));
 }
 
-static u64 erst_writer(enum pstore_type_id type, size_t size)
+static u64 erst_writer(enum pstore_type_id type, size_t size,
+		       struct pstore_info *psi)
 {
 	struct cper_pstore_record *rcd = (struct cper_pstore_record *)
 					(erst_info.buf - sizeof(*rcd));
@@ -1080,6 +1083,11 @@ static u64 erst_writer(enum pstore_type_id type, size_t size)
 	return rcd->hdr.record_id;
 }
 
+static int erst_clearer(u64 id, struct pstore_info *psi)
+{
+	return erst_clear(id);
+}
+
 static int __init erst_init(void)
 {
 	int rc = 0;
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 977ed27..b19884a 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -40,7 +40,7 @@
 
 struct pstore_private {
 	u64	id;
-	int	(*erase)(u64);
+	struct pstore_info *psi;
 	ssize_t	size;
 	char	data[];
 };
@@ -73,7 +73,7 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
 {
 	struct pstore_private *p = dentry->d_inode->i_private;
 
-	p->erase(p->id);
+	p->psi->erase(p->id, p->psi);
 
 	return simple_unlink(dir, dentry);
 }
@@ -175,8 +175,8 @@ int pstore_is_mounted(void)
  * Set the mtime & ctime to the date that this record was originally stored.
  */
 int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id,
-			      char *data, size_t size,
-			      struct timespec time, int (*erase)(u64))
+		  char *data, size_t size, struct timespec time,
+		  struct pstore_info *psi)
 {
 	struct dentry		*root = pstore_sb->s_root;
 	struct dentry		*dentry;
@@ -193,7 +193,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id,
 	if (!private)
 		goto fail_alloc;
 	private->id = id;
-	private->erase = erase;
+	private->psi = psi;
 
 	switch (type) {
 	case PSTORE_TYPE_DMESG:
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index 8c9f23e..611c1b3 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -2,5 +2,5 @@ extern void	pstore_set_kmsg_bytes(int);
 extern void	pstore_get_records(void);
 extern int	pstore_mkfile(enum pstore_type_id, char *psname, u64 id,
 			      char *data, size_t size,
-			      struct timespec time, int (*erase)(u64));
+			      struct timespec time, struct pstore_info *psi);
 extern int	pstore_is_mounted(void);
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index f2c3ff2..221c04e 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -94,11 +94,12 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 		memcpy(dst, s1 + s1_start, l1_cpy);
 		memcpy(dst + l1_cpy, s2 + s2_start, l2_cpy);
 
-		id = psinfo->write(PSTORE_TYPE_DMESG, hsize + l1_cpy + l2_cpy);
+		id = psinfo->write(PSTORE_TYPE_DMESG, hsize + l1_cpy + l2_cpy,
+				   psinfo);
 		if (reason == KMSG_DUMP_OOPS && pstore_is_mounted())
 			pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id,
 				      psinfo->buf, hsize + l1_cpy + l2_cpy,
-				      CURRENT_TIME, psinfo->erase);
+				      CURRENT_TIME, psinfo);
 		l1 -= l1_cpy;
 		l2 -= l2_cpy;
 		total += l1_cpy + l2_cpy;
@@ -166,9 +167,9 @@ void pstore_get_records(void)
 	if (rc)
 		goto out;
 
-	while ((size = psi->read(&id, &type, &time)) > 0) {
+	while ((size = psi->read(&id, &type, &time, psi)) > 0) {
 		if (pstore_mkfile(type, psi->name, id, psi->buf, (size_t)size,
-				  time, psi->erase))
+				  time, psi))
 			failed++;
 	}
 	psi->close(psi);
@@ -196,10 +197,10 @@ int pstore_write(enum pstore_type_id type, char *buf, size_t size)
 
 	mutex_lock(&psinfo->buf_mutex);
 	memcpy(psinfo->buf, buf, size);
-	id = psinfo->write(type, size);
+	id = psinfo->write(type, size, psinfo);
 	if (pstore_is_mounted())
 		pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id, psinfo->buf,
-			      size, CURRENT_TIME, psinfo->erase);
+			      size, CURRENT_TIME, psinfo);
 	mutex_unlock(&psinfo->buf_mutex);
 
 	return 0;
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 2455ef2..b2f1d97 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -38,9 +38,11 @@ struct pstore_info {
 	int		(*open)(struct pstore_info *psi);
 	int		(*close)(struct pstore_info *psi);
 	ssize_t		(*read)(u64 *id, enum pstore_type_id *type,
-			struct timespec *time);
-	u64		(*write)(enum pstore_type_id type, size_t size);
-	int		(*erase)(u64 id);
+			struct timespec *time, struct pstore_info *psi);
+	u64		(*write)(enum pstore_type_id type, size_t size,
+			struct pstore_info *psi);
+	int		(*erase)(u64 id, struct pstore_info *psi);
+	void		*data;
 };
 
 #ifdef CONFIG_PSTORE
-- 
1.7.5.2

[PATCH v2 2/3] pstore: Add extra context for writes and erases

[Matthew Garrett]

EFI only provides small amounts of individual storage, and conventionally
puts metadata in the storage variable name. Rather than add a metadata
header to the (already limited) variable storage, it's easier for us to
modify pstore to pass all the information we need to construct a unique
variable name to the appropriate functions.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/apei/erst.c |   10 ++++++----
 fs/pstore/inode.c        |    6 ++++--
 fs/pstore/platform.c     |    9 +++++----
 include/linux/pstore.h   |    5 +++--
 4 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index de3ae92..d842ac4 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -933,9 +933,10 @@ static int erst_open_pstore(struct pstore_info *psi);
 static int erst_close_pstore(struct pstore_info *psi);
 static ssize_t erst_reader(u64 *id, enum pstore_type_id *type,
 			   struct timespec *time, struct pstore_info *psi);
-static u64 erst_writer(enum pstore_type_id type, size_t size,
+static u64 erst_writer(enum pstore_type_id type, int part, size_t size,
 		       struct pstore_info *psi);
-static int erst_clearer(u64 id, struct pstore_info *psi);
+static int erst_clearer(enum pstore_type_id type, u64 id,
+			struct pstore_info *psi);
 
 static struct pstore_info erst_info = {
 	.owner		= THIS_MODULE,
@@ -1039,7 +1040,7 @@ out:
 	return (rc < 0) ? rc : (len - sizeof(*rcd));
 }
 
-static u64 erst_writer(enum pstore_type_id type, size_t size,
+static u64 erst_writer(enum pstore_type_id type, int part, size_t size,
 		       struct pstore_info *psi)
 {
 	struct cper_pstore_record *rcd = (struct cper_pstore_record *)
@@ -1083,7 +1084,8 @@ static u64 erst_writer(enum pstore_type_id type, size_t size,
 	return rcd->hdr.record_id;
 }
 
-static int erst_clearer(u64 id, struct pstore_info *psi)
+static int erst_clearer(enum pstore_type_id type, u64 id,
+			struct pstore_info *psi)
 {
 	return erst_clear(id);
 }
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index b19884a..893b961 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -39,8 +39,9 @@
 #define	PSTORE_NAMELEN	64
 
 struct pstore_private {
-	u64	id;
 	struct pstore_info *psi;
+	enum pstore_type_id type;
+	u64	id;
 	ssize_t	size;
 	char	data[];
 };
@@ -73,7 +74,7 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
 {
 	struct pstore_private *p = dentry->d_inode->i_private;
 
-	p->psi->erase(p->id, p->psi);
+	p->psi->erase(p->type, p->id, p->psi);
 
 	return simple_unlink(dir, dentry);
 }
@@ -192,6 +193,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id,
 	private = kmalloc(sizeof *private + size, GFP_KERNEL);
 	if (!private)
 		goto fail_alloc;
+	private->type = type;
 	private->id = id;
 	private->psi = psi;
 
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 221c04e..163bb40 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -78,7 +78,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 	oopscount++;
 	while (total < kmsg_bytes) {
 		dst = psinfo->buf;
-		hsize = sprintf(dst, "%s#%d Part%d\n", why, oopscount, part++);
+		hsize = sprintf(dst, "%s#%d Part%d\n", why, oopscount, part);
 		size = psinfo->bufsize - hsize;
 		dst += hsize;
 
@@ -94,8 +94,8 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 		memcpy(dst, s1 + s1_start, l1_cpy);
 		memcpy(dst + l1_cpy, s2 + s2_start, l2_cpy);
 
-		id = psinfo->write(PSTORE_TYPE_DMESG, hsize + l1_cpy + l2_cpy,
-				   psinfo);
+		id = psinfo->write(PSTORE_TYPE_DMESG, part,
+				   hsize + l1_cpy + l2_cpy, psinfo);
 		if (reason == KMSG_DUMP_OOPS && pstore_is_mounted())
 			pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id,
 				      psinfo->buf, hsize + l1_cpy + l2_cpy,
@@ -103,6 +103,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 		l1 -= l1_cpy;
 		l2 -= l2_cpy;
 		total += l1_cpy + l2_cpy;
+		part++;
 	}
 	mutex_unlock(&psinfo->buf_mutex);
 }
@@ -197,7 +198,7 @@ int pstore_write(enum pstore_type_id type, char *buf, size_t size)
 
 	mutex_lock(&psinfo->buf_mutex);
 	memcpy(psinfo->buf, buf, size);
-	id = psinfo->write(type, size, psinfo);
+	id = psinfo->write(type, 0, size, psinfo);
 	if (pstore_is_mounted())
 		pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id, psinfo->buf,
 			      size, CURRENT_TIME, psinfo);
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index b2f1d97..12be8f1 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -39,9 +39,10 @@ struct pstore_info {
 	int		(*close)(struct pstore_info *psi);
 	ssize_t		(*read)(u64 *id, enum pstore_type_id *type,
 			struct timespec *time, struct pstore_info *psi);
-	u64		(*write)(enum pstore_type_id type, size_t size,
+	u64		(*write)(enum pstore_type_id type, int part,
+			size_t size, struct pstore_info *psi);
+	int		(*erase)(enum pstore_type_id type, u64 id,
 			struct pstore_info *psi);
-	int		(*erase)(u64 id, struct pstore_info *psi);
 	void		*data;
 };
 
-- 
1.7.5.2

[PATCH v2 3/3] efi: Add support for using efivars as a pstore backend

[Matthew Garrett]

EFI provides an area of nonvolatile storage managed by the firmware. We
can use this as a pstore backend to maintain copies of oopses, aiding
diagnosis.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/firmware/efivars.c |  188 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/efi.h        |    3 +
 2 files changed, 189 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 5f29aaf..32a312b 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -78,6 +78,7 @@
 #include <linux/kobject.h>
 #include <linux/device.h>
 #include <linux/slab.h>
+#include <linux/pstore.h>
 
 #include <asm/uaccess.h>
 
@@ -89,6 +90,8 @@ MODULE_DESCRIPTION("sysfs interface to EFI Variables");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(EFIVARS_VERSION);
 
+#define DUMP_NAME_LEN 52
+
 /*
  * The maximum size of VariableName + Data = 1024
  * Therefore, it's reasonable to save that much
@@ -161,18 +164,28 @@ utf8_strsize(efi_char16_t *data, unsigned long maxlength)
 }
 
 static efi_status_t
-get_var_data(struct efivars *efivars, struct efi_variable *var)
+get_var_data_locked(struct efivars *efivars, struct efi_variable *var)
 {
 	efi_status_t status;
 
-	spin_lock(&efivars->lock);
 	var->DataSize = 1024;
 	status = efivars->ops->get_variable(var->VariableName,
 					    &var->VendorGuid,
 					    &var->Attributes,
 					    &var->DataSize,
 					    var->Data);
+	return status;
+}
+
+static efi_status_t
+get_var_data(struct efivars *efivars, struct efi_variable *var)
+{
+	efi_status_t status;
+
+	spin_lock(&efivars->lock);
+	status = get_var_data_locked(efivars, var);
 	spin_unlock(&efivars->lock);
+
 	if (status != EFI_SUCCESS) {
 		printk(KERN_WARNING "efivars: get_variable() failed 0x%lx!\n",
 			status);
@@ -387,12 +400,175 @@ static struct kobj_type efivar_ktype = {
 	.default_attrs = def_attrs,
 };
 
+static struct pstore_info efi_pstore_info;
+
 static inline void
 efivar_unregister(struct efivar_entry *var)
 {
 	kobject_put(&var->kobj);
 }
 
+#ifdef CONFIG_PSTORE
+static struct efivar_entry *walk_entry;
+
+static int efi_pstore_open(struct pstore_info *psi)
+{
+	struct efivars *efivars = psi->data;
+
+	spin_lock(&efivars->lock);
+	walk_entry = list_first_entry(&efivars->list, struct efivar_entry,
+				      list);
+	return 0;
+}
+
+static int efi_pstore_close(struct pstore_info *psi)
+{
+	struct efivars *efivars = psi->data;
+
+	spin_unlock(&efivars->lock);
+	return 0;
+}
+
+static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
+			       struct timespec *timespec, struct pstore_info *psi)
+{
+	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
+	struct efivars *efivars = psi->data;
+	char name[DUMP_NAME_LEN];
+	int i;
+	unsigned int part, size;
+	unsigned long time;
+
+	while (&walk_entry->list != &efivars->list) {
+		if (!efi_guidcmp(walk_entry->var.VendorGuid, vendor)) {
+			for (i = 0; i < DUMP_NAME_LEN; i++) {
+				name[i] = walk_entry->var.VariableName[i];
+			}
+			if (sscanf(name, "dump-type%u-%u-%lu", type, &part, &time) == 3) {
+				*id = part;
+				timespec->tv_sec = time;
+				timespec->tv_nsec = 0;
+				get_var_data_locked(efivars, &walk_entry->var);
+				size = walk_entry->var.DataSize;
+				memcpy(psi->buf, walk_entry->var.Data, size);
+				walk_entry = list_entry(walk_entry->list.next,
+					           struct efivar_entry, list);
+				return size;
+			}
+		}
+		walk_entry = list_entry(walk_entry->list.next,
+					struct efivar_entry, list);
+	}
+	return 0;
+}
+
+static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
+			    struct pstore_info *psi)
+{
+	char name[DUMP_NAME_LEN];
+	char stub_name[DUMP_NAME_LEN];
+	efi_char16_t efi_name[DUMP_NAME_LEN];
+	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
+	struct efivars *efivars = psi->data;
+	struct efivar_entry *entry, *found = NULL;
+	int i;
+
+	sprintf(stub_name, "dump-type%u-%u-", type, part);
+	sprintf(name, "%s%lu", stub_name, get_seconds());
+
+	spin_lock(&efivars->lock);
+
+	for (i = 0; i < DUMP_NAME_LEN; i++)
+		efi_name[i] = stub_name[i];
+
+	/*
+	 * Clean up any entries with the same name
+	 */
+
+	list_for_each_entry(entry, &efivars->list, list) {
+		get_var_data_locked(efivars, &entry->var);
+
+		for (i = 0; i < DUMP_NAME_LEN; i++) {
+			if (efi_name[i] == 0) {
+				found = entry;
+				efi.set_variable(entry->var.VariableName, &entry->var.VendorGuid,
+						 EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+						 0, NULL);
+				break;
+			} else if (efi_name[i] != entry->var.VariableName[i]) {
+				break;
+			}
+		}
+	}
+
+	if (found)
+		list_del(&found->list);
+
+	for (i = 0; i < DUMP_NAME_LEN; i++)
+		efi_name[i] = name[i];
+
+	efi.set_variable(efi_name, &vendor,
+			 EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+			 size, psi->buf);
+
+	spin_unlock(&efivars->lock);
+
+	if (found)
+		efivar_unregister(found);
+
+	if (size)
+		efivar_create_sysfs_entry(efivars, utf8_strsize(efi_name, DUMP_NAME_LEN * 2),
+					  efi_name, &vendor);
+
+	return part;
+};
+
+static int efi_pstore_erase(enum pstore_type_id type, u64 id,
+			    struct pstore_info *psi)
+{
+	efi_pstore_write(type, id, 0, psi);
+
+	return 0;
+}
+#else
+static int efi_pstore_open(struct pstore_info *psi)
+{
+	return 0;
+}
+
+static int efi_pstore_close(struct pstore_info *psi)
+{
+	return 0;
+}
+
+static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
+			       struct timespec *time, struct pstore_info *psi)
+{
+	return -1;
+}
+
+static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
+			    struct pstore_info *psi)
+{
+	return 0;
+}
+
+static int efi_pstore_erase(enum pstore_type_id type, u64 id,
+			    struct pstore_info *psi)
+{
+	return 0;
+}
+#endif
+
+static struct pstore_info efi_pstore_info = {
+	.owner		= THIS_MODULE,
+	.name		= "efi",
+	.open		= efi_pstore_open,
+	.close		= efi_pstore_close,
+	.read		= efi_pstore_read,
+	.write		= efi_pstore_write,
+	.erase		= efi_pstore_erase,
+};
 
 static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
 			     struct bin_attribute *bin_attr,
@@ -763,6 +939,14 @@ int register_efivars(struct efivars *efivars,
 	if (error)
 		unregister_efivars(efivars);
 
+	efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
+	if (efi_pstore_info.buf) {
+		efi_pstore_info.bufsize = 1024;
+		efi_pstore_info.data = efivars;
+		mutex_init(&efi_pstore_info.buf_mutex);
+		pstore_register(&efi_pstore_info);
+	}
+
 out:
 	kfree(variable_name);
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ec25726..8dd9a01 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -232,6 +232,9 @@ typedef efi_status_t efi_query_capsule_caps_t(efi_capsule_header_t **capsules,
 #define UV_SYSTEM_TABLE_GUID \
     EFI_GUID(  0x3b13a7d4, 0x633e, 0x11dd, 0x93, 0xec, 0xda, 0x25, 0x56, 0xd8, 0x95, 0x93 )
 
+#define LINUX_EFI_CRASH_GUID \
+    EFI_GUID(  0xcfc8fc79, 0xbe2e, 0x4ddc, 0x97, 0xf0, 0x9f, 0x98, 0xbf, 0xe2, 0x98, 0xa0 )
+
 typedef struct {
 	efi_guid_t guid;
 	unsigned long table;
-- 
1.7.5.2

RE: [PATCH v2 3/3] efi: Add support for using efivars as a pstore backend

[Luck, Tony]

+	efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
+	if (efi_pstore_info.buf) {
+		efi_pstore_info.bufsize = 1024;
+		efi_pstore_info.data = efivars;
+		mutex_init(&efi_pstore_info.buf_mutex);
+		pstore_register(&efi_pstore_info);
+	}

I'd imagined #ifdef CONFIG_PSTORE around this (and the
efi_pstore_info definition) rather than providing stubs
for the !PSTORE case.

But your way works too.

You should avoid a memory leak with:

		if (!pstore_register(&efi_pstore_info))
			kfree(efi_pstore_info.buf);


I've also been thinking some more about how to handle a
system that has more than one pstore back-end available.
I still don't have any good ideas how to make use of more
than one backend - but it occurs to me that we may need
a way to let the user choose which to use (e.g. in the
unlikely event that a BIOS bug made one of ERST or EFI
unusable by pstore). The current "whoever registers first
gets to use it" now seems inadequate.

Perhaps a command line "pstore={backend}" might work, so
the user could specify pstore=efi to get your code, and
pstore=erst to get mine.

	
-Tony

Re: [PATCH v2 3/3] efi: Add support for using efivars as a pstore backend

[Matthew Garrett]

On Tue, Jun 07, 2011 at 01:16:55PM -0700, Luck, Tony wrote:
> +	efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
> +	if (efi_pstore_info.buf) {
> +		efi_pstore_info.bufsize = 1024;
> +		efi_pstore_info.data = efivars;
> +		mutex_init(&efi_pstore_info.buf_mutex);
> +		pstore_register(&efi_pstore_info);
> +	}
> 
> I'd imagined #ifdef CONFIG_PSTORE around this (and the
> efi_pstore_info definition) rather than providing stubs
> for the !PSTORE case.

We usually seem to frown on #ifdefs in the middle of functions. We could 
probably make this easier by adding a pstore_enabled that's #defined to 
0 in the !PSTORE case, then the compiler ought to just get rid of it 
all.

> You should avoid a memory leak with:
> 
> 		if (!pstore_register(&efi_pstore_info))
> 			kfree(efi_pstore_info.buf);

Ah, true.

> I've also been thinking some more about how to handle a
> system that has more than one pstore back-end available.
> I still don't have any good ideas how to make use of more
> than one backend - but it occurs to me that we may need
> a way to let the user choose which to use (e.g. in the
> unlikely event that a BIOS bug made one of ERST or EFI
> unusable by pstore). The current "whoever registers first
> gets to use it" now seems inadequate.

Mm. Given that the name of the source is in the file, there's no 
namespacing issues. In an ideal world I think we'd register all of them 
in order to provide a better chance of at least one of them ending up in 
actual persistent storage.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

RE: [PATCH v2 3/3] efi: Add support for using efivars as a pstore backend

[Luck, Tony]

> Mm. Given that the name of the source is in the file, there's no 
> namespacing issues. In an ideal world I think we'd register all of them 
> in order to provide a better chance of at least one of them ending up in 
> actual persistent storage.

It depends on your level of paranoia - and on what problems
you think might occur. In an ideal world all the back-ends
would work - and we'd just use one until it filled up, and
then move to the next.  in the real world one of the back-ends
might just randomly hang the system - converting a crashing
system into a hung system (which many people consider a more
severe problem). Giving a user an opt-out method for a back-end
looks good in that scenario.

-Tony

Re: [PATCH v2 3/3] efi: Add support for using efivars as a pstore backend

[Matthew Garrett]

On Tue, Jun 07, 2011 at 01:52:32PM -0700, Luck, Tony wrote:

> It depends on your level of paranoia - and on what problems
> you think might occur. In an ideal world all the back-ends
> would work - and we'd just use one until it filled up, and
> then move to the next.  in the real world one of the back-ends
> might just randomly hang the system - converting a crashing
> system into a hung system (which many people consider a more
> severe problem). Giving a user an opt-out method for a back-end
> looks good in that scenario.

Yeah. I guess the question then is whether the mediation should be in 
pstore or at the platform backend level. strcmp in pstore_register seems 
somehow fragile for this, but I guess it'd work?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

RE: [PATCH v2 3/3] efi: Add support for using efivars as a pstore backend

[Luck, Tony]

> Yeah. I guess the question then is whether the mediation should be in 
> pstore or at the platform backend level. strcmp in pstore_register seems 
> somehow fragile for this, but I guess it'd work?

I don't think it all that fragile.  All the back ends will have
unique names.  If the user doesn't specify a pstore= argument we
keep the current "first past the post" method. If they do, then they
can choose which one, or specify pstore=none (or even pstore=not-bloody-likely,
since any not matching string will disable) to turn it off altogether.

-Tony

Re: [PATCH v2 3/3] efi: Add support for using efivars as a pstore backend

[Matthew Garrett]

On Tue, Jun 07, 2011 at 02:35:56PM -0700, Luck, Tony wrote:

> I don't think it all that fragile.  All the back ends will have
> unique names.  If the user doesn't specify a pstore= argument we
> keep the current "first past the post" method. If they do, then they
> can choose which one, or specify pstore=none (or even pstore=not-bloody-likely,
> since any not matching string will disable) to turn it off altogether.

Seems fair. I'll write a patch?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

RE: [PATCH v2 3/3] efi: Add support for using efivars as a pstore backend

[Luck, Tony]

> Seems fair. I'll write a patch?

Sure. Don't forget Documentation/... to explain what it does.

-Tony