Hi all,
Currently pstore doesn't support logging kernel messages in run-time,
it only dumps dmesg when kernel oopses/panics. This makes pstore
useless for debugging hangs caused by HW issues or improper use of HW
(e.g. weird device inserted -> driver tried to write reserved bits ->
SoC hanged. In that case we don't get any messages in the pstore.
This series add a new message type for pstore, i.e. PSTORE_TYPE_CONSOLE,
plus make pstore/ram.c handle the new messages.
The old ram_console driver is removed. This might probably cause
some pain for out-of-tree code, as it would need to be adjusted...
but "no pain, no gain"? :-) Though, if there's some serious resistance,
we can probably postpone the last two patches.
These patches depend on the following series:
http://thread.gmane.org/gmane.linux.kernel/1298179
"[PATCH v2 0/6] Merge ramoops and persistent_ram, generic pstore RAM backend"
Thanks!
---
Documentation/ramoops.txt | 15 +++
drivers/staging/android/Kconfig | 5 -
drivers/staging/android/Makefile | 1 -
drivers/staging/android/ram_console.c | 179 ---------------------------------
fs/pstore/Kconfig | 7 ++
fs/pstore/inode.c | 3 +
fs/pstore/platform.c | 25 +++++
fs/pstore/ram.c | 39 +++++--
fs/pstore/ram_core.c | 81 +--------------
include/linux/pstore.h | 1 +
include/linux/pstore_ram.h | 19 ----
11 files changed, 83 insertions(+), 292 deletions(-)
--
Anton Vorontsov
Email: [email protected]
Pstore doesn't support logging kernel messages in run-time, it only
dumps dmesg when kernel oopses/panics. This makes pstore useless for
debugging hangs caused by HW issues or improper use of HW (e.g.
weird device inserted -> driver tried to write a reserved bits ->
SoC hanged. In that case we don't get any messages in the pstore.
Therefore, let's add a runtime logging support: PSTORE_TYPE_CONSOLE.
Signed-off-by: Anton Vorontsov <[email protected]>
---
fs/pstore/Kconfig | 7 +++++++
fs/pstore/inode.c | 3 +++
fs/pstore/platform.c | 25 +++++++++++++++++++++++++
include/linux/pstore.h | 1 +
4 files changed, 36 insertions(+)
diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
index 23ade26..d044de6 100644
--- a/fs/pstore/Kconfig
+++ b/fs/pstore/Kconfig
@@ -12,6 +12,13 @@ config PSTORE
If you don't have a platform persistent store driver,
say N.
+config PSTORE_CONSOLE
+ bool "Log kernel console messages"
+ depends on PSTORE
+ help
+ When the option is enabled, pstore will log all kernel
+ messages, even if no oops or panic happened.
+
config PSTORE_RAM
tristate "Log panic/oops to a RAM buffer"
depends on PSTORE
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 1950788..97d35ee 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -212,6 +212,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id,
case PSTORE_TYPE_DMESG:
sprintf(name, "dmesg-%s-%lld", psname, id);
break;
+ case PSTORE_TYPE_CONSOLE:
+ sprintf(name, "console-%s", psname);
+ break;
case PSTORE_TYPE_MCE:
sprintf(name, "mce-%s-%lld", psname, id);
break;
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 82c585f..3a384c8 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -1,6 +1,7 @@
/*
* Persistent Storage - platform driver interface parts.
*
+ * Copyright (C) 2007-2008 Google, Inc.
* Copyright (C) 2010 Intel Corporation <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
@@ -22,6 +23,7 @@
#include <linux/errno.h>
#include <linux/init.h>
#include <linux/kmsg_dump.h>
+#include <linux/console.h>
#include <linux/module.h>
#include <linux/pstore.h>
#include <linux/string.h>
@@ -156,6 +158,28 @@ static struct kmsg_dumper pstore_dumper = {
.dump = pstore_dump,
};
+#ifdef CONFIG_PSTORE_CONSOLE
+static void pstore_console_write(struct console *con, const char *s, unsigned c)
+{
+ strncpy(psinfo->buf, s, c);
+ psinfo->write(PSTORE_TYPE_CONSOLE, 0, NULL, 0, c, psinfo);
+}
+
+static struct console pstore_console = {
+ .name = "pstore",
+ .write = pstore_console_write,
+ .flags = CON_PRINTBUFFER | CON_ENABLED | CON_ANYTIME,
+ .index = -1,
+};
+
+static void pstore_register_console(void)
+{
+ register_console(&pstore_console);
+}
+#else
+static void pstore_register_console(void) {}
+#endif
+
/*
* platform specific persistent storage driver registers with
* us here. If pstore is already mounted, call the platform
@@ -193,6 +217,7 @@ int pstore_register(struct pstore_info *psi)
pstore_get_records(0);
kmsg_dump_register(&pstore_dumper);
+ pstore_register_console();
pstore_timer.expires = jiffies + PSTORE_INTERVAL;
add_timer(&pstore_timer);
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index e1461e1..1bd014b 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -29,6 +29,7 @@
enum pstore_type_id {
PSTORE_TYPE_DMESG = 0,
PSTORE_TYPE_MCE = 1,
+ PSTORE_TYPE_CONSOLE = 2,
PSTORE_TYPE_UNKNOWN = 255
};
--
1.7.9.2
This is all straightforward: we just use the last region for
console logging. If there's just one region, we fall-back to
the old behaviour: just a oops/dumps logging.
Signed-off-by: Anton Vorontsov <[email protected]>
---
fs/pstore/ram.c | 39 ++++++++++++++++++++++++++++++---------
1 file changed, 30 insertions(+), 9 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 9123cce..54213eb 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -63,6 +63,7 @@ MODULE_PARM_DESC(ramoops_ecc,
struct ramoops_context {
struct persistent_ram_zone **przs;
+ struct persistent_ram_zone *cprz;
phys_addr_t phys_addr;
unsigned long size;
size_t record_size;
@@ -70,6 +71,7 @@ struct ramoops_context {
bool ecc;
unsigned int count;
unsigned int max_count;
+ unsigned int max_dump_count;
unsigned int read_count;
struct pstore_info pstore;
};
@@ -92,16 +94,20 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
{
ssize_t size;
struct ramoops_context *cxt = psi->data;
- struct persistent_ram_zone *prz;
-
- if (cxt->read_count >= cxt->max_count)
- return -EINVAL;
+ struct persistent_ram_zone *prz = cxt->przs[cxt->read_count];
*id = cxt->read_count++;
- prz = cxt->przs[*id];
- /* Only supports dmesg output so far. */
- *type = PSTORE_TYPE_DMESG;
+ if (*id < cxt->max_dump_count && persistent_ram_old_size(prz)) {
+ *type = PSTORE_TYPE_DMESG;
+ } else if (*id < cxt->max_count) {
+ *type = PSTORE_TYPE_CONSOLE;
+ prz = cxt->cprz;
+ cxt->read_count = cxt->max_count;
+ } else {
+ return 0;
+ }
+
/* TODO(kees): Bogus time for the moment. */
time->tv_sec = 0;
time->tv_nsec = 0;
@@ -142,7 +148,12 @@ static int ramoops_pstore_write(enum pstore_type_id type,
struct persistent_ram_zone *prz = cxt->przs[cxt->count];
size_t hlen;
- /* Currently ramoops is designed to only store dmesg dumps. */
+ if (type == PSTORE_TYPE_CONSOLE) {
+ if (!cxt->cprz)
+ return -ENOMEM;
+ persistent_ram_write(cxt->cprz, cxt->pstore.buf, size);
+ }
+
if (type != PSTORE_TYPE_DMESG)
return -EINVAL;
@@ -170,7 +181,7 @@ static int ramoops_pstore_write(enum pstore_type_id type,
size = prz->buffer_size - hlen;
persistent_ram_write(prz, cxt->pstore.buf, size);
- cxt->count = (cxt->count + 1) % cxt->max_count;
+ cxt->count = (cxt->count + 1) % cxt->max_dump_count;
return 0;
}
@@ -264,6 +275,16 @@ static int __init ramoops_probe(struct platform_device *pdev)
}
}
+ /*
+ * The last zone is for TYPE_CONSOLE, unless we have only
+ * one, in which case we use it for oops/panic logging.
+ */
+ cxt->max_dump_count = cxt->max_count;
+ if (cxt->max_count > 1) {
+ cxt->max_dump_count--;
+ cxt->cprz = cxt->przs[cxt->max_count - 1];
+ }
+
cxt->pstore.data = cxt;
cxt->pstore.bufsize = cxt->przs[0]->buffer_size;
cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
--
1.7.9.2
Since we use multiple regions, the messages are somewhat annoying.
We do print total mapped memory already, so no need to print the
information for each region in the library routines.
Signed-off-by: Anton Vorontsov <[email protected]>
---
fs/pstore/ram_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 31f8d18..e0d8429 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -401,13 +401,13 @@ static int __init persistent_ram_post_init(struct persistent_ram_zone *prz, bool
" size %zu, start %zu\n",
buffer_size(prz), buffer_start(prz));
else {
- pr_info("persistent_ram: found existing buffer,"
+ pr_debug("persistent_ram: found existing buffer,"
" size %zu, start %zu\n",
buffer_size(prz), buffer_start(prz));
persistent_ram_save_old(prz);
}
} else {
- pr_info("persistent_ram: no valid data in buffer"
+ pr_debug("persistent_ram: no valid data in buffer"
" (sig = 0x%08x)\n", prz->buffer->sig);
}
--
1.7.9.2
Suggested-by: Shuah Khan <[email protected]>
Signed-off-by: Anton Vorontsov <[email protected]>
---
Documentation/ramoops.txt | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
index 4ba7db2..138823b 100644
--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -40,6 +40,12 @@ corrupt, but usually it is restorable.
Setting the ramoops parameters can be done in 2 different manners:
1. Use the module parameters (which have the names of the variables described
as before).
+ For quick debugging, you can also reserve parts of memory during boot
+ and then use the reserved memory for ramoops. For example, assuming a machine
+ with > 128 MB of memory, the following kernel command line will tell the
+ kernel to use only the first 128 MB of memory, and place ECC-protected ramoops
+ region at 128 MB boundary:
+ "mem=128M ramoops.mem_address=0x8000000 ramoops.ecc=1"
2. Use a platform device and set the platform data. The parameters can then
be set through that platform data. An example of doing that is:
@@ -70,6 +76,15 @@ if (ret) {
return ret;
}
+You can specify either RAM memory or peripheral devices' memory. However, when
+specifying RAM, be sure to reserve the memory by issuing memblock_reserve()
+very early in the architecture code, just before platform device registration,
+e.g.:
+
+#include <linux/memblock.h>
+
+memblock_reserve(ramoops_data.mem_address, ramoops_data.mem_size);
+
3. Dump format
The data dump begins with a header, currently defined as "====" followed by a
--
1.7.9.2
All the functionality is now supported by pstore and pstore_ram drivers.
Signed-off-by: Anton Vorontsov <[email protected]>
---
drivers/staging/android/Kconfig | 5 -
drivers/staging/android/Makefile | 1 -
drivers/staging/android/ram_console.c | 179 ---------------------------------
3 files changed, 185 deletions(-)
delete mode 100644 drivers/staging/android/ram_console.c
diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
index 4bfcceb..0ce50d1 100644
--- a/drivers/staging/android/Kconfig
+++ b/drivers/staging/android/Kconfig
@@ -25,11 +25,6 @@ config ANDROID_LOGGER
tristate "Android log driver"
default n
-config ANDROID_RAM_CONSOLE
- bool "Android RAM buffer console"
- depends on !S390 && !UML && HAVE_MEMBLOCK && PSTORE_RAM
- default n
-
config ANDROID_TIMED_OUTPUT
bool "Timed output class driver"
default y
diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
index 98711e2..e16fcd5 100644
--- a/drivers/staging/android/Makefile
+++ b/drivers/staging/android/Makefile
@@ -1,7 +1,6 @@
obj-$(CONFIG_ANDROID_BINDER_IPC) += binder.o
obj-$(CONFIG_ASHMEM) += ashmem.o
obj-$(CONFIG_ANDROID_LOGGER) += logger.o
-obj-$(CONFIG_ANDROID_RAM_CONSOLE) += ram_console.o
obj-$(CONFIG_ANDROID_TIMED_OUTPUT) += timed_output.o
obj-$(CONFIG_ANDROID_TIMED_GPIO) += timed_gpio.o
obj-$(CONFIG_ANDROID_LOW_MEMORY_KILLER) += lowmemorykiller.o
diff --git a/drivers/staging/android/ram_console.c b/drivers/staging/android/ram_console.c
deleted file mode 100644
index 82323bb..0000000
--- a/drivers/staging/android/ram_console.c
+++ /dev/null
@@ -1,179 +0,0 @@
-/* drivers/android/ram_console.c
- *
- * Copyright (C) 2007-2008 Google, Inc.
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- */
-
-#include <linux/console.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/proc_fs.h>
-#include <linux/string.h>
-#include <linux/uaccess.h>
-#include <linux/io.h>
-#include <linux/pstore_ram.h>
-#include "ram_console.h"
-
-static struct persistent_ram_zone *ram_console_zone;
-static const char *bootinfo;
-static size_t bootinfo_size;
-
-static void
-ram_console_write(struct console *console, const char *s, unsigned int count)
-{
- struct persistent_ram_zone *prz = console->data;
- persistent_ram_write(prz, s, count);
-}
-
-static struct console ram_console = {
- .name = "ram",
- .write = ram_console_write,
- .flags = CON_PRINTBUFFER | CON_ENABLED | CON_ANYTIME,
- .index = -1,
-};
-
-void ram_console_enable_console(int enabled)
-{
- if (enabled)
- ram_console.flags |= CON_ENABLED;
- else
- ram_console.flags &= ~CON_ENABLED;
-}
-
-static int __init ram_console_probe(struct platform_device *pdev)
-{
- struct ram_console_platform_data *pdata = pdev->dev.platform_data;
- struct persistent_ram_zone *prz;
-
- prz = persistent_ram_init_ringbuffer(&pdev->dev, true);
- if (IS_ERR(prz))
- return PTR_ERR(prz);
-
-
- if (pdata) {
- bootinfo = kstrdup(pdata->bootinfo, GFP_KERNEL);
- if (bootinfo)
- bootinfo_size = strlen(bootinfo);
- }
-
- ram_console_zone = prz;
- ram_console.data = prz;
-
- register_console(&ram_console);
-
- return 0;
-}
-
-static struct platform_driver ram_console_driver = {
- .driver = {
- .name = "ram_console",
- },
-};
-
-static int __init ram_console_module_init(void)
-{
- return platform_driver_probe(&ram_console_driver, ram_console_probe);
-}
-
-#ifndef CONFIG_PRINTK
-#define dmesg_restrict 0
-#endif
-
-static ssize_t ram_console_read_old(struct file *file, char __user *buf,
- size_t len, loff_t *offset)
-{
- loff_t pos = *offset;
- ssize_t count;
- struct persistent_ram_zone *prz = ram_console_zone;
- size_t old_log_size = persistent_ram_old_size(prz);
- const char *old_log = persistent_ram_old(prz);
- char *str;
- int ret;
-
- if (dmesg_restrict && !capable(CAP_SYSLOG))
- return -EPERM;
-
- /* Main last_kmsg log */
- if (pos < old_log_size) {
- count = min(len, (size_t)(old_log_size - pos));
- if (copy_to_user(buf, old_log + pos, count))
- return -EFAULT;
- goto out;
- }
-
- /* ECC correction notice */
- pos -= old_log_size;
- count = persistent_ram_ecc_string(prz, NULL, 0);
- if (pos < count) {
- str = kmalloc(count, GFP_KERNEL);
- if (!str)
- return -ENOMEM;
- persistent_ram_ecc_string(prz, str, count + 1);
- count = min(len, (size_t)(count - pos));
- ret = copy_to_user(buf, str + pos, count);
- kfree(str);
- if (ret)
- return -EFAULT;
- goto out;
- }
-
- /* Boot info passed through pdata */
- pos -= count;
- if (pos < bootinfo_size) {
- count = min(len, (size_t)(bootinfo_size - pos));
- if (copy_to_user(buf, bootinfo + pos, count))
- return -EFAULT;
- goto out;
- }
-
- /* EOF */
- return 0;
-
-out:
- *offset += count;
- return count;
-}
-
-static const struct file_operations ram_console_file_ops = {
- .owner = THIS_MODULE,
- .read = ram_console_read_old,
-};
-
-static int __init ram_console_late_init(void)
-{
- struct proc_dir_entry *entry;
- struct persistent_ram_zone *prz = ram_console_zone;
-
- if (!prz)
- return 0;
-
- if (persistent_ram_old_size(prz) == 0)
- return 0;
-
- entry = create_proc_entry("last_kmsg", S_IFREG | S_IRUGO, NULL);
- if (!entry) {
- printk(KERN_ERR "ram_console: failed to create proc entry\n");
- persistent_ram_free_old(prz);
- return 0;
- }
-
- entry->proc_fops = &ram_console_file_ops;
- entry->size = persistent_ram_old_size(prz) +
- persistent_ram_ecc_string(prz, NULL, 0) +
- bootinfo_size;
-
- return 0;
-}
-
-late_initcall(ram_console_late_init);
-postcore_initcall(ram_console_module_init);
--
1.7.9.2
The code tried to maintain the global list of persistent ram zones,
which isn't a great idea overall, plus since Android's ram_console
is no longer there, we can remove some unused functions.
Signed-off-by: Anton Vorontsov <[email protected]>
---
fs/pstore/ram_core.c | 77 --------------------------------------------
include/linux/pstore_ram.h | 19 -----------
2 files changed, 96 deletions(-)
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index e0d8429..b216381 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -35,8 +35,6 @@ struct persistent_ram_buffer {
#define PERSISTENT_RAM_SIG (0x43474244) /* DBGC */
-static __initdata LIST_HEAD(persistent_ram_list);
-
static inline size_t buffer_size(struct persistent_ram_zone *prz)
{
return atomic_read(&prz->buffer->size);
@@ -455,78 +453,3 @@ err:
kfree(prz);
return ERR_PTR(ret);
}
-
-#ifndef MODULE
-static int __init persistent_ram_buffer_init(const char *name,
- struct persistent_ram_zone *prz)
-{
- int i;
- struct persistent_ram *ram;
- struct persistent_ram_descriptor *desc;
- phys_addr_t start;
-
- list_for_each_entry(ram, &persistent_ram_list, node) {
- start = ram->start;
- for (i = 0; i < ram->num_descs; i++) {
- desc = &ram->descs[i];
- if (!strcmp(desc->name, name))
- return persistent_ram_buffer_map(start,
- desc->size, prz);
- start += desc->size;
- }
- }
-
- return -EINVAL;
-}
-
-static __init
-struct persistent_ram_zone *__persistent_ram_init(struct device *dev, bool ecc)
-{
- struct persistent_ram_zone *prz;
- int ret = -ENOMEM;
-
- prz = kzalloc(sizeof(struct persistent_ram_zone), GFP_KERNEL);
- if (!prz) {
- pr_err("persistent_ram: failed to allocate persistent ram zone\n");
- goto err;
- }
-
- ret = persistent_ram_buffer_init(dev_name(dev), prz);
- if (ret) {
- pr_err("persistent_ram: failed to initialize buffer\n");
- goto err;
- }
-
- persistent_ram_post_init(prz, ecc);
-
- return prz;
-err:
- kfree(prz);
- return ERR_PTR(ret);
-}
-
-struct persistent_ram_zone * __init
-persistent_ram_init_ringbuffer(struct device *dev, bool ecc)
-{
- return __persistent_ram_init(dev, ecc);
-}
-
-int __init persistent_ram_early_init(struct persistent_ram *ram)
-{
- int ret;
-
- ret = memblock_reserve(ram->start, ram->size);
- if (ret) {
- pr_err("Failed to reserve persistent memory from %08lx-%08lx\n",
- (long)ram->start, (long)(ram->start + ram->size - 1));
- return ret;
- }
-
- list_add_tail(&ram->node, &persistent_ram_list);
-
- pr_info("Initialized persistent memory from %08lx-%08lx\n",
- (long)ram->start, (long)(ram->start + ram->size - 1));
-
- return 0;
-}
-#endif
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 7ed7fd4..d1d7d51 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -25,21 +25,6 @@
struct persistent_ram_buffer;
-struct persistent_ram_descriptor {
- const char *name;
- phys_addr_t size;
-};
-
-struct persistent_ram {
- phys_addr_t start;
- phys_addr_t size;
-
- int num_descs;
- struct persistent_ram_descriptor *descs;
-
- struct list_head node;
-};
-
struct persistent_ram_zone {
phys_addr_t paddr;
size_t size;
@@ -63,14 +48,10 @@ struct persistent_ram_zone {
size_t old_log_size;
};
-int persistent_ram_early_init(struct persistent_ram *ram);
-
struct persistent_ram_zone * __init persistent_ram_new(phys_addr_t start,
size_t size,
bool ecc);
void persistent_ram_free(struct persistent_ram_zone *prz);
-struct persistent_ram_zone *persistent_ram_init_ringbuffer(struct device *dev,
- bool ecc);
int persistent_ram_write(struct persistent_ram_zone *prz, const void *s,
unsigned int count);
--
1.7.9.2
Hi Anton,
Ignore my previous email. I am still getting caught up with your
patches.
On Wed, 2012-05-16 at 05:56 -0700, Anton Vorontsov wrote:
> Suggested-by: Shuah Khan <[email protected]>
> Signed-off-by: Anton Vorontsov <[email protected]>
> ---
> Documentation/ramoops.txt | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
> index 4ba7db2..138823b 100644
> --- a/Documentation/ramoops.txt
> +++ b/Documentation/ramoops.txt
> @@ -40,6 +40,12 @@ corrupt, but usually it is restorable.
> Setting the ramoops parameters can be done in 2 different manners:
> 1. Use the module parameters (which have the names of the variables described
> as before).
> + For quick debugging, you can also reserve parts of memory during boot
> + and then use the reserved memory for ramoops. For example, assuming a machine
> + with > 128 MB of memory, the following kernel command line will tell the
> + kernel to use only the first 128 MB of memory, and place ECC-protected ramoops
> + region at 128 MB boundary:
> + "mem=128M ramoops.mem_address=0x8000000 ramoops.ecc=1"
> 2. Use a platform device and set the platform data. The parameters can then
> be set through that platform data. An example of doing that is:
Thanks much. This is great information to have. Exactly the detail I was
asking for.
>
> @@ -70,6 +76,15 @@ if (ret) {
> return ret;
> }
>
> +You can specify either RAM memory or peripheral devices' memory. However, when
> +specifying RAM, be sure to reserve the memory by issuing memblock_reserve()
> +very early in the architecture code, just before platform device registration,
> +e.g.:
> +
> +#include <linux/memblock.h>
> +
> +memblock_reserve(ramoops_data.mem_address, ramoops_data.mem_size);
Same here. :)
-- Shuah
> +
> 3. Dump format
>
> The data dump begins with a header, currently defined as "====" followed by a
On Wed, 2012-05-16 at 05:56 -0700, Anton Vorontsov wrote:
> This is all straightforward: we just use the last region for
> console logging. If there's just one region, we fall-back to
> the old behaviour: just a oops/dumps logging.
What about the saving /proc/last_kmsg? Are you not bringing that feature
from ramconsole? Sorry if I missed it in reading the patches.
-- Shuah
On Wed, May 16, 2012 at 5:56 AM, Anton Vorontsov
<[email protected]> wrote:
> This is all straightforward: we just use the last region for
> console logging. If there's just one region, we fall-back to
> the old behaviour: just a oops/dumps logging.
>
> Signed-off-by: Anton Vorontsov <[email protected]>
> ---
> ?fs/pstore/ram.c | ? 39 ++++++++++++++++++++++++++++++---------
> ?1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 9123cce..54213eb 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> [...]
> @@ -142,7 +148,12 @@ static int ramoops_pstore_write(enum pstore_type_id type,
> ? ? ? ?struct persistent_ram_zone *prz = cxt->przs[cxt->count];
> ? ? ? ?size_t hlen;
>
> - ? ? ? /* Currently ramoops is designed to only store dmesg dumps. */
> + ? ? ? if (type == PSTORE_TYPE_CONSOLE) {
> + ? ? ? ? ? ? ? if (!cxt->cprz)
> + ? ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
> + ? ? ? ? ? ? ? persistent_ram_write(cxt->cprz, cxt->pstore.buf, size);
> + ? ? ? }
> +
> ? ? ? ?if (type != PSTORE_TYPE_DMESG)
> ? ? ? ? ? ? ? ?return -EINVAL;
Doesn't this mean that type == PSTORE_TYPE_CONSOLE will write to the
ram, but then fail with -EINVAL?
-Kees
--
Kees Cook
Chrome OS Security
On Wed, May 16, 2012 at 5:56 AM, Anton Vorontsov
<[email protected]> wrote:
> Pstore doesn't support logging kernel messages in run-time, it only
> dumps dmesg when kernel oopses/panics. This makes pstore useless for
> debugging hangs caused by HW issues or improper use of HW (e.g.
> weird device inserted -> driver tried to write a reserved bits ->
> SoC hanged. In that case we don't get any messages in the pstore.
>
> Therefore, let's add a runtime logging support: PSTORE_TYPE_CONSOLE.
>
> Signed-off-by: Anton Vorontsov <[email protected]>
> ---
> ?fs/pstore/Kconfig ? ? ?| ? ?7 +++++++
> ?fs/pstore/inode.c ? ? ?| ? ?3 +++
> ?fs/pstore/platform.c ? | ? 25 +++++++++++++++++++++++++
> ?include/linux/pstore.h | ? ?1 +
> ?4 files changed, 36 insertions(+)
>
> diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
> index 23ade26..d044de6 100644
> --- a/fs/pstore/Kconfig
> +++ b/fs/pstore/Kconfig
> @@ -12,6 +12,13 @@ config PSTORE
> ? ? ? ? ? If you don't have a platform persistent store driver,
> ? ? ? ? ? say N.
>
> +config PSTORE_CONSOLE
> + ? ? ? bool "Log kernel console messages"
> + ? ? ? depends on PSTORE
> + ? ? ? help
> + ? ? ? ? When the option is enabled, pstore will log all kernel
> + ? ? ? ? messages, even if no oops or panic happened.
> +
> ?config PSTORE_RAM
> ? ? ? ?tristate "Log panic/oops to a RAM buffer"
> ? ? ? ?depends on PSTORE
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 1950788..97d35ee 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -212,6 +212,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id,
> ? ? ? ?case PSTORE_TYPE_DMESG:
> ? ? ? ? ? ? ? ?sprintf(name, "dmesg-%s-%lld", psname, id);
> ? ? ? ? ? ? ? ?break;
> + ? ? ? case PSTORE_TYPE_CONSOLE:
> + ? ? ? ? ? ? ? sprintf(name, "console-%s", psname);
> + ? ? ? ? ? ? ? break;
> ? ? ? ?case PSTORE_TYPE_MCE:
> ? ? ? ? ? ? ? ?sprintf(name, "mce-%s-%lld", psname, id);
> ? ? ? ? ? ? ? ?break;
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 82c585f..3a384c8 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -1,6 +1,7 @@
> ?/*
> ?* Persistent Storage - platform driver interface parts.
> ?*
> + * Copyright (C) 2007-2008 Google, Inc.
> ?* Copyright (C) 2010 Intel Corporation <[email protected]>
> ?*
> ?* ?This program is free software; you can redistribute it and/or modify
> @@ -22,6 +23,7 @@
> ?#include <linux/errno.h>
> ?#include <linux/init.h>
> ?#include <linux/kmsg_dump.h>
> +#include <linux/console.h>
> ?#include <linux/module.h>
> ?#include <linux/pstore.h>
> ?#include <linux/string.h>
> @@ -156,6 +158,28 @@ static struct kmsg_dumper pstore_dumper = {
> ? ? ? ?.dump = pstore_dump,
> ?};
>
> +#ifdef CONFIG_PSTORE_CONSOLE
> +static void pstore_console_write(struct console *con, const char *s, unsigned c)
> +{
> + ? ? ? strncpy(psinfo->buf, s, c);
The size of psinfo->buf needs to be the length argument to strncpy,
not the size of "s". If "s" isn't NULL terminated, then the min of c
and buf's size should be used. And if this should be NULL terminated
in buf, that needs to be added manually since strncpy won't do it if
it hits the max length argument.
-Kees
--
Kees Cook
Chrome OS Security
On Wed, May 16, 2012 at 5:56 AM, Anton Vorontsov
<[email protected]> wrote:
> Suggested-by: Shuah Khan <[email protected]>
> Signed-off-by: Anton Vorontsov <[email protected]>
> ---
> ?Documentation/ramoops.txt | ? 15 +++++++++++++++
> ?1 file changed, 15 insertions(+)
>
> diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
> index 4ba7db2..138823b 100644
> --- a/Documentation/ramoops.txt
> +++ b/Documentation/ramoops.txt
> @@ -40,6 +40,12 @@ corrupt, but usually it is restorable.
> ?Setting the ramoops parameters can be done in 2 different manners:
> ?1. Use the module parameters (which have the names of the variables described
> ?as before).
> + For quick debugging, you can also reserve parts of memory during boot
> + and then use the reserved memory for ramoops. For example, assuming a machine
> + with > 128 MB of memory, the following kernel command line will tell the
> + kernel to use only the first 128 MB of memory, and place ECC-protected ramoops
> + region at 128 MB boundary:
> + "mem=128M ramoops.mem_address=0x8000000 ramoops.ecc=1"
> ?2. Use a platform device and set the platform data. The parameters can then
> ?be set through that platform data. An example of doing that is:
>
> @@ -70,6 +76,15 @@ if (ret) {
> ? ? ? ?return ret;
> ?}
>
> +You can specify either RAM memory or peripheral devices' memory. However, when
> +specifying RAM, be sure to reserve the memory by issuing memblock_reserve()
> +very early in the architecture code, just before platform device registration,
Just before platform device registration is way too late. ARM
provides a machine reserve callback to allow board files to call
memblock_reserve inside arm_memblock_init() and before mm_init().
> +e.g.:
> +
> +#include <linux/memblock.h>
> +
> +memblock_reserve(ramoops_data.mem_address, ramoops_data.mem_size);
> +
> ?3. Dump format
>
> ?The data dump begins with a header, currently defined as "====" followed by a
> --
> 1.7.9.2
>
On Wed, May 16, 2012 at 10:56:09AM -0700, Colin Cross wrote:
[...]
> > +You can specify either RAM memory or peripheral devices' memory. However, when
> > +specifying RAM, be sure to reserve the memory by issuing memblock_reserve()
> > +very early in the architecture code, just before platform device registration,
>
> Just before platform device registration is way too late. ARM
> provides a machine reserve callback to allow board files to call
> memblock_reserve inside arm_memblock_init() and before mm_init().
Yeah, and I guess that 'too late' is also true for all architectures,
the platform_device_register and friends are most probably unusable
before mm_init, and after that it's always too late.
So the word 'just' is misleading indeed, I'll remove it.
Thanks!
--
Anton Vorontsov
Email: [email protected]
On Wed, May 16, 2012 at 09:30:52AM -0600, Shuah Khan wrote:
> On Wed, 2012-05-16 at 05:56 -0700, Anton Vorontsov wrote:
> > This is all straightforward: we just use the last region for
> > console logging. If there's just one region, we fall-back to
> > the old behaviour: just a oops/dumps logging.
>
> What about the saving /proc/last_kmsg? Are you not bringing that feature
> from ramconsole? Sorry if I missed it in reading the patches.
Nope, sorry. The /proc/last_kms ABI is just a duplication of
pstore's console logging interface, and since last_kmsg thing is
in drivers/staging/, there's no point in keeping it, unless there's
a really good reason.
Thanks!
--
Anton Vorontsov
Email: [email protected]
On Wed, May 16, 2012 at 3:11 PM, Anton Vorontsov <[email protected]> wrote:
> On Wed, May 16, 2012 at 10:56:09AM -0700, Colin Cross wrote:
> [...]
>> > +You can specify either RAM memory or peripheral devices' memory. However, when
>> > +specifying RAM, be sure to reserve the memory by issuing memblock_reserve()
>> > +very early in the architecture code, just before platform device registration,
>>
>> Just before platform device registration is way too late. ?ARM
>> provides a machine reserve callback to allow board files to call
>> memblock_reserve inside arm_memblock_init() and before mm_init().
>
> Yeah, and I guess that 'too late' is also true for all architectures,
> the platform_device_register and friends are most probably unusable
> before mm_init, and after that it's always too late.
>
> So the word 'just' is misleading indeed, I'll remove it.
I think any reference to the time when platform devices are registered
is misleading. There is a very specific point during arch init where
memblock_reserve is valid, and it is nowhere near platform device
registration.
On Wed, May 16, 2012 at 09:45:35AM -0700, Kees Cook wrote:
> On Wed, May 16, 2012 at 5:56 AM, Anton Vorontsov
> <[email protected]> wrote:
> > This is all straightforward: we just use the last region for
> > console logging. If there's just one region, we fall-back to
> > the old behaviour: just a oops/dumps logging.
> >
> > Signed-off-by: Anton Vorontsov <[email protected]>
> > ---
> > fs/pstore/ram.c | 39 ++++++++++++++++++++++++++++++---------
> > 1 file changed, 30 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index 9123cce..54213eb 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > [...]
> > @@ -142,7 +148,12 @@ static int ramoops_pstore_write(enum pstore_type_id type,
> > struct persistent_ram_zone *prz = cxt->przs[cxt->count];
> > size_t hlen;
> >
> > - /* Currently ramoops is designed to only store dmesg dumps. */
> > + if (type == PSTORE_TYPE_CONSOLE) {
> > + if (!cxt->cprz)
> > + return -ENOMEM;
> > + persistent_ram_write(cxt->cprz, cxt->pstore.buf, size);
> > + }
> > +
> > if (type != PSTORE_TYPE_DMESG)
> > return -EINVAL;
>
> Doesn't this mean that type == PSTORE_TYPE_CONSOLE will write to the
> ram, but then fail with -EINVAL?
Right you are, there should be 'return 0' for TYPE_CONSOLE. It is
harmless tho (and unnoticeable, since we can't check return value of
the ->write() calback and print error, since this would recurse),
but I'll surely amend that.
Thanks!
--
Anton Vorontsov
Email: [email protected]
On Wed, May 16, 2012 at 09:49:11AM -0700, Kees Cook wrote:
[...]
> > +#ifdef CONFIG_PSTORE_CONSOLE
> > +static void pstore_console_write(struct console *con, const char *s, unsigned c)
> > +{
> > + strncpy(psinfo->buf, s, c);
>
> The size of psinfo->buf needs to be the length argument to strncpy,
> not the size of "s". If "s" isn't NULL terminated, then the min of c
> and buf's size should be used. And if this should be NULL terminated
> in buf, that needs to be added manually since strncpy won't do it if
> it hits the max length argument.
Whoops. Will fix it, thanks!
--
Anton Vorontsov
Email: [email protected]
On Wed, May 16, 2012 at 03:33:45PM -0700, Anton Vorontsov wrote:
> On Wed, May 16, 2012 at 09:49:11AM -0700, Kees Cook wrote:
> [...]
> > > +#ifdef CONFIG_PSTORE_CONSOLE
> > > +static void pstore_console_write(struct console *con, const char *s, unsigned c)
> > > +{
> > > + strncpy(psinfo->buf, s, c);
> >
> > The size of psinfo->buf needs to be the length argument to strncpy,
> > not the size of "s". If "s" isn't NULL terminated, then the min of c
> > and buf's size should be used. And if this should be NULL terminated
> > in buf, that needs to be added manually since strncpy won't do it if
> > it hits the max length argument.
>
> Whoops. Will fix it, thanks!
Actually, we shouldn't bother with NUL-termination at all. We should
do, is just carefully copy all 'c' elements of 's' into pstore in
'pstore->bufsize' chunks.
So, the code should be something along these lines:
static void pstore_console_write(struct console *con, const char *s, unsigned c)
{
const char *e = s + c;
while (s < e) {
if (c > psinfo->bufsize)
c = psinfo->bufsize;
memcpy(psinfo->buf, s, c);
psinfo->write(PSTORE_TYPE_CONSOLE, 0, NULL, 0, c, psinfo);
s += c;
c = e - s;
}
}
Here, in case of c > bufsize, we can't just copy bufsize bytes once and
exit, since 'bufsize' doesn't tell anything about TYPE_CONSOLE's internal
storage size (today console's persistent ram zone size and pstore's
bufsize are equal, but this is something we may change soon, i.e. make
TYPE_DMESG and TYPE_CONSOLE record sizes configurable).
Thanks,
--
Anton Vorontsov
Email: [email protected]