* pstore: enhancement for pstore write interface
@ 2011-09-03 5:30 Chen Gong
2011-09-03 5:30 ` [PATCH 1/2] pstore: change the purpose of return value for pstore write operation Chen Gong
2011-09-03 5:30 ` [PATCH 2/2] pstore: update the policy of the UEFI-based backend Chen Gong
0 siblings, 2 replies; 4+ messages in thread
From: Chen Gong @ 2011-09-03 5:30 UTC (permalink / raw)
To: tony.luck, mjg59; +Cc: linux-kernel
[PATCH 1/2] pstore: change the purpose of return value for pstore
[PATCH 2/2] pstore: update the policy of the UEFI-based backend
These two patches are for pstore write interface enhancement.
The 1st one updates the original write interface to return a valid
write result. The 2nd one updates the UEFI-based pstore backend
write policy. For the 2nd one, though Matthew current design is to
reduce UEFI variable space usage, but I think it is still too strict
as a log storage. :-)
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] pstore: change the purpose of return value for pstore write operation
2011-09-03 5:30 pstore: enhancement for pstore write interface Chen Gong
@ 2011-09-03 5:30 ` Chen Gong
2011-09-03 5:30 ` [PATCH 2/2] pstore: update the policy of the UEFI-based backend Chen Gong
1 sibling, 0 replies; 4+ messages in thread
From: Chen Gong @ 2011-09-03 5:30 UTC (permalink / raw)
To: tony.luck, mjg59; +Cc: linux-kernel, Chen Gong
currently pstore write interface employs record id as return
value, but it is not enough because it can't tell caller if
the write operation is successful. To get this purpose, change
the return value from record id to new meaning.
Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
---
drivers/acpi/apei/erst.c | 10 ++++++----
drivers/firmware/efivars.c | 17 +++++++++--------
fs/pstore/platform.c | 11 ++++++-----
include/linux/pstore.h | 4 ++--
4 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 2ca59dc..d498cd7 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -933,7 +933,7 @@ 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, unsigned int part,
+static int erst_writer(enum pstore_type_id type, u64 *id, unsigned int part,
size_t size, struct pstore_info *psi);
static int erst_clearer(enum pstore_type_id type, u64 id,
struct pstore_info *psi);
@@ -1040,11 +1040,12 @@ out:
return (rc < 0) ? rc : (len - sizeof(*rcd));
}
-static u64 erst_writer(enum pstore_type_id type, unsigned int part,
+static int erst_writer(enum pstore_type_id type, u64 *id, unsigned int part,
size_t size, struct pstore_info *psi)
{
struct cper_pstore_record *rcd = (struct cper_pstore_record *)
(erst_info.buf - sizeof(*rcd));
+ int ret;
memset(rcd, 0, sizeof(*rcd));
memcpy(rcd->hdr.signature, CPER_SIG_RECORD, CPER_SIG_SIZE);
@@ -1079,9 +1080,10 @@ static u64 erst_writer(enum pstore_type_id type, unsigned int part,
}
rcd->sec_hdr.section_severity = CPER_SEV_FATAL;
- erst_write(&rcd->hdr);
+ ret = erst_write(&rcd->hdr);
+ *id = rcd->hdr.record_id;
- return rcd->hdr.record_id;
+ return ret;
}
static int erst_clearer(enum pstore_type_id type, u64 id,
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index eb80b54..fe07673 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -490,8 +490,8 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
return 0;
}
-static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
- size_t size, struct pstore_info *psi)
+static int efi_pstore_write(enum pstore_type_id type, u64 *id,
+ unsigned int part, size_t size, struct pstore_info *psi)
{
char name[DUMP_NAME_LEN];
char stub_name[DUMP_NAME_LEN];
@@ -499,7 +499,7 @@ static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
struct efivars *efivars = psi->data;
struct efivar_entry *entry, *found = NULL;
- int i;
+ int i, ret = 0;
sprintf(stub_name, "dump-type%u-%u-", type, part);
sprintf(name, "%s%lu", stub_name, get_seconds());
@@ -548,18 +548,19 @@ static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
efivar_unregister(found);
if (size)
- efivar_create_sysfs_entry(efivars,
+ ret = efivar_create_sysfs_entry(efivars,
utf16_strsize(efi_name,
DUMP_NAME_LEN * 2),
efi_name, &vendor);
- return part;
+ *id = part;
+ return ret;
};
static int efi_pstore_erase(enum pstore_type_id type, u64 id,
struct pstore_info *psi)
{
- efi_pstore_write(type, id, 0, psi);
+ efi_pstore_write(type, &id, (unsigned int)id, 0, psi);
return 0;
}
@@ -580,8 +581,8 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
return -1;
}
-static u64 efi_pstore_write(enum pstore_type_id type, unsigned int part,
- size_t size, struct pstore_info *psi)
+static int efi_pstore_write(enum pstore_type_id type, u64 *id,
+ unsigned int part, size_t size, struct pstore_info *psi)
{
return 0;
}
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index c5300ec..633c0e5 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -69,7 +69,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
unsigned long size, total = 0;
char *dst, *why;
u64 id;
- int hsize;
+ int hsize, ret;
unsigned int part = 1;
if (reason < ARRAY_SIZE(reason_str))
@@ -97,9 +97,9 @@ 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, part,
+ ret = psinfo->write(PSTORE_TYPE_DMESG, &id, part,
hsize + l1_cpy + l2_cpy, psinfo);
- if (reason == KMSG_DUMP_OOPS && pstore_is_mounted())
+ if (ret == 0 && 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);
@@ -198,6 +198,7 @@ out:
int pstore_write(enum pstore_type_id type, char *buf, size_t size)
{
u64 id;
+ int ret;
if (!psinfo)
return -ENODEV;
@@ -207,8 +208,8 @@ 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, 0, size, psinfo);
- if (pstore_is_mounted())
+ ret = psinfo->write(type, &id, 0, size, psinfo);
+ if (ret == 0 && pstore_is_mounted())
pstore_mkfile(PSTORE_TYPE_DMESG, psinfo->name, id, psinfo->buf,
size, CURRENT_TIME, psinfo);
mutex_unlock(&psinfo->buf_mutex);
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index cc03bbf..976a3db 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -39,8 +39,8 @@ 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, unsigned int part,
- size_t size, struct pstore_info *psi);
+ int (*write)(enum pstore_type_id type, u64 *id,
+ unsigned int part, size_t size, struct pstore_info *psi);
int (*erase)(enum pstore_type_id type, u64 id,
struct pstore_info *psi);
void *data;
--
1.7.7.rc0.70.g82660
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] pstore: update the policy of the UEFI-based backend
2011-09-03 5:30 pstore: enhancement for pstore write interface Chen Gong
2011-09-03 5:30 ` [PATCH 1/2] pstore: change the purpose of return value for pstore write operation Chen Gong
@ 2011-09-03 5:30 ` Chen Gong
2011-09-03 12:36 ` Matthew Garrett
1 sibling, 1 reply; 4+ messages in thread
From: Chen Gong @ 2011-09-03 5:30 UTC (permalink / raw)
To: tony.luck, mjg59; +Cc: linux-kernel, Chen Gong
UEFI-based backend only employs one group records to save
dumped information, which means new log will overwrite old one.
It doesn't make sense because in fact the older log is nearer
to the truth. Update its policy to comply with ERST logic.
Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
---
drivers/firmware/efivars.c | 108 +++++++++++++++++++++++++------------------
1 files changed, 63 insertions(+), 45 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index fe07673..f33302d 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -463,8 +463,7 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
struct efivars *efivars = psi->data;
char name[DUMP_NAME_LEN];
int i;
- unsigned int part, size;
- unsigned long time;
+ unsigned int size;
while (&efivars->walk_entry->list != &efivars->list) {
if (!efi_guidcmp(efivars->walk_entry->var.VendorGuid,
@@ -472,9 +471,8 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
for (i = 0; i < DUMP_NAME_LEN; i++) {
name[i] = efivars->walk_entry->var.VariableName[i];
}
- if (sscanf(name, "dump-type%u-%u-%lu", type, &part, &time) == 3) {
- *id = part;
- timespec->tv_sec = time;
+ if (sscanf(name, "dump-type%u-%llu", type, id) == 2) {
+ timespec->tv_sec = *id;
timespec->tv_nsec = 0;
get_var_data_locked(efivars, &efivars->walk_entry->var);
size = efivars->walk_entry->var.DataSize;
@@ -494,24 +492,59 @@ static int efi_pstore_write(enum pstore_type_id type, u64 *id,
unsigned int part, size_t size, struct pstore_info *psi)
{
char 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;
+ efi_status_t status = EFI_NOT_FOUND;
+ int i, ret;
+ unsigned int time;
+
+ if (size == 0)
+ return -1;
+
+ time = get_seconds();
+ /* assume time + part = monotonic increasing */
+ *id = time + part;
+ sprintf(name, "dump-type%u-%llu", type, *id);
+
+ spin_lock(&efivars->lock);
+
+ for (i = 0; i < DUMP_NAME_LEN; i++)
+ efi_name[i] = name[i];
+
+ status = efivars->ops->set_variable(efi_name, &vendor,
+ PSTORE_EFI_ATTRIBUTES, size, psi->buf);
+ spin_unlock(&efivars->lock);
+
+ if (status != EFI_SUCCESS) {
+ printk(KERN_WARNING "efivars: set_variable() failed: "
+ "status=%lx\n", status);
+ return -EIO;
+ }
+
+ ret = efivar_create_sysfs_entry(efivars,
+ utf16_strsize(efi_name, DUMP_NAME_LEN * 2), efi_name, &vendor);
+
+ return ret;
+};
+
+static int efi_pstore_erase(enum pstore_type_id type, u64 id,
+ struct pstore_info *psi)
+{
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, ret = 0;
-
- sprintf(stub_name, "dump-type%u-%u-", type, part);
- sprintf(name, "%s%lu", stub_name, get_seconds());
+ efi_status_t status = EFI_NOT_FOUND;
+ int i;
- spin_lock(&efivars->lock);
+ sprintf(stub_name, "dump-type%u-%llu", type, id);
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name[i] = stub_name[i];
- /*
- * Clean up any entries with the same name
- */
+ spin_lock(&efivars->lock);
list_for_each_entry(entry, &efivars->list, list) {
get_var_data_locked(efivars, &entry->var);
@@ -521,46 +554,31 @@ static int efi_pstore_write(enum pstore_type_id type, u64 *id,
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,
- PSTORE_EFI_ATTRIBUTES,
- 0, NULL);
+ break;
}
- if (found)
- list_del(&found->list);
-
- for (i = 0; i < DUMP_NAME_LEN; i++)
- efi_name[i] = name[i];
+ if (!found) {
+ spin_unlock(&efivars->lock);
+ return -EINVAL;
+ }
- efivars->ops->set_variable(efi_name, &vendor, PSTORE_EFI_ATTRIBUTES,
- size, psi->buf);
+ status = efivars->ops->set_variable(entry->var.VariableName,
+ &entry->var.VendorGuid,
+ PSTORE_EFI_ATTRIBUTES,
+ 0, NULL);
+ if (status != EFI_SUCCESS) {
+ printk(KERN_WARNING "efivars: set_variable() failed: "
+ "status=%lx\n", status);
+ spin_unlock(&efivars->lock);
+ return -EIO;
+ }
+ list_del(&found->list);
spin_unlock(&efivars->lock);
-
- if (found)
- efivar_unregister(found);
-
- if (size)
- ret = efivar_create_sysfs_entry(efivars,
- utf16_strsize(efi_name,
- DUMP_NAME_LEN * 2),
- efi_name, &vendor);
-
- *id = part;
- return ret;
-};
-
-static int efi_pstore_erase(enum pstore_type_id type, u64 id,
- struct pstore_info *psi)
-{
- efi_pstore_write(type, &id, (unsigned int)id, 0, psi);
+ efivar_unregister(found);
return 0;
}
--
1.7.7.rc0.70.g82660
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] pstore: update the policy of the UEFI-based backend
2011-09-03 5:30 ` [PATCH 2/2] pstore: update the policy of the UEFI-based backend Chen Gong
@ 2011-09-03 12:36 ` Matthew Garrett
0 siblings, 0 replies; 4+ messages in thread
From: Matthew Garrett @ 2011-09-03 12:36 UTC (permalink / raw)
To: Chen Gong; +Cc: tony.luck, linux-kernel
I don't think this is reasonable. EFI variable space is a shared
resource. We have no idea what the failure modes of entirely filling it
would be. If we're going to store multiple records then at the very
least we need to use QueryVariableInfo() to identify how much space
there is left, and if it's a 2.1 or later system then we ideally need to
store these as EFI_VARIABLE_HARDWARE_ERROR_RECORD rather than just plain
runtime variables. The reason the current implementation doesn't do that
is that there's still hardware floating around that implements EFI 1.10
rather than UEFI 2.0
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-09-03 12:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-03 5:30 pstore: enhancement for pstore write interface Chen Gong
2011-09-03 5:30 ` [PATCH 1/2] pstore: change the purpose of return value for pstore write operation Chen Gong
2011-09-03 5:30 ` [PATCH 2/2] pstore: update the policy of the UEFI-based backend Chen Gong
2011-09-03 12:36 ` Matthew Garrett
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.