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.
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
+ 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
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 | [email protected]
> 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
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 | [email protected]
> 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
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 | [email protected]
> Seems fair. I'll write a patch?
Sure. Don't forget Documentation/... to explain what it does.
-Tony