Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760363Ab2J3XFG (ORCPT ); Tue, 30 Oct 2012 19:05:06 -0400 Received: from ogre.sisk.pl ([193.178.161.156]:47081 "EHLO ogre.sisk.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758621Ab2J3XFC (ORCPT ); Tue, 30 Oct 2012 19:05:02 -0400 From: "Rafael J. Wysocki" To: Seiji Aguchi Cc: "linux-kernel@vger.kernel.org" , "linux-efi@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "Luck, Tony (tony.luck@intel.com)" , "Matthew Garrett (mjg@redhat.com)" , "dzickus@redhat.com" , "dle-develop@lists.sourceforge.net" , Satoru Moriya Subject: Re: [PATCH v4 5/7] efi_pstore: Add a sequence counter to a variable name Date: Wed, 31 Oct 2012 00:09:06 +0100 Message-ID: <7522525.tXjogSGO9Q@vostro.rjw.lan> User-Agent: KMail/4.8.5 (Linux/3.7.0-rc3; KDE/4.8.5; x86_64; ; ) In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13366 Lines: 361 On Tuesday, October 30, 2012 08:02:54 PM Seiji Aguchi wrote: > > [Issue] > > Currently, a variable name, which identifies each entry, consists of type, id and ctime. > But if multiple events happens in a short time, a second/third event may fail to log because > efi_pstore can't distinguish each event with current variable name. > > [Solution] > > A reasonable way to identify all events precisely is introducing a sequence counter to > the variable name. > > The sequence counter has already supported in a pstore layer with "oopscount". > So, this patch adds it to a variable name. > Also, it is passed to read/erase callbacks of platform drivers in accordance with > the modification of the variable name. > > > a variable name of first event: dump-type0-1-12345678 > a variable name of second event: dump-type0-1-12345678 > > type:0 > id:1 > ctime:12345678 > > If multiple events happen in a short time, efi_pstore can't distinguish them because > variable names are same among them. > > > > it can be distinguishable by adding a sequence counter as follows. > > a variable name of first event: dump-type0-1-1-12345678 > a variable name of Second event: dump-type0-1-2-12345678 > > type:0 > id:1 > sequence counter: 1(first event), 2(second event) > ctime:12345678 > > Signed-off-by: Seiji Aguchi Please feel free to add Acked-by: Rafael J. Wysocki for the erst.c change. Thanks! > --- > drivers/acpi/apei/erst.c | 12 ++++++------ > drivers/firmware/efivars.c | 23 ++++++++++++++--------- > fs/pstore/inode.c | 8 +++++--- > fs/pstore/internal.h | 2 +- > fs/pstore/platform.c | 11 ++++++----- > fs/pstore/ram.c | 7 +++---- > include/linux/pstore.h | 8 +++++--- > 7 files changed, 40 insertions(+), 31 deletions(-) > > diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c > index 0bd6ae4..6d894bf 100644 > --- a/drivers/acpi/apei/erst.c > +++ b/drivers/acpi/apei/erst.c > @@ -931,13 +931,13 @@ 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, > +static ssize_t erst_reader(u64 *id, enum pstore_type_id *type, int *count, > struct timespec *time, char **buf, > struct pstore_info *psi); > static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason, > - u64 *id, unsigned int part, > + u64 *id, unsigned int part, int count, > size_t size, struct pstore_info *psi); > -static int erst_clearer(enum pstore_type_id type, u64 id, > +static int erst_clearer(enum pstore_type_id type, u64 id, int count, > struct timespec time, struct pstore_info *psi); > > static struct pstore_info erst_info = { > @@ -987,7 +987,7 @@ static int erst_close_pstore(struct pstore_info *psi) > return 0; > } > > -static ssize_t erst_reader(u64 *id, enum pstore_type_id *type, > +static ssize_t erst_reader(u64 *id, enum pstore_type_id *type, int *count, > struct timespec *time, char **buf, > struct pstore_info *psi) > { > @@ -1055,7 +1055,7 @@ out: > } > > static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason, > - u64 *id, unsigned int part, > + u64 *id, unsigned int part, int count, > size_t size, struct pstore_info *psi) > { > struct cper_pstore_record *rcd = (struct cper_pstore_record *) > @@ -1101,7 +1101,7 @@ static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason, > return ret; > } > > -static int erst_clearer(enum pstore_type_id type, u64 id, > +static int erst_clearer(enum pstore_type_id type, u64 id, int count, > struct timespec time, struct pstore_info *psi) > { > return erst_clear(id); > diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c > index 3803621..7ad3aae 100644 > --- a/drivers/firmware/efivars.c > +++ b/drivers/firmware/efivars.c > @@ -658,13 +658,14 @@ static int efi_pstore_close(struct pstore_info *psi) > } > > static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type, > - struct timespec *timespec, > + int *count, struct timespec *timespec, > char **buf, struct pstore_info *psi) > { > efi_guid_t vendor = LINUX_EFI_CRASH_GUID; > struct efivars *efivars = psi->data; > char name[DUMP_NAME_LEN]; > int i; > + int cnt; > unsigned int part, size; > unsigned long time; > > @@ -674,8 +675,10 @@ 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) { > + if (sscanf(name, "dump-type%u-%u-%d-%lu", > + type, &part, &cnt, &time) == 4) { > *id = part; > + *count = cnt; > timespec->tv_sec = time; > timespec->tv_nsec = 0; > get_var_data_locked(efivars, &efivars->walk_entry->var); > @@ -698,7 +701,8 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type, > > static int efi_pstore_write(enum pstore_type_id type, > enum kmsg_dump_reason reason, u64 *id, > - unsigned int part, size_t size, struct pstore_info *psi) > + unsigned int part, int count, size_t size, > + struct pstore_info *psi) > { > char name[DUMP_NAME_LEN]; > efi_char16_t efi_name[DUMP_NAME_LEN]; > @@ -725,7 +729,7 @@ static int efi_pstore_write(enum pstore_type_id type, > return -ENOSPC; > } > > - sprintf(name, "dump-type%u-%u-%lu", type, part, > + sprintf(name, "dump-type%u-%u-%d-%lu", type, part, count, > get_seconds()); > > for (i = 0; i < DUMP_NAME_LEN; i++) > @@ -746,7 +750,7 @@ static int efi_pstore_write(enum pstore_type_id type, > return ret; > }; > > -static int efi_pstore_erase(enum pstore_type_id type, u64 id, > +static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count, > struct timespec time, struct pstore_info *psi) > { > char name[DUMP_NAME_LEN]; > @@ -756,7 +760,7 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, > struct efivar_entry *entry, *found = NULL; > int i; > > - sprintf(name, "dump-type%u-%u-%lu", type, (unsigned int)id, > + sprintf(name, "dump-type%u-%u-%d-%lu", type, (unsigned int)id, count, > time.tv_sec); > > spin_lock(&efivars->lock); > @@ -807,7 +811,7 @@ static int efi_pstore_close(struct pstore_info *psi) > return 0; > } > > -static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type, > +static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type, int *count, > struct timespec *timespec, > char **buf, struct pstore_info *psi) > { > @@ -816,12 +820,13 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type, > > static int efi_pstore_write(enum pstore_type_id type, > enum kmsg_dump_reason reason, u64 *id, > - unsigned int part, size_t size, struct pstore_info *psi) > + unsigned int part, int count, size_t size, > + struct pstore_info *psi) > { > return 0; > } > > -static int efi_pstore_erase(enum pstore_type_id type, u64 id, > +static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count, > struct timespec time, struct pstore_info *psi) > { > return 0; > diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c > index 4300af6..ed1d8c7 100644 > --- a/fs/pstore/inode.c > +++ b/fs/pstore/inode.c > @@ -49,6 +49,7 @@ struct pstore_private { > struct pstore_info *psi; > enum pstore_type_id type; > u64 id; > + int count; > ssize_t size; > char data[]; > }; > @@ -175,8 +176,8 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry) > struct pstore_private *p = dentry->d_inode->i_private; > > if (p->psi->erase) > - p->psi->erase(p->type, p->id, dentry->d_inode->i_ctime, > - p->psi); > + p->psi->erase(p->type, p->id, p->count, > + dentry->d_inode->i_ctime, p->psi); > > return simple_unlink(dir, dentry); > } > @@ -271,7 +272,7 @@ int pstore_is_mounted(void) > * Load it up with "size" bytes of data from "buf". > * 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, > +int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count, > char *data, size_t size, struct timespec time, > struct pstore_info *psi) > { > @@ -307,6 +308,7 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, > goto fail_alloc; > private->type = type; > private->id = id; > + private->count = count; > private->psi = psi; > > switch (type) { > diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h > index 4847f58..937d820 100644 > --- a/fs/pstore/internal.h > +++ b/fs/pstore/internal.h > @@ -50,7 +50,7 @@ extern struct pstore_info *psinfo; > extern void pstore_set_kmsg_bytes(int); > extern void pstore_get_records(int); > extern int pstore_mkfile(enum pstore_type_id, char *psname, u64 id, > - char *data, size_t size, > + int count, char *data, size_t size, > 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 a40da07..f911bbd 100644 > --- a/fs/pstore/platform.c > +++ b/fs/pstore/platform.c > @@ -136,7 +136,7 @@ static void pstore_dump(struct kmsg_dumper *dumper, > break; > > ret = psinfo->write(PSTORE_TYPE_DMESG, reason, &id, part, > - hsize + len, psinfo); > + oopscount, hsize + len, psinfo); > if (ret == 0 && reason == KMSG_DUMP_OOPS && pstore_is_mounted()) > pstore_new_entry = 1; > > @@ -196,7 +196,7 @@ static void pstore_register_console(void) {} > > static int pstore_write_compat(enum pstore_type_id type, > enum kmsg_dump_reason reason, > - u64 *id, unsigned int part, > + u64 *id, unsigned int part, int count, > size_t size, struct pstore_info *psi) > { > return psi->write_buf(type, reason, id, part, psinfo->buf, size, psi); > @@ -266,6 +266,7 @@ void pstore_get_records(int quiet) > char *buf = NULL; > ssize_t size; > u64 id; > + int count; > enum pstore_type_id type; > struct timespec time; > int failed = 0, rc; > @@ -277,9 +278,9 @@ void pstore_get_records(int quiet) > if (psi->open && psi->open(psi)) > goto out; > > - while ((size = psi->read(&id, &type, &time, &buf, psi)) > 0) { > - rc = pstore_mkfile(type, psi->name, id, buf, (size_t)size, > - time, psi); > + while ((size = psi->read(&id, &type, &count, &time, &buf, psi)) > 0) { > + rc = pstore_mkfile(type, psi->name, id, count, buf, > + (size_t)size, time, psi); > kfree(buf); > buf = NULL; > if (rc && (rc != -EEXIST || !quiet)) > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c > index 749693f..2bfa36e 100644 > --- a/fs/pstore/ram.c > +++ b/fs/pstore/ram.c > @@ -132,9 +132,8 @@ ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max, > } > > static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type, > - struct timespec *time, > - char **buf, > - struct pstore_info *psi) > + int *count, struct timespec *time, > + char **buf, struct pstore_info *psi) > { > ssize_t size; > struct ramoops_context *cxt = psi->data; > @@ -236,7 +235,7 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type, > return 0; > } > > -static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, > +static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count, > struct timespec time, struct pstore_info *psi) > { > struct ramoops_context *cxt = psi->data; > diff --git a/include/linux/pstore.h b/include/linux/pstore.h > index f6e9336..1788909 100644 > --- a/include/linux/pstore.h > +++ b/include/linux/pstore.h > @@ -50,17 +50,19 @@ 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, char **buf, > + int *count, struct timespec *time, char **buf, > struct pstore_info *psi); > int (*write)(enum pstore_type_id type, > enum kmsg_dump_reason reason, u64 *id, > - unsigned int part, size_t size, struct pstore_info *psi); > + unsigned int part, int count, size_t size, > + struct pstore_info *psi); > int (*write_buf)(enum pstore_type_id type, > enum kmsg_dump_reason reason, u64 *id, > unsigned int part, const char *buf, size_t size, > struct pstore_info *psi); > int (*erase)(enum pstore_type_id type, u64 id, > - struct timespec time, struct pstore_info *psi); > + int count, struct timespec time, > + struct pstore_info *psi); > void *data; > }; > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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/