Merge ram_console into pstore, and more
Subsystems (3)
v3 cover Merge ram_console into pstore, and more
Hi all,
Here comes v3:
- Rebased on top of current staging-next;
- The series are getting bigger. This is partly because we now support different persistent zone sizes for oops records and console log, per Colin Cross' request.
And I believe the code is now more manageable for further enhancements (e.g. if we'd want to add other message types, e.g. tracing);
- Addressed Kees Cook's comments on the unlinking matters;
- Removed automatic updates support. Please see the last patch description for rationale;
- A new fixup for pstore/inode, just getting rid of a sparse warning.
In v2:
- Updated documentation per Colin Cross' comments;
- Corrected return value in ramoops_pstore_write() (noticed by Kees Cook);
- Fixed large writes handling in pstore_console_write(), i.e. when log_buf write is larger than pstore bufsize. Also Noticed by Kees Cook.
And a boilerplate for the series:
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.
Thanks!
---
Documentation/ramoops.txt | 14 ++ 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 | 5 +- fs/pstore/platform.c | 70 +++++----- fs/pstore/ram.c | 228 ++++++++++++++++++++++++++------- fs/pstore/ram_core.c | 109 ++++------------ include/linux/pstore.h | 1 + include/linux/pstore_ram.h | 22 +--- 11 files changed, 269 insertions(+), 372 deletions(-)
Signature · 2 lines
Anton Vorontsov Email: [email protected]
PATCH v3 1/14 pstore/inode: Make pstore_fill_super() static
There's no reason to extern it. The patch fixes the annoying sparse warning:
CHECK fs/pstore/inode.cfs/pstore/inode.c:264:5: warning: symbol 'pstore_fill_super' was not declared. Should it be static?
--- fs/pstore/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/pstore/inode.c +1 -1
@@ -258,7 +258,7 @@ fail:return rc; } -int pstore_fill_super (struct super_block *sb,void *data,int silent) +static int pstore_fill_super (struct super_block *sb,void *data,int silent) {struct inode *inode;
Signature · 1 line
1.7.9.2
PATCH v3 2/14 pstore/ram: Should update old dmesg buffer before reading
In reply to Anton VorontsovWithout the update, we'll only see the new dmesg buffer after the reboot, but previously we could see it right away. Making an oops visible in pstore filesystem before reboot is a somewhat dubious feature, but removing it wasn't an intentional change, so let's restore it for now.
--- fs/pstore/ram.c | 2 ++ fs/pstore/ram_core.c | 21 +++++++++++++++++++-- include/linux/pstore_ram.h | 1 + 3 files changed, 22 insertions(+), 2 deletions(-)
fs/pstore/ram.c +2 -0
@@ -106,6 +106,8 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type, time->tv_sec =0 ; time->tv_nsec =0 ; +/* Update old/shadowed buffer. */ +persistent_ram_copy_old (prz); size =persistent_ram_old_size (prz); *buf =kmalloc (size,GFP_KERNEL );if (*buf ==NULL )
fs/pstore/ram_core.c +19 -2
@@ -250,8 +250,7 @@ static void notrace persistent_ram_update(struct persistent_ram_zone *prz,persistent_ram_update_ecc (prz, start, count); } -static void __init -persistent_ram_save_old (struct persistent_ram_zone *prz) +static void persistent_ram_save_old (struct persistent_ram_zone *prz) {struct persistent_ram_buffer *buffer = prz->buffer;size_t size =buffer_size (prz);
@@ -272,6 +271,24 @@ persistent_ram_save_old(struct persistent_ram_zone *prz)memcpy (prz->old_log + size - start, &buffer->data[0 ], start); } +void persistent_ram_copy_old (struct persistent_ram_zone *prz) +{ +struct persistent_ram_buffer *buffer = prz->buffer; +size_t size =buffer_size (prz); +size_t start =buffer_start (prz); + +if (!size) +return ; + +if (!prz->old_log) { +persistent_ram_save_old (prz); +return ; + } + prz->old_log_size = size; +memcpy (prz->old_log, &buffer->data[start], size - start); +memcpy (prz->old_log + size - start, &buffer->data[0 ], start); +} + int notrace persistent_ram_write(struct persistent_ram_zone *prz,const void *s,unsigned int count) {
include/linux/pstore_ram.h +1 -0
@@ -75,6 +75,7 @@ struct persistent_ram_zone *persistent_ram_init_ringbuffer(struct device *dev,int persistent_ram_write (struct persistent_ram_zone *prz,const void *s,unsigned int count); +void persistent_ram_copy_old (struct persistent_ram_zone *prz);size_t persistent_ram_old_size (struct persistent_ram_zone *prz);void *persistent_ram_old (struct persistent_ram_zone *prz);void persistent_ram_free_old (struct persistent_ram_zone *prz);
Signature · 1 line
1.7.9.2
Re: [PATCH 02/14] pstore/ram: Should update old dmesg buffer before reading
Quoted 51 lines · Anton Vorontsov
Without the update, we'll only see the new dmesg buffer after the reboot, but previously we could see it right away. Making an oops visible in pstore filesystem before reboot is a somewhat dubious feature, but removing it wasn't an intentional change, so let's restore it for now.
Signed-off-by Anton Vorontsov <[email protected]>---
fs/pstore/ram.c | 2 ++fs/pstore/ram_core.c| 21 +++++++++++++++++++--include/linux/pstore_ram.h| 1 +3 files changed, 22 insertions(+), 2 deletions(-)fs/pstore/ram.c +2
-0@@ -106,6 +106,8 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type, time->tv_sec = 0; time->tv_nsec = 0; + /* Update old/shadowed buffer. */ + persistent_ram_copy_old(prz); size = persistent_ram_old_size(prz); *buf = kmalloc(size, GFP_KERNEL); if (*buf == NULL)fs/pstore/ram_core.c +8
-2@@ -250,8 +250,7 @@ static void notrace persistent_ram_update(struct persistent_ram_zone *prz, persistent_ram_update_ecc(prz, start, count); } -static void __init -persistent_ram_save_old (struct persistent_ram_zone *prz) +static void persistent_ram_save_old (struct persistent_ram_zone *prz) { struct persistent_ram_buffer *buffer = prz->buffer; size_t size = buffer_size(prz);@@ -272,6 +271,24 @@ persistent_ram_save_old(struct persistent_ram_zone *prz) memcpy(prz->old_log + size - start, &buffer->data[0], start); } +void persistent_ram_copy_old (struct persistent_ram_zone *prz) +{ + struct persistent_ram_buffer *buffer = prz->buffer; + size_t size = buffer_size(prz); + size_t start = buffer_start(prz); + + if (!size)
fs/pstore/ram_core.c persistent_ram_save_old(struct persistent_ram_zone *prz) · line 290
Quoted 10 lines
fs/pstore/ram_core.c +10
-0+ return; + + if (!prz->old_log) { + persistent_ram_save_old(prz); + return; + } + prz->old_log_size = size; + memcpy(prz->old_log, &buffer->data[start], size - start); + memcpy(prz->old_log + size - start, &buffer->data[0], start); +}
This duplicates most of persistent_ram_save_old, including all the ringbuffer math that may change. Can they be the same function, and only malloc and ecc if old_log doesn't exist?
Quoted 18 lines
fs/pstore/ram_core.c +0
-0int notrace persistent_ram_write(struct persistent_ram_zone *prz, const void *s, unsigned int count) {include/linux/pstore_ram.h +1
-1@@ -75,6 +75,7 @@ struct persistent_ram_zone *persistent_ram_init_ringbuffer(struct device *dev, int persistent_ram_write(struct persistent_ram_zone *prz, const void *s, unsigned int count); +void persistent_ram_copy_old (struct persistent_ram_zone *prz); size_t persistent_ram_old_size(struct persistent_ram_zone *prz); void *persistent_ram_old(struct persistent_ram_zone *prz); void persistent_ram_free_old(struct persistent_ram_zone *prz); --1.7.9.2
PATCH v3 3/14 pstore/ram_core: Do not reset restored zone's position and size
In reply to Anton VorontsovOtherwise, the files will survive just one reboot, and on a subsequent boot they will disappear.
--- fs/pstore/ram_core.c | 1 + 1 file changed, 1 insertion(+)
fs/pstore/ram_core.c +1 -0
@@ -422,6 +422,7 @@ static int __init persistent_ram_post_init(struct persistent_ram_zone *prz, bool" size %zu, start %zu\n" , buffer_size(prz), buffer_start(prz));persistent_ram_save_old (prz); +return 0 ; } }else { pr_info("persistent_ram: no valid data in buffer"
Signature · 1 line
1.7.9.2
Re: [PATCH 03/14] pstore/ram_core: Do not reset restored zone's position and size
Quoted 19 lines · Anton Vorontsov
Otherwise, the files will survive just one reboot, and on a subsequent boot they will disappear.
Signed-off-by Anton Vorontsov <[email protected]>---
fs/pstore/ram_core.c | 1 +1 file changed, 1 insertion(+)fs/pstore/ram_core.c +1
-0@@ -422,6 +422,7 @@ static int __init persistent_ram_post_init(struct persistent_ram_zone *prz, bool " size %zu, start %zu\n", buffer_size(prz), buffer_start(prz)); persistent_ram_save_old(prz); + return 0; } } else {
Quoted 4 lines
pr_info("persistent_ram: no valid data in buffer" --
1.7.9.2
This causes an interesting behavior change in the console logging. Before this change, the console log would show only the messages from the last reboot. After this change, the console log will have logs from multiple boots appended to each other. I can think of some places where that could very handy, so I'm not against the change, but as is it makes reading the logs much harder - the first oops you see while skimming the log may not be from the last reboot.
One possibility would be to insert an obvious (and script parseable) header during probe to separate the boots. Another option would be to expand the ringbuffer metadata to contain duplicate start and size fields that only cover the most recent reboot, and export two files, console-ramoops that contains the last log, and console-all-ramoops that contains all the logs. Or you could just zap the console buffer at boot to keep the old behavior.
Re: [PATCH 03/14] pstore/ram_core: Do not reset restored zone's position and size
On Fri, May 18, 2012 at 04:42:03PM -0700, Colin Cross wrote:
[...]
Quoted 4 lines
This causes an interesting behavior change in the console logging. Before this change, the console log would show only the messages from the last reboot. After this change, the console log will have logs from multiple boots appended to each other.
Heh. A nice feature, but frankly speaking, that wasn't my intent to introduce it. :-)
I will document the new behaviour in the patch description.
Quoted 7 lines
I can think of some
places where that could very handy, so I'm not against the change, but as is it makes reading the logs much harder - the first oops you see while skimming the log may not be from the last reboot.One possibility would be to insert an obvious (and script parseable) header during probe to separate the boots.
Yep, we have it already, i.e. linux_banner. This script should be reliable enough:
tac ramoops-console | sed '/^Linux version.*(.*@.*)/ q' | tac
Quoted 5 lines
Another option would be to
expand the ringbuffer metadata to contain duplicate start and size fields that only cover the most recent reboot, and export two files, console-ramoops that contains the last log, and console-all-ramoops that contains all the logs.
This seems like an overkill. :-)
Quoted 2 lines
Or you could just zap the console buffer
at boot to keep the old behavior.
Well, I actually like the new behaviour, so assuming that we're all happy with the new feature, there's no need to zap it at boot. Or we can at least make it configurable...
Thanks!
Signature · 2 lines
Anton Vorontsov Email: [email protected]
PATCH v3 4/14 pstore/ram: Should zap persistent zone on unlink
In reply to Anton VorontsovOtherwise, unlinked file will reappear on the next boot.
--- fs/pstore/ram.c | 1 + fs/pstore/ram_core.c | 6 ++++++ include/linux/pstore_ram.h | 1 + 3 files changed, 8 insertions(+)
fs/pstore/ram.c +1 -0
@@ -186,6 +186,7 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,return -EINVAL ;persistent_ram_free_old (cxt->przs[id]); +persistent_ram_zap (cxt->przs[id]);return 0 ; }
fs/pstore/ram_core.c +6 -0
@@ -336,6 +336,12 @@ void persistent_ram_free_old(struct persistent_ram_zone *prz) prz->old_log_size =0 ; } +void persistent_ram_zap (struct persistent_ram_zone *prz) +{ +atomic_set (&prz->buffer->size,0 ); +persistent_ram_update_header_ecc (prz); +} +static void *persistent_ram_vmap (phys_addr_t start,size_t size) {struct page **pages;
include/linux/pstore_ram.h +1 -0
@@ -69,6 +69,7 @@ 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); +void persistent_ram_zap (struct persistent_ram_zone *prz);struct persistent_ram_zone *persistent_ram_init_ringbuffer (struct device *dev,bool ecc);
Signature · 1 line
1.7.9.2
Re: [PATCH 04/14] pstore/ram: Should zap persistent zone on unlink
Quoted 27 lines · Anton Vorontsov
Otherwise, unlinked file will reappear on the next boot.
Reported-by Kees Cook <[email protected]>Signed-off-by Anton Vorontsov <[email protected]>---
fs/pstore/ram.c | 1 +
fs/pstore/ram_core.c | 6 ++++++
include/linux/pstore_ram.h | 1 +3 files changed, 8 insertions(+)fs/pstore/ram.c +1
-0@@ -186,6 +186,7 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, return -EINVAL; persistent_ram_free_old(cxt->przs[id]); + persistent_ram_zap(cxt->przs[id]); return 0; }fs/pstore/ram_core.c +0
-0
fs/pstore/ram_core.c void persistent_ram_free_old(struct persistent_ram_zone *prz) · line 341
Quoted 6 lines
fs/pstore/ram_core.c +3
-0@@ -336,6 +336,12 @@ void persistent_ram_free_old(struct persistent_ram_zone *prz) prz->old_log_size = 0; } +void persistent_ram_zap (struct persistent_ram_zone *prz) +{ + atomic_set(&prz->buffer->size, 0);
There are a couple places in the ringbuffer code that assume that the data is located in the buffer in the range [0, size). It may start anywhere in that range and wrap, but address 0 in the buffer always contains data. Resetting size to 0 but leaving start non-zero may result in no data at address zero, and will cause problems in at least persistent_ram_save_old, persistent_ram_ecc_old, persistent_ram_copy_old, and the detection code in persistent_ram_post_init. These would all need to be fixed to support the dual-metadata ringbuffer option I mentioned in response to patch 3, but if you don't like that idea then just clear start here.
Quoted 21 lines
fs/pstore/ram_core.c +3
-0+ persistent_ram_update_header_ecc(prz); +} + static void *persistent_ram_vmap(phys_addr_t start, size_t size) { struct page **pages;include/linux/pstore_ram.h +1
-1@@ -69,6 +69,7 @@ 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); +void persistent_ram_zap (struct persistent_ram_zone *prz); struct persistent_ram_zone *persistent_ram_init_ringbuffer(struct device *dev, bool ecc); --1.7.9.2
PATCH v3 5/14 pstore: Add console log messages support
In reply to Anton VorontsovPstore 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.
--- fs/pstore/Kconfig | 7 +++++++ fs/pstore/inode.c | 3 +++ fs/pstore/platform.c | 33 +++++++++++++++++++++++++++++++++ include/linux/pstore.h | 1 + 4 files changed, 44 insertions(+)
fs/pstore/Kconfig +7 -0
@@ -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
fs/pstore/inode.c +3 -0
@@ -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 ;
fs/pstore/platform.c +33 -0
@@ -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,36 @@ 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) +{ +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; + } +} + +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 +225,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);
include/linux/pstore.h +1 -0
@@ -29,6 +29,7 @@enum pstore_type_id {PSTORE_TYPE_DMESG =0 ,PSTORE_TYPE_MCE =1 , +PSTORE_TYPE_CONSOLE =2 ,PSTORE_TYPE_UNKNOWN =255 };
Signature · 1 line
1.7.9.2
PATCH v3 6/14 pstore/ram: Introduce ramoops_context.max_dump_count
In reply to Anton VorontsovSo far it is the same as max_count, but this will change when we'll add support for other message types (e.g. console, mce, tracing).
--- fs/pstore/ram.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
fs/pstore/ram.c +9 -6
@@ -70,6 +70,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; };
@@ -94,7 +95,7 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,struct ramoops_context *cxt = psi->data;struct persistent_ram_zone *prz; -if (cxt->read_count >= cxt->max_count) +if (cxt->read_count >= cxt->max_dump_count)return -EINVAL ; *id = cxt->read_count++;
@@ -172,7 +173,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 ; }
@@ -182,7 +183,7 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, {struct ramoops_context *cxt = psi->data; -if (id >= cxt->max_count) +if (id >= cxt->max_dump_count)return -EINVAL ;persistent_ram_free_old (cxt->przs[id]);
@@ -213,7 +214,7 @@ static int __init ramoops_probe(struct platform_device *pdev)/* Only a single ramoops area allowed at a time, so fail extra * probes. */ -if (cxt->max_count) +if (cxt->max_dump_count) goto fail_out;if (!pdata->mem_size || !pdata->record_size) {
@@ -240,6 +241,7 @@ static int __init ramoops_probe(struct platform_device *pdev) } cxt->max_count = pdata->mem_size / pdata->record_size; + cxt->max_dump_count = cxt->max_count; cxt->count =0 ; cxt->size = pdata->mem_size; cxt->phys_addr = pdata->mem_address;
@@ -247,14 +249,14 @@ static int __init ramoops_probe(struct platform_device *pdev) cxt->dump_oops = pdata->dump_oops; cxt->ecc = pdata->ecc; - cxt->przs =kzalloc (sizeof (*cxt->przs) * cxt->max_count,GFP_KERNEL ); + cxt->przs =kzalloc (sizeof (*cxt->przs) * cxt->max_dump_count,GFP_KERNEL );if (!cxt->przs) { err = -ENOMEM ;dev_err (dev,"failed to initialize a prz array\n" ); goto fail_out; } -for (i =0 ; i < cxt->max_count; i++) { +for (i =0 ; i < cxt->max_dump_count; i++) {size_t sz = cxt->record_size;phys_addr_t start = cxt->phys_addr + sz * i;
@@ -303,6 +305,7 @@ fail_buf: fail_clear: cxt->pstore.bufsize =0 ; cxt->max_count =0 ; + cxt->max_dump_count =0 ; fail_przs:for (i =0 ; cxt->przs[i]; i++)persistent_ram_free (cxt->przs[i]);
Signature · 1 line
1.7.9.2
PATCH v3 7/14 pstore/ram: Factor dmesg przs initialization out of probe()
In reply to Anton VorontsovThis will help make code clearer when we'll add support for other message types.
This also makes probe() much shorter and understandable, plus makes mem/record size checking a bit easier.
Implementation detail: we now use a paddr pointer, this will be used for allocating persistent ram zones for other message types.
--- fs/pstore/ram.c | 105 ++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 66 insertions(+), 39 deletions(-)
fs/pstore/ram.c +66 -39
@@ -203,13 +203,67 @@ static struct ramoops_context oops_cxt = { }, }; +static void ramoops_free_przs (struct ramoops_context *cxt) +{ +int i; + +if (!cxt->przs) +return ; + +for (i =0 ; cxt->przs[i]; i++) +persistent_ram_free (cxt->przs[i]); +kfree (cxt->przs); +} + +static int ramoops_init_przs (struct device *dev,struct ramoops_context *cxt, +phys_addr_t *paddr,size_t dump_mem_sz) +{ +int err = -ENOMEM ; +int i; + +if (!cxt->record_size) +return 0 ; + + cxt->max_dump_count = dump_mem_sz / cxt->record_size; +if (!cxt->max_dump_count) +return -ENOMEM ; + + cxt->przs =kzalloc (sizeof (*cxt->przs) * cxt->max_dump_count, +GFP_KERNEL ); +if (!cxt->przs) { +dev_err (dev,"failed to initialize a prz array for dumps\n" ); +return -ENOMEM ; + } + +for (i =0 ; i < cxt->max_dump_count; i++) { +size_t sz = cxt->record_size; + + cxt->przs[i] =persistent_ram_new (*paddr, sz, cxt->ecc); +if (IS_ERR (cxt->przs[i])) { + err =PTR_ERR (cxt->przs[i]); +dev_err (dev,"failed to request mem region (0x%zx@0x%llx): %d\n" , + sz, (unsigned long long )*paddr, err); + goto fail_prz; + } + *paddr += sz; + } + + cxt->max_count += cxt->max_dump_count; + +return 0 ; +fail_prz: +ramoops_free_przs (cxt); +return err; +} +static int __init ramoops_probe (struct platform_device *pdev) {struct device *dev = &pdev->dev;struct ramoops_platform_data *pdata = pdev->dev.platform_data;struct ramoops_context *cxt = &oops_cxt; +size_t dump_mem_sz; +phys_addr_t paddr;int err = -EINVAL ; -int i;/* Only a single ramoops area allowed at a time, so fail extra * probes.
@@ -226,22 +280,7 @@ static int __init ramoops_probe(struct platform_device *pdev) pdata->mem_size =rounddown_pow_of_two (pdata->mem_size); pdata->record_size =rounddown_pow_of_two (pdata->record_size); -/* Check for the minimum memory size */ -if (pdata->mem_size <MIN_MEM_SIZE && - pdata->record_size <MIN_MEM_SIZE ) { -pr_err ("memory size too small, minimum is %lu\n" , -MIN_MEM_SIZE ); - goto fail_out; - } - -if (pdata->mem_size < pdata->record_size) { -pr_err ("The memory size must be larger than the " -"records size\n" ); - goto fail_out; - } - - cxt->max_count = pdata->mem_size / pdata->record_size; - cxt->max_dump_count = cxt->max_count; + cxt->max_count =0 ; cxt->count =0 ; cxt->size = pdata->mem_size; cxt->phys_addr = pdata->mem_address;
@@ -249,24 +288,14 @@ static int __init ramoops_probe(struct platform_device *pdev) cxt->dump_oops = pdata->dump_oops; cxt->ecc = pdata->ecc; - cxt->przs =kzalloc (sizeof (*cxt->przs) * cxt->max_dump_count,GFP_KERNEL ); -if (!cxt->przs) { - err = -ENOMEM ; -dev_err (dev,"failed to initialize a prz array\n" ); - goto fail_out; - } - -for (i =0 ; i < cxt->max_dump_count; i++) { -size_t sz = cxt->record_size; -phys_addr_t start = cxt->phys_addr + sz * i; + paddr = cxt->phys_addr; - cxt->przs[i] =persistent_ram_new (start, sz, cxt->ecc); -if (IS_ERR (cxt->przs[i])) { - err =PTR_ERR (cxt->przs[i]); -dev_err (dev,"failed to request mem region (0x%zx@0x%llx): %d\n" , - sz, (unsigned long long )start, err); - goto fail_przs; - } + dump_mem_sz = cxt->size; + err =ramoops_init_przs (dev, cxt, &paddr, dump_mem_sz); +if (err) { +pr_err ("memory size too small, minimum is %lu\n" , + cxt->record_size); + goto fail_count; } cxt->pstore.data = cxt;
@@ -279,7 +308,7 @@ static int __init ramoops_probe(struct platform_device *pdev) } err =pstore_register (&cxt->pstore); -if (err) { +if (err || !cxt->max_count) {pr_err ("registering with pstore failed\n" ); goto fail_buf; }
@@ -306,10 +335,8 @@ fail_clear: cxt->pstore.bufsize =0 ; cxt->max_count =0 ; cxt->max_dump_count =0 ; -fail_przs: -for (i =0 ; cxt->przs[i]; i++) -persistent_ram_free (cxt->przs[i]); -kfree (cxt->przs); +fail_count: +ramoops_free_przs (cxt); fail_out:return err; }
Signature · 1 line
1.7.9.2
PATCH v3 8/14 pstore/ram: Factor ramoops_get_dump_prz() out of ramoops_pstore_read()
In reply to Anton VorontsovThis will help make code clearer when we'll add support for other message types.
The patch also changes return value from -EINVAL to 0 in case of end-of-records. The exact value doesn't matter for pstore (it should be just <= 0), but 0 feels more correct.
--- fs/pstore/ram.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)
fs/pstore/ram.c +22 -6
@@ -86,6 +86,24 @@ static int ramoops_pstore_open(struct pstore_info *psi)return 0 ; } +static struct persistent_ram_zone * +ramoops_get_dump_prz (u64 id,enum pstore_type_id *type, +struct ramoops_context *cxt) +{ +struct persistent_ram_zone *prz; + +if (id > cxt->max_dump_count) +return NULL ; + + prz = cxt->przs[id]; +if (!persistent_ram_old_size (prz)) +return NULL ; + + *type =PSTORE_TYPE_DMESG ; + +return prz; +} + static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type, struct timespec *time, char **buf,
@@ -95,14 +113,12 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,struct ramoops_context *cxt = psi->data;struct persistent_ram_zone *prz; -if (cxt->read_count >= cxt->max_dump_count) -return -EINVAL ; - *id = cxt->read_count++; - prz = cxt->przs[*id]; -/* Only supports dmesg output so far. */ - *type =PSTORE_TYPE_DMESG ; + prz =ramoops_get_dump_prz (*id, type, cxt); +if (!prz) +return 0 ; +/* TODO(kees): Bogus time for the moment. */ time->tv_sec =0 ; time->tv_nsec =0 ;
Signature · 1 line
1.7.9.2
PATCH v3 9/14 pstore/ram: Add console messages handling
In reply to Anton VorontsovThe console log size is configurable via ramoops.console_size module option, and the log itself is available via <pstore-mount>/console-ramoops file.
--- fs/pstore/ram.c | 107 ++++++++++++++++++++++++++++++++++++++------ include/linux/pstore_ram.h | 1 + 2 files changed, 95 insertions(+), 13 deletions(-)
fs/pstore/ram.c +94 -13
@@ -41,6 +41,10 @@ module_param(record_size, ulong, 0400);MODULE_PARM_DESC (record_size,"size of each dump done on oops/panic" ); +static ulong ramoops_console_size =MIN_MEM_SIZE ; +module_param_named (console_size, ramoops_console_size, ulong,0400 ); +MODULE_PARM_DESC (console_size,"size of kernel console log" ); +static ulong mem_address;module_param (mem_address, ulong,0400 ); MODULE_PARM_DESC(mem_address,
@@ -63,9 +67,11 @@ 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; +size_t console_size;int dump_oops;bool ecc;unsigned int count;
@@ -96,6 +102,9 @@ ramoops_get_dump_prz(u64 id, enum pstore_type_id *type,return NULL ; prz = cxt->przs[id]; + +/* Update old/shadowed buffer. */ +persistent_ram_copy_old (prz);if (!persistent_ram_old_size (prz))return NULL ;
@@ -104,6 +113,19 @@ ramoops_get_dump_prz(u64 id, enum pstore_type_id *type,return prz; } +static struct persistent_ram_zone * +ramoops_get_console_prz (u64 id,enum pstore_type_id *type, +struct ramoops_context *cxt) +{ +if (id >= cxt->max_count) +return NULL ; + + *type =PSTORE_TYPE_CONSOLE ; + cxt->read_count = cxt->max_count; + +return cxt->cprz; +} + static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type, struct timespec *time, char **buf,
@@ -117,14 +139,14 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type, prz =ramoops_get_dump_prz (*id, type, cxt);if (!prz) + prz =ramoops_get_console_prz (*id, type, cxt); +if (!prz)return 0 ;/* TODO(kees): Bogus time for the moment. */ time->tv_sec =0 ; time->tv_nsec =0 ; -/* Update old/shadowed buffer. */ -persistent_ram_copy_old (prz); size =persistent_ram_old_size (prz); *buf =kmalloc (size,GFP_KERNEL );if (*buf ==NULL )
@@ -161,7 +183,13 @@ 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); +return 0 ; + } +if (type !=PSTORE_TYPE_DMESG )return -EINVAL ;
@@ -198,12 +226,17 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, struct pstore_info *psi) {struct ramoops_context *cxt = psi->data; +struct persistent_ram_zone *prz; -if (id >= cxt->max_dump_count) +if (id >= cxt->max_dump_count && id < cxt->max_count) + prz = cxt->cprz; +else if (id < cxt->max_dump_count) + prz = cxt->przs[id]; +else return -EINVAL ; -persistent_ram_free_old (cxt->przs[id]); -persistent_ram_zap (cxt->przs[id]); +persistent_ram_free_old (prz); +persistent_ram_zap (prz);return 0 ; }
@@ -272,6 +305,35 @@ fail_prz:return err; } +static void ramoops_free_cprz (struct ramoops_context *cxt) +{ +kfree (cxt->cprz); +} + +static int ramoops_init_cprz (struct device *dev,struct ramoops_context *cxt, +phys_addr_t *paddr,size_t console_mem_sz) +{ +if (!console_mem_sz) +return 0 ; + +if (*paddr + console_mem_sz > *paddr + cxt->size) +return -ENOMEM ; + + cxt->cprz =persistent_ram_new (*paddr, console_mem_sz, cxt->ecc); +if (IS_ERR (cxt->cprz)) { +int err =PTR_ERR (cxt->cprz); + +dev_err (dev,"failed to request mem region (0x%zx@0x%llx): %d\n" , + console_mem_sz, (unsigned long long )*paddr, err); +return err; + } + + *paddr += console_mem_sz; + cxt->max_count++; + +return 0 ; +} +static int __init ramoops_probe (struct platform_device *pdev) {struct device *dev = &pdev->dev;
@@ -287,35 +349,51 @@ static int __init ramoops_probe(struct platform_device *pdev)if (cxt->max_dump_count) goto fail_out; -if (!pdata->mem_size || !pdata->record_size) { -pr_err ("The memory size and the record size must be " +if (!pdata->mem_size || (!pdata->record_size && !pdata->console_size)) { +pr_err ("The memory size and the record/console size must be " "non-zero\n" ); goto fail_out; } pdata->mem_size =rounddown_pow_of_two (pdata->mem_size); pdata->record_size =rounddown_pow_of_two (pdata->record_size); + pdata->console_size =rounddown_pow_of_two (pdata->console_size); cxt->max_count =0 ; cxt->count =0 ; cxt->size = pdata->mem_size; cxt->phys_addr = pdata->mem_address; cxt->record_size = pdata->record_size; + cxt->console_size = pdata->console_size; cxt->dump_oops = pdata->dump_oops; cxt->ecc = pdata->ecc; paddr = cxt->phys_addr; - dump_mem_sz = cxt->size; + dump_mem_sz = cxt->size - cxt->console_size; err =ramoops_init_przs (dev, cxt, &paddr, dump_mem_sz); -if (err) { +if (err) + goto fail_out; + + err =ramoops_init_cprz (dev, cxt, &paddr, cxt->console_size); +if (err) + goto fail_init_cprz; + +if (!cxt->max_count) {pr_err ("memory size too small, minimum is %lu\n" , - cxt->record_size); + cxt->console_size + cxt->record_size); goto fail_count; } cxt->pstore.data = cxt; - cxt->pstore.bufsize = cxt->przs[0 ]->buffer_size; +/* +* Console can handle any buffer size, so prefer dumps buffer +* size since usually it is smaller. +*/ +if (cxt->przs) + cxt->pstore.bufsize = cxt->przs[0 ]->buffer_size; +else + cxt->pstore.bufsize = cxt->cprz->buffer_size; cxt->pstore.buf =kmalloc (cxt->pstore.bufsize,GFP_KERNEL );spin_lock_init (&cxt->pstore.buf_lock);if (!cxt->pstore.buf) {
@@ -324,7 +402,7 @@ static int __init ramoops_probe(struct platform_device *pdev) } err =pstore_register (&cxt->pstore); -if (err || !cxt->max_count) { +if (err) {pr_err ("registering with pstore failed\n" ); goto fail_buf; }
@@ -352,6 +430,8 @@ fail_clear: cxt->max_count =0 ; cxt->max_dump_count =0 ; fail_count: +ramoops_free_cprz (cxt); +fail_init_cprz:ramoops_free_przs (cxt); fail_out:return err;
@@ -403,6 +483,7 @@ static int __init ramoops_init(void) dummy_data->mem_size = mem_size; dummy_data->mem_address = mem_address; dummy_data->record_size = record_size; + dummy_data->console_size = ramoops_console_size; dummy_data->dump_oops = dump_oops; dummy_data->ecc = ramoops_ecc; dummy = platform_create_bundle(&ramoops_driver, ramoops_probe,
include/linux/pstore_ram.h +1 -0
@@ -93,6 +93,7 @@ struct ramoops_platform_data {unsigned long mem_size;unsigned long mem_address;unsigned long record_size; +unsigned long console_size;int dump_oops;bool ecc; };
Signature · 1 line
1.7.9.2
PATCH v3 10/14 pstore/ram_core: Silence some printks
In reply to Anton VorontsovSince 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.
--- fs/pstore/ram_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
fs/pstore/ram_core.c +2 -2
@@ -424,14 +424,14 @@ 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);return 0 ; } }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); }
Signature · 1 line
1.7.9.2
PATCH v3 11/14 pstore/ram: Add some more documentation and examples
In reply to Anton Vorontsov--- Documentation/ramoops.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
Documentation/ramoops.txt +14 -0
@@ -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,14 @@ 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, 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
Signature · 1 line
1.7.9.2
PATCH v3 12/14 staging/android: Remove ram_console driver
In reply to Anton VorontsovAll the functionality is now supported by pstore and pstore_ram drivers.
--- 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
drivers/staging/android/Kconfig +0 -5
@@ -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=y - default n - config ANDROID_TIMED_OUTPUT bool "Timed output class driver" default y
drivers/staging/android/Makefile +0 -1
@@ -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
drivers/staging/android/ram_console.c deleted +0 -179
179 lines not on this page · raw message
Signature · 1 line
1.7.9.2
PATCH v3 13/14 pstore/ram_core: Remove now unused code
In reply to Anton VorontsovThe 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.
--- fs/pstore/ram_core.c | 77 -------------------------------------------- include/linux/pstore_ram.h | 19 ----------- 2 files changed, 96 deletions(-)
fs/pstore/ram_core.c +0 -77
@@ -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);
@@ -479,78 +477,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
include/linux/pstore_ram.h +0 -19
@@ -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,15 +48,11 @@ 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);void persistent_ram_zap (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);
Signature · 1 line
1.7.9.2
PATCH v3 14/14 pstore/platform: Remove automatic updates
In reply to Anton VorontsovHaving automatic updates seems pointless, and even dangerous and thus counter-productive:
1. If we can mount pstore, or read files, we can as well read /proc/kmsg. So, there's little point in duplicating the functionality and present the same information but via another userland ABI;
2. Expecting the kernel to behave sanely after oops/panic is naive. It might work, but you'd rather not try it. Screwed up kernel can do rather bad things, like recursive faults[1]; and pstore rather provoking bad things to happen. It uses:
1. Timers (assumes sane interrupts state);
2. Workqueues and mutexes (assumes scheduler in a sane state);
3. kzalloc (a working slab allocator);
That's too much for a dead kernel, so the debugging facility itself might just make debugging harder, which is not what we want.
So, let's remove the automatic updates, this keeps things simple and safe.
(Maybe for non-oops message types it would make sense to add automatic updates, but so far I don't see any use case for this. Even for tracing, it has its own run-time/normal ABI, so we're only interested in pstore upon next boot, to retrieve what has gone wrong with HW or SW.)
[1]
BUG: unable to handle kernel paging request at fffffffffffffff8 IP: [<ffffffff8104801b>] kthread_data+0xb/0x20 [...] Process kworker/0:1 (pid: 14, threadinfo ffff8800072c0000, task ffff88000725b100) [... Call Trace: [<ffffffff81043710>] wq_worker_sleeping+0x10/0xa0 [<ffffffff813687a8>] __schedule+0x568/0x7d0 [<ffffffff8106c24d>] ? trace_hardirqs_on+0xd/0x10 [<ffffffff81087e22>] ? call_rcu_sched+0x12/0x20 [<ffffffff8102b596>] ? release_task+0x156/0x2d0 [<ffffffff8102b45e>] ? release_task+0x1e/0x2d0 [<ffffffff8106c24d>] ? trace_hardirqs_on+0xd/0x10 [<ffffffff81368ac4>] schedule+0x24/0x70 [<ffffffff8102cba8>] do_exit+0x1f8/0x370 [<ffffffff810051e7>] oops_end+0x77/0xb0 [<ffffffff8135c301>] no_context+0x1a6/0x1b5 [<ffffffff8135c4de>] __bad_area_nosemaphore+0x1ce/0x1ed [<ffffffff81053156>] ? ttwu_queue+0xc6/0xe0 [<ffffffff8135c50b>] bad_area_nosemaphore+0xe/0x10 [<ffffffff8101fa47>] do_page_fault+0x2c7/0x450 [<ffffffff8106e34b>] ? __lock_release+0x6b/0xe0 [<ffffffff8106bf21>] ? mark_held_locks+0x61/0x140 [<ffffffff810502fe>] ? __wake_up+0x4e/0x70 [<ffffffff81185f7d>] ? trace_hardirqs_off_thunk+0x3a/0x3c [<ffffffff81158970>] ? pstore_register+0x120/0x120 [<ffffffff8136a37f>] page_fault+0x1f/0x30 [<ffffffff81158970>] ? pstore_register+0x120/0x120 [<ffffffff81185ab8>] ? memcpy+0x68/0x110 [<ffffffff8115875a>] ? pstore_get_records+0x3a/0x130 [<ffffffff811590f4>] ? persistent_ram_copy_old+0x64/0x90 [<ffffffff81158bf4>] ramoops_pstore_read+0x84/0x130 [<ffffffff81158799>] pstore_get_records+0x79/0x130 [<ffffffff81042536>] ? process_one_work+0x116/0x450 [<ffffffff81158970>] ? pstore_register+0x120/0x120 [<ffffffff8115897e>] pstore_dowork+0xe/0x10 [<ffffffff81042594>] process_one_work+0x174/0x450 [<ffffffff81042536>] ? process_one_work+0x116/0x450 [<ffffffff81042e13>] worker_thread+0x123/0x2d0 [<ffffffff81042cf0>] ? manage_workers.isra.28+0x120/0x120 [<ffffffff81047d8e>] kthread+0x8e/0xa0 [<ffffffff8136ba74>] kernel_thread_helper+0x4/0x10 [<ffffffff8136a199>] ? retint_restore_args+0xe/0xe [<ffffffff81047d00>] ? __init_kthread_worker+0x70/0x70 [<ffffffff8136ba70>] ? gs_change+0xb/0xb Code: be e2 00 00 00 48 c7 c7 d1 2a 4e 81 e8 bf fb fd ff 48 8b 5d f0 4c 8b 65 f8 c9 c3 0f 1f 44 00 00 48 8b 87 08 02 00 00 55 48 89 e5 <48> 8b 40 f8 5d c3 66 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 RIP [<ffffffff8104801b>] kthread_data+0xb/0x20 RSP <ffff8800072c1888> CR2: fffffffffffffff8 ---[ end trace 996a332dc399111d ]---
Fixing recursive fault but reboot is needed!
--- fs/pstore/platform.c | 37 ------------------------------------- 1 file changed, 37 deletions(-)
fs/pstore/platform.c +0 -37
@@ -27,30 +27,13 @@#include <linux/module.h> #include <linux/pstore.h> #include <linux/string.h> -#include <linux/timer.h> #include <linux/slab.h> #include <linux/uaccess.h> #include <linux/hardirq.h> -#include <linux/workqueue.h> #include "internal.h" /* -* We defer making "oops" entries appear in pstore - see -* whether the system is actually still running well enough -* to let someone see the entry -*/ -#define PSTORE_INTERVAL (60 * HZ) - -static int pstore_new_entry; - -static void pstore_timefunc (unsigned long ); -static DEFINE_TIMER(pstore_timer, pstore_timefunc, 0, 0); - -static void pstore_dowork (struct work_struct *); -static DECLARE_WORK(pstore_work, pstore_dowork); - -/* * pstore_lock just protects "psinfo" during * calls to pstore_register() */
@@ -140,8 +123,6 @@ static void pstore_dump(struct kmsg_dumper *dumper, ret = psinfo->write (PSTORE_TYPE_DMESG , reason, &id, part, hsize + l1_cpy + l2_cpy, psinfo); -if (ret ==0 && reason ==KMSG_DUMP_OOPS &&pstore_is_mounted ()) - pstore_new_entry =1 ; l1 -= l1_cpy; l2 -= l2_cpy; total += l1_cpy + l2_cpy;
@@ -227,9 +208,6 @@ int pstore_register(struct pstore_info *psi)kmsg_dump_register (&pstore_dumper);pstore_register_console (); - pstore_timer.expires = jiffies +PSTORE_INTERVAL ; -add_timer (&pstore_timer); -return 0 ; }EXPORT_SYMBOL_GPL (pstore_register);
@@ -275,20 +253,5 @@ out: failed, psi->name); } -static void pstore_dowork (struct work_struct *work) -{ -pstore_get_records (1 ); -} - -static void pstore_timefunc (unsigned long dummy) -{ -if (pstore_new_entry) { - pstore_new_entry =0 ; -schedule_work (&pstore_work); - } - -mod_timer (&pstore_timer, jiffies +PSTORE_INTERVAL ); -} -module_param (backend, charp,0444 );MODULE_PARM_DESC (backend,"Pstore backend to use" );
Signature · 1 line
1.7.9.2
Re: [PATCH 14/14] pstore/platform: Remove automatic updates
Quoted 21 lines · Anton Vorontsov
Having automatic updates seems pointless, and even dangerous and thus counter-productive:
1. If we can mount pstore, or read files, we can as well read
/proc/kmsg. So, there's little point in duplicating the functionality and present the same information but via another userland ABI;2. Expecting the kernel to behave sanely after oops/panic is naive. It might work, but you'd rather not try it. Screwed up kernel can do rather bad things, like recursive faults[1]; and pstore rather provoking bad things to happen. It uses:
1. Timers (assumes sane interrupts state);
2. Workqueues and mutexes (assumes scheduler in a sane state);
3. kzalloc (a working slab allocator);That's too much for a dead kernel, so the debugging facility itself might just make debugging harder, which is not what we want.
Quoted 8 lines
So, let's remove the automatic updates, this keeps things simple and safe.
(Maybe for non-oops message types it would make sense to add automatic updates, but so far I don't see any use case for this. Even for tracing, it has its own run-time/normal ABI, so we're only interested in pstore upon next boot, to retrieve what has gone wrong with HW or SW.)
Hrm. This complicates testing a bit. I need more convincing. :)
Systems run with panic_on_oops=0, and plenty of failure paths will just kill "current" instead of bringing the entire system down. I would much rather allow for the possibility to get oopses when they happen than to have to wait a full reboot cycle to "notice" them.
-Kees
Signature · 2 lines
Kees Cook Chrome OS Security
Re: [PATCH 14/14] pstore/platform: Remove automatic updates
On Mon, May 21, 2012 at 12:59:59PM -0700, Kees Cook wrote:
[...]
Quoted 6 lines
Hrm. This complicates testing a bit. I need more convincing. :)
Systems run with panic_on_oops=0, and plenty of failure paths will just kill "current" instead of bringing the entire system down. I would much rather allow for the possibility to get oopses when they happen than to have to wait a full reboot cycle to "notice" them.
Well, as I use qemu/kvm for testing, rebooting is actually faster than waiting for 60 seconds, so I didn't consider this use-case. :-) But yes, I see the point: as pstore's debug function of itself, updates might make sense.
So, you mentioning the panic_on_oops made me think of a kernel command line option, this will also eliminate the 60 seconds hard- coded interval.
But personally I'd still like it disabled by default, otherwise it is possible pstore to screw things because of itself, and that eliminates the point of having it as a reliable debug facility; IMO, it should as much non-intrusive by default as possible, and that's what we would want for production kernels anyway.
I'll replace this patch with another one that will add pstore.update_ms kernel command line option.
Thanks!
Signature · 2 lines
Anton Vorontsov Email: [email protected]