dle-develop Mailing List for Dependable Linux Effort (Page 29)

Brought to you by: h-aoki, hiramatu, s-oshima, y-sugita

dle-develop — Dependable Linux Effort mailing list for developers

You can subscribe to this list here.

2009	Jan	Feb	Mar	Apr	May (32)	Jun (66)	Jul (102)	Aug (78)	Sep (106)	Oct (137)	Nov (147)	Dec (147)
2010	Jan (71)	Feb (139)	Mar (86)	Apr (76)	May (57)	Jun (10)	Jul (12)	Aug (6)	Sep (8)	Oct (12)	Nov (12)	Dec (18)
2011	Jan (16)	Feb (19)	Mar (3)	Apr (1)	May (16)	Jun (17)	Jul (74)	Aug (22)	Sep (18)	Oct (24)	Nov (21)	Dec (30)
2012	Jan (31)	Feb (16)	Mar (22)	Apr (25)	May (18)	Jun (13)	Jul (83)	Aug (49)	Sep (20)	Oct (60)	Nov (35)	Dec (28)
2013	Jan (39)	Feb (61)	Mar (35)	Apr (21)	May (45)	Jun (56)	Jul (20)	Aug (9)	Sep (10)	Oct (31)	Nov (8)	Dec (4)
2014	Jan (6)	Feb (7)	Mar (7)	Apr (6)	May (4)	Jun (8)	Jul (5)	Aug (2)	Sep (4)	Oct (4)	Nov (11)	Dec (5)
2015	Jan (4)	Feb (4)	Mar (3)	Apr (4)	May (9)	Jun (4)	Jul (15)	Aug (8)	Sep (16)	Oct (18)	Nov (15)	Dec (7)
2016	Jan (20)	Feb (9)	Mar (15)	Apr (24)	May (16)	Jun (28)	Jul (22)	Aug (23)	Sep (18)	Oct (30)	Nov (40)	Dec (9)
2017	Jan (1)	Feb (8)	Mar (37)	Apr (26)	May (25)	Jun (46)	Jul (24)	Aug (9)	Sep	Oct	Nov	Dec

Flat | Threaded

<< < 1 .. 27 28 29 30 31 .. 99 > >> (Page 29 of 99)

Re: [Dle-develop] [PATCH] Add realtime option

From: Satoru M. <sat...@hd...> - 2012-11-05 23:50:01

On 11/03/2012 03:45 AM, Jan Kiszka wrote:> On 2012-11-03 05:43, Satoru Moriya wrote:
>> We have some plans to migrate old enterprise/control systems which
>> require low latency (msec order) to kvm virtualized environment.
>> In order to satisfy the requirements, this patch adds realtime option
>> to qemu:
>>
>>  -realtime maxprio=<prio>,policy=<pol>
>>
>> This option change the scheduling policy and priority to realtime one
>> (only vcpu thread) as specified with argument and mlock all qemu and
>> guest memory.
> 
> This patch breaks win32 build. All the POSIX stuff has to be pushed into
> os-posix.c e.g. I'm introducing some os_prioritize() function for that
> purpose, empty on win32.
>
> Then another question is how to get the parameters around. I played with
> many options, ending up so far with
> 
> /* called by os_prioritize */
> void qemu_init_realtime(int rt_sched_policy, int max_sched_priority);
> /* called by threaded subsystems */
> bool qemu_realtime_is_enabled(void);
> void qemu_realtime_get_parameters(int *policy, int *max_priority);
> 
> all hosted by qemu-thread-*.c (empty/aborting on win32). This allows to
> adjust subsystems to realtime without pushing all the parameters into
> global variables.

Thanks. I'll re-implement the patch based on your comment.

>> Benchmark: cyclictest
>> https://rt.wiki.kernel.org/index.php/Cyclictest
>>
>> Command:
>>  $ cyclictest -p 99 -n -m -q -l 100000
>>
>> Results:
>>  - no load (1:normal qemu, 2:realtime qemu)
>>    1. T: 0 ( 544) P:99 I:1000 C:100000 Min: 11 Act: 32 Avg: 157 Max: 10029
>>    2. T: 0 ( 449) P:99 I:1000 C:100000 Min: 16 Act: 30 Avg:  29 Max:   540
>>
>>  - load (heavy network traffic) (3:normal qemu, 4: realtime qemu)
>>    3. T: 0 (3455) P:99 I:1000 C:100000 Min: 10 Act: 38 Avg: 364 Max: 18394
>>    4. T: 0 ( 493) P:99 I:1000 C:100000 Min: 12 Act: 21 Avg:  76 Max: 10796
> 
> What are the numbers of "chrt -f -p 99 <vcpu_tid>" compared to this?

I'm afraid that I don't have the results now. I'll post it later or
next version.

> My point is: This alone is not yet a good justification for the switch
> and its current semantic. The approach of just raising the VCPU priority
> is quite fragile without [V]CPU isolation. If you raise the VCPU over
> its event threads, specifically the iothread, you risk starvation, e.g
> during boot (BIOS will poll endlessly for PIT or disk).

I think it doesn't happen if host has enough cpu core (at least vcpu+1).
Is it wrong?

> Yes, there is
> /proc/sys/kernel/sched_rt_*, but this is what you typically disable when
> doing realtime seriously, particularly if your guest doesn't idle during
> operation.
>
> The model I would propose for mainline first is different: maxprio goes
> to the event threads, maxprio - 1 to all vcpus (means that maxprio must
> be > 1). This setup is less likely to starve and makes more sense
> (interrupts must have higher prio than CPUs).

Ok, I'll try your approach and test it.

> However, that's also not yet generic as we will have scenarios where
> only part of the event sources and VCPUs will be prioritized and the
> rest shall remain low prio / SCHED_OTHER. Besides defining a way to
> express such configurations, the problem is that they may not work
> during guest boot. So some realtime profile switching concept may also
> be needed. I haven't made up my mind on these issues yet. Not to speak
> of the horrible mess of configuring a PREEMPT-RT host...
>
> What is clear, though, is that we need a reference show case for
> realtime QEMU/KVM. One that is as easy to reproduce as possible, doesn't
> depend on proprietary realtime guests and clearly shows the advantages
> of all the needed changes for a reasonable use case. I'd like to discuss
> this at the RT-KVM BoF at the KVM Forum next week. Will you and/or any
> of your colleagues be there?

Yes. I'll attend the BOF.

Regards,
Satoru

Re: [Dle-develop] [PATCH] Add realtime option

From: Jan K. <jan...@we...> - 2012-11-03 07:45:57

Attachments: signature.asc

On 2012-11-03 05:43, Satoru Moriya wrote:
> We have some plans to migrate old enterprise/control systems which
> require low latency (msec order) to kvm virtualized environment.
> In order to satisfy the requirements, this patch adds realtime option
> to qemu:
> 
>  -realtime maxprio=<prio>,policy=<pol>
> 
> This option change the scheduling policy and priority to realtime one
> (only vcpu thread) as specified with argument and mlock all qemu and
> guest memory.

This patch breaks win32 build. All the POSIX stuff has to be pushed into
os-posix.c e.g. I'm introducing some os_prioritize() function for that
purpose, empty on win32.

Then another question is how to get the parameters around. I played with
many options, ending up so far with

/* called by os_prioritize */
void qemu_init_realtime(int rt_sched_policy, int max_sched_priority);
/* called by threaded subsystems */
bool qemu_realtime_is_enabled(void);
void qemu_realtime_get_parameters(int *policy, int *max_priority);

all hosted by qemu-thread-*.c (empty/aborting on win32). This allows to
adjust subsystems to realtime without pushing all the parameters into
global variables.

> 
> Of course, we need much more improvements to keep latency low in qemu
> virtualized environment and this is a first step. OTOH, we can meet the
> requirement of our first migration project with this patch.
> 
> These are basic performance test results:
> 
> Host : 4 core, 4GB, 3.7.0-rc3
> Guest: 1 core, 512MB, 3.6.3-1.fc17
> 
> Benchmark: cyclictest
> https://rt.wiki.kernel.org/index.php/Cyclictest
> 
> Command:
>  $ cyclictest -p 99 -n -m -q -l 100000
> 
> Results:
>  - no load (1:normal qemu, 2:realtime qemu)
>    1. T: 0 ( 544) P:99 I:1000 C:100000 Min: 11 Act: 32 Avg: 157 Max: 10029
>    2. T: 0 ( 449) P:99 I:1000 C:100000 Min: 16 Act: 30 Avg:  29 Max:   540
> 
>  - load (heavy network traffic) (3:normal qemu, 4: realtime qemu)
>    3. T: 0 (3455) P:99 I:1000 C:100000 Min: 10 Act: 38 Avg: 364 Max: 18394
>    4. T: 0 ( 493) P:99 I:1000 C:100000 Min: 12 Act: 21 Avg:  76 Max: 10796

What are the numbers of "chrt -f -p 99 <vcpu_tid>" compared to this?

My point is: This alone is not yet a good justification for the switch
and its current semantic. The approach of just raising the VCPU priority
is quite fragile without [V]CPU isolation. If you raise the VCPU over
its event threads, specifically the iothread, you risk starvation, e.g
during boot (BIOS will poll endlessly for PIT or disk). Yes, there is
/proc/sys/kernel/sched_rt_*, but this is what you typically disable when
doing realtime seriously, particularly if your guest doesn't idle during
operation.

The model I would propose for mainline first is different: maxprio goes
to the event threads, maxprio - 1 to all vcpus (means that maxprio must
be > 1). This setup is less likely to starve and makes more sense
(interrupts must have higher prio than CPUs).

However, that's also not yet generic as we will have scenarios where
only part of the event sources and VCPUs will be prioritized and the
rest shall remain low prio / SCHED_OTHER. Besides defining a way to
express such configurations, the problem is that they may not work
during guest boot. So some realtime profile switching concept may also
be needed. I haven't made up my mind on these issues yet. Not to speak
of the horrible mess of configuring a PREEMPT-RT host...

What is clear, though, is that we need a reference show case for
realtime QEMU/KVM. One that is as easy to reproduce as possible, doesn't
depend on proprietary realtime guests and clearly shows the advantages
of all the needed changes for a reasonable use case. I'd like to discuss
this at the RT-KVM BoF at the KVM Forum next week. Will you and/or any
of your colleagues be there?

Jan

[Dle-develop] [PATCH] Add realtime option

From: Satoru M. <sat...@hd...> - 2012-11-03 04:43:36

We have some plans to migrate old enterprise/control systems which
require low latency (msec order) to kvm virtualized environment.
In order to satisfy the requirements, this patch adds realtime option
to qemu:

 -realtime maxprio=<prio>,policy=<pol>

This option change the scheduling policy and priority to realtime one
(only vcpu thread) as specified with argument and mlock all qemu and
guest memory.

Of course, we need much more improvements to keep latency low in qemu
virtualized environment and this is a first step. OTOH, we can meet the
requirement of our first migration project with this patch.

These are basic performance test results:

Host : 4 core, 4GB, 3.7.0-rc3
Guest: 1 core, 512MB, 3.6.3-1.fc17

Benchmark: cyclictest
https://rt.wiki.kernel.org/index.php/Cyclictest

Command:
 $ cyclictest -p 99 -n -m -q -l 100000

Results:
 - no load (1:normal qemu, 2:realtime qemu)
   1. T: 0 ( 544) P:99 I:1000 C:100000 Min: 11 Act: 32 Avg: 157 Max: 10029
   2. T: 0 ( 449) P:99 I:1000 C:100000 Min: 16 Act: 30 Avg:  29 Max:   540

 - load (heavy network traffic) (3:normal qemu, 4: realtime qemu)
   3. T: 0 (3455) P:99 I:1000 C:100000 Min: 10 Act: 38 Avg: 364 Max: 18394
   4. T: 0 ( 493) P:99 I:1000 C:100000 Min: 12 Act: 21 Avg:  76 Max: 10796

Signed-off-by: Satoru Moriya <sat...@hd...>
---
 cpus.c          | 10 ++++++++++
 cpus.h          |  3 +++
 qemu-config.c   | 16 ++++++++++++++++
 qemu-options.hx |  9 +++++++++
 vl.c            | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 89 insertions(+)

diff --git a/cpus.c b/cpus.c
index d9c332f..456e6ea 100644
--- a/cpus.c
+++ b/cpus.c
@@ -734,6 +734,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
     CPUArchState *env = arg;
     CPUState *cpu = ENV_GET_CPU(env);
     int r;
+    struct sched_param sp;
 
     qemu_mutex_lock(&qemu_global_mutex);
     qemu_thread_get_self(cpu->thread);
@@ -746,6 +747,15 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
         exit(1);
     }
 
+    if (realtime) {
+        sp.sched_priority = realtime_prio;
+        r = sched_setscheduler(0, realtime_pol, &sp);
+        if (r < 0) {
+            perror("Setting realtime policy failed");
+            exit(1);
+        }
+    }
+
     qemu_kvm_init_cpu_signals(env);
 
     /* signal CPU creation */
diff --git a/cpus.h b/cpus.h
index 81bd817..a6b2688 100644
--- a/cpus.h
+++ b/cpus.h
@@ -16,6 +16,9 @@ void qtest_clock_warp(int64_t dest);
 /* vl.c */
 extern int smp_cores;
 extern int smp_threads;
+extern int realtime;
+extern int realtime_prio;
+extern int realtime_pol;
 void set_numa_modes(void);
 void set_cpu_log(const char *optarg);
 void set_cpu_log_filename(const char *optarg);
diff --git a/qemu-config.c b/qemu-config.c
index 3154cac..13290c6 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -658,6 +658,21 @@ QemuOptsList qemu_boot_opts = {
             .type = QEMU_OPT_STRING,
         },
         { /*End of list */ }
+    },
+};
+
+QemuOptsList qemu_realtime_opts = {
+    .name = "realtime",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_realtime_opts.head),
+    .desc = {
+        {
+            .name = "maxprio",
+            .type = QEMU_OPT_NUMBER,
+        }, {
+            .name = "policy",
+            .type = QEMU_OPT_STRING,
+        },
+        { /* End of List */ }
     },
 };
 
@@ -699,6 +714,7 @@ static QemuOptsList *vm_config_groups[32] = {
     &qemu_iscsi_opts,
     &qemu_sandbox_opts,
     &qemu_add_fd_opts,
+    &qemu_realtime_opts,
     NULL,
 };
 
diff --git a/qemu-options.hx b/qemu-options.hx
index fe8f15c..eb8ba05 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2405,6 +2405,15 @@ STEXI
 Do not start CPU at startup (you must type 'c' in the monitor).
 ETEXI
 
+DEF("realtime", HAS_ARG, QEMU_OPTION_realtime,
+    "-realtime maxprio=prio[,policy=pol]\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -realtime maxprio=@var{prio}[,policy=@var{pol}]
+@findex -realtime
+run qemu as a realtime process with priority @var{prio} and policy @var{pol}.
+ETEXI
+
 DEF("gdb", HAS_ARG, QEMU_OPTION_gdb, \
     "-gdb dev        wait for gdb connection on 'dev'\n", QEMU_ARCH_ALL)
 STEXI
diff --git a/vl.c b/vl.c
index 0f5b07b..a08fe79 100644
--- a/vl.c
+++ b/vl.c
@@ -248,6 +248,10 @@ int nb_numa_nodes;
 uint64_t node_mem[MAX_NODES];
 unsigned long *node_cpumask[MAX_NODES];
 
+int realtime;
+int realtime_prio;
+int realtime_pol;
+
 uint8_t qemu_uuid[16];
 
 static QEMUBootSetHandler *boot_set_handler;
@@ -1151,6 +1155,45 @@ static void smp_parse(const char *optarg)
         max_cpus = smp_cpus;
 }
 
+static void configure_realtime(QemuOpts *opts) {
+    int prio, max_prio, min_prio;
+    const char *pol;
+
+    pol = qemu_opt_get(opts, "policy");
+    if (pol) {
+        if (!strcmp(pol, "rr")) {
+            realtime_pol = SCHED_RR;
+        } else if (!strcmp(pol, "fifo")) {
+            realtime_pol = SCHED_FIFO;
+        } else {
+            fprintf(stderr, "qemu: invalid option value '%s'\n", pol);
+            exit(1);
+        }
+    } else {
+        realtime_pol = SCHED_RR;
+    }
+    prio = qemu_opt_get_number(opts, "maxprio", 1);
+
+    min_prio = sched_get_priority_min(realtime_pol);
+    max_prio = sched_get_priority_max(realtime_pol);
+
+    if (prio < min_prio) {
+        realtime_prio = min_prio;
+    } else if (max_prio < prio) {
+        realtime_prio = max_prio;
+    } else {
+        realtime_prio = prio;
+    }
+
+    if (mlockall(MCL_CURRENT | MCL_FUTURE)) {
+        perror("mlock");
+        exit(1);
+    }
+
+    realtime = 1;
+}
+
 /***********************************************************/
 /* USB devices */
 
@@ -2712,6 +2755,14 @@ int main(int argc, char **argv, char **envp)
                 }
                 numa_add(optarg);
                 break;
+            case QEMU_OPTION_realtime:
+                opts = qemu_opts_parse(qemu_find_opts("realtime"), optarg, 0);
+                if (!opts) {
+                    fprintf(stderr, "parse error: %s\n", optarg);
+                    exit(1);
+                }
+                configure_realtime(opts);
+                break;
             case QEMU_OPTION_display:
                 display_type = select_display(optarg);
                 break;
--
1.7.11.7

Re: [Dle-develop] [RESEND][PATCH v4 0/7] efi_pstore: multiple event logging support

From: Mike W. <mi...@go...> - 2012-11-03 00:04:00

Series Acked-by: Mike Waychison <mi...@go...>

Tony: Did you want to pull this?

On Thu, Nov 1, 2012 at 3:59 PM, Seiji Aguchi <sei...@hd...> wrote:
> Changelog
>
> v3 -> v4
>    - Rebase to 3.7-rc3
>    - Add ctime to an argument of efi_pstore_erase to build successfully
>      in case where CONFIG_PTORE=n is specified. (Patch 4/7)
>    - Add count to an argument of efi_pstore_erase and efi_pstore_write to
>      build successfully in case where CONFIG_PTORE=n is specified (Patch 5/7)
>    - Fix a white space damage (Patch 6/7)
>    - Modify a logic checking a 3-variable format of a variable name (Patch 7/7)
>
> v2 -> v3
>    - Create patches 6/7 and 7/7 to work with an existing format of variable name
>
> v1 -> v2
>    - Separate into 5 patches in accordance with Mike's comment
>    - Erase an extra line of comment in patch 1/5
>
> [Issue]
>
>     Currently, efi_pstore driver simply overwrites existing panic messages in NVRAM.
>     So, in the following scenario, we will lose 1st panic messages.
>
>     1. kernel panics.
>     2. efi_pstore is kicked and writes panic messages to NVRAM.
>     3. system reboots.
>     4. kernel panics again before a user checks the 1st panic messages in NVRAM.
>
> [Solution]
>
>    Solutions of this problem has been discussed among Tony, Matthew, Don, Mike and me.
>
>    http://marc.info/?l=linux-kernel&m=134273270704586&w=2
>
>    And there are two possible solutions right now.
>      - First one is introducing some policy overwriting existing logs.
>      - Second one is simply holding multiple log without overwriting any entries.
>
>    We haven't decided the overwriting policy which is reasonable to all users yet.
>    But I believe we agree that just holding multiple logs is a reasonable way.
>
>    We may need further discussions to find the possibility of introducing overwriting
>    policy, especially getting critical messages in multiple oops case.
>    But I would like to begin with a simple and reasonable way to everyone.
>    So, this patch takes an approach just holding multiple logs.
>
> [Patch Description]
>
> Seiji Aguchi (7):
>   efi_pstore: Check remaining space with QueryVariableInfo() before
>     writing data
>   efi_pstore: Add a logic erasing entries to an erase callback
>   efi_pstore: Remove a logic erasing entries from a write callback to
>     hold multiple logs
>   efi_pstore: Add ctime to argument of erase callback
>   efi_pstore: Add a sequence counter to a variable name
>   efi_pstore: Add a format check for an existing variable name at
>     reading time
>   Add a format check for an existing variable name at erasing time
>
>  drivers/acpi/apei/erst.c   |   16 ++--
>  drivers/firmware/efivars.c |  163 ++++++++++++++++++++++++++++++-------------
>  fs/pstore/inode.c          |    7 ++-
>  fs/pstore/internal.h       |    2 +-
>  fs/pstore/platform.c       |   11 ++--
>  fs/pstore/ram.c            |    9 +--
>  include/linux/efi.h        |    1 +
>  include/linux/pstore.h     |    6 +-
>  8 files changed, 143 insertions(+), 72 deletions(-)

[Dle-develop] [RESEND][PATCH v4 7/7] Add a format check for an existing variable name at erasing time

From: Seiji A. <sei...@hd...> - 2012-11-01 23:03:11

[Issue]

a format of variable name has been updated to type, id, count and ctime
to support holding multiple logs.

Format of current variable name
  dump-type0-1-2-12345678

  type:0
  id:1
  count:2
  ctime:12345678

On the other hand, if an old variable name before being updated
remains, users can't erase it via /dev/pstore.

Format of old variable name
  dump-type0-1-12345678

  type:0
  id:1
  ctime:12345678

[Solution]

This patch add a format check for the old variable name in a erase callback to make it erasable.

Signed-off-by: Seiji Aguchi <sei...@hd...>
---
 drivers/firmware/efivars.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index bf7a6f9..6e51c1e 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -773,6 +773,8 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
 {
 	char name[DUMP_NAME_LEN];
 	efi_char16_t efi_name[DUMP_NAME_LEN];
+	char name_old[DUMP_NAME_LEN];
+	efi_char16_t efi_name_old[DUMP_NAME_LEN];
 	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
 	struct efivars *efivars = psi->data;
 	struct efivar_entry *entry, *found = NULL;
@@ -796,8 +798,22 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
 		if (efi_guidcmp(entry->var.VendorGuid, vendor))
 			continue;
 		if (utf16_strncmp(entry->var.VariableName, efi_name,
-				  utf16_strlen(efi_name)))
-			continue;
+				  utf16_strlen(efi_name))) {
+			/*
+			 * Check if an old format,
+			 * which doesn't support holding
+			 * multiple logs, remains.
+			 */
+			sprintf(name_old, "dump-type%u-%u-%lu", type,
+				(unsigned int)id, time.tv_sec);
+
+			for (i = 0; i < DUMP_NAME_LEN; i++)
+				efi_name_old[i] = name_old[i];
+
+			if (utf16_strncmp(entry->var.VariableName, efi_name_old,
+					  utf16_strlen(efi_name_old)))
+				continue;
+		}
 
 		/* found */
 		found = entry;
-- 
1.7.1

[Dle-develop] [RESEND][PATCH v4 6/7] efi_pstore: Add a format check for an existing variable name at reading time

From: Seiji A. <sei...@hd...> - 2012-11-01 23:02:35

[Issue]

a format of variable name has been updated to type, id, count and ctime
to support holding multiple logs.

Format of current variable name
  dump-type0-1-2-12345678

  type:0
  id:1
  count:2
  ctime:12345678

On the other hand, if an old variable name before being updated
remains, users can't read it via /dev/pstore.

Format of old variable name
  dump-type0-1-12345678

  type:0
  id:1
  ctime:12345678

[Solution]

This patch add a format check for the old variable name in a read callback
to make it readable.

Signed-off-by: Seiji Aguchi <sei...@hd...>
---
 drivers/firmware/efivars.c |   38 ++++++++++++++++++++++++++++----------
 1 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 7ad3aae..bf7a6f9 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -681,17 +681,35 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
 				*count = cnt;
 				timespec->tv_sec = time;
 				timespec->tv_nsec = 0;
-				get_var_data_locked(efivars, &efivars->walk_entry->var);
-				size = efivars->walk_entry->var.DataSize;
-				*buf = kmalloc(size, GFP_KERNEL);
-				if (*buf == NULL)
-					return -ENOMEM;
-				memcpy(*buf, efivars->walk_entry->var.Data,
-				       size);
-				efivars->walk_entry = list_entry(efivars->walk_entry->list.next,
-					           struct efivar_entry, list);
-				return size;
+			} else if (sscanf(name, "dump-type%u-%u-%lu",
+				   type, &part, &time) == 3) {
+				/*
+				 * Check if an old format,
+				 * which doesn't support holding
+				 * multiple logs, remains.
+				 */
+				*id = part;
+				*count = 0;
+				timespec->tv_sec = time;
+				timespec->tv_nsec = 0;
+			} else {
+				efivars->walk_entry = list_entry(
+						efivars->walk_entry->list.next,
+						struct efivar_entry, list);
+				continue;
 			}
+
+			get_var_data_locked(efivars, &efivars->walk_entry->var);
+			size = efivars->walk_entry->var.DataSize;
+			*buf = kmalloc(size, GFP_KERNEL);
+			if (*buf == NULL)
+				return -ENOMEM;
+			memcpy(*buf, efivars->walk_entry->var.Data,
+			       size);
+			efivars->walk_entry = list_entry(
+					efivars->walk_entry->list.next,
+					struct efivar_entry, list);
+			return size;
 		}
 		efivars->walk_entry = list_entry(efivars->walk_entry->list.next,
 						 struct efivar_entry, list);
-- 
1.7.1

[Dle-develop] [RESEND][PATCH v4 5/7] efi_pstore: Add a sequence counter to a variable name

From: Seiji A. <sei...@hd...> - 2012-11-01 23:02:03

[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.

  <before applying this patch>
 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.

  <after applying this patch>

 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 <sei...@hd...>
Acked-by: Rafael J. Wysocki <raf...@in...>
---
 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;
 };
 
-- 
1.7.1

[Dle-develop] [RESEND][PATCH v4 4/7] efi_pstore: Add ctime to argument of erase callback

From: Seiji A. <sei...@hd...> - 2012-11-01 23:01:37

[Issue]

Currently, a variable name, which is used to identify each log entry, consists of type,
id and ctime. But an erase callback does not use ctime.

If efi_pstore supported just one log, type and id were enough.
However, in case of supporting multiple logs, it doesn't work because
it can't distinguish each entry without ctime at erasing time.

 <Example>

 As you can see below, efi_pstore can't differentiate first event from second one without ctime.

 a variable name of first event: dump-type0-1-12345678
 a variable name of second event: dump-type0-1-23456789

  type:0
  id:1
  ctime:12345678, 23456789

[Solution]

This patch adds ctime to an argument of an erase callback.

It works across reboots because ctime of pstore means the date that the record was originally stored.
To do this, efi_pstore saves the ctime to variable name at writing time and passes it to pstore
at reading time.

Signed-off-by: Seiji Aguchi <sei...@hd...>
---
 drivers/acpi/apei/erst.c   |    4 ++--
 drivers/firmware/efivars.c |   17 ++++++++---------
 fs/pstore/inode.c          |    3 ++-
 fs/pstore/ram.c            |    2 +-
 include/linux/pstore.h     |    2 +-
 5 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index e4d9d24..0bd6ae4 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -938,7 +938,7 @@ static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason,
 		       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);
+			struct timespec time, struct pstore_info *psi);
 
 static struct pstore_info erst_info = {
 	.owner		= THIS_MODULE,
@@ -1102,7 +1102,7 @@ static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason,
 }
 
 static int erst_clearer(enum pstore_type_id type, u64 id,
-			struct pstore_info *psi)
+			struct timespec time, struct pstore_info *psi)
 {
 	return erst_clear(id);
 }
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index fbe9202..3803621 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -747,24 +747,25 @@ static int efi_pstore_write(enum pstore_type_id type,
 };
 
 static int efi_pstore_erase(enum pstore_type_id type, u64 id,
-			    struct pstore_info *psi)
+			    struct timespec time, struct pstore_info *psi)
 {
-	char stub_name[DUMP_NAME_LEN];
+	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;
 	struct efivar_entry *entry, *found = NULL;
 	int i;
 
-	sprintf(stub_name, "dump-type%u-%u-", type, (unsigned int)id);
+	sprintf(name, "dump-type%u-%u-%lu", type, (unsigned int)id,
+		time.tv_sec);
 
 	spin_lock(&efivars->lock);
 
 	for (i = 0; i < DUMP_NAME_LEN; i++)
-		efi_name[i] = stub_name[i];
+		efi_name[i] = name[i];
 
 	/*
-	 * Clean up any entries with the same name
+	 * Clean up an entry with the same name
 	 */
 
 	list_for_each_entry(entry, &efivars->list, list) {
@@ -775,9 +776,6 @@ static int efi_pstore_erase(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;
@@ -785,6 +783,7 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id,
 					   &entry->var.VendorGuid,
 					   PSTORE_EFI_ATTRIBUTES,
 					   0, NULL);
+		break;
 	}
 
 	if (found)
@@ -823,7 +822,7 @@ static int efi_pstore_write(enum pstore_type_id type,
 }
 
 static int efi_pstore_erase(enum pstore_type_id type, u64 id,
-			    struct pstore_info *psi)
+			    struct timespec time, struct pstore_info *psi)
 {
 	return 0;
 }
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 4ab572e..4300af6 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -175,7 +175,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, p->psi);
+		p->psi->erase(p->type, p->id, dentry->d_inode->i_ctime,
+			      p->psi);
 
 	return simple_unlink(dir, dentry);
 }
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 1a4f6da..749693f 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -237,7 +237,7 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
 }
 
 static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,
-				struct pstore_info *psi)
+				struct timespec time, struct pstore_info *psi)
 {
 	struct ramoops_context *cxt = psi->data;
 	struct persistent_ram_zone *prz;
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index ee3034a..f6e9336 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -60,7 +60,7 @@ struct pstore_info {
 			unsigned int part, const char *buf, size_t size,
 			struct pstore_info *psi);
 	int		(*erase)(enum pstore_type_id type, u64 id,
-			struct pstore_info *psi);
+			struct timespec time, struct pstore_info *psi);
 	void		*data;
 };
 
-- 
1.7.1

[Dle-develop] [RESEND][PATCH v4 3/7] efi_pstore: Remove a logic erasing entries from a write callback to hold multiple logs

From: Seiji A. <sei...@hd...> - 2012-11-01 23:01:23

[Issue]

Currently, efi_pstore driver simply overwrites existing panic messages in NVRAM.
So, in the following scenario, we will lose 1st panic messages.

1. kernel panics.
2. efi_pstore is kicked and writes panic messages to NVRAM.
3. system reboots.
4. kernel panics again before a user checks the 1st panic messages in NVRAM.

[Solution]

A reasonable solution to fix the issue is just holding multiple logs without erasing
existing entries.
This patch removes a logic erasing existing entries in a write callback
because the logic is not needed in the write callback to support holding multiple logs.

Signed-off-by: Seiji Aguchi <sei...@hd...>
---
 drivers/firmware/efivars.c |   39 ++-------------------------------------
 1 files changed, 2 insertions(+), 37 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index bee14cc..fbe9202 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -701,18 +701,13 @@ static int efi_pstore_write(enum pstore_type_id type,
 		unsigned 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, ret = 0;
 	u64 storage_space, remaining_space, max_variable_size;
 	efi_status_t status = EFI_NOT_FOUND;
 
-	sprintf(stub_name, "dump-type%u-%u-", type, part);
-	sprintf(name, "%s%lu", stub_name, get_seconds());
-
 	spin_lock(&efivars->lock);
 
 	/*
@@ -730,35 +725,8 @@ static int efi_pstore_write(enum pstore_type_id type,
 		return -ENOSPC;
 	}
 
-	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);
-
-		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,
-					   PSTORE_EFI_ATTRIBUTES,
-					   0, NULL);
-	}
-
-	if (found)
-		list_del(&found->list);
+	sprintf(name, "dump-type%u-%u-%lu", type, part,
+		get_seconds());
 
 	for (i = 0; i < DUMP_NAME_LEN; i++)
 		efi_name[i] = name[i];
@@ -768,9 +736,6 @@ static int efi_pstore_write(enum pstore_type_id type,
 
 	spin_unlock(&efivars->lock);
 
-	if (found)
-		efivar_unregister(found);
-
 	if (size)
 		ret = efivar_create_sysfs_entry(efivars,
 					  utf16_strsize(efi_name,
-- 
1.7.1

[Dle-develop] [RESEND][PATCH v4 2/7] efi_pstore: Add a logic erasing entries to an erase callback

From: Seiji A. <sei...@hd...> - 2012-11-01 23:00:50

[Issue]

Currently, efi_pstore driver simply overwrites existing panic messages in NVRAM.
So, in the following scenario, we will lose 1st panic messages.

 1. kernel panics.
 2. efi_pstore is kicked and writes panic messages to NVRAM.
 3. system reboots.
 4. kernel panics again before a user checks the 1st panic messages in NVRAM.

[Solution]

A reasonable solution to fix the issue is just holding multiple logs without erasing
existing entries.

This patch freshly adds a logic erasing existing entries, which shared with a write callback,
to an erase callback.
To support holding multiple logs, the write callback doesn't need to erase any entries and
it will be removed in a subsequent patch.

Signed-off-by: Seiji Aguchi <sei...@hd...>
---
 drivers/firmware/efivars.c |   46 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 37ac21a..bee14cc 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -784,7 +784,51 @@ static int efi_pstore_write(enum pstore_type_id type,
 static int efi_pstore_erase(enum pstore_type_id type, u64 id,
 			    struct pstore_info *psi)
 {
-	efi_pstore_write(type, 0, &id, (unsigned int)id, 0, 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;
+
+	sprintf(stub_name, "dump-type%u-%u-", type, (unsigned int)id);
+
+	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);
+
+		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,
+					   PSTORE_EFI_ATTRIBUTES,
+					   0, NULL);
+	}
+
+	if (found)
+		list_del(&found->list);
+
+	spin_unlock(&efivars->lock);
+
+	if (found)
+		efivar_unregister(found);
 
 	return 0;
 }
-- 
1.7.1

[Dle-develop] [RESEND][PATCH v4 1/7] efi_pstore: Check remaining space with QueryVariableInfo() before writing data

From: Seiji A. <sei...@hd...> - 2012-11-01 22:59:49

[Issue]

As discussed in a thread below, Running out of space in EFI isn't a well-tested scenario.
And we wouldn't expect all firmware to handle it gracefully.
http://marc.info/?l=linux-kernel&m=134305325801789&w=2

On the other hand, current efi_pstore doesn't check a remaining space of storage at writing time.
Therefore, efi_pstore may not work if it tries to write a large amount of data.

[Patch Description]

To avoid handling the situation above, this patch checks if there is a space enough to log with
QueryVariableInfo() before writing data.

Signed-off-by: Seiji Aguchi <sei...@hd...>
Acked-by: Mike Waychison <mi...@go...>
---
 drivers/firmware/efivars.c |   18 ++++++++++++++++++
 include/linux/efi.h        |    1 +
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index d10c987..37ac21a 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -707,12 +707,29 @@ static int efi_pstore_write(enum pstore_type_id type,
 	struct efivars *efivars = psi->data;
 	struct efivar_entry *entry, *found = NULL;
 	int i, ret = 0;
+	u64 storage_space, remaining_space, max_variable_size;
+	efi_status_t status = EFI_NOT_FOUND;
 
 	sprintf(stub_name, "dump-type%u-%u-", type, part);
 	sprintf(name, "%s%lu", stub_name, get_seconds());
 
 	spin_lock(&efivars->lock);
 
+	/*
+	 * Check if there is a space enough to log.
+	 * size: a size of logging data
+	 * DUMP_NAME_LEN * 2: a maximum size of variable name
+	 */
+	status = efivars->ops->query_variable_info(PSTORE_EFI_ATTRIBUTES,
+						   &storage_space,
+						   &remaining_space,
+						   &max_variable_size);
+	if (status || remaining_space < size + DUMP_NAME_LEN * 2) {
+		spin_unlock(&efivars->lock);
+		*id = part;
+		return -ENOSPC;
+	}
+
 	for (i = 0; i < DUMP_NAME_LEN; i++)
 		efi_name[i] = stub_name[i];
 
@@ -1237,6 +1254,7 @@ efivars_init(void)
 	ops.get_variable = efi.get_variable;
 	ops.set_variable = efi.set_variable;
 	ops.get_next_variable = efi.get_next_variable;
+	ops.query_variable_info = efi.query_variable_info;
 	error = register_efivars(&__efivars, &ops, efi_kobj);
 	if (error)
 		goto err_put;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8670eb1..c47ec36 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -643,6 +643,7 @@ struct efivar_operations {
 	efi_get_variable_t *get_variable;
 	efi_get_next_variable_t *get_next_variable;
 	efi_set_variable_t *set_variable;
+	efi_query_variable_info_t *query_variable_info;
 };
 
 struct efivars {
-- 
1.7.1

[Dle-develop] [RESEND][PATCH v4 0/7] efi_pstore: multiple event logging support

From: Seiji A. <sei...@hd...> - 2012-11-01 22:59:13

Changelog

v3 -> v4
   - Rebase to 3.7-rc3
   - Add ctime to an argument of efi_pstore_erase to build successfully
     in case where CONFIG_PTORE=n is specified. (Patch 4/7)
   - Add count to an argument of efi_pstore_erase and efi_pstore_write to
     build successfully in case where CONFIG_PTORE=n is specified (Patch 5/7)
   - Fix a white space damage (Patch 6/7)
   - Modify a logic checking a 3-variable format of a variable name (Patch 7/7)

v2 -> v3
   - Create patches 6/7 and 7/7 to work with an existing format of variable name

v1 -> v2
   - Separate into 5 patches in accordance with Mike's comment
   - Erase an extra line of comment in patch 1/5

[Issue]

    Currently, efi_pstore driver simply overwrites existing panic messages in NVRAM.
    So, in the following scenario, we will lose 1st panic messages.

    1. kernel panics.
    2. efi_pstore is kicked and writes panic messages to NVRAM.
    3. system reboots.
    4. kernel panics again before a user checks the 1st panic messages in NVRAM.

[Solution]

   Solutions of this problem has been discussed among Tony, Matthew, Don, Mike and me.

   http://marc.info/?l=linux-kernel&m=134273270704586&w=2

   And there are two possible solutions right now.
     - First one is introducing some policy overwriting existing logs.
     - Second one is simply holding multiple log without overwriting any entries.

   We haven't decided the overwriting policy which is reasonable to all users yet.
   But I believe we agree that just holding multiple logs is a reasonable way.

   We may need further discussions to find the possibility of introducing overwriting
   policy, especially getting critical messages in multiple oops case.
   But I would like to begin with a simple and reasonable way to everyone.
   So, this patch takes an approach just holding multiple logs.

[Patch Description]

Seiji Aguchi (7):
  efi_pstore: Check remaining space with QueryVariableInfo() before
    writing data
  efi_pstore: Add a logic erasing entries to an erase callback
  efi_pstore: Remove a logic erasing entries from a write callback to
    hold multiple logs
  efi_pstore: Add ctime to argument of erase callback
  efi_pstore: Add a sequence counter to a variable name
  efi_pstore: Add a format check for an existing variable name at
    reading time
  Add a format check for an existing variable name at erasing time

 drivers/acpi/apei/erst.c   |   16 ++--
 drivers/firmware/efivars.c |  163 ++++++++++++++++++++++++++++++-------------
 fs/pstore/inode.c          |    7 ++-
 fs/pstore/internal.h       |    2 +-
 fs/pstore/platform.c       |   11 ++--
 fs/pstore/ram.c            |    9 +--
 include/linux/efi.h        |    1 +
 include/linux/pstore.h     |    6 +-
 8 files changed, 143 insertions(+), 72 deletions(-)

Re: [Dle-develop] [PATCH v4 5/7] efi_pstore: Add a sequence counter to a variable name

From: Rafael J. W. <rj...@si...> - 2012-10-30 23:05:11

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.
> 
>   <before applying this patch>
>  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.
> 
>   <after applying this patch>
> 
>  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 <sei...@hd...>

Please feel free to add

Acked-by: Rafael J. Wysocki <raf...@in...>

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.

[Dle-develop] [PATCH v4 7/7] efi_pstore: Add a format check for an existing variable name at erasing time

From: Seiji A. <sei...@hd...> - 2012-10-30 20:04:25

[Issue]

a format of variable name has been updated to type, id, count and ctime
to support holding multiple logs.

Format of current variable name
  dump-type0-1-2-12345678

  type:0
  id:1
  count:2
  ctime:12345678

On the other hand, if an old variable name before being updated
remains, users can't erase it via /dev/pstore.

Format of old variable name
  dump-type0-1-12345678

  type:0
  id:1
  ctime:12345678

[Solution]

This patch add a format check for the old variable name in a erase callback to make it erasable.

Signed-off-by: Seiji Aguchi <sei...@hd...>
---
 drivers/firmware/efivars.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index bf7a6f9..6e51c1e 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -773,6 +773,8 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
 {
 	char name[DUMP_NAME_LEN];
 	efi_char16_t efi_name[DUMP_NAME_LEN];
+	char name_old[DUMP_NAME_LEN];
+	efi_char16_t efi_name_old[DUMP_NAME_LEN];
 	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
 	struct efivars *efivars = psi->data;
 	struct efivar_entry *entry, *found = NULL;
@@ -796,8 +798,22 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
 		if (efi_guidcmp(entry->var.VendorGuid, vendor))
 			continue;
 		if (utf16_strncmp(entry->var.VariableName, efi_name,
-				  utf16_strlen(efi_name)))
-			continue;
+				  utf16_strlen(efi_name))) {
+			/*
+			 * Check if an old format,
+			 * which doesn't support holding
+			 * multiple logs, remains.
+			 */
+			sprintf(name_old, "dump-type%u-%u-%lu", type,
+				(unsigned int)id, time.tv_sec);
+
+			for (i = 0; i < DUMP_NAME_LEN; i++)
+				efi_name_old[i] = name_old[i];
+
+			if (utf16_strncmp(entry->var.VariableName, efi_name_old,
+					  utf16_strlen(efi_name_old)))
+				continue;
+		}
 
 		/* found */
 		found = entry;
-- 
1.7.1

[Dle-develop] [PATCH v4 6/7] efi_pstore: Add a format check for an existing variable name at reading time

From: Seiji A. <sei...@hd...> - 2012-10-30 20:03:43

[Issue]

a format of variable name has been updated to type, id, count and ctime
to support holding multiple logs.

Format of current variable name
  dump-type0-1-2-12345678

  type:0
  id:1
  count:2
  ctime:12345678

On the other hand, if an old variable name before being updated
remains, users can't read it via /dev/pstore.

Format of old variable name
  dump-type0-1-12345678

  type:0
  id:1
  ctime:12345678

[Solution]

This patch add a format check for the old variable name in a read callback
to make it readable.

Signed-off-by: Seiji Aguchi <sei...@hd...>
---
 drivers/firmware/efivars.c |   38 ++++++++++++++++++++++++++++----------
 1 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 7ad3aae..bf7a6f9 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -681,17 +681,35 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
 				*count = cnt;
 				timespec->tv_sec = time;
 				timespec->tv_nsec = 0;
-				get_var_data_locked(efivars, &efivars->walk_entry->var);
-				size = efivars->walk_entry->var.DataSize;
-				*buf = kmalloc(size, GFP_KERNEL);
-				if (*buf == NULL)
-					return -ENOMEM;
-				memcpy(*buf, efivars->walk_entry->var.Data,
-				       size);
-				efivars->walk_entry = list_entry(efivars->walk_entry->list.next,
-					           struct efivar_entry, list);
-				return size;
+			} else if (sscanf(name, "dump-type%u-%u-%lu",
+				   type, &part, &time) == 3) {
+				/*
+				 * Check if an old format,
+				 * which doesn't support holding
+				 * multiple logs, remains.
+				 */
+				*id = part;
+				*count = 0;
+				timespec->tv_sec = time;
+				timespec->tv_nsec = 0;
+			} else {
+				efivars->walk_entry = list_entry(
+						efivars->walk_entry->list.next,
+						struct efivar_entry, list);
+				continue;
 			}
+
+			get_var_data_locked(efivars, &efivars->walk_entry->var);
+			size = efivars->walk_entry->var.DataSize;
+			*buf = kmalloc(size, GFP_KERNEL);
+			if (*buf == NULL)
+				return -ENOMEM;
+			memcpy(*buf, efivars->walk_entry->var.Data,
+			       size);
+			efivars->walk_entry = list_entry(
+					efivars->walk_entry->list.next,
+					struct efivar_entry, list);
+			return size;
 		}
 		efivars->walk_entry = list_entry(efivars->walk_entry->list.next,
 						 struct efivar_entry, list);
-- 
1.7.1

[Dle-develop] [PATCH v4 5/7] efi_pstore: Add a sequence counter to a variable name

From: Seiji A. <sei...@hd...> - 2012-10-30 20:03:02

[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.

  <before applying this patch>
 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.

  <after applying this patch>

 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 <sei...@hd...>
---
 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;
 };
 
-- 
1.7.1

[Dle-develop] [PATCH v4 4/7] efi_pstore: Add ctime to argument of erase callback

From: Seiji A. <sei...@hd...> - 2012-10-30 20:02:15

[Issue]

Currently, a variable name, which is used to identify each log entry, consists of type,
id and ctime. But an erase callback does not use ctime.

If efi_pstore supported just one log, type and id were enough.
However, in case of supporting multiple logs, it doesn't work because
it can't distinguish each entry without ctime at erasing time.

 <Example>

 As you can see below, efi_pstore can't differentiate first event from second one without ctime.

 a variable name of first event: dump-type0-1-12345678
 a variable name of second event: dump-type0-1-23456789

  type:0
  id:1
  ctime:12345678, 23456789

[Solution]

This patch adds ctime to an argument of an erase callback.

It works across reboots because ctime of pstore means the date that the record was originally stored.
To do this, efi_pstore saves the ctime to variable name at writing time and passes it to pstore
at reading time.

Signed-off-by: Seiji Aguchi <sei...@hd...>
---
 drivers/acpi/apei/erst.c   |    4 ++--
 drivers/firmware/efivars.c |   17 ++++++++---------
 fs/pstore/inode.c          |    3 ++-
 fs/pstore/ram.c            |    2 +-
 include/linux/pstore.h     |    2 +-
 5 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index e4d9d24..0bd6ae4 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -938,7 +938,7 @@ static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason,
 		       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);
+			struct timespec time, struct pstore_info *psi);
 
 static struct pstore_info erst_info = {
 	.owner		= THIS_MODULE,
@@ -1102,7 +1102,7 @@ static int erst_writer(enum pstore_type_id type, enum kmsg_dump_reason reason,
 }
 
 static int erst_clearer(enum pstore_type_id type, u64 id,
-			struct pstore_info *psi)
+			struct timespec time, struct pstore_info *psi)
 {
 	return erst_clear(id);
 }
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index fbe9202..3803621 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -747,24 +747,25 @@ static int efi_pstore_write(enum pstore_type_id type,
 };
 
 static int efi_pstore_erase(enum pstore_type_id type, u64 id,
-			    struct pstore_info *psi)
+			    struct timespec time, struct pstore_info *psi)
 {
-	char stub_name[DUMP_NAME_LEN];
+	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;
 	struct efivar_entry *entry, *found = NULL;
 	int i;
 
-	sprintf(stub_name, "dump-type%u-%u-", type, (unsigned int)id);
+	sprintf(name, "dump-type%u-%u-%lu", type, (unsigned int)id,
+		time.tv_sec);
 
 	spin_lock(&efivars->lock);
 
 	for (i = 0; i < DUMP_NAME_LEN; i++)
-		efi_name[i] = stub_name[i];
+		efi_name[i] = name[i];
 
 	/*
-	 * Clean up any entries with the same name
+	 * Clean up an entry with the same name
 	 */
 
 	list_for_each_entry(entry, &efivars->list, list) {
@@ -775,9 +776,6 @@ static int efi_pstore_erase(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;
@@ -785,6 +783,7 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id,
 					   &entry->var.VendorGuid,
 					   PSTORE_EFI_ATTRIBUTES,
 					   0, NULL);
+		break;
 	}
 
 	if (found)
@@ -823,7 +822,7 @@ static int efi_pstore_write(enum pstore_type_id type,
 }
 
 static int efi_pstore_erase(enum pstore_type_id type, u64 id,
-			    struct pstore_info *psi)
+			    struct timespec time, struct pstore_info *psi)
 {
 	return 0;
 }
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 4ab572e..4300af6 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -175,7 +175,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, p->psi);
+		p->psi->erase(p->type, p->id, dentry->d_inode->i_ctime,
+			      p->psi);
 
 	return simple_unlink(dir, dentry);
 }
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 1a4f6da..749693f 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -237,7 +237,7 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
 }
 
 static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,
-				struct pstore_info *psi)
+				struct timespec time, struct pstore_info *psi)
 {
 	struct ramoops_context *cxt = psi->data;
 	struct persistent_ram_zone *prz;
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index ee3034a..f6e9336 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -60,7 +60,7 @@ struct pstore_info {
 			unsigned int part, const char *buf, size_t size,
 			struct pstore_info *psi);
 	int		(*erase)(enum pstore_type_id type, u64 id,
-			struct pstore_info *psi);
+			struct timespec time, struct pstore_info *psi);
 	void		*data;
 };
 
-- 
1.7.1

[Dle-develop] [PATCH v4 3/7] efi_pstore: Remove a logic erasing entries from a write callback to hold multiple logs

From: Seiji A. <sei...@hd...> - 2012-10-30 20:01:51

[Issue]

Currently, efi_pstore driver simply overwrites existing panic messages in NVRAM.
So, in the following scenario, we will lose 1st panic messages.

1. kernel panics.
2. efi_pstore is kicked and writes panic messages to NVRAM.
3. system reboots.
4. kernel panics again before a user checks the 1st panic messages in NVRAM.

[Solution]

A reasonable solution to fix the issue is just holding multiple logs without erasing
existing entries.
This patch removes a logic erasing existing entries in a write callback
because the logic is not needed in the write callback to support holding multiple logs.

Signed-off-by: Seiji Aguchi <sei...@hd...>
---
 drivers/firmware/efivars.c |   39 ++-------------------------------------
 1 files changed, 2 insertions(+), 37 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index bee14cc..fbe9202 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -701,18 +701,13 @@ static int efi_pstore_write(enum pstore_type_id type,
 		unsigned 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, ret = 0;
 	u64 storage_space, remaining_space, max_variable_size;
 	efi_status_t status = EFI_NOT_FOUND;
 
-	sprintf(stub_name, "dump-type%u-%u-", type, part);
-	sprintf(name, "%s%lu", stub_name, get_seconds());
-
 	spin_lock(&efivars->lock);
 
 	/*
@@ -730,35 +725,8 @@ static int efi_pstore_write(enum pstore_type_id type,
 		return -ENOSPC;
 	}
 
-	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);
-
-		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,
-					   PSTORE_EFI_ATTRIBUTES,
-					   0, NULL);
-	}
-
-	if (found)
-		list_del(&found->list);
+	sprintf(name, "dump-type%u-%u-%lu", type, part,
+		get_seconds());
 
 	for (i = 0; i < DUMP_NAME_LEN; i++)
 		efi_name[i] = name[i];
@@ -768,9 +736,6 @@ static int efi_pstore_write(enum pstore_type_id type,
 
 	spin_unlock(&efivars->lock);
 
-	if (found)
-		efivar_unregister(found);
-
 	if (size)
 		ret = efivar_create_sysfs_entry(efivars,
 					  utf16_strsize(efi_name,
-- 
1.7.1

[Dle-develop] [PATCH v4 2/7] efi_pstore: Add a logic erasing entries to an erase callback

From: Seiji A. <sei...@hd...> - 2012-10-30 20:01:27

[Issue]

Currently, efi_pstore driver simply overwrites existing panic messages in NVRAM.
So, in the following scenario, we will lose 1st panic messages.

 1. kernel panics.
 2. efi_pstore is kicked and writes panic messages to NVRAM.
 3. system reboots.
 4. kernel panics again before a user checks the 1st panic messages in NVRAM.

[Solution]

A reasonable solution to fix the issue is just holding multiple logs without erasing
existing entries.

This patch freshly adds a logic erasing existing entries, which shared with a write callback,
to an erase callback.
To support holding multiple logs, the write callback doesn't need to erase any entries and
it will be removed in a subsequent patch.

Signed-off-by: Seiji Aguchi <sei...@hd...>
---
 drivers/firmware/efivars.c |   46 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 37ac21a..bee14cc 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -784,7 +784,51 @@ static int efi_pstore_write(enum pstore_type_id type,
 static int efi_pstore_erase(enum pstore_type_id type, u64 id,
 			    struct pstore_info *psi)
 {
-	efi_pstore_write(type, 0, &id, (unsigned int)id, 0, 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;
+
+	sprintf(stub_name, "dump-type%u-%u-", type, (unsigned int)id);
+
+	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);
+
+		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,
+					   PSTORE_EFI_ATTRIBUTES,
+					   0, NULL);
+	}
+
+	if (found)
+		list_del(&found->list);
+
+	spin_unlock(&efivars->lock);
+
+	if (found)
+		efivar_unregister(found);
 
 	return 0;
 }
-- 
1.7.1

[Dle-develop] [PATCH v4 1/7] efi_pstore: Check remaining space with QueryVariableInfo() before writing data

From: Seiji A. <sei...@hd...> - 2012-10-30 20:00:56

[Issue]

As discussed in a thread below, Running out of space in EFI isn't a well-tested scenario.
And we wouldn't expect all firmware to handle it gracefully.
http://marc.info/?l=linux-kernel&m=134305325801789&w=2

On the other hand, current efi_pstore doesn't check a remaining space of storage at writing time.
Therefore, efi_pstore may not work if it tries to write a large amount of data.

[Patch Description]

To avoid handling the situation above, this patch checks if there is a space enough to log with
QueryVariableInfo() before writing data.

Signed-off-by: Seiji Aguchi <sei...@hd...>
Acked-by: Mike Waychison <mi...@go...>
---
 drivers/firmware/efivars.c |   18 ++++++++++++++++++
 include/linux/efi.h        |    1 +
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index d10c987..37ac21a 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -707,12 +707,29 @@ static int efi_pstore_write(enum pstore_type_id type,
 	struct efivars *efivars = psi->data;
 	struct efivar_entry *entry, *found = NULL;
 	int i, ret = 0;
+	u64 storage_space, remaining_space, max_variable_size;
+	efi_status_t status = EFI_NOT_FOUND;
 
 	sprintf(stub_name, "dump-type%u-%u-", type, part);
 	sprintf(name, "%s%lu", stub_name, get_seconds());
 
 	spin_lock(&efivars->lock);
 
+	/*
+	 * Check if there is a space enough to log.
+	 * size: a size of logging data
+	 * DUMP_NAME_LEN * 2: a maximum size of variable name
+	 */
+	status = efivars->ops->query_variable_info(PSTORE_EFI_ATTRIBUTES,
+						   &storage_space,
+						   &remaining_space,
+						   &max_variable_size);
+	if (status || remaining_space < size + DUMP_NAME_LEN * 2) {
+		spin_unlock(&efivars->lock);
+		*id = part;
+		return -ENOSPC;
+	}
+
 	for (i = 0; i < DUMP_NAME_LEN; i++)
 		efi_name[i] = stub_name[i];
 
@@ -1237,6 +1254,7 @@ efivars_init(void)
 	ops.get_variable = efi.get_variable;
 	ops.set_variable = efi.set_variable;
 	ops.get_next_variable = efi.get_next_variable;
+	ops.query_variable_info = efi.query_variable_info;
 	error = register_efivars(&__efivars, &ops, efi_kobj);
 	if (error)
 		goto err_put;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8670eb1..c47ec36 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -643,6 +643,7 @@ struct efivar_operations {
 	efi_get_variable_t *get_variable;
 	efi_get_next_variable_t *get_next_variable;
 	efi_set_variable_t *set_variable;
+	efi_query_variable_info_t *query_variable_info;
 };
 
 struct efivars {
-- 
1.7.1

[Dle-develop] [PATCH v4 0/7] efi_pstore: multiple event logging support

From: Seiji A. <sei...@hd...> - 2012-10-30 20:00:25

Changelog

v3 -> v4
   - Rebase to 3.7-rc3
   - Add ctime to an argument of efi_pstore_erase to build successfully
     in case where CONFIG_PTORE=n is specified. (Patch 4/7)
   - Add count to an argument of efi_pstore_erase and efi_pstore_write to
     build successfully in case where CONFIG_PTORE=n is specified (Patch 5/7)
   - Fix a white space damage (Patch 6/7)
   - Modify a logic checking a 3-variable format of a variable name (Patch 7/7)

v2 -> v3
   - Create patches 6/7 and 7/7 to work with an existing format of variable name

v1 -> v2
   - Separate into 5 patches in accordance with Mike's comment
   - Erase an extra line of comment in patch 1/5

[Issue]

    Currently, efi_pstore driver simply overwrites existing panic messages in NVRAM.
    So, in the following scenario, we will lose 1st panic messages.

    1. kernel panics.
    2. efi_pstore is kicked and writes panic messages to NVRAM.
    3. system reboots.
    4. kernel panics again before a user checks the 1st panic messages in NVRAM.

[Solution]

   Solutions of this problem has been discussed among Tony, Matthew, Don, Mike and me.

   http://marc.info/?l=linux-kernel&m=134273270704586&w=2

   And there are two possible solutions right now.
     - First one is introducing some policy overwriting existing logs.
     - Second one is simply holding multiple log without overwriting any entries.

   We haven't decided the overwriting policy which is reasonable to all users yet.
   But I believe we agree that just holding multiple logs is a reasonable way.

   We may need further discussions to find the possibility of introducing overwriting
   policy, especially getting critical messages in multiple oops case.
   But I would like to begin with a simple and reasonable way to everyone.
   So, this patch takes an approach just holding multiple logs.

[Patch Description]

Seiji Aguchi (7):
  efi_pstore: Check remaining space with QueryVariableInfo() before
    writing data
  efi_pstore: Add a logic erasing entries to an erase callback
  efi_pstore: Remove a logic erasing entries from a write callback to
    hold multiple logs
  efi_pstore: Add ctime to argument of erase callback
  efi_pstore: Add a sequence counter to a variable name
  efi_pstore: Add a format check for an existing variable name at
    reading time
  efi_pstore: Add a format check for an existing variable name at erasing
    time

 drivers/acpi/apei/erst.c   |   16 ++--
 drivers/firmware/efivars.c |  163 ++++++++++++++++++++++++++++++-------------
 fs/pstore/inode.c          |    7 ++-
 fs/pstore/internal.h       |    2 +-
 fs/pstore/platform.c       |   11 ++--
 fs/pstore/ram.c            |    9 +--
 include/linux/efi.h        |    1 +
 include/linux/pstore.h     |    6 +-
 8 files changed, 143 insertions(+), 72 deletions(-)

Re: [Dle-develop] [PATCH v3 7/7] Add a format check for an existing variable name at erasing time

From: Seiji A. <sei...@hd...> - 2012-10-29 20:41:10

> > +                                 utf16_strlen(efi_name))) {
> > +                       /*
> > +                        * Check if an old format,
> > +                        * which doesn't support holding
> > +                        * multiple logs, remains.
> > +                        */
> > +                       if (sscanf(name, "dump-type%u-%u-%lu",
> > +                                  &type_old, &part_old, &time_old) !=
> > + 3)
> 
> This doesn't look right.  This should probably mirror the sprintf() at the top of the function using a new string, convert it to 16-bit
> Unicodeand then compare it like we do for the 4-variable version above (please ignore the fact that this driver incorrectly calls these
> strings utf16 everywhere -- that needs to be fixed).

I will fix it as well.

Seiji

Re: [Dle-develop] [PATCH v3 6/7] efi_pstore: Add a format check for an existing variable name at reading time

From: Seiji A. <sei...@hd...> - 2012-10-29 20:39:45

> > +                               efivars->walk_entry = list_entry(
> > +                                       efivars->walk_entry->list.next,
> > +                                        struct efivar_entry, list);
> 
> odd wrap.
> 

I will fix it.

Thanks,

Seiji

Re: [Dle-develop] [PATCH v3 7/7] Add a format check for an existing variable name at erasing time

From: Mike W. <mi...@go...> - 2012-10-29 20:24:53

On Fri, Oct 26, 2012 at 2:43 PM, Seiji Aguchi <sei...@hd...> wrote:
> [Issue]
>
> A format of variable name has been updated to type, id, count and ctime
> to support holding multiple logs.
>
> Format of current variable name
>   dump-type0-1-2-12345678
>
>   type:0
>   id:1
>   count:2
>   ctime:12345678
>
> On the other hand, if an old variable name before being updated
> remains, users can't erase it via /dev/pstore.
>
> Format of old variable name
>   dump-type0-1-12345678
>
>   type:0
>   id:1
>   ctime:12345678
>
> [Solution]
>
> This patch adds a format check for the old variable name in a erase callback to make it erasable.
>
> Signed-off-by: Seiji Aguchi <sei...@hd...>
> ---
>  drivers/firmware/efivars.c |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index dd228d5..b1cd028 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -777,6 +777,8 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
>         struct efivars *efivars = psi->data;
>         struct efivar_entry *entry, *found = NULL;
>         int i;
> +       unsigned int type_old, part_old;
> +       unsigned long time_old;
>
>         sprintf(name, "dump-type%u-%u-%d-%lu", type, (unsigned int)id, count,
>                 time.tv_sec);
> @@ -796,8 +798,16 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
>                 if (efi_guidcmp(entry->var.VendorGuid, vendor))
>                         continue;
>                 if (utf16_strncmp(entry->var.VariableName, efi_name,
> -                                 utf16_strlen(efi_name)))
> -                       continue;
> +                                 utf16_strlen(efi_name))) {
> +                       /*
> +                        * Check if an old format,
> +                        * which doesn't support holding
> +                        * multiple logs, remains.
> +                        */
> +                       if (sscanf(name, "dump-type%u-%u-%lu",
> +                                  &type_old, &part_old, &time_old) != 3)

This doesn't look right.  This should probably mirror the sprintf() at
the top of the function using a new string, convert it to 16-bit
Unicodeand then compare it like we do for the 4-variable version above
(please ignore the fact that this driver incorrectly calls these
strings utf16 everywhere -- that needs to be fixed).

> +                               continue;
> +               }
>
>                 /* found */
>                 found = entry;
> -- 1.7.1

Re: [Dle-develop] [PATCH v3 6/7] efi_pstore: Add a format check for an existing variable name at reading time

From: Mike W. <mi...@go...> - 2012-10-29 20:13:28

On Fri, Oct 26, 2012 at 2:43 PM, Seiji Aguchi <sei...@hd...> wrote:
> [Issue]
>
> A format of variable name has been updated to type, id, count and ctime
> to support holding multiple logs.
>
> Format of current variable name
>   dump-type0-1-2-12345678
>
>   type:0
>   id:1
>   count:2
>   ctime:12345678
>
> On the other hand, if an old variable name before being updated
> remains, users can't read it via /dev/pstore.
>
> Format of old variable name
>   dump-type0-1-12345678
>
>   type:0
>   id:1
>   ctime:12345678
>
> [Solution]
>
> This patch adds a format check for the old variable name in a read callback
> to make it readable.
>
> Signed-off-by: Seiji Aguchi <sei...@hd...>
> ---
>  drivers/firmware/efivars.c |   38 ++++++++++++++++++++++++++++----------
>  1 files changed, 28 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index dc69802..dd228d5 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -681,17 +681,35 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
>                                 *count = cnt;
>                                 timespec->tv_sec = time;
>                                 timespec->tv_nsec = 0;
> -                               get_var_data_locked(efivars, &efivars->walk_entry->var);
> -                               size = efivars->walk_entry->var.DataSize;
> -                               *buf = kmalloc(size, GFP_KERNEL);
> -                               if (*buf == NULL)
> -                                       return -ENOMEM;
> -                               memcpy(*buf, efivars->walk_entry->var.Data,
> -                                      size);
> -                               efivars->walk_entry = list_entry(efivars->walk_entry->list.next,
> -                                                  struct efivar_entry, list);
> -                               return size;
> +                       } else if (sscanf(name, "dump-type%u-%u-%lu",
> +                                  type, &part, &time) == 3) {
> +                               /*
> +                                * Check if an old format,
> +                                * which doesn't support holding
> +                                * multiple logs, remains.
> +                                */
> +                               *id = part;
> +                               *count = 0;
> +                               timespec->tv_sec = time;
> +                               timespec->tv_nsec = 0;
> +                       } else {
> +                               efivars->walk_entry = list_entry(
> +                                       efivars->walk_entry->list.next,
> +                                        struct efivar_entry, list);

odd wrap.

> +                               continue;
>                         }
> +
> +                       get_var_data_locked(efivars, &efivars->walk_entry->var);
> +                       size = efivars->walk_entry->var.DataSize;
> +                       *buf = kmalloc(size, GFP_KERNEL);
> +                       if (*buf == NULL)
> +                               return -ENOMEM;
> +                       memcpy(*buf, efivars->walk_entry->var.Data,
> +                              size);
> +                       efivars->walk_entry = list_entry(
> +                                       efivars->walk_entry->list.next,
> +                                       struct efivar_entry, list);
> +                       return size;
>                 }
>                 efivars->walk_entry = list_entry(efivars->walk_entry->list.next,
>                                                  struct efivar_entry, list);
> -- 1.7.1

454 messages has been excluded from this view by a project administrator.

Flat | Threaded

<< < 1 .. 27 28 29 30 31 .. 99 > >> (Page 29 of 99)