[PATCH 1/3] pstore: Extend API

[PATCH 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 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     |   14 +++++---------
 include/linux/pstore.h   |    5 +++--
 4 files changed, 18 insertions(+), 17 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..958397c 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);
 }
@@ -132,11 +133,6 @@ int pstore_register(struct pstore_info *psi)
 	psinfo = psi;
 	spin_unlock(&pstore_lock);
 
-	if (owner && !try_module_get(owner)) {
-		psinfo = NULL;
-		return -EINVAL;
-	}
-
 	if (pstore_is_mounted())
 		pstore_get_records();
 
@@ -197,7 +193,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 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 |  158 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/efi.h        |    3 +
 2 files changed, 159 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 5f29aaf..89e6a3a 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,145 @@ static struct kobj_type efivar_ktype = {
 	.default_attrs = def_attrs,
 };
 
+static struct efivar_entry *walk_entry;
+
+static struct pstore_info efi_pstore_info;
+
 static inline void
 efivar_unregister(struct efivar_entry *var)
 {
 	kobject_put(&var->kobj);
 }
 
+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;
+}
+
+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 +909,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 2/3] pstore: Add extra context for writes and erases

[Tony Luck]

On Mon, Jun 6, 2011 at 12:38 PM, Matthew Garrett <mjg@redhat.com> wrote:
-       if (owner && !try_module_get(owner)) {
-               psinfo = NULL;
-               return -EINVAL;
-       }
-

Apart from the accidental deletion of this chunk (as discussed on
IRC), it all looks
good to me.  The ERST stuff still works, I didn't try the new EFI bits.  So you
can have:

Acked-by: Tony Luck <tony.luck@intel.com>

to add to these ... unless I'm the one pushing them to Linus, in which case I'll
add my Signed-off-by for parts 1 & 2

Does someone claim maintainership of  drivers/firmware/efivars.c for part3?

-Tony

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

[Matthew Garrett]

On Mon, Jun 06, 2011 at 02:43:46PM -0700, Tony Luck wrote:

> Does someone claim maintainership of  drivers/firmware/efivars.c for part3?

I think that'd be Matt.

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

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

[Tony Luck]

On Mon, Jun 6, 2011 at 12:38 PM, Matthew Garrett <mjg@redhat.com> wrote:
> 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.

Most of the new lines you add to efivars.c should be inside

#ifdef CONFIG_PSTORE
  ...
#endif

so that they don't bloat the kernel if PSTORE isn't configured.

I may have made a poor decision having drivers/acpi/apei/Kconfig
do a "select PSTORE" for the ACPI_APEI option.
It should probably be a normal "def_bool n" sort of thing.

-Tony

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

[Seiji Aguchi]

Hi,

Recently, I have suggested similar patches.
Does your patch satisfy my purpose?


[RFC][PATCH] kmsg_dumper for NVRAM
http://choon.net/forum/read.php?21,84718


[RFC][PATCH] pstore: EFI Support
http://choon.net/forum/read.php?21,84718,85162

Seiji

>-----Original Message-----
>From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Matthew Garrett
>Sent: Monday, June 06, 2011 3:39 PM
>To: tony.luck@intel.com
>Cc: linux-kernel@vger.kernel.org; Matt_Domsch@dell.com; Matthew Garrett
>Subject: [PATCH 3/3] efi: Add support for using efivars as a pstore backend
>
>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 |  158 +++++++++++++++++++++++++++++++++++++++++++-
> include/linux/efi.h        |    3 +
> 2 files changed, 159 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
>index 5f29aaf..89e6a3a 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,145 @@ static struct kobj_type efivar_ktype = {
> 	.default_attrs = def_attrs,
> };
>
>+static struct efivar_entry *walk_entry;
>+
>+static struct pstore_info efi_pstore_info;
>+
> static inline void
> efivar_unregister(struct efivar_entry *var)
> {
> 	kobject_put(&var->kobj);
> }
>
>+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;
>+}
>+
>+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 +909,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
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/

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

[Matthew Garrett]

On Tue, Jun 07, 2011 at 11:47:38AM -0400, Seiji Aguchi wrote:
> Hi,
> 
> Recently, I have suggested similar patches.
> Does your patch satisfy my purpose?

I think so. I've been testing it without problems on one machine and I'm 
about to try it on a different vendor. It seemed easier to reuse the 
efivars infrastructure than writing another implementation, and the 
efivars code has at least been tested enough for boot configuration to 
work.

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

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

[Randy Dunlap]

On Mon,  6 Jun 2011 15:38:55 -0400 Matthew Garrett wrote:

> 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 |  158 +++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/efi.h        |    3 +
>  2 files changed, 159 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 5f29aaf..89e6a3a 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -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)

__get_var_data() would be a more common/typical name for this (unlocked,
needs locking version), I think.

>  {
>  	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);


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

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

[Matt Domsch]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

On Mon, Jun 06, 2011 at 10:47:35PM +0100, Matthew Garrett wrote:
> On Mon, Jun 06, 2011 at 02:43:46PM -0700, Tony Luck wrote:
> 
> > Does someone claim maintainership of  drivers/firmware/efivars.c for part3?
> 
> I think that'd be Matt.

I'm out of the office this week and next, and haven't looked at it
closely.  Mike Waychison has done most of the editing in there
recently, I'd be happy if he wishes to be considered the maintainer
thereof.  

Thanks,
Matt

-- 
Matt Domsch
Technology Strategist
Dell | Office of the CTO

[-- Attachment #2: Type: application/pgp-signature, Size: 481 bytes --]

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

[Mike Waychison]

On Thu, Jun 9, 2011 at 9:00 PM, Matt Domsch <Matt_Domsch@dell.com> wrote:
> On Mon, Jun 06, 2011 at 10:47:35PM +0100, Matthew Garrett wrote:
>> On Mon, Jun 06, 2011 at 02:43:46PM -0700, Tony Luck wrote:
>>
>> > Does someone claim maintainership of  drivers/firmware/efivars.c for part3?
>>
>> I think that'd be Matt.
>
> I'm out of the office this week and next, and haven't looked at it
> closely.  Mike Waychison has done most of the editing in there
> recently, I'd be happy if he wishes to be considered the maintainer
> thereof.

I'm out of the office too until next Tuesday.  I should be able to
look at this between now and mid next-week.

>
> Thanks,
> Matt
>
> --
> Matt Domsch
> Technology Strategist
> Dell | Office of the CTO
>

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

[Tony Luck]

On Fri, Jun 10, 2011 at 12:12 AM, Mike Waychison <mikew@google.com> wrote:
> On Thu, Jun 9, 2011 at 9:00 PM, Matt Domsch <Matt_Domsch@dell.com> wrote:
>> On Mon, Jun 06, 2011 at 10:47:35PM +0100, Matthew Garrett wrote:
>>> On Mon, Jun 06, 2011 at 02:43:46PM -0700, Tony Luck wrote:
>>>
>>> > Does someone claim maintainership of  drivers/firmware/efivars.c for part3?
>>>
>>> I think that'd be Matt.
>>
>> I'm out of the office this week and next, and haven't looked at it
>> closely.  Mike Waychison has done most of the editing in there
>> recently, I'd be happy if he wishes to be considered the maintainer
>> thereof.
>
> I'm out of the office too until next Tuesday.  I should be able to
> look at this between now and mid next-week.

Mike; Did you find time to look at Matthew's efi-for-pstore patches yet?
If we want to hit the 3.1 merge window we should be trying to get these
beaten into shape pretty soon (Linus will presumably declare -rc4 tonight,
so not much time left).

-Tony

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

[Mike Waychison]

On Mon, Jun 20, 2011 at 3:27 PM, Tony Luck <tony.luck@intel.com> wrote:
> On Fri, Jun 10, 2011 at 12:12 AM, Mike Waychison <mikew@google.com> wrote:
>> On Thu, Jun 9, 2011 at 9:00 PM, Matt Domsch <Matt_Domsch@dell.com> wrote:
>>> On Mon, Jun 06, 2011 at 10:47:35PM +0100, Matthew Garrett wrote:
>>>> On Mon, Jun 06, 2011 at 02:43:46PM -0700, Tony Luck wrote:
>>>>
>>>> > Does someone claim maintainership of  drivers/firmware/efivars.c for part3?
>>>>
>>>> I think that'd be Matt.
>>>
>>> I'm out of the office this week and next, and haven't looked at it
>>> closely.  Mike Waychison has done most of the editing in there
>>> recently, I'd be happy if he wishes to be considered the maintainer
>>> thereof.
>>
>> I'm out of the office too until next Tuesday.  I should be able to
>> look at this between now and mid next-week.
>
> Mike; Did you find time to look at Matthew's efi-for-pstore patches yet?
> If we want to hit the 3.1 merge window we should be trying to get these
> beaten into shape pretty soon (Linus will presumably declare -rc4 tonight,
> so not much time left).
>

My bad.  This fell off my plate :(   Looking now.

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

[Mike Waychison]

On 06/06/11 12:38, Matthew Garrett wrote:
> 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.

How do I configure this thing?

Inline notes below.  Sorry for the delayed response.

>
> Signed-off-by: Matthew Garrett<mjg@redhat.com>
> ---
>   drivers/firmware/efivars.c |  158 +++++++++++++++++++++++++++++++++++++++++++-
>   include/linux/efi.h        |    3 +
>   2 files changed, 159 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 5f29aaf..89e6a3a 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,145 @@ static struct kobj_type efivar_ktype = {
>   	.default_attrs = def_attrs,
>   };
>
> +static struct efivar_entry *walk_entry;
> +
> +static struct pstore_info efi_pstore_info;

Can you move these into struct efivars in efi.h?

> +
>   static inline void
>   efivar_unregister(struct efivar_entry *var)
>   {
>   	kobject_put(&var->kobj);
>   }
>
> +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);

The format specifier here uses an unsigned, but your series passes part 
around as a signed int.  If part is truely non-negative, consider 
changing it to unsigned?

> +	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) {

Test for entry->var.VariableName[i] == 0 too.  Actually, could we just 
turn this string comparing loop into a strncmp test?

> +				found = entry;
> +				efi.set_variable(entry->var.VariableName,&entry->var.VendorGuid,

space after comma

> +						 EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
> +						 0, NULL);

We shouldn't be calling the global efi ops structure here.  Instead, we 
should be using efivars->ops->set_variable(...)

> +				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);

efivars->ops->set_variable.

> +
> +	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;
> +}
> +
> +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 +909,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);
> +	}
> +

Hmm.  pstore doesn't have a pstore_unregister?  This is unfortunate 
because this breaks efivars module unloading :(

>   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;

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

[Matthew Garrett]

On Mon, Jun 20, 2011 at 05:56:27PM -0700, Mike Waychison wrote:
> On 06/06/11 12:38, Matthew Garrett wrote:
> >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.
> 
> How do I configure this thing?

You don't. I'll be posting a patch for pstore that lets you choose the 
backend - that can be used to disable this functionality at boot time.

> >@@ -387,12 +400,145 @@ static struct kobj_type efivar_ktype = {
> >  	.default_attrs = def_attrs,
> >  };
> >
> >+static struct efivar_entry *walk_entry;
> >+
> >+static struct pstore_info efi_pstore_info;
> 
> Can you move these into struct efivars in efi.h?

In theory, but I don't really understand the benefit. You can't have 
more than one efivars implementation on a system.

> >+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);
> 
> The format specifier here uses an unsigned, but your series passes
> part around as a signed int.  If part is truely non-negative,
> consider changing it to unsigned?

The variable name is fundamentally meaningless. Just think of it as a 
binary representation of the data. Formatting it as a signed integer 
would break the parsing. But you're right that there's probably no 
reason for it to be signed in the first place - Tony?

> >+	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) {
> 
> Test for entry->var.VariableName[i] == 0 too.  Actually, could we
> just turn this string comparing loop into a strncmp test?

The idea is to check for prefixes. If efi_name[i] is non-zero but 
VariableName[i] is zero then we'll break due to them not matching, which 
is the desired behaviour.

> >+				found = entry;
> >+				efi.set_variable(entry->var.VariableName,&entry->var.VendorGuid,
> 
> space after comma

Oops.

> >+						 EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
> >+						 0, NULL);
> 
> We shouldn't be calling the global efi ops structure here.  Instead,
> we should be using efivars->ops->set_variable(...)

Ok.

> >  static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
> >  			     struct bin_attribute *bin_attr,
> >@@ -763,6 +909,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);
> >+	}
> >+
> 
> Hmm.  pstore doesn't have a pstore_unregister?  This is unfortunate
> because this breaks efivars module unloading :(

Userspace really ought to depend on efivars being available on EFI 
systems. I don't think losing the ability to unload it is a big loss.

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

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

[Tony Luck]

>> How do I configure this thing?
>
> You don't. I'll be posting a patch for pstore that lets you choose the
> backend - that can be used to disable this functionality at boot time.

For EFI - you need to configure CONFIG_PSTORE (which you may not be
able to see until you first set CONFIG_MISCFILESYSTEMS) ... and of course
you'll need CONFIG_EFIVARS.  At the moment (until Matthew provides the
new pstore boot time choosing argument, you might need to set CONFIG_ACPI_APEI=n
because otherwise its a link order race to see who gets to register
their back end first.

Once you boot into that kernel you can mount pstore:

 # mount -t pstore - /dev/pstore

and once you crash/shutdown/reboot and mount it again - you should see
some files
in /dev/pstore with the tail of the saved console log from the
previous kernel. Remove
the files to clear the underlying store (in your case delete the efi
variables that saved
the text of the console log).

>> >+    sprintf(stub_name, "dump-type%u-%u-", type, part);
>>
>> The format specifier here uses an unsigned, but your series passes
>> part around as a signed int.  If part is truely non-negative,
>> consider changing it to unsigned?
>
> The variable name is fundamentally meaningless. Just think of it as a
> binary representation of the data. Formatting it as a signed integer
> would break the parsing. But you're right that there's probably no
> reason for it to be signed in the first place - Tony?

part numbers are integers (1, 2, 3 ...) - so making it unsigned makes sense.

>> Hmm.  pstore doesn't have a pstore_unregister?  This is unfortunate
>> because this breaks efivars module unloading :(
>
> Userspace really ought to depend on efivars being available on EFI
> systems. I don't think losing the ability to unload it is a big loss.

If there is a strong demand for this - maybe someone will write a patch
for it?  But in general, once you attach a backend to pstore, I'd expect
you to leave it attached - you don't know whether the system is about
to crash, and you'd lose the kernel log if you crashed while the backend
was not registered.

-Tony

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

[Mike Waychison]

On Tue, Jun 21, 2011 at 8:10 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Mon, Jun 20, 2011 at 05:56:27PM -0700, Mike Waychison wrote:
>> On 06/06/11 12:38, Matthew Garrett wrote:
>> >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.
>>
>> How do I configure this thing?
>
> You don't. I'll be posting a patch for pstore that lets you choose the
> backend - that can be used to disable this functionality at boot time.

Hmm.  Okay.

>
>> >@@ -387,12 +400,145 @@ static struct kobj_type efivar_ktype = {
>> >     .default_attrs = def_attrs,
>> >  };
>> >
>> >+static struct efivar_entry *walk_entry;
>> >+
>> >+static struct pstore_info efi_pstore_info;
>>
>> Can you move these into struct efivars in efi.h?
>
> In theory, but I don't really understand the benefit. You can't have
> more than one efivars implementation on a system.

It's just cleaner and keeps the state of things in a single place,
rather than in globals.  I just cleaned all this global state up, and
would like to keep it clean.

>
>> >+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);
>>
>> The format specifier here uses an unsigned, but your series passes
>> part around as a signed int.  If part is truely non-negative,
>> consider changing it to unsigned?
>
> The variable name is fundamentally meaningless. Just think of it as a
> binary representation of the data. Formatting it as a signed integer
> would break the parsing.

What do you mean it would break the parsing?

> But you're right that there's probably no
> reason for it to be signed in the first place - Tony?
>
>> >+    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) {
>>
>> Test for entry->var.VariableName[i] == 0 too.  Actually, could we
>> just turn this string comparing loop into a strncmp test?
>
> The idea is to check for prefixes. If efi_name[i] is non-zero but
> VariableName[i] is zero then we'll break due to them not matching, which
> is the desired behaviour.

That's fine, but it's not what the code does.  It's also a lot clearer
to the reader if this isn't open coded.  We should also be checking
that this variable is ours with a guid check.  I'd be happier with a
utf16_strncmp here.  Will follow up with patches that stack on top of
yours.

>> >  static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
>> >                          struct bin_attribute *bin_attr,
>> >@@ -763,6 +909,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);
>> >+    }
>> >+
>>
>> Hmm.  pstore doesn't have a pstore_unregister?  This is unfortunate
>> because this breaks efivars module unloading :(
>
> Userspace really ought to depend on efivars being available on EFI
> systems. I don't think losing the ability to unload it is a big loss.

I don't know of any such dependencies.  Am I missing something?

I'll follow up with more patches that apply on top of yours.  I'm not
happy with the string operations in the driver as it is, and I've
cleaned some of this up.  Feel free to collapse whatever is needed
from them into your series or pick them up as is.

[PATCH 1/4] efivars: String functions

[Mike Waychison]

Fix the string functions in the efivars driver to be called utf16_*
instead of utf8_* as the encoding is utf16, not utf8.

As well, rename utf16_strlen to utf16_strnlen as it takes a maxlength
argument and the name should be consistent with the standard C function
names.  utf16_strlen is still provided for convenience in a subsequent
patch.

Signed-off-by: Mike Waychison <mikew@google.com>
---
 drivers/firmware/efivars.c |   30 +++++++++++++++++++-----------
 1 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 89e6a3a..f10c760 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -144,23 +144,29 @@ efivar_create_sysfs_entry(struct efivars *efivars,
 
 /* Return the number of unicode characters in data */
 static unsigned long
-utf8_strlen(efi_char16_t *data, unsigned long maxlength)
+utf16_strnlen(efi_char16_t *s, size_t maxlength)
 {
 	unsigned long length = 0;
 
-	while (*data++ != 0 && length < maxlength)
+	while (*s++ != 0 && length < maxlength)
 		length++;
 	return length;
 }
 
+static unsigned long
+utf16_strlen(efi_char16_t *s)
+{
+	return utf16_strnlen(s, ~0UL);
+}
+
 /*
  * Return the number of bytes is the length of this string
  * Note: this is NOT the same as the number of unicode characters
  */
 static inline unsigned long
-utf8_strsize(efi_char16_t *data, unsigned long maxlength)
+utf16_strsize(efi_char16_t *data, unsigned long maxlength)
 {
-	return utf8_strlen(data, maxlength/sizeof(efi_char16_t)) * sizeof(efi_char16_t);
+	return utf16_strnlen(data, maxlength/sizeof(efi_char16_t)) * sizeof(efi_char16_t);
 }
 
 static efi_status_t
@@ -516,7 +522,9 @@ static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
 		efivar_unregister(found);
 
 	if (size)
-		efivar_create_sysfs_entry(efivars, utf8_strsize(efi_name, DUMP_NAME_LEN * 2),
+		efivar_create_sysfs_entry(efivars,
+					  utf16_strsize(efi_name,
+							DUMP_NAME_LEN * 2),
 					  efi_name, &vendor);
 
 	return part;
@@ -560,8 +568,8 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
 	 * Does this variable already exist?
 	 */
 	list_for_each_entry_safe(search_efivar, n, &efivars->list, list) {
-		strsize1 = utf8_strsize(search_efivar->var.VariableName, 1024);
-		strsize2 = utf8_strsize(new_var->VariableName, 1024);
+		strsize1 = utf16_strsize(search_efivar->var.VariableName, 1024);
+		strsize2 = utf16_strsize(new_var->VariableName, 1024);
 		if (strsize1 == strsize2 &&
 			!memcmp(&(search_efivar->var.VariableName),
 				new_var->VariableName, strsize1) &&
@@ -593,8 +601,8 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
 
 	/* Create the entry in sysfs.  Locking is not required here */
 	status = efivar_create_sysfs_entry(efivars,
-					   utf8_strsize(new_var->VariableName,
-							1024),
+					   utf16_strsize(new_var->VariableName,
+							 1024),
 					   new_var->VariableName,
 					   &new_var->VendorGuid);
 	if (status) {
@@ -623,8 +631,8 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
 	 * Does this variable already exist?
 	 */
 	list_for_each_entry_safe(search_efivar, n, &efivars->list, list) {
-		strsize1 = utf8_strsize(search_efivar->var.VariableName, 1024);
-		strsize2 = utf8_strsize(del_var->VariableName, 1024);
+		strsize1 = utf16_strsize(search_efivar->var.VariableName, 1024);
+		strsize2 = utf16_strsize(del_var->VariableName, 1024);
 		if (strsize1 == strsize2 &&
 			!memcmp(&(search_efivar->var.VariableName),
 				del_var->VariableName, strsize1) &&
-- 
1.7.3.1

[PATCH 2/4] efivars: introduce utf16_strncmp

[Mike Waychison]

Introduce utf16_strncmp which is used in the next patch.  Semantics
should be the same as the strncmp C function.

Signed-off-by: Mike Waychison <mikew@google.com>
---
 drivers/firmware/efivars.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index f10c760..fb1219a 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -169,6 +169,24 @@ utf16_strsize(efi_char16_t *data, unsigned long maxlength)
 	return utf16_strnlen(data, maxlength/sizeof(efi_char16_t)) * sizeof(efi_char16_t);
 }
 
+static inline int
+utf16_strncmp(const efi_char16_t *a, const efi_char16_t *b, size_t len)
+{
+	while (1) {
+		if (len == 0)
+			return 0;
+		if (*a < *b)
+			return -1;
+		if (*a > *b)
+			return 1;
+		if (*a == 0) /* implies *b == 0 */
+			return 0;
+		a++;
+		b++;
+		len--;
+	}
+}
+
 static efi_status_t
 get_var_data_locked(struct efivars *efivars, struct efi_variable *var)
 {
-- 
1.7.3.1

[PATCH 3/4] efivars: Use string functions in pstore_write

[Mike Waychison]

Instead of open-coding the string operations for comparing the prefix of
the variable names, use the provided utf16_* string functions.

This patch also changes the calls to efi.set_variable to
efivars->ops->set_variable so that the right function gets called in the
case of gsmi (which doesn't have a valid efi structure).

As well, make sure that we only consider variables with the right vendor
string.

Signed-off-by: Mike Waychison <mikew@google.com>
---
 drivers/firmware/efivars.c |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index fb1219a..f424b10 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -511,17 +511,20 @@ static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
 	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 (efi_guidcmp(entry->var.VendorGuid, vendor))
+			continue;
+		if (utf16_strncmp(entry->var.VariableName, efi_name,
+				  utf16_strlen(efi_name)))
+			continue;
+		/* Needs to be a prefix */
+		if (entry->var.VariableName[utf16_strlen(efi_name)] == 0)
+			continue;
+
+		/* found */
+		found = entry;
+		efivars->ops->set_variable(entry->var.VariableName, &entry->var.VendorGuid,
+					   EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+					   0, NULL);
 	}
 
 	if (found)
@@ -530,9 +533,9 @@ static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
 	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);
+	efivars->ops->set_variable(efi_name, &vendor,
+				   EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+				   size, psi->buf);
 
 	spin_unlock(&efivars->lock);
 
-- 
1.7.3.1

[PATCH 4/4] efivars: Introduce PSTORE_EFI_ATTRIBUTES

[Mike Waychison]

Consolidate the attributes listed for pstore operations in one place,
PSTORE_EFI_ATTRIBUTES.

Signed-off-by: Mike Waychison <mikew@google.com>
---
 drivers/firmware/efivars.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index f424b10..739994b8 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -122,6 +122,10 @@ struct efivar_attribute {
 	ssize_t (*store)(struct efivar_entry *entry, const char *buf, size_t count);
 };
 
+#define PSTORE_EFI_ATTRIBUTES \
+	(EFI_VARIABLE_NON_VOLATILE | \
+	 EFI_VARIABLE_BOOTSERVICE_ACCESS | \
+	 EFI_VARIABLE_RUNTIME_ACCESS)
 
 #define EFIVAR_ATTR(_name, _mode, _show, _store) \
 struct efivar_attribute efivar_attr_##_name = { \
@@ -522,8 +526,9 @@ static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
 
 		/* found */
 		found = entry;
-		efivars->ops->set_variable(entry->var.VariableName, &entry->var.VendorGuid,
-					   EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+		efivars->ops->set_variable(entry->var.VariableName,
+					   &entry->var.VendorGuid,
+					   PSTORE_EFI_ATTRIBUTES,
 					   0, NULL);
 	}
 
@@ -533,8 +538,7 @@ static u64 efi_pstore_write(enum pstore_type_id type, int part, size_t size,
 	for (i = 0; i < DUMP_NAME_LEN; i++)
 		efi_name[i] = name[i];
 
-	efivars->ops->set_variable(efi_name, &vendor,
-				   EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+	efivars->ops->set_variable(efi_name, &vendor, PSTORE_EFI_ATTRIBUTES,
 				   size, psi->buf);
 
 	spin_unlock(&efivars->lock);
-- 
1.7.3.1

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

[Matthew Garrett]

On Tue, Jun 21, 2011 at 01:16:29PM -0700, Mike Waychison wrote:
> On Tue, Jun 21, 2011 at 8:10 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > You don't. I'll be posting a patch for pstore that lets you choose the
> > backend - that can be used to disable this functionality at boot time.
> 
> Hmm.  Okay.
> 
> > In theory, but I don't really understand the benefit. You can't have
> > more than one efivars implementation on a system.
> 
> It's just cleaner and keeps the state of things in a single place,
> rather than in globals.  I just cleaned all this global state up, and
> would like to keep it clean.

Ok, I'll do that.

> > The variable name is fundamentally meaningless. Just think of it as a
> > binary representation of the data. Formatting it as a signed integer
> > would break the parsing.
> 
> What do you mean it would break the parsing?

Actually, thinking about it, it'd be ok - but I'll take Tony's 
suggestion of just switching it to an unsigned.

> > The idea is to check for prefixes. If efi_name[i] is non-zero but
> > VariableName[i] is zero then we'll break due to them not matching, which
> > is the desired behaviour.
> 
> That's fine, but it's not what the code does.  It's also a lot clearer
> to the reader if this isn't open coded.  We should also be checking
> that this variable is ours with a guid check.  I'd be happier with a
> utf16_strncmp here.  Will follow up with patches that stack on top of
> yours.

Ok.

> > Userspace really ought to depend on efivars being available on EFI
> > systems. I don't think losing the ability to unload it is a big loss.
> 
> I don't know of any such dependencies.  Am I missing something?

Having a situation where efivars can appear and disappear while 
efibootmgr (for instance) is using it is kind of awkward. In the long 
run we'll probably need to interface with more EFI variables to play 
nice with the platform firmware (Apple have some expectations about this 
kind of thing), so we'll want to be able to guarantee it's there at boot 
time and shut down. And, as Tony says, the pstore use case does pretty 
much expect backends to be there.

> I'll follow up with more patches that apply on top of yours.  I'm not
> happy with the string operations in the driver as it is, and I've
> cleaned some of this up.  Feel free to collapse whatever is needed
> from them into your series or pick them up as is.

Gladly. Thanks!

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