[PATCH v2] ramoops: use pstore interface

[PATCH v2] ramoops: use pstore interface

[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>
---
This depends on the pstore changes waiting for -next in:
http://git.kernel.org/?p=linux/kernel/git/aegl/linux.git;a=shortlog;h=refs/heads/next
---
 Documentation/ramoops.txt |    8 +-
 drivers/char/Kconfig      |    1 +
 drivers/char/ramoops.c    |  206 ++++++++++++++++++++++++++++++++++-----------
 3 files changed, 160 insertions(+), 55 deletions(-)

diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
index 8fb1ba7..a0b9d8e 100644
--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
 
 Sergiu Iordache <sergiu@chromium.org>
 
-Updated: 8 August 2011
+Updated: 17 November 2011
 
 0. Introduction
 
@@ -71,6 +71,6 @@ timestamp and a new line. The dump then continues with the actual data.
 
 4. Reading the data
 
-The dump data can be read from memory (through /dev/mem or other means).
-Getting the module parameters, which are needed in order to parse the data, can
-be done through /sys/module/ramoops/parameters/* .
+The dump data can be read from the pstore filesystem. The format for these
+files is "dmesg-ramoops-N", where N is the record number in memory. To delete
+a stored record from RAM, simply unlink the respective pstore file.
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..c544213 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>
@@ -56,74 +57,154 @@ 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;
+struct ramoops_context {
 	void *virt_addr;
 	phys_addr_t phys_addr;
 	unsigned long size;
-	unsigned long record_size;
+	size_t record_size;
 	int dump_oops;
-	int count;
-	int max_count;
-} oops_cxt;
+	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 int ramoops_pstore_open(struct pstore_info *psi)
+{
+	struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
+
+	cxt->read_count = 0;
+	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 = (struct ramoops_context *)psi->data;
+
+	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)
 {
-	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;
+	char *buf;
+	size_t res;
 	struct timeval timestamp;
+	struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
+	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;
+		return -EINVAL;
+
+	/* 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;
 
 	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);
-
-	s2_start = l2 - l2_cpy;
-	s1_start = l1 - l1_cpy;
+	if (size > available)
+		size = available;
 
-	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 = (struct ramoops_context *)psi->data;
+
+	if (id >= cxt->max_count)
+		return -EINVAL;
+
+	buf = cxt->virt_addr + (id * cxt->record_size);
+	memset(buf, '\0', cxt->record_size);
+
+	return 0;
+}
+
+static struct ramoops_context oops_cxt = {
+	.pstore = {
+		.owner	= THIS_MODULE,
+		.name	= "ramoops",
+		.open	= ramoops_pstore_open,
+		.read	= ramoops_pstore_read,
+		.write	= ramoops_pstore_write,
+		.erase	= ramoops_pstore_erase,
+	},
+};
+
 static int __init ramoops_probe(struct platform_device *pdev)
 {
 	struct ramoops_platform_data *pdata = pdev->dev.platform_data;
 	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 fail_out;
+
 	if (!pdata->mem_size || !pdata->record_size) {
 		pr_err("The memory size and the record size must be "
 			"non-zero\n");
-		goto fail3;
+		goto fail_out;
 	}
 
 	rounddown_pow_of_two(pdata->mem_size);
@@ -133,13 +214,13 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	if (pdata->mem_size < MIN_MEM_SIZE &&
 			pdata->record_size < MIN_MEM_SIZE) {
 		pr_err("memory size too small, minium is %lu\n", MIN_MEM_SIZE);
-		goto fail3;
+		goto fail_out;
 	}
 
 	if (pdata->mem_size < pdata->record_size) {
 		pr_err("The memory size must be larger than the "
 			"records size\n");
-		goto fail3;
+		goto fail_out;
 	}
 
 	cxt->max_count = pdata->mem_size / pdata->record_size;
@@ -148,41 +229,55 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	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/
-	 */
-	mem_size = pdata->mem_size;
-	mem_address = pdata->mem_address;
-	record_size = pdata->record_size;
-	dump_oops = pdata->dump_oops;
+
+	cxt->pstore.data = cxt;
+	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 fail_clear;
+	}
 
 	if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
 		pr_err("request mem region failed\n");
 		err = -EINVAL;
-		goto fail3;
+		goto fail_buf;
 	}
 
 	cxt->virt_addr = ioremap(cxt->phys_addr,  cxt->size);
 	if (!cxt->virt_addr) {
 		pr_err("ioremap failed\n");
-		goto fail2;
+		goto fail_mem_region;
 	}
 
-	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");
-		goto fail1;
+		pr_err("registering with pstore failed\n");
+		goto fail_iounmap;
 	}
 
+	/*
+	 * Update the module parameter variables as well so they are visible
+	 * through /sys/module/ramoops/parameters/
+	 */
+	mem_size = pdata->mem_size;
+	mem_address = pdata->mem_address;
+	record_size = pdata->record_size;
+	dump_oops = pdata->dump_oops;
+
 	return 0;
 
-fail1:
+fail_iounmap:
 	iounmap(cxt->virt_addr);
-fail2:
+fail_mem_region:
 	release_mem_region(cxt->phys_addr, cxt->size);
-fail3:
+fail_buf:
+	kfree(cxt->pstore.buf);
+fail_clear:
+	cxt->pstore.bufsize = 0;
+	cxt->max_count = 0;
+fail_out:
 	return err;
 }
 
@@ -190,12 +285,21 @@ 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");
-
+#if 0
+	/* TODO(kees): We cannot unload ramoops since pstore doesn't support
+	 * unregistering yet.
+	 */
 	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;
+#endif
+	return -EBUSY;
 }
 
 static struct platform_driver ramoops_driver = {
-- 
1.7.0.4

Re: [PATCH v2] ramoops: use pstore interface

[Chen Gong]

于 2011/11/29 4:09, 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>
> ---
> This depends on the pstore changes waiting for -next in:
> http://git.kernel.org/?p=linux/kernel/git/aegl/linux.git;a=shortlog;h=refs/heads/next
> ---
>   Documentation/ramoops.txt |    8 +-
>   drivers/char/Kconfig      |    1 +
>   drivers/char/ramoops.c    |  206 ++++++++++++++++++++++++++++++++++-----------
>   3 files changed, 160 insertions(+), 55 deletions(-)
>
> diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
> index 8fb1ba7..a0b9d8e 100644
> --- a/Documentation/ramoops.txt
> +++ b/Documentation/ramoops.txt
> @@ -3,7 +3,7 @@ Ramoops oops/panic logger
>
>   Sergiu Iordache<sergiu@chromium.org>
>
> -Updated: 8 August 2011
> +Updated: 17 November 2011
>
>   0. Introduction
>
> @@ -71,6 +71,6 @@ timestamp and a new line. The dump then continues with the actual data.
>
>   4. Reading the data
>
> -The dump data can be read from memory (through /dev/mem or other means).
> -Getting the module parameters, which are needed in order to parse the data, can
> -be done through /sys/module/ramoops/parameters/* .
> +The dump data can be read from the pstore filesystem. The format for these
> +files is "dmesg-ramoops-N", where N is the record number in memory. To delete
> +a stored record from RAM, simply unlink the respective pstore file.

I think the definition of "mem_address" in the doc is not very clear. It is not 
a normal memory instead of a persistent RAM. I suggest adding more descriptions.
It's better if there is a real example.

> 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..c544213 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>
> @@ -56,74 +57,154 @@ 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;
> +struct ramoops_context {
>   	void *virt_addr;
>   	phys_addr_t phys_addr;
>   	unsigned long size;
> -	unsigned long record_size;
> +	size_t record_size;
>   	int dump_oops;
> -	int count;
> -	int max_count;
> -} oops_cxt;
> +	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 int ramoops_pstore_open(struct pstore_info *psi)
> +{
> +	struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
> +
> +	cxt->read_count = 0;
> +	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 = (struct ramoops_context *)psi->data;
> +
> +	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)
>   {
> -	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;
> +	char *buf;
> +	size_t res;
>   	struct timeval timestamp;
> +	struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
> +	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)

As Andrew mentioned here (http://lkml.org/lkml/2011/11/28/495),
who should take this responsibility to fix this issue when conflicting

Re: [PATCH v2] ramoops: use pstore interface

[Kees Cook]

On Mon, Nov 28, 2011 at 8:50 PM, Chen Gong <gong.chen@linux.intel.com> wrote:
> 于 2011/11/29 4:09, 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>
>> ---
>> This depends on the pstore changes waiting for -next in:
>>
>> http://git.kernel.org/?p=linux/kernel/git/aegl/linux.git;a=shortlog;h=refs/heads/next
>> ---
>>  Documentation/ramoops.txt |    8 +-
>>  drivers/char/Kconfig      |    1 +
>>  drivers/char/ramoops.c    |  206
>> ++++++++++++++++++++++++++++++++++-----------
>>  3 files changed, 160 insertions(+), 55 deletions(-)
>>
>> diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
>> index 8fb1ba7..a0b9d8e 100644
>> --- a/Documentation/ramoops.txt
>> +++ b/Documentation/ramoops.txt
>> @@ -3,7 +3,7 @@ Ramoops oops/panic logger
>>
>>  Sergiu Iordache<sergiu@chromium.org>
>>
>> -Updated: 8 August 2011
>> +Updated: 17 November 2011
>>
>>  0. Introduction
>>
>> @@ -71,6 +71,6 @@ timestamp and a new line. The dump then continues with
>> the actual data.
>>
>>  4. Reading the data
>>
>> -The dump data can be read from memory (through /dev/mem or other means).
>> -Getting the module parameters, which are needed in order to parse the
>> data, can
>> -be done through /sys/module/ramoops/parameters/* .
>> +The dump data can be read from the pstore filesystem. The format for
>> these
>> +files is "dmesg-ramoops-N", where N is the record number in memory. To
>> delete
>> +a stored record from RAM, simply unlink the respective pstore file.
>
> I think the definition of "mem_address" in the doc is not very clear. It is
> not a normal memory instead of a persistent RAM. I suggest adding more
> descriptions.
> It's better if there is a real example.

Okay. I'm not sure it's in the scope of this patch, but I can try.

Marco, do you have suggestions for how this could be enhanced?

>...
>> +       /* Only store crash dumps. */
>>        if (reason != KMSG_DUMP_OOPS&&
>>        reason != KMSG_DUMP_PANIC&&
>>        reason != KMSG_DUMP_KEXEC)
>
> As Andrew mentioned here (http://lkml.org/lkml/2011/11/28/495),
> who should take this responsibility to fix this issue when conflicting

Has the "remove KMSG_DUMP_KEXEC" patch been taken? I'm happy to drop
it from the ramoops patch. It's trivial to do so.

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH v2] ramoops: use pstore interface

[Marco Stornelli]

Il 29/11/2011 18:24, Kees Cook ha scritto:
> On Mon, Nov 28, 2011 at 8:50 PM, Chen Gong<gong.chen@linux.intel.com>  wrote:
>> 于 2011/11/29 4:09, 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>
>>> ---
>>> This depends on the pstore changes waiting for -next in:
>>>
>>> http://git.kernel.org/?p=linux/kernel/git/aegl/linux.git;a=shortlog;h=refs/heads/next
>>> ---
>>>   Documentation/ramoops.txt |    8 +-
>>>   drivers/char/Kconfig      |    1 +
>>>   drivers/char/ramoops.c    |  206
>>> ++++++++++++++++++++++++++++++++++-----------
>>>   3 files changed, 160 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
>>> index 8fb1ba7..a0b9d8e 100644
>>> --- a/Documentation/ramoops.txt
>>> +++ b/Documentation/ramoops.txt
>>> @@ -3,7 +3,7 @@ Ramoops oops/panic logger
>>>
>>>   Sergiu Iordache<sergiu@chromium.org>
>>>
>>> -Updated: 8 August 2011
>>> +Updated: 17 November 2011
>>>
>>>   0. Introduction
>>>
>>> @@ -71,6 +71,6 @@ timestamp and a new line. The dump then continues with
>>> the actual data.
>>>
>>>   4. Reading the data
>>>
>>> -The dump data can be read from memory (through /dev/mem or other means).
>>> -Getting the module parameters, which are needed in order to parse the
>>> data, can
>>> -be done through /sys/module/ramoops/parameters/* .
>>> +The dump data can be read from the pstore filesystem. The format for
>>> these
>>> +files is "dmesg-ramoops-N", where N is the record number in memory. To
>>> delete
>>> +a stored record from RAM, simply unlink the respective pstore file.
>>
>> I think the definition of "mem_address" in the doc is not very clear. It is
>> not a normal memory instead of a persistent RAM. I suggest adding more
>> descriptions.
>> It's better if there is a real example.
>
> Okay. I'm not sure it's in the scope of this patch, but I can try.
>
> Marco, do you have suggestions for how this could be enhanced?
>

I don't know actually. It's not mandatory use a persistent memory. A 
simple piece of reserved RAM is ok. Obviously it will work only over 
reboot and not over power down. I define mem_address as a generic piece 
of reserved memory.

Marco

Re: [PATCH v2] ramoops: use pstore interface

[Chen Gong]

于 2011/12/1 18:31, Marco Stornelli 写道:
> Il 29/11/2011 18:24, Kees Cook ha scritto:
>> On Mon, Nov 28, 2011 at 8:50 PM, Chen Gong<gong.chen@linux.intel.com> wrote:
>>> 于 2011/11/29 4:09, 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>
>>>> ---
>>>> This depends on the pstore changes waiting for -next in:
>>>>
>>>> http://git.kernel.org/?p=linux/kernel/git/aegl/linux.git;a=shortlog;h=refs/heads/next
>>>>
>>>> ---
>>>> Documentation/ramoops.txt | 8 +-
>>>> drivers/char/Kconfig | 1 +
>>>> drivers/char/ramoops.c | 206
>>>> ++++++++++++++++++++++++++++++++++-----------
>>>> 3 files changed, 160 insertions(+), 55 deletions(-)
>>>>
>>>> diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
>>>> index 8fb1ba7..a0b9d8e 100644
>>>> --- a/Documentation/ramoops.txt
>>>> +++ b/Documentation/ramoops.txt
>>>> @@ -3,7 +3,7 @@ Ramoops oops/panic logger
>>>>
>>>> Sergiu Iordache<sergiu@chromium.org>
>>>>
>>>> -Updated: 8 August 2011
>>>> +Updated: 17 November 2011
>>>>
>>>> 0. Introduction
>>>>
>>>> @@ -71,6 +71,6 @@ timestamp and a new line. The dump then continues with
>>>> the actual data.
>>>>
>>>> 4. Reading the data
>>>>
>>>> -The dump data can be read from memory (through /dev/mem or other means).
>>>> -Getting the module parameters, which are needed in order to parse the
>>>> data, can
>>>> -be done through /sys/module/ramoops/parameters/* .
>>>> +The dump data can be read from the pstore filesystem. The format for
>>>> these
>>>> +files is "dmesg-ramoops-N", where N is the record number in memory. To
>>>> delete
>>>> +a stored record from RAM, simply unlink the respective pstore file.
>>>
>>> I think the definition of "mem_address" in the doc is not very clear. It is
>>> not a normal memory instead of a persistent RAM. I suggest adding more
>>> descriptions.
>>> It's better if there is a real example.
>>
>> Okay. I'm not sure it's in the scope of this patch, but I can try.
>>
>> Marco, do you have suggestions for how this could be enhanced?
>>
>
> I don't know actually. It's not mandatory use a persistent memory. A simple
> piece of reserved RAM is ok. Obviously it will work only over reboot and not
> over power down. I define mem_address as a generic piece of reserved memory.
>
> Marco

Anyway, we need a pratical exmaple to instruct us how to use this diver.

Re: [PATCH v2] ramoops: use pstore interface

[Marco Stornelli]

Il 02/12/2011 03:40, Chen Gong ha scritto:
> 于 2011/12/1 18:31, Marco Stornelli 写道:
>> Il 29/11/2011 18:24, Kees Cook ha scritto:
>>> On Mon, Nov 28, 2011 at 8:50 PM, Chen Gong<gong.chen@linux.intel.com>
>>> wrote:
>>>> 于 2011/11/29 4:09, 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>
>>>>> ---
>>>>> This depends on the pstore changes waiting for -next in:
>>>>>
>>>>> http://git.kernel.org/?p=linux/kernel/git/aegl/linux.git;a=shortlog;h=refs/heads/next
>>>>>
>>>>>
>>>>> ---
>>>>> Documentation/ramoops.txt | 8 +-
>>>>> drivers/char/Kconfig | 1 +
>>>>> drivers/char/ramoops.c | 206
>>>>> ++++++++++++++++++++++++++++++++++-----------
>>>>> 3 files changed, 160 insertions(+), 55 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
>>>>> index 8fb1ba7..a0b9d8e 100644
>>>>> --- a/Documentation/ramoops.txt
>>>>> +++ b/Documentation/ramoops.txt
>>>>> @@ -3,7 +3,7 @@ Ramoops oops/panic logger
>>>>>
>>>>> Sergiu Iordache<sergiu@chromium.org>
>>>>>
>>>>> -Updated: 8 August 2011
>>>>> +Updated: 17 November 2011
>>>>>
>>>>> 0. Introduction
>>>>>
>>>>> @@ -71,6 +71,6 @@ timestamp and a new line. The dump then continues
>>>>> with
>>>>> the actual data.
>>>>>
>>>>> 4. Reading the data
>>>>>
>>>>> -The dump data can be read from memory (through /dev/mem or other
>>>>> means).
>>>>> -Getting the module parameters, which are needed in order to parse the
>>>>> data, can
>>>>> -be done through /sys/module/ramoops/parameters/* .
>>>>> +The dump data can be read from the pstore filesystem. The format for
>>>>> these
>>>>> +files is "dmesg-ramoops-N", where N is the record number in
>>>>> memory. To
>>>>> delete
>>>>> +a stored record from RAM, simply unlink the respective pstore file.
>>>>
>>>> I think the definition of "mem_address" in the doc is not very
>>>> clear. It is
>>>> not a normal memory instead of a persistent RAM. I suggest adding more
>>>> descriptions.
>>>> It's better if there is a real example.
>>>
>>> Okay. I'm not sure it's in the scope of this patch, but I can try.
>>>
>>> Marco, do you have suggestions for how this could be enhanced?
>>>
>>
>> I don't know actually. It's not mandatory use a persistent memory. A
>> simple
>> piece of reserved RAM is ok. Obviously it will work only over reboot
>> and not
>> over power down. I define mem_address as a generic piece of reserved
>> memory.
>>
>> Marco
>
> Anyway, we need a pratical exmaple to instruct us how to use this diver.
>
>

For example we can use the mem parameter to reserve memory and use it as 
ramoops buffer, very simple.

Re: [PATCH v2] ramoops: use pstore interface

[Kees Cook]

On Fri, Dec 2, 2011 at 12:20 AM, Marco Stornelli
<marco.stornelli@gmail.com> wrote:
> Il 02/12/2011 03:40, Chen Gong ha scritto:
>
>> 于 2011/12/1 18:31, Marco Stornelli 写道:
>>>
>>> Il 29/11/2011 18:24, Kees Cook ha scritto:
>>>>
>>>> On Mon, Nov 28, 2011 at 8:50 PM, Chen Gong<gong.chen@linux.intel.com>
>>>> wrote:
>>>>>
>>>>> 于 2011/11/29 4:09, 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>
>>>>>> ---
>>>>>> This depends on the pstore changes waiting for -next in:
>>>>>>
>>>>>>
>>>>>> http://git.kernel.org/?p=linux/kernel/git/aegl/linux.git;a=shortlog;h=refs/heads/next
>>>>>>
>>>>>>
>>>>>> ---
>>>>>> Documentation/ramoops.txt | 8 +-
>>>>>> drivers/char/Kconfig | 1 +
>>>>>> drivers/char/ramoops.c | 206
>>>>>> ++++++++++++++++++++++++++++++++++-----------
>>>>>> 3 files changed, 160 insertions(+), 55 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
>>>>>> index 8fb1ba7..a0b9d8e 100644
>>>>>> --- a/Documentation/ramoops.txt
>>>>>> +++ b/Documentation/ramoops.txt
>>>>>> @@ -3,7 +3,7 @@ Ramoops oops/panic logger
>>>>>>
>>>>>> Sergiu Iordache<sergiu@chromium.org>
>>>>>>
>>>>>> -Updated: 8 August 2011
>>>>>> +Updated: 17 November 2011
>>>>>>
>>>>>> 0. Introduction
>>>>>>
>>>>>> @@ -71,6 +71,6 @@ timestamp and a new line. The dump then continues
>>>>>> with
>>>>>> the actual data.
>>>>>>
>>>>>> 4. Reading the data
>>>>>>
>>>>>> -The dump data can be read from memory (through /dev/mem or other
>>>>>> means).
>>>>>> -Getting the module parameters, which are needed in order to parse the
>>>>>> data, can
>>>>>> -be done through /sys/module/ramoops/parameters/* .
>>>>>> +The dump data can be read from the pstore filesystem. The format for
>>>>>> these
>>>>>> +files is "dmesg-ramoops-N", where N is the record number in
>>>>>> memory. To
>>>>>> delete
>>>>>> +a stored record from RAM, simply unlink the respective pstore file.
>>>>>
>>>>>
>>>>> I think the definition of "mem_address" in the doc is not very
>>>>> clear. It is
>>>>> not a normal memory instead of a persistent RAM. I suggest adding more
>>>>> descriptions.
>>>>> It's better if there is a real example.
>>>>
>>>> Okay. I'm not sure it's in the scope of this patch, but I can try.
>>>>
>>>> Marco, do you have suggestions for how this could be enhanced?
>>>
>>> I don't know actually. It's not mandatory use a persistent memory. A
>>> simple
>>> piece of reserved RAM is ok. Obviously it will work only over reboot
>>> and not
>>> over power down. I define mem_address as a generic piece of reserved
>>> memory.
>>
>> Anyway, we need a pratical exmaple to instruct us how to use this diver.
>
> For example we can use the mem parameter to reserve memory and use it as
> ramoops buffer, very simple.

I tried both "mem" and "memmap":

mem=0x3f000000 ramoops.mem_address=0x3f000000 ramoops.mem_size=0x40000
ramoops.record_size=0x10000

memmap=256K$0x3f000000 ramoops.mem_address=0x3f000000
ramoops.mem_size=0x40000 ramoops.record_size=0x10000

Neither works for me (I end up triggering a panic via BUG in kfree). I
wonder if it's some bad interaction between the cmdline and the memory
tables? When I boot with mem=0x3f000000 and without ramoops, I see in
/proc/iomem:

00100000-3effffff : System RAM
  01000000-0137c35f : Kernel code
  0137c360-0151e85f : Kernel data
  01591000-01627fff : Kernel bss
3f000000-3fffcfff : RAM buffer

It seems like the system isn't ignoring the region? What's the right
way to do this?

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH v2] ramoops: use pstore interface

[Marco Stornelli]

Il 02/12/2011 20:19, Kees Cook ha scritto:
> On Fri, Dec 2, 2011 at 12:20 AM, Marco Stornelli
> <marco.stornelli@gmail.com>  wrote:
>> Il 02/12/2011 03:40, Chen Gong ha scritto:
>>
>>> 于 2011/12/1 18:31, Marco Stornelli 写道:
>>>>
>>>> Il 29/11/2011 18:24, Kees Cook ha scritto:
>>>>>
>>>>> On Mon, Nov 28, 2011 at 8:50 PM, Chen Gong<gong.chen@linux.intel.com>
>>>>> wrote:
>>>>>>
>>>>>> 于 2011/11/29 4:09, 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>
>>>>>>> ---
>>>>>>> This depends on the pstore changes waiting for -next in:
>>>>>>>
>>>>>>>
>>>>>>> http://git.kernel.org/?p=linux/kernel/git/aegl/linux.git;a=shortlog;h=refs/heads/next
>>>>>>>
>>>>>>>
>>>>>>> ---
>>>>>>> Documentation/ramoops.txt | 8 +-
>>>>>>> drivers/char/Kconfig | 1 +
>>>>>>> drivers/char/ramoops.c | 206
>>>>>>> ++++++++++++++++++++++++++++++++++-----------
>>>>>>> 3 files changed, 160 insertions(+), 55 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
>>>>>>> index 8fb1ba7..a0b9d8e 100644
>>>>>>> --- a/Documentation/ramoops.txt
>>>>>>> +++ b/Documentation/ramoops.txt
>>>>>>> @@ -3,7 +3,7 @@ Ramoops oops/panic logger
>>>>>>>
>>>>>>> Sergiu Iordache<sergiu@chromium.org>
>>>>>>>
>>>>>>> -Updated: 8 August 2011
>>>>>>> +Updated: 17 November 2011
>>>>>>>
>>>>>>> 0. Introduction
>>>>>>>
>>>>>>> @@ -71,6 +71,6 @@ timestamp and a new line. The dump then continues
>>>>>>> with
>>>>>>> the actual data.
>>>>>>>
>>>>>>> 4. Reading the data
>>>>>>>
>>>>>>> -The dump data can be read from memory (through /dev/mem or other
>>>>>>> means).
>>>>>>> -Getting the module parameters, which are needed in order to parse the
>>>>>>> data, can
>>>>>>> -be done through /sys/module/ramoops/parameters/* .
>>>>>>> +The dump data can be read from the pstore filesystem. The format for
>>>>>>> these
>>>>>>> +files is "dmesg-ramoops-N", where N is the record number in
>>>>>>> memory. To
>>>>>>> delete
>>>>>>> +a stored record from RAM, simply unlink the respective pstore file.
>>>>>>
>>>>>>
>>>>>> I think the definition of "mem_address" in the doc is not very
>>>>>> clear. It is
>>>>>> not a normal memory instead of a persistent RAM. I suggest adding more
>>>>>> descriptions.
>>>>>> It's better if there is a real example.
>>>>>
>>>>> Okay. I'm not sure it's in the scope of this patch, but I can try.
>>>>>
>>>>> Marco, do you have suggestions for how this could be enhanced?
>>>>
>>>> I don't know actually. It's not mandatory use a persistent memory. A
>>>> simple
>>>> piece of reserved RAM is ok. Obviously it will work only over reboot
>>>> and not
>>>> over power down. I define mem_address as a generic piece of reserved
>>>> memory.
>>>
>>> Anyway, we need a pratical exmaple to instruct us how to use this diver.
>>
>> For example we can use the mem parameter to reserve memory and use it as
>> ramoops buffer, very simple.
> 
> I tried both "mem" and "memmap":
> 
> mem=0x3f000000 ramoops.mem_address=0x3f000000 ramoops.mem_size=0x40000
> ramoops.record_size=0x10000
> 
> memmap=256K$0x3f000000 ramoops.mem_address=0x3f000000
> ramoops.mem_size=0x40000 ramoops.record_size=0x10000
> 
> Neither works for me (I end up triggering a panic via BUG in kfree). I
> wonder if it's some bad interaction between the cmdline and the memory
> tables? When I boot with mem=0x3f000000 and without ramoops, I see in
> /proc/iomem:
> 
> 00100000-3effffff : System RAM
>    01000000-0137c35f : Kernel code
>    0137c360-0151e85f : Kernel data
>    01591000-01627fff : Kernel bss
> 3f000000-3fffcfff : RAM buffer
> 
> It seems like the system isn't ignoring the region? What's the right
> way to do this?
> 
> -Kees
> 

I don't know, however in that situation it's normal to have a crash, if
you use the RAM buffer region, you should see "reserved" in iomem. It
can be an other kind of problem.

Marco