lkml.org 
[lkml]   [2011]   [Nov]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] ramoops: use pstore interface
于 2011/11/17 5:25, Kees Cook 写道:
> Instead of using /dev/mem directly, use the common pstore infrastructure
> to handle Oops gathering and extraction.
>
> Signed-off-by: Kees Cook<keescook@chromium.org>
> ---
> drivers/char/Kconfig | 1 +
> drivers/char/ramoops.c | 195 +++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 152 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 4364303..f166499 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -603,6 +603,7 @@ source "drivers/s390/char/Kconfig"
> config RAMOOPS
> tristate "Log panic/oops to a RAM buffer"
> depends on HAS_IOMEM
> + depends on PSTORE
> default n
> help
> This enables panic and oops messages to be logged to a circular
> diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
> index 7c7f42a..129d79a 100644
> --- a/drivers/char/ramoops.c
> +++ b/drivers/char/ramoops.c
> @@ -2,6 +2,7 @@
> * RAM Oops/Panic logger
> *
> * Copyright (C) 2010 Marco Stornelli<marco.stornelli@gmail.com>
> + * Copyright (C) 2011 Kees Cook<keescook@chromium.org>
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License
> @@ -24,7 +25,7 @@
> #include<linux/kernel.h>
> #include<linux/err.h>
> #include<linux/module.h>
> -#include<linux/kmsg_dump.h>
> +#include<linux/pstore.h>
> #include<linux/time.h>
> #include<linux/err.h>
> #include<linux/io.h>
> @@ -51,67 +52,150 @@ module_param(mem_size, ulong, 0400);
> MODULE_PARM_DESC(mem_size,
> "size of reserved RAM used to store oops/panic logs");
>
> -static int dump_oops = 1;
> -module_param(dump_oops, int, 0600);
> -MODULE_PARM_DESC(dump_oops,
> - "set to 1 to dump oopses, 0 to only dump panics (default 1)");
> -
> -static struct ramoops_context {
> - struct kmsg_dumper dump;
> +static int ramoops_pstore_open(struct pstore_info *psi);
> +static int ramoops_pstore_close(struct pstore_info *psi);
> +static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
> + struct timespec *time,
> + char **buf,
> + struct pstore_info *psi);
> +static int ramoops_pstore_write(enum pstore_type_id type,
> + enum kmsg_dump_reason reason, u64 *id,
> + unsigned int part,
> + size_t size, struct pstore_info *psi);
> +static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,
> + struct pstore_info *psi);
> +
> +struct ramoops_context {
> void *virt_addr;
> phys_addr_t phys_addr;
> unsigned long size;
> - unsigned long record_size;
> - int dump_oops;
> - int count;
> - int max_count;
> -} oops_cxt;
> + size_t record_size;
> + unsigned int count;
> + unsigned int max_count;
> + unsigned int read_count;
> + struct pstore_info pstore;
> +};
>
> static struct platform_device *dummy;
> static struct ramoops_platform_data *dummy_data;
>
> -static void ramoops_do_dump(struct kmsg_dumper *dumper,
> - enum kmsg_dump_reason reason, const char *s1, unsigned long l1,
> - const char *s2, unsigned long l2)
> +static struct ramoops_context oops_cxt = {
> + .pstore = {
> + .owner = THIS_MODULE,
> + .name = "ramoops",
> + .open = ramoops_pstore_open,
> + .close = ramoops_pstore_close,
> + .read = ramoops_pstore_read,
> + .write = ramoops_pstore_write,
> + .erase = ramoops_pstore_erase,
> + },
> +};
> +
> +static int ramoops_pstore_open(struct pstore_info *psi)
> {
> - struct ramoops_context *cxt = container_of(dumper,
> - struct ramoops_context, dump);
> - unsigned long s1_start, s2_start;
> - unsigned long l1_cpy, l2_cpy;
> - int res, hdr_size;
> - char *buf, *buf_orig;
> + struct ramoops_context *cxt =&oops_cxt;
> +
> + cxt->read_count = 0;
> + return 0;
> +}
> +
> +static int ramoops_pstore_close(struct pstore_info *psi)
> +{
> + return 0;
> +}
> +
> +static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
> + struct timespec *time,
> + char **buf,
> + struct pstore_info *psi)
> +{
> + ssize_t size;
> + char *rambuf;
> + struct ramoops_context *cxt =&oops_cxt;
> +
> + if (cxt->read_count>= cxt->max_count)
> + return -EINVAL;
> + *id = cxt->read_count++;
> + /* Only supports dmesg output so far. */
> + *type = PSTORE_TYPE_DMESG;
> + /* TODO(kees): Bogus time for the moment. */
> + time->tv_sec = 0;
> + time->tv_nsec = 0;
> +
> + rambuf = cxt->virt_addr + (*id * cxt->record_size);
> + size = strnlen(rambuf, cxt->record_size);
> + *buf = kmalloc(size, GFP_KERNEL);
> + if (*buf == NULL)
> + return -ENOMEM;
> + memcpy(*buf, rambuf, size);
> +
> + return size;
> +}
> +
> +static int ramoops_pstore_write(enum pstore_type_id type,
> + enum kmsg_dump_reason reason,
> + u64 *id,
> + unsigned int part,
> + size_t size, struct pstore_info *psi)
> +{
> + char *buf;
> + size_t res;
> struct timeval timestamp;
> + struct ramoops_context *cxt =&oops_cxt;
> + size_t available = cxt->record_size;
>
> + /* Only store dmesg dumps. */
> + if (type != PSTORE_TYPE_DMESG)
> + return -EINVAL;
> +
> + /* Only store crash dumps. */
> if (reason != KMSG_DUMP_OOPS&&
> reason != KMSG_DUMP_PANIC&&
> reason != KMSG_DUMP_KEXEC)
> - return;
> + return -EINVAL;
>
> - /* Only dump oopses if dump_oops is set */
> - if (reason == KMSG_DUMP_OOPS&& !cxt->dump_oops)
> - return;
> + /* Explicitly only take the first part of any new crash.
> + * If our buffer is larger than kmsg_bytes, this can never happen,
> + * and if our buffer is smaller than kmsg_bytes, we don't want the
> + * report split across multiple records. */
> + if (part != 1)
> + return -ENOSPC;

why only one part is accepted? You are afraid about your filename style?

>
> buf = cxt->virt_addr + (cxt->count * cxt->record_size);
> - buf_orig = buf;
>
> - memset(buf, '\0', cxt->record_size);
> res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
> buf += res;
> + available -= res;
> +
> do_gettimeofday(&timestamp);
> res = sprintf(buf, "%lu.%lu\n", (long)timestamp.tv_sec, (long)timestamp.tv_usec);
> buf += res;
> + available -= res;
>
> - hdr_size = buf - buf_orig;
> - l2_cpy = min(l2, cxt->record_size - hdr_size);
> - l1_cpy = min(l1, cxt->record_size - hdr_size - l2_cpy);
> + if (size> available)
> + size = available;
>
> - s2_start = l2 - l2_cpy;
> - s1_start = l1 - l1_cpy;
> -
> - memcpy(buf, s1 + s1_start, l1_cpy);
> - memcpy(buf + l1_cpy, s2 + s2_start, l2_cpy);
> + memcpy(buf, cxt->pstore.buf, size);
> + memset(buf + size, '\0', available - size);
>
> cxt->count = (cxt->count + 1) % cxt->max_count;
> +
> + return 0;
> +}
> +
> +static int ramoops_pstore_erase(enum pstore_type_id type, u64 id,
> + struct pstore_info *psi)
> +{
> + char *buf;
> + struct ramoops_context *cxt =&oops_cxt;
> +
> + if (id>= cxt->max_count)
> + return -EINVAL;
> +
> + buf = cxt->virt_addr + (id * cxt->record_size);
> + memset(buf, '\0', cxt->record_size);
> +
> + return 0;
> }
>
> static int __init ramoops_probe(struct platform_device *pdev)
> @@ -120,6 +204,12 @@ static int __init ramoops_probe(struct platform_device *pdev)
> struct ramoops_context *cxt =&oops_cxt;
> int err = -EINVAL;
>
> + /* Only a single ramoops area allowed at a time, so fail extra
> + * probes.
> + */
> + if (cxt->max_count)
> + goto fail3;

Should be fail4

> +
> if (!pdata->mem_size || !pdata->record_size) {
> pr_err("The memory size and the record size must be "
> "non-zero\n");
> @@ -147,7 +237,6 @@ static int __init ramoops_probe(struct platform_device *pdev)
> cxt->size = pdata->mem_size;
> cxt->phys_addr = pdata->mem_address;
> cxt->record_size = pdata->record_size;
> - cxt->dump_oops = pdata->dump_oops;
> /*
> * Update the module parameter variables as well so they are visible
> * through /sys/module/ramoops/parameters/
> @@ -155,7 +244,14 @@ static int __init ramoops_probe(struct platform_device *pdev)
> mem_size = pdata->mem_size;
> mem_address = pdata->mem_address;
> record_size = pdata->record_size;
> - dump_oops = pdata->dump_oops;
> +
> + cxt->pstore.bufsize = cxt->record_size;
> + cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
> + spin_lock_init(&cxt->pstore.buf_lock);
> + if (!cxt->pstore.buf) {
> + pr_err("cannot allocate pstore buffer\n");
> + goto fail4;
> + }
>
> if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
> pr_err("request mem region failed\n");
> @@ -169,10 +265,9 @@ static int __init ramoops_probe(struct platform_device *pdev)
> goto fail2;
> }
>
> - cxt->dump.dump = ramoops_do_dump;
> - err = kmsg_dump_register(&cxt->dump);
> + err = pstore_register(&cxt->pstore);
> if (err) {
> - pr_err("registering kmsg dumper failed\n");
> + pr_err("registering with pstore failed\n");
> goto fail1;
> }
>
> @@ -182,7 +277,11 @@ fail1:
> iounmap(cxt->virt_addr);
> fail2:
> release_mem_region(cxt->phys_addr, cxt->size);
> + cxt->max_count = 0;
> fail3:
> + kfree(cxt->pstore.buf);
> +fail4:
> + cxt->pstore.bufsize = 0;

In some situations fail4 maybe hits max_count != 0, so here max_count should be
cleared. I think you should rearrange the logic in this function carefully.

> return err;
> }
>
> @@ -190,11 +289,20 @@ static int __exit ramoops_remove(struct platform_device *pdev)
> {
> struct ramoops_context *cxt =&oops_cxt;
>
> - if (kmsg_dump_unregister(&cxt->dump)< 0)
> - pr_warn("could not unregister kmsg_dumper\n");
> + /* TODO(kees): It shouldn't be possible to remove ramoops since
> + * pstore doesn't support unregistering yet. When it does, remove
> + * this early return and add the unregister where noted below.
> + */
> + return -EBUSY;

This style is not reasonable. Maybe it should have a better wrap.

>
> iounmap(cxt->virt_addr);
> release_mem_region(cxt->phys_addr, cxt->size);
> + cxt->max_count = 0;
> +
> + /* TODO(kees): When pstore supports unregistering, call it here. */
> + kfree(cxt->pstore.buf);
> + cxt->pstore.bufsize = 0;
> +
> return 0;
> }
>
> @@ -223,7 +331,6 @@ 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->dump_oops = dump_oops;
> dummy = platform_create_bundle(&ramoops_driver, ramoops_probe,
> NULL, 0, dummy_data,
> sizeof(struct ramoops_platform_data));

BTW, you need to update Documentation/ramoops.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2011-11-17 06:37    [W:0.059 / U:0.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site