[PATCH v2 0/3] char drivers: ramoops improvements

[PATCH v2 0/3] char drivers: ramoops improvements

[Sergiu Iordache]

Improves the ramoops module by adding a dump_oops to the platform data, 
adding a record_size parameter and adding a debugfs for memory area access.

The patch was built on the 2.6.38 kernel and is based on the following
patches which were applied from the mmotm tree:
ramoops-add-new-line-to-each-print
ramoops-use-module-parameters-instead-of-platform-data-if-not-available
ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes

This series is different from the initial by being built on the ramoops 
modifications present in the mmotm tree and by adding the debugfs patch along
with minor improvements.

The changes were tested on a CR-48 machine using the 2.6.38 kernel.

Sergiu Iordache (3):
  char drivers: ramoops dump_oops platform data
  char drivers: ramoops record_size module parameter
  char drivers: ramoops debugfs entry

 drivers/char/ramoops.c  |  139 ++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/ramoops.h |    2 +
 2 files changed, 133 insertions(+), 8 deletions(-)

-- 
1.7.3.1

[PATCH v2 1/3] char drivers: ramoops dump_oops platform data

[Sergiu Iordache]

The platform driver currently allows setting the mem_size and mem_address.
Since dump_oops is also a module parameter it would be more consistent
if it could be set through platform data as well.

Change-Id: I27e541a51c9722047c4163bf408e778caa77ecc9
Signed-off-by: Sergiu Iordache <sergiu@chromium.org>
---
The patch was built on the 2.6.38 kernel and is based on the following
patches which were applied from the mmotm tree:
ramoops-add-new-line-to-each-print
ramoops-use-module-parameters-instead-of-platform-data-if-not-available
ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes

 drivers/char/ramoops.c  |    5 ++++-
 include/linux/ramoops.h |    1 +
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index c9e1028..5349d94 100644
--- a/drivers/char/ramoops.c
+++ b/drivers/char/ramoops.c
@@ -55,6 +55,7 @@ static struct ramoops_context {
 	void *virt_addr;
 	phys_addr_t phys_addr;
 	unsigned long size;
+	int dump_oops;
 	int count;
 	int max_count;
 } oops_cxt;
@@ -80,7 +81,7 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
 		return;
 
 	/* Only dump oopses if dump_oops is set */
-	if (reason == KMSG_DUMP_OOPS && !dump_oops)
+	if (reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
 		return;
 
 	buf = cxt->virt_addr + (cxt->count * RECORD_SIZE);
@@ -128,6 +129,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	cxt->count = 0;
 	cxt->size = pdata->mem_size;
 	cxt->phys_addr = pdata->mem_address;
+	cxt->dump_oops = pdata->dump_oops;
 
 	if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
 		pr_err("request mem region failed\n");
@@ -194,6 +196,7 @@ static int __init ramoops_init(void)
 			return -ENOMEM;
 		dummy_data->mem_size = mem_size;
 		dummy_data->mem_address = mem_address;
+		dummy_data->dump_oops = dump_oops;
 		dummy = platform_create_bundle(&ramoops_driver, ramoops_probe,
 			NULL, 0, dummy_data,
 			sizeof(struct ramoops_platform_data));
diff --git a/include/linux/ramoops.h b/include/linux/ramoops.h
index 0ae68a2..7105c4b 100644
--- a/include/linux/ramoops.h
+++ b/include/linux/ramoops.h
@@ -10,6 +10,7 @@
 struct ramoops_platform_data {
 	unsigned long	mem_size;
 	unsigned long	mem_address;
+	int		dump_oops;
 };
 
 #endif
-- 
1.7.3.1

[PATCH v2 2/3] char drivers: ramoops record_size module parameter

[Sergiu Iordache]

The size of the dump is currently set using the RECORD_SIZE macro which
is set to a page size. This patch makes the record size a module
parameter and allows it to be set through platform data as well to allow
larger dumps if needed.

Signed-off-by: Sergiu Iordache <sergiu@chromium.org>
Change-Id: Ie5a53acb50d5881d51354f5d9d13e3d6bedf6a2e
---
The patch was built on the 2.6.38 kernel and is based on the following
patches which were applied from the mmotm tree:
ramoops-add-new-line-to-each-print
ramoops-use-module-parameters-instead-of-platform-data-if-not-available
ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes

 drivers/char/ramoops.c  |   33 ++++++++++++++++++++++++++-------
 include/linux/ramoops.h |    1 +
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index 5349d94..f34077e 100644
--- a/drivers/char/ramoops.c
+++ b/drivers/char/ramoops.c
@@ -32,8 +32,12 @@
 #include <linux/ramoops.h>
 
 #define RAMOOPS_KERNMSG_HDR "===="
+#define MIN_MEM_SIZE 4096UL
 
-#define RECORD_SIZE 4096UL
+static ulong record_size = 4096UL;
+module_param(record_size, ulong, 0400);
+MODULE_PARM_DESC(record_size,
+		"size of each dump done on oops/panic");
 
 static ulong mem_address;
 module_param(mem_address, ulong, 0400);
@@ -55,6 +59,7 @@ static 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;
@@ -84,10 +89,10 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
 	if (reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
 		return;
 
-	buf = cxt->virt_addr + (cxt->count * RECORD_SIZE);
+	buf = cxt->virt_addr + (cxt->count * cxt->record_size);
 	buf_orig = buf;
 
-	memset(buf, '\0', RECORD_SIZE);
+	memset(buf, '\0', cxt->record_size);
 	res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
 	buf += res;
 	do_gettimeofday(&timestamp);
@@ -95,8 +100,8 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
 	buf += res;
 
 	hdr_size = buf - buf_orig;
-	l2_cpy = min(l2, RECORD_SIZE - hdr_size);
-	l1_cpy = min(l1, RECORD_SIZE - hdr_size - l2_cpy);
+	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;
@@ -119,16 +124,29 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	}
 
 	rounddown_pow_of_two(pdata->mem_size);
+	rounddown_pow_of_two(pdata->record_size);
 
-	if (pdata->mem_size < RECORD_SIZE) {
+	/* Check for the minimum memory size */
+	if (pdata->mem_size < MIN_MEM_SIZE) {
+		pr_err("memory size too small, min %lu\n", MIN_MEM_SIZE);
+		goto fail3;
+	}
+
+	if (pdata->mem_size < pdata->record_size) {
 		pr_err("size too small\n");
 		goto fail3;
 	}
 
-	cxt->max_count = pdata->mem_size / RECORD_SIZE;
+	if (pdata->record_size <= 0) {
+		pr_err("record size too small\n");
+		goto fail3;
+	}
+
+	cxt->max_count = pdata->mem_size / pdata->record_size;
 	cxt->count = 0;
 	cxt->size = pdata->mem_size;
 	cxt->phys_addr = pdata->mem_address;
+	cxt->record_size = pdata->record_size;
 	cxt->dump_oops = pdata->dump_oops;
 
 	if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
@@ -196,6 +214,7 @@ static int __init ramoops_init(void)
 			return -ENOMEM;
 		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,
diff --git a/include/linux/ramoops.h b/include/linux/ramoops.h
index 7105c4b..484fef8 100644
--- a/include/linux/ramoops.h
+++ b/include/linux/ramoops.h
@@ -10,6 +10,7 @@
 struct ramoops_platform_data {
 	unsigned long	mem_size;
 	unsigned long	mem_address;
+	unsigned long	record_size;
 	int		dump_oops;
 };
 
-- 
1.7.3.1

[PATCH v2 3/3] char drivers: ramoops debugfs entry

[Sergiu Iordache]

While ramoops writes to ram, accessing the dump requires using /dev/mem
and knowing the memory location (or a similar solution). This patch
provides a debugfs interface through which the respective memory
area can be easily accessed. It also adds an entry to expose the record
size which must be used to divide the memory area into individual dumps
and a dump count entry.

The entries added are:
/sys/kernel/debug/ramoops/full - memory dump of the whole reserved area.
/sys/kernel/debug/ramoops/count - number of dumps currently present
(will be 0 after a restart).

Change-Id: Ifbab9af833be9ee0bc1c33bc9871f2fc0eb9d228
Signed-off-by: Sergiu Iordache <sergiu@chromium.org>
---
The patch was built on the 2.6.38 kernel and is based on the following
patches which were applied from the mmotm tree:
ramoops-add-new-line-to-each-print
ramoops-use-module-parameters-instead-of-platform-data-if-not-available
ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes

 drivers/char/ramoops.c |  101 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 101 insertions(+), 0 deletions(-)

diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index f34077e..9c0e30e 100644
--- a/drivers/char/ramoops.c
+++ b/drivers/char/ramoops.c
@@ -30,9 +30,15 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/ramoops.h>
+#include <linux/uaccess.h>
+#include <linux/debugfs.h>
 
 #define RAMOOPS_KERNMSG_HDR "===="
 #define MIN_MEM_SIZE 4096UL
+#define RAMOOPS_DIR "ramoops"
+#define RAMOOPS_FULL "full"
+#define RAMOOPS_RS "record_size"
+#define RAMOOPS_COUNT "count"
 
 static ulong record_size = 4096UL;
 module_param(record_size, ulong, 0400);
@@ -67,6 +73,39 @@ static struct ramoops_context {
 
 static struct platform_device *dummy;
 static struct ramoops_platform_data *dummy_data;
+static DEFINE_MUTEX(ramoops_mutex);
+
+/* Debugfs entries for ramoops */
+static struct dentry *ramoops_dir, *ramoops_full_entry, *ramoops_rs_entry,
+				*ramoops_count_entry;
+
+/* Entry to have access to the whole memory area */
+static ssize_t ramoops_read_full(struct file *file, char __user *buf,
+					size_t count, loff_t *ppos)
+{
+	struct ramoops_context *cxt = &oops_cxt;
+
+	mutex_lock(&ramoops_mutex);
+	if (*ppos + count > cxt->size)
+		count = cxt->size - *ppos;
+	if (*ppos > cxt->size) {
+		count = 0;
+		goto out;
+	}
+	if (copy_to_user(buf, cxt->virt_addr + *ppos, count)) {
+		count = -EFAULT;
+		goto out;
+	}
+	*ppos += count;
+
+out:
+	mutex_unlock(&ramoops_mutex);
+	return count;
+}
+
+static const struct file_operations ramoops_full_fops = {
+	.read = ramoops_read_full,
+};
 
 static void ramoops_do_dump(struct kmsg_dumper *dumper,
 		enum kmsg_dump_reason reason, const char *s1, unsigned long l1,
@@ -89,6 +128,7 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
 	if (reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
 		return;
 
+	mutex_lock(&ramoops_mutex);
 	buf = cxt->virt_addr + (cxt->count * cxt->record_size);
 	buf_orig = buf;
 
@@ -110,6 +150,7 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
 	memcpy(buf + l1_cpy, s2 + s2_start, l2_cpy);
 
 	cxt->count = (cxt->count + 1) % cxt->max_count;
+	mutex_unlock(&ramoops_mutex);
 }
 
 static int __init ramoops_probe(struct platform_device *pdev)
@@ -168,6 +209,51 @@ static int __init ramoops_probe(struct platform_device *pdev)
 		goto fail1;
 	}
 
+	/* Initialize debugfs entry so the memory can be easily accessed */
+	ramoops_dir = debugfs_create_dir(RAMOOPS_DIR, NULL);
+	if (ramoops_dir == NULL) {
+		err = -ENOMEM;
+		pr_err("debugfs directory entry creation failed\n");
+		goto out;
+	}
+
+	ramoops_full_entry = debugfs_create_file(RAMOOPS_FULL, 0444,
+					ramoops_dir, NULL, &ramoops_full_fops);
+
+	if (ramoops_full_entry == NULL) {
+		err = -ENOMEM;
+		pr_err("debugfs full entry creation failed\n");
+		goto no_ramoops_full;
+	}
+
+	/*
+	 * Since ramoops returns records of record_size it is useful to
+	 * know the record size from userspace so we can parse the result
+	 * Since the record size is usually small we don't mind converting
+	 * it to a u32 from ulong.
+	 */
+	ramoops_rs_entry = debugfs_create_u32(RAMOOPS_RS, 0444,
+					ramoops_dir, (u32 *)&cxt->record_size);
+
+	if (ramoops_rs_entry == NULL) {
+		err = -ENOMEM;
+		pr_err("debugfs record size entry creation failed\n");
+		goto no_ramoops_rs;
+	}
+
+	ramoops_count_entry = debugfs_create_u32(RAMOOPS_COUNT, 0444,
+					ramoops_dir, (u32 *)&cxt->count);
+
+	/*
+	 * Also add a count entry in debugfs. However this count is reset
+	 * in case the machine resets so it is not always useful.
+	 */
+	if (ramoops_count_entry == NULL) {
+		err = -ENOMEM;
+		pr_err("debugfs record size entry creation failed\n");
+		goto no_ramoops_count;
+	}
+
 	return 0;
 
 fail1:
@@ -176,6 +262,15 @@ fail2:
 	release_mem_region(cxt->phys_addr, cxt->size);
 fail3:
 	return err;
+
+no_ramoops_count:
+	debugfs_remove(ramoops_rs_entry);
+no_ramoops_rs:
+	debugfs_remove(ramoops_full_entry);
+no_ramoops_full:
+	debugfs_remove(ramoops_dir);
+out:
+	return err;
 }
 
 static int __exit ramoops_remove(struct platform_device *pdev)
@@ -233,6 +328,12 @@ static void __exit ramoops_exit(void)
 {
 	platform_driver_unregister(&ramoops_driver);
 	kfree(dummy_data);
+
+	/* Clean up debugfs entries */
+	debugfs_remove(ramoops_full_entry);
+	debugfs_remove(ramoops_rs_entry);
+	debugfs_remove(ramoops_count_entry);
+	debugfs_remove(ramoops_dir);
 }
 
 module_init(ramoops_init);
-- 
1.7.3.1

Re: [PATCH v2 2/3] char drivers: ramoops record_size module parameter

[Marco Stornelli]

Hi,

Il 01/07/2011 03:28, Sergiu Iordache ha scritto:
> The size of the dump is currently set using the RECORD_SIZE macro which
> is set to a page size. This patch makes the record size a module
> parameter and allows it to be set through platform data as well to allow
> larger dumps if needed.
>
> Signed-off-by: Sergiu Iordache<sergiu@chromium.org>
> Change-Id: Ie5a53acb50d5881d51354f5d9d13e3d6bedf6a2e
> ---
> The patch was built on the 2.6.38 kernel and is based on the following
> patches which were applied from the mmotm tree:
> ramoops-add-new-line-to-each-print
> ramoops-use-module-parameters-instead-of-platform-data-if-not-available
> ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes
>
>   drivers/char/ramoops.c  |   33 ++++++++++++++++++++++++++-------
>   include/linux/ramoops.h |    1 +
>   2 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
> index 5349d94..f34077e 100644
> --- a/drivers/char/ramoops.c
> +++ b/drivers/char/ramoops.c
> @@ -32,8 +32,12 @@
>   #include<linux/ramoops.h>
>
>   #define RAMOOPS_KERNMSG_HDR "===="
> +#define MIN_MEM_SIZE 4096UL
>
> -#define RECORD_SIZE 4096UL
> +static ulong record_size = 4096UL;
> +module_param(record_size, ulong, 0400);
> +MODULE_PARM_DESC(record_size,
> +		"size of each dump done on oops/panic");
>
>   static ulong mem_address;
>   module_param(mem_address, ulong, 0400);
> @@ -55,6 +59,7 @@ static 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;
> @@ -84,10 +89,10 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
>   	if (reason == KMSG_DUMP_OOPS&&  !cxt->dump_oops)
>   		return;
>
> -	buf = cxt->virt_addr + (cxt->count * RECORD_SIZE);
> +	buf = cxt->virt_addr + (cxt->count * cxt->record_size);
>   	buf_orig = buf;
>
> -	memset(buf, '\0', RECORD_SIZE);
> +	memset(buf, '\0', cxt->record_size);
>   	res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
>   	buf += res;
>   	do_gettimeofday(&timestamp);
> @@ -95,8 +100,8 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
>   	buf += res;
>
>   	hdr_size = buf - buf_orig;
> -	l2_cpy = min(l2, RECORD_SIZE - hdr_size);
> -	l1_cpy = min(l1, RECORD_SIZE - hdr_size - l2_cpy);
> +	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;
> @@ -119,16 +124,29 @@ static int __init ramoops_probe(struct platform_device *pdev)
>   	}
>
>   	rounddown_pow_of_two(pdata->mem_size);
> +	rounddown_pow_of_two(pdata->record_size);
>
> -	if (pdata->mem_size<  RECORD_SIZE) {
> +	/* Check for the minimum memory size */
> +	if (pdata->mem_size<  MIN_MEM_SIZE) {
> +		pr_err("memory size too small, min %lu\n", MIN_MEM_SIZE);
> +		goto fail3;
> +	}
> +
> +	if (pdata->mem_size<  pdata->record_size) {
>   		pr_err("size too small\n");
>   		goto fail3;
>   	}
>
> -	cxt->max_count = pdata->mem_size / RECORD_SIZE;
> +	if (pdata->record_size<= 0) {
> +		pr_err("record size too small\n");
> +		goto fail3;
> +	}

There is something wrong here. First of all if record_size is unsigned 
it can't negative. In addition, if we are here, we know that:

record_size >= mem_size >= MIN_MEM_SIZE

So this check have no sense.

Marco

Re: [PATCH v2 3/3] char drivers: ramoops debugfs entry

[Marco Stornelli]

Il 01/07/2011 03:28, Sergiu Iordache ha scritto:
> While ramoops writes to ram, accessing the dump requires using /dev/mem
> and knowing the memory location (or a similar solution). This patch
> provides a debugfs interface through which the respective memory
> area can be easily accessed. It also adds an entry to expose the record
> size which must be used to divide the memory area into individual dumps
> and a dump count entry.
>

Good.

> The entries added are:
> /sys/kernel/debug/ramoops/full - memory dump of the whole reserved area.
> /sys/kernel/debug/ramoops/count - number of dumps currently present
> (will be 0 after a restart).

Is this count really needed?

>
> Change-Id: Ifbab9af833be9ee0bc1c33bc9871f2fc0eb9d228
> Signed-off-by: Sergiu Iordache<sergiu@chromium.org>
> ---
> The patch was built on the 2.6.38 kernel and is based on the following
> patches which were applied from the mmotm tree:
> ramoops-add-new-line-to-each-print
> ramoops-use-module-parameters-instead-of-platform-data-if-not-available
> ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes
>
>   drivers/char/ramoops.c |  101 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 101 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
> index f34077e..9c0e30e 100644
> --- a/drivers/char/ramoops.c
> +++ b/drivers/char/ramoops.c
> @@ -30,9 +30,15 @@
>   #include<linux/platform_device.h>
>   #include<linux/slab.h>
>   #include<linux/ramoops.h>
> +#include<linux/uaccess.h>
> +#include<linux/debugfs.h>
>
>   #define RAMOOPS_KERNMSG_HDR "===="
>   #define MIN_MEM_SIZE 4096UL
> +#define RAMOOPS_DIR "ramoops"
> +#define RAMOOPS_FULL "full"
> +#define RAMOOPS_RS "record_size"
> +#define RAMOOPS_COUNT "count"
>
>   static ulong record_size = 4096UL;
>   module_param(record_size, ulong, 0400);
> @@ -67,6 +73,39 @@ static struct ramoops_context {
>
>   static struct platform_device *dummy;
>   static struct ramoops_platform_data *dummy_data;
> +static DEFINE_MUTEX(ramoops_mutex);
> +
> +/* Debugfs entries for ramoops */
> +static struct dentry *ramoops_dir, *ramoops_full_entry, *ramoops_rs_entry,
> +				*ramoops_count_entry;
> +
> +/* Entry to have access to the whole memory area */
> +static ssize_t ramoops_read_full(struct file *file, char __user *buf,
> +					size_t count, loff_t *ppos)
> +{
> +	struct ramoops_context *cxt =&oops_cxt;
> +
> +	mutex_lock(&ramoops_mutex);
> +	if (*ppos + count>  cxt->size)
> +		count = cxt->size - *ppos;
> +	if (*ppos>  cxt->size) {
> +		count = 0;
> +		goto out;
> +	}
> +	if (copy_to_user(buf, cxt->virt_addr + *ppos, count)) {
> +		count = -EFAULT;
> +		goto out;
> +	}
> +	*ppos += count;
> +
> +out:
> +	mutex_unlock(&ramoops_mutex);
> +	return count;
> +}
> +
> +static const struct file_operations ramoops_full_fops = {
> +	.read = ramoops_read_full,
> +};
>
>   static void ramoops_do_dump(struct kmsg_dumper *dumper,
>   		enum kmsg_dump_reason reason, const char *s1, unsigned long l1,
> @@ -89,6 +128,7 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
>   	if (reason == KMSG_DUMP_OOPS&&  !cxt->dump_oops)
>   		return;
>
> +	mutex_lock(&ramoops_mutex);
>   	buf = cxt->virt_addr + (cxt->count * cxt->record_size);
>   	buf_orig = buf;
>
> @@ -110,6 +150,7 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
>   	memcpy(buf + l1_cpy, s2 + s2_start, l2_cpy);
>
>   	cxt->count = (cxt->count + 1) % cxt->max_count;
> +	mutex_unlock(&ramoops_mutex);
>   }
>
>   static int __init ramoops_probe(struct platform_device *pdev)
> @@ -168,6 +209,51 @@ static int __init ramoops_probe(struct platform_device *pdev)
>   		goto fail1;
>   	}
>
> +	/* Initialize debugfs entry so the memory can be easily accessed */
> +	ramoops_dir = debugfs_create_dir(RAMOOPS_DIR, NULL);
> +	if (ramoops_dir == NULL) {
> +		err = -ENOMEM;
> +		pr_err("debugfs directory entry creation failed\n");
> +		goto out;
> +	}
> +
> +	ramoops_full_entry = debugfs_create_file(RAMOOPS_FULL, 0444,
> +					ramoops_dir, NULL,&ramoops_full_fops);
> +
> +	if (ramoops_full_entry == NULL) {
> +		err = -ENOMEM;
> +		pr_err("debugfs full entry creation failed\n");
> +		goto no_ramoops_full;
> +	}
> +
> +	/*
> +	 * Since ramoops returns records of record_size it is useful to
> +	 * know the record size from userspace so we can parse the result
> +	 * Since the record size is usually small we don't mind converting
> +	 * it to a u32 from ulong.
> +	 */
> +	ramoops_rs_entry = debugfs_create_u32(RAMOOPS_RS, 0444,
> +					ramoops_dir, (u32 *)&cxt->record_size);
> +

Like above. The result can be parsed in an easy way due to 
RAMOOPS_KERNMSG_HDR.

Marco

Re: [PATCH v2 1/3] char drivers: ramoops dump_oops platform data

[Marco Stornelli]

Il 01/07/2011 03:28, Sergiu Iordache ha scritto:
> The platform driver currently allows setting the mem_size and mem_address.
> Since dump_oops is also a module parameter it would be more consistent
> if it could be set through platform data as well.
>
> Change-Id: I27e541a51c9722047c4163bf408e778caa77ecc9
> Signed-off-by: Sergiu Iordache<sergiu@chromium.org>
> ---

Acked-by: Marco Stornelli <marco.stornelli@gmail.com>

Re: [PATCH v2 3/3] char drivers: ramoops debugfs entry

[Sergiu Iordache]

On Fri, Jul 1, 2011 at 11:08 AM, Marco Stornelli
<marco.stornelli@gmail.com> wrote:
> Il 01/07/2011 03:28, Sergiu Iordache ha scritto:
>>
>> While ramoops writes to ram, accessing the dump requires using /dev/mem
>> and knowing the memory location (or a similar solution). This patch
>> provides a debugfs interface through which the respective memory
>> area can be easily accessed. It also adds an entry to expose the record
>> size which must be used to divide the memory area into individual dumps
>> and a dump count entry.
>>
>
> Good.
>
>> The entries added are:
>> /sys/kernel/debug/ramoops/full - memory dump of the whole reserved area.
>> /sys/kernel/debug/ramoops/count - number of dumps currently present
>> (will be 0 after a restart).
>
> Is this count really needed?
>
>>
>> Change-Id: Ifbab9af833be9ee0bc1c33bc9871f2fc0eb9d228
>> Signed-off-by: Sergiu Iordache<sergiu@chromium.org>
>> ---
>> The patch was built on the 2.6.38 kernel and is based on the following
>> patches which were applied from the mmotm tree:
>> ramoops-add-new-line-to-each-print
>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available
>>
>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes
>>
>>  drivers/char/ramoops.c |  101
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 101 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
>> index f34077e..9c0e30e 100644
>> --- a/drivers/char/ramoops.c
>> +++ b/drivers/char/ramoops.c
>> @@ -30,9 +30,15 @@
>>  #include<linux/platform_device.h>
>>  #include<linux/slab.h>
>>  #include<linux/ramoops.h>
>> +#include<linux/uaccess.h>
>> +#include<linux/debugfs.h>
>>
>>  #define RAMOOPS_KERNMSG_HDR "===="
>>  #define MIN_MEM_SIZE 4096UL
>> +#define RAMOOPS_DIR "ramoops"
>> +#define RAMOOPS_FULL "full"
>> +#define RAMOOPS_RS "record_size"
>> +#define RAMOOPS_COUNT "count"
>>
>>  static ulong record_size = 4096UL;
>>  module_param(record_size, ulong, 0400);
>> @@ -67,6 +73,39 @@ static struct ramoops_context {
>>
>>  static struct platform_device *dummy;
>>  static struct ramoops_platform_data *dummy_data;
>> +static DEFINE_MUTEX(ramoops_mutex);
>> +
>> +/* Debugfs entries for ramoops */
>> +static struct dentry *ramoops_dir, *ramoops_full_entry,
>> *ramoops_rs_entry,
>> +                               *ramoops_count_entry;
>> +
>> +/* Entry to have access to the whole memory area */
>> +static ssize_t ramoops_read_full(struct file *file, char __user *buf,
>> +                                       size_t count, loff_t *ppos)
>> +{
>> +       struct ramoops_context *cxt =&oops_cxt;
>> +
>> +       mutex_lock(&ramoops_mutex);
>> +       if (*ppos + count>  cxt->size)
>> +               count = cxt->size - *ppos;
>> +       if (*ppos>  cxt->size) {
>> +               count = 0;
>> +               goto out;
>> +       }
>> +       if (copy_to_user(buf, cxt->virt_addr + *ppos, count)) {
>> +               count = -EFAULT;
>> +               goto out;
>> +       }
>> +       *ppos += count;
>> +
>> +out:
>> +       mutex_unlock(&ramoops_mutex);
>> +       return count;
>> +}
>> +
>> +static const struct file_operations ramoops_full_fops = {
>> +       .read = ramoops_read_full,
>> +};
>>
>>  static void ramoops_do_dump(struct kmsg_dumper *dumper,
>>                enum kmsg_dump_reason reason, const char *s1, unsigned long
>> l1,
>> @@ -89,6 +128,7 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
>>        if (reason == KMSG_DUMP_OOPS&&  !cxt->dump_oops)
>>                return;
>>
>> +       mutex_lock(&ramoops_mutex);
>>        buf = cxt->virt_addr + (cxt->count * cxt->record_size);
>>        buf_orig = buf;
>>
>> @@ -110,6 +150,7 @@ static void ramoops_do_dump(struct kmsg_dumper
>> *dumper,
>>        memcpy(buf + l1_cpy, s2 + s2_start, l2_cpy);
>>
>>        cxt->count = (cxt->count + 1) % cxt->max_count;
>> +       mutex_unlock(&ramoops_mutex);
>>  }
>>
>>  static int __init ramoops_probe(struct platform_device *pdev)
>> @@ -168,6 +209,51 @@ static int __init ramoops_probe(struct
>> platform_device *pdev)
>>                goto fail1;
>>        }
>>
>> +       /* Initialize debugfs entry so the memory can be easily accessed
>> */
>> +       ramoops_dir = debugfs_create_dir(RAMOOPS_DIR, NULL);
>> +       if (ramoops_dir == NULL) {
>> +               err = -ENOMEM;
>> +               pr_err("debugfs directory entry creation failed\n");
>> +               goto out;
>> +       }
>> +
>> +       ramoops_full_entry = debugfs_create_file(RAMOOPS_FULL, 0444,
>> +                                       ramoops_dir,
>> NULL,&ramoops_full_fops);
>> +
>> +       if (ramoops_full_entry == NULL) {
>> +               err = -ENOMEM;
>> +               pr_err("debugfs full entry creation failed\n");
>> +               goto no_ramoops_full;
>> +       }
>> +
>> +       /*
>> +        * Since ramoops returns records of record_size it is useful to
>> +        * know the record size from userspace so we can parse the result
>> +        * Since the record size is usually small we don't mind converting
>> +        * it to a u32 from ulong.
>> +        */
>> +       ramoops_rs_entry = debugfs_create_u32(RAMOOPS_RS, 0444,
>> +                                       ramoops_dir, (u32
>> *)&cxt->record_size);
>> +
>
> Like above. The result can be parsed in an easy way due to
> RAMOOPS_KERNMSG_HDR.

I've added the count to make it easier to parse the records by getting
record_size chunks out of the file. One of the problems I see is that
you know the header (where it starts) but it's hard to find out where
the last record in a series ends.
By the way, is there any reason why the (whole) preserved ram area
doesn't get cleared in ramoops when the first dump is taken? On one
hand you could overwrite old dumps but they will get overwritten
anyway and they are probably old and of no interest.
Because right now after a dump you usually have:

Useful data record (record_size size)
Random data from ram (record_size size)
Random data from ram (record_size size)
Etc

This makes it harder to parse (even if you know the record size, as
there's a very small but possible chance that the header is in found
in the random data without it being a valid dump).

Sergiu

Re: [PATCH v2 2/3] char drivers: ramoops record_size module parameter

[Sergiu Iordache]

On Fri, Jul 1, 2011 at 10:57 AM, Marco Stornelli
<marco.stornelli@gmail.com> wrote:
> Hi,
>
> Il 01/07/2011 03:28, Sergiu Iordache ha scritto:
>>
>> The size of the dump is currently set using the RECORD_SIZE macro which
>> is set to a page size. This patch makes the record size a module
>> parameter and allows it to be set through platform data as well to allow
>> larger dumps if needed.
>>
>> Signed-off-by: Sergiu Iordache<sergiu@chromium.org>
>> Change-Id: Ie5a53acb50d5881d51354f5d9d13e3d6bedf6a2e
>> ---
>> The patch was built on the 2.6.38 kernel and is based on the following
>> patches which were applied from the mmotm tree:
>> ramoops-add-new-line-to-each-print
>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available
>>
>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes
>>
>>  drivers/char/ramoops.c  |   33 ++++++++++++++++++++++++++-------
>>  include/linux/ramoops.h |    1 +
>>  2 files changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
>> index 5349d94..f34077e 100644
>> --- a/drivers/char/ramoops.c
>> +++ b/drivers/char/ramoops.c
>> @@ -32,8 +32,12 @@
>>  #include<linux/ramoops.h>
>>
>>  #define RAMOOPS_KERNMSG_HDR "===="
>> +#define MIN_MEM_SIZE 4096UL
>>
>> -#define RECORD_SIZE 4096UL
>> +static ulong record_size = 4096UL;
>> +module_param(record_size, ulong, 0400);
>> +MODULE_PARM_DESC(record_size,
>> +               "size of each dump done on oops/panic");
>>
>>  static ulong mem_address;
>>  module_param(mem_address, ulong, 0400);
>> @@ -55,6 +59,7 @@ static 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;
>> @@ -84,10 +89,10 @@ static void ramoops_do_dump(struct kmsg_dumper
>> *dumper,
>>        if (reason == KMSG_DUMP_OOPS&&  !cxt->dump_oops)
>>                return;
>>
>> -       buf = cxt->virt_addr + (cxt->count * RECORD_SIZE);
>> +       buf = cxt->virt_addr + (cxt->count * cxt->record_size);
>>        buf_orig = buf;
>>
>> -       memset(buf, '\0', RECORD_SIZE);
>> +       memset(buf, '\0', cxt->record_size);
>>        res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
>>        buf += res;
>>        do_gettimeofday(&timestamp);
>> @@ -95,8 +100,8 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
>>        buf += res;
>>
>>        hdr_size = buf - buf_orig;
>> -       l2_cpy = min(l2, RECORD_SIZE - hdr_size);
>> -       l1_cpy = min(l1, RECORD_SIZE - hdr_size - l2_cpy);
>> +       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;
>> @@ -119,16 +124,29 @@ static int __init ramoops_probe(struct
>> platform_device *pdev)
>>        }
>>
>>        rounddown_pow_of_two(pdata->mem_size);
>> +       rounddown_pow_of_two(pdata->record_size);
>>
>> -       if (pdata->mem_size<  RECORD_SIZE) {
>> +       /* Check for the minimum memory size */
>> +       if (pdata->mem_size<  MIN_MEM_SIZE) {
>> +               pr_err("memory size too small, min %lu\n", MIN_MEM_SIZE);
>> +               goto fail3;
>> +       }
>> +
>> +       if (pdata->mem_size<  pdata->record_size) {
>>                pr_err("size too small\n");
>>                goto fail3;
>>        }
>>
>> -       cxt->max_count = pdata->mem_size / RECORD_SIZE;
>> +       if (pdata->record_size<= 0) {
>> +               pr_err("record size too small\n");
>> +               goto fail3;
>> +       }
>
> There is something wrong here. First of all if record_size is unsigned it
> can't negative. In addition, if we are here, we know that:
>
> record_size >= mem_size >= MIN_MEM_SIZE
>
> So this check have no sense.

The pdata->record size <= 0 check is indeed redundant and should be removed.

I didn't completely understand the second comment, the module errors
if mem_size < MIN_MEM_SIZE or mem_size < record_size, which means that
mem_size should be larger than MIN_MEM_SIZE and record_size (which
leads to record_size being between 0 and mem_size). Am I missing
something?

(Resent after not reply-ing to all by mistake)

Sergiu

Re: [PATCH v2 3/3] char drivers: ramoops debugfs entry

[Marco Stornelli]

Il 01/07/2011 20:38, Sergiu Iordache ha scritto:
> On Fri, Jul 1, 2011 at 11:08 AM, Marco Stornelli
> <marco.stornelli@gmail.com>  wrote:
>> Il 01/07/2011 03:28, Sergiu Iordache ha scritto:
>>>
>>> While ramoops writes to ram, accessing the dump requires using /dev/mem
>>> and knowing the memory location (or a similar solution). This patch
>>> provides a debugfs interface through which the respective memory
>>> area can be easily accessed. It also adds an entry to expose the record
>>> size which must be used to divide the memory area into individual dumps
>>> and a dump count entry.
>>>
>>
>> Good.
>>
>>> The entries added are:
>>> /sys/kernel/debug/ramoops/full - memory dump of the whole reserved area.
>>> /sys/kernel/debug/ramoops/count - number of dumps currently present
>>> (will be 0 after a restart).
>>
>> Is this count really needed?
>>
>>>
>>> Change-Id: Ifbab9af833be9ee0bc1c33bc9871f2fc0eb9d228
>>> Signed-off-by: Sergiu Iordache<sergiu@chromium.org>
>>> ---
>>> The patch was built on the 2.6.38 kernel and is based on the following
>>> patches which were applied from the mmotm tree:
>>> ramoops-add-new-line-to-each-print
>>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available
>>>
>>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes
>>>
>>>   drivers/char/ramoops.c |  101
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 files changed, 101 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
>>> index f34077e..9c0e30e 100644
>>> --- a/drivers/char/ramoops.c
>>> +++ b/drivers/char/ramoops.c
>>> @@ -30,9 +30,15 @@
>>>   #include<linux/platform_device.h>
>>>   #include<linux/slab.h>
>>>   #include<linux/ramoops.h>
>>> +#include<linux/uaccess.h>
>>> +#include<linux/debugfs.h>
>>>
>>>   #define RAMOOPS_KERNMSG_HDR "===="
>>>   #define MIN_MEM_SIZE 4096UL
>>> +#define RAMOOPS_DIR "ramoops"
>>> +#define RAMOOPS_FULL "full"
>>> +#define RAMOOPS_RS "record_size"
>>> +#define RAMOOPS_COUNT "count"
>>>
>>>   static ulong record_size = 4096UL;
>>>   module_param(record_size, ulong, 0400);
>>> @@ -67,6 +73,39 @@ static struct ramoops_context {
>>>
>>>   static struct platform_device *dummy;
>>>   static struct ramoops_platform_data *dummy_data;
>>> +static DEFINE_MUTEX(ramoops_mutex);
>>> +
>>> +/* Debugfs entries for ramoops */
>>> +static struct dentry *ramoops_dir, *ramoops_full_entry,
>>> *ramoops_rs_entry,
>>> +                               *ramoops_count_entry;
>>> +
>>> +/* Entry to have access to the whole memory area */
>>> +static ssize_t ramoops_read_full(struct file *file, char __user *buf,
>>> +                                       size_t count, loff_t *ppos)
>>> +{
>>> +       struct ramoops_context *cxt =&oops_cxt;
>>> +
>>> +       mutex_lock(&ramoops_mutex);
>>> +       if (*ppos + count>    cxt->size)
>>> +               count = cxt->size - *ppos;
>>> +       if (*ppos>    cxt->size) {
>>> +               count = 0;
>>> +               goto out;
>>> +       }
>>> +       if (copy_to_user(buf, cxt->virt_addr + *ppos, count)) {
>>> +               count = -EFAULT;
>>> +               goto out;
>>> +       }
>>> +       *ppos += count;
>>> +
>>> +out:
>>> +       mutex_unlock(&ramoops_mutex);
>>> +       return count;
>>> +}
>>> +
>>> +static const struct file_operations ramoops_full_fops = {
>>> +       .read = ramoops_read_full,
>>> +};
>>>
>>>   static void ramoops_do_dump(struct kmsg_dumper *dumper,
>>>                 enum kmsg_dump_reason reason, const char *s1, unsigned long
>>> l1,
>>> @@ -89,6 +128,7 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
>>>         if (reason == KMSG_DUMP_OOPS&&    !cxt->dump_oops)
>>>                 return;
>>>
>>> +       mutex_lock(&ramoops_mutex);
>>>         buf = cxt->virt_addr + (cxt->count * cxt->record_size);
>>>         buf_orig = buf;
>>>
>>> @@ -110,6 +150,7 @@ static void ramoops_do_dump(struct kmsg_dumper
>>> *dumper,
>>>         memcpy(buf + l1_cpy, s2 + s2_start, l2_cpy);
>>>
>>>         cxt->count = (cxt->count + 1) % cxt->max_count;
>>> +       mutex_unlock(&ramoops_mutex);
>>>   }
>>>
>>>   static int __init ramoops_probe(struct platform_device *pdev)
>>> @@ -168,6 +209,51 @@ static int __init ramoops_probe(struct
>>> platform_device *pdev)
>>>                 goto fail1;
>>>         }
>>>
>>> +       /* Initialize debugfs entry so the memory can be easily accessed
>>> */
>>> +       ramoops_dir = debugfs_create_dir(RAMOOPS_DIR, NULL);
>>> +       if (ramoops_dir == NULL) {
>>> +               err = -ENOMEM;
>>> +               pr_err("debugfs directory entry creation failed\n");
>>> +               goto out;
>>> +       }
>>> +
>>> +       ramoops_full_entry = debugfs_create_file(RAMOOPS_FULL, 0444,
>>> +                                       ramoops_dir,
>>> NULL,&ramoops_full_fops);
>>> +
>>> +       if (ramoops_full_entry == NULL) {
>>> +               err = -ENOMEM;
>>> +               pr_err("debugfs full entry creation failed\n");
>>> +               goto no_ramoops_full;
>>> +       }
>>> +
>>> +       /*
>>> +        * Since ramoops returns records of record_size it is useful to
>>> +        * know the record size from userspace so we can parse the result
>>> +        * Since the record size is usually small we don't mind converting
>>> +        * it to a u32 from ulong.
>>> +        */
>>> +       ramoops_rs_entry = debugfs_create_u32(RAMOOPS_RS, 0444,
>>> +                                       ramoops_dir, (u32
>>> *)&cxt->record_size);
>>> +
>>
>> Like above. The result can be parsed in an easy way due to
>> RAMOOPS_KERNMSG_HDR.
>
> I've added the count to make it easier to parse the records by getting
> record_size chunks out of the file. One of the problems I see is that
> you know the header (where it starts) but it's hard to find out where
> the last record in a series ends.

It was easy because the record size had a fixed length (4096), so maybe 
at this point it can be sufficient the record size information. I see a 
little problem however. I think we could use debugfs interface to dump 
the log in an easy way but we should be able to dump it even with 
/dev/mem. Specially on embedded systems, debugfs can be no mounted or 
not available at all. So maybe, as you said below, with these new 
patches we need a memset over all the memory area when the first dump is 
taken. However, the original idea was to store even old dumps. In 
addition, there's the problem to catch an oops after a start-up that 
"clean" the area before we read it. At that point we lost the previous 
dumps. To solve this we could use a "reset" paramater, but I think all 
of this is a little overkilling. Maybe we can only bump up the record 
size if needed. What do you think?

> By the way, is there any reason why the (whole) preserved ram area
> doesn't get cleared in ramoops when the first dump is taken? On one
> hand you could overwrite old dumps but they will get overwritten
> anyway and they are probably old and of no interest.
> Because right now after a dump you usually have:
>
> Useful data record (record_size size)
> Random data from ram (record_size size)
> Random data from ram (record_size size)
> Etc
>
> This makes it harder to parse (even if you know the record size, as
> there's a very small but possible chance that the header is in found
> in the random data without it being a valid dump).

The string "====" plus a timestamp? Maybe but really really difficult.

Marco

Re: [PATCH v2 2/3] char drivers: ramoops record_size module parameter

[Marco Stornelli]

Il 01/07/2011 20:41, Sergiu Iordache ha scritto:
> On Fri, Jul 1, 2011 at 10:57 AM, Marco Stornelli
> <marco.stornelli@gmail.com>  wrote:
>> Hi,
>>
>> Il 01/07/2011 03:28, Sergiu Iordache ha scritto:
>>>
>>> The size of the dump is currently set using the RECORD_SIZE macro which
>>> is set to a page size. This patch makes the record size a module
>>> parameter and allows it to be set through platform data as well to allow
>>> larger dumps if needed.
>>>
>>> Signed-off-by: Sergiu Iordache<sergiu@chromium.org>
>>> Change-Id: Ie5a53acb50d5881d51354f5d9d13e3d6bedf6a2e
>>> ---
>>> The patch was built on the 2.6.38 kernel and is based on the following
>>> patches which were applied from the mmotm tree:
>>> ramoops-add-new-line-to-each-print
>>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available
>>>
>>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes
>>>
>>>   drivers/char/ramoops.c  |   33 ++++++++++++++++++++++++++-------
>>>   include/linux/ramoops.h |    1 +
>>>   2 files changed, 27 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
>>> index 5349d94..f34077e 100644
>>> --- a/drivers/char/ramoops.c
>>> +++ b/drivers/char/ramoops.c
>>> @@ -32,8 +32,12 @@
>>>   #include<linux/ramoops.h>
>>>
>>>   #define RAMOOPS_KERNMSG_HDR "===="
>>> +#define MIN_MEM_SIZE 4096UL
>>>
>>> -#define RECORD_SIZE 4096UL
>>> +static ulong record_size = 4096UL;
>>> +module_param(record_size, ulong, 0400);
>>> +MODULE_PARM_DESC(record_size,
>>> +               "size of each dump done on oops/panic");
>>>
>>>   static ulong mem_address;
>>>   module_param(mem_address, ulong, 0400);
>>> @@ -55,6 +59,7 @@ static 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;
>>> @@ -84,10 +89,10 @@ static void ramoops_do_dump(struct kmsg_dumper
>>> *dumper,
>>>         if (reason == KMSG_DUMP_OOPS&&    !cxt->dump_oops)
>>>                 return;
>>>
>>> -       buf = cxt->virt_addr + (cxt->count * RECORD_SIZE);
>>> +       buf = cxt->virt_addr + (cxt->count * cxt->record_size);
>>>         buf_orig = buf;
>>>
>>> -       memset(buf, '\0', RECORD_SIZE);
>>> +       memset(buf, '\0', cxt->record_size);
>>>         res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
>>>         buf += res;
>>>         do_gettimeofday(&timestamp);
>>> @@ -95,8 +100,8 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
>>>         buf += res;
>>>
>>>         hdr_size = buf - buf_orig;
>>> -       l2_cpy = min(l2, RECORD_SIZE - hdr_size);
>>> -       l1_cpy = min(l1, RECORD_SIZE - hdr_size - l2_cpy);
>>> +       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;
>>> @@ -119,16 +124,29 @@ static int __init ramoops_probe(struct
>>> platform_device *pdev)
>>>         }
>>>
>>>         rounddown_pow_of_two(pdata->mem_size);
>>> +       rounddown_pow_of_two(pdata->record_size);
>>>
>>> -       if (pdata->mem_size<    RECORD_SIZE) {
>>> +       /* Check for the minimum memory size */
>>> +       if (pdata->mem_size<    MIN_MEM_SIZE) {
>>> +               pr_err("memory size too small, min %lu\n", MIN_MEM_SIZE);
>>> +               goto fail3;
>>> +       }
>>> +
>>> +       if (pdata->mem_size<    pdata->record_size) {
>>>                 pr_err("size too small\n");
>>>                 goto fail3;
>>>         }
>>>
>>> -       cxt->max_count = pdata->mem_size / RECORD_SIZE;
>>> +       if (pdata->record_size<= 0) {
>>> +               pr_err("record size too small\n");
>>> +               goto fail3;
>>> +       }
>>
>> There is something wrong here. First of all if record_size is unsigned it
>> can't negative. In addition, if we are here, we know that:
>>
>> record_size>= mem_size>= MIN_MEM_SIZE
>>
>> So this check have no sense.
>
> The pdata->record size<= 0 check is indeed redundant and should be removed.
>
> I didn't completely understand the second comment, the module errors
> if mem_size<  MIN_MEM_SIZE or mem_size<  record_size, which means that
> mem_size should be larger than MIN_MEM_SIZE and record_size (which
> leads to record_size being between 0 and mem_size). Am I missing
> something?
>
> (Resent after not reply-ing to all by mistake)
>
> Sergiu
>

Yes, my fault. I meant we should check that mem_size *and* record_size 
are bigger or equals than MIN_MEM_SIZE. After that, we can check that 
record_size is lesser than mem_size (I guess has no sense to use 
record_size lesser than MIN_MEM_SIZE).

Marco

Re: [PATCH v2 3/3] char drivers: ramoops debugfs entry

[Sergiu Iordache]

On Sat, Jul 2, 2011 at 1:01 AM, Marco Stornelli
<marco.stornelli@gmail.com> wrote:
> Il 01/07/2011 20:38, Sergiu Iordache ha scritto:
>>
>> On Fri, Jul 1, 2011 at 11:08 AM, Marco Stornelli
>> <marco.stornelli@gmail.com>  wrote:
>>>
>>> Il 01/07/2011 03:28, Sergiu Iordache ha scritto:
>>>>
>>>> While ramoops writes to ram, accessing the dump requires using /dev/mem
>>>> and knowing the memory location (or a similar solution). This patch
>>>> provides a debugfs interface through which the respective memory
>>>> area can be easily accessed. It also adds an entry to expose the record
>>>> size which must be used to divide the memory area into individual dumps
>>>> and a dump count entry.
>>>>
>>>
>>> Good.
>>>
>>>> The entries added are:
>>>> /sys/kernel/debug/ramoops/full - memory dump of the whole reserved area.
>>>> /sys/kernel/debug/ramoops/count - number of dumps currently present
>>>> (will be 0 after a restart).
>>>
>>> Is this count really needed?
>>>
>>>>
>>>> Change-Id: Ifbab9af833be9ee0bc1c33bc9871f2fc0eb9d228
>>>> Signed-off-by: Sergiu Iordache<sergiu@chromium.org>
>>>> ---
>>>> The patch was built on the 2.6.38 kernel and is based on the following
>>>> patches which were applied from the mmotm tree:
>>>> ramoops-add-new-line-to-each-print
>>>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available
>>>>
>>>>
>>>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes
>>>>
>>>>  drivers/char/ramoops.c |  101
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 files changed, 101 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
>>>> index f34077e..9c0e30e 100644
>>>> --- a/drivers/char/ramoops.c
>>>> +++ b/drivers/char/ramoops.c
>>>> @@ -30,9 +30,15 @@
>>>>  #include<linux/platform_device.h>
>>>>  #include<linux/slab.h>
>>>>  #include<linux/ramoops.h>
>>>> +#include<linux/uaccess.h>
>>>> +#include<linux/debugfs.h>
>>>>
>>>>  #define RAMOOPS_KERNMSG_HDR "===="
>>>>  #define MIN_MEM_SIZE 4096UL
>>>> +#define RAMOOPS_DIR "ramoops"
>>>> +#define RAMOOPS_FULL "full"
>>>> +#define RAMOOPS_RS "record_size"
>>>> +#define RAMOOPS_COUNT "count"
>>>>
>>>>  static ulong record_size = 4096UL;
>>>>  module_param(record_size, ulong, 0400);
>>>> @@ -67,6 +73,39 @@ static struct ramoops_context {
>>>>
>>>>  static struct platform_device *dummy;
>>>>  static struct ramoops_platform_data *dummy_data;
>>>> +static DEFINE_MUTEX(ramoops_mutex);
>>>> +
>>>> +/* Debugfs entries for ramoops */
>>>> +static struct dentry *ramoops_dir, *ramoops_full_entry,
>>>> *ramoops_rs_entry,
>>>> +                               *ramoops_count_entry;
>>>> +
>>>> +/* Entry to have access to the whole memory area */
>>>> +static ssize_t ramoops_read_full(struct file *file, char __user *buf,
>>>> +                                       size_t count, loff_t *ppos)
>>>> +{
>>>> +       struct ramoops_context *cxt =&oops_cxt;
>>>> +
>>>> +       mutex_lock(&ramoops_mutex);
>>>> +       if (*ppos + count>    cxt->size)
>>>> +               count = cxt->size - *ppos;
>>>> +       if (*ppos>    cxt->size) {
>>>> +               count = 0;
>>>> +               goto out;
>>>> +       }
>>>> +       if (copy_to_user(buf, cxt->virt_addr + *ppos, count)) {
>>>> +               count = -EFAULT;
>>>> +               goto out;
>>>> +       }
>>>> +       *ppos += count;
>>>> +
>>>> +out:
>>>> +       mutex_unlock(&ramoops_mutex);
>>>> +       return count;
>>>> +}
>>>> +
>>>> +static const struct file_operations ramoops_full_fops = {
>>>> +       .read = ramoops_read_full,
>>>> +};
>>>>
>>>>  static void ramoops_do_dump(struct kmsg_dumper *dumper,
>>>>                enum kmsg_dump_reason reason, const char *s1, unsigned
>>>> long
>>>> l1,
>>>> @@ -89,6 +128,7 @@ static void ramoops_do_dump(struct kmsg_dumper
>>>> *dumper,
>>>>        if (reason == KMSG_DUMP_OOPS&&    !cxt->dump_oops)
>>>>                return;
>>>>
>>>> +       mutex_lock(&ramoops_mutex);
>>>>        buf = cxt->virt_addr + (cxt->count * cxt->record_size);
>>>>        buf_orig = buf;
>>>>
>>>> @@ -110,6 +150,7 @@ static void ramoops_do_dump(struct kmsg_dumper
>>>> *dumper,
>>>>        memcpy(buf + l1_cpy, s2 + s2_start, l2_cpy);
>>>>
>>>>        cxt->count = (cxt->count + 1) % cxt->max_count;
>>>> +       mutex_unlock(&ramoops_mutex);
>>>>  }
>>>>
>>>>  static int __init ramoops_probe(struct platform_device *pdev)
>>>> @@ -168,6 +209,51 @@ static int __init ramoops_probe(struct
>>>> platform_device *pdev)
>>>>                goto fail1;
>>>>        }
>>>>
>>>> +       /* Initialize debugfs entry so the memory can be easily accessed
>>>> */
>>>> +       ramoops_dir = debugfs_create_dir(RAMOOPS_DIR, NULL);
>>>> +       if (ramoops_dir == NULL) {
>>>> +               err = -ENOMEM;
>>>> +               pr_err("debugfs directory entry creation failed\n");
>>>> +               goto out;
>>>> +       }
>>>> +
>>>> +       ramoops_full_entry = debugfs_create_file(RAMOOPS_FULL, 0444,
>>>> +                                       ramoops_dir,
>>>> NULL,&ramoops_full_fops);
>>>> +
>>>> +       if (ramoops_full_entry == NULL) {
>>>> +               err = -ENOMEM;
>>>> +               pr_err("debugfs full entry creation failed\n");
>>>> +               goto no_ramoops_full;
>>>> +       }
>>>> +
>>>> +       /*
>>>> +        * Since ramoops returns records of record_size it is useful to
>>>> +        * know the record size from userspace so we can parse the
>>>> result
>>>> +        * Since the record size is usually small we don't mind
>>>> converting
>>>> +        * it to a u32 from ulong.
>>>> +        */
>>>> +       ramoops_rs_entry = debugfs_create_u32(RAMOOPS_RS, 0444,
>>>> +                                       ramoops_dir, (u32
>>>> *)&cxt->record_size);
>>>> +
>>>
>>> Like above. The result can be parsed in an easy way due to
>>> RAMOOPS_KERNMSG_HDR.
>>
>> I've added the count to make it easier to parse the records by getting
>> record_size chunks out of the file. One of the problems I see is that
>> you know the header (where it starts) but it's hard to find out where
>> the last record in a series ends.
>
> It was easy because the record size had a fixed length (4096), so maybe at
> this point it can be sufficient the record size information. I see a little
> problem however. I think we could use debugfs interface to dump the log in
> an easy way but we should be able to dump it even with /dev/mem. Specially
> on embedded systems, debugfs can be no mounted or not available at all. So
> maybe, as you said below, with these new patches we need a memset over all
> the memory area when the first dump is taken. However, the original idea was
> to store even old dumps. In addition, there's the problem to catch an oops
> after a start-up that "clean" the area before we read it. At that point we
> lost the previous dumps. To solve this we could use a "reset" paramater, but
> I think all of this is a little overkilling. Maybe we can only bump up the
> record size if needed. What do you think?
The problem with a fixed record size of 4K is that it is not very
flexible as some setups may need more dump data (and 4K doesn't mean
that much). Setting the record size via a module parameter or platform
data doesn't seem as a huge problem to me if you are not using debugfs
as you should be able to somehow export the record size (since you
were the one who set it through the parameter in the first place) and
get the dumps from /dev/mem.

I've thought more about this problem today and I have thought of the
following alternative solution: Have a debugfs entry which returns a
record size chunk at a time by starting with the first entry and then
checking each of the entries for the header (and the presence of the
timestamp maybe to be sure). It will then return each entry that is
valid skipping over the invalid ones and it will return an empty
result when it reaches the end of the memory zone. It could also have
an entry to reset to the first entry so you can start over. This way
you wouldn't lose old entries and you could still get a pretty easy to
parse result.

>> By the way, is there any reason why the (whole) preserved ram area
>> doesn't get cleared in ramoops when the first dump is taken? On one
>> hand you could overwrite old dumps but they will get overwritten
>> anyway and they are probably old and of no interest.
>> Because right now after a dump you usually have:
>>
>> Useful data record (record_size size)
>> Random data from ram (record_size size)
>> Random data from ram (record_size size)
>> Etc
>>
>> This makes it harder to parse (even if you know the record size, as
>> there's a very small but possible chance that the header is in found
>> in the random data without it being a valid dump).
>
> The string "====" plus a timestamp? Maybe but really really difficult.
Checking for the presence of the timestamp in addition to the presence
of the ==== header is probably a good idea, just to make sure.

I'll be off for a couple of days but I'll be back on it next week.
Sergiu

Re: [PATCH v2 3/3] char drivers: ramoops debugfs entry

[Marco Stornelli]

Il 02/07/2011 11:01, Sergiu Iordache ha scritto:
> On Sat, Jul 2, 2011 at 1:01 AM, Marco Stornelli
> <marco.stornelli@gmail.com>  wrote:
 >>
>> It was easy because the record size had a fixed length (4096), so maybe at
>> this point it can be sufficient the record size information. I see a little
>> problem however. I think we could use debugfs interface to dump the log in
>> an easy way but we should be able to dump it even with /dev/mem. Specially
>> on embedded systems, debugfs can be no mounted or not available at all. So
>> maybe, as you said below, with these new patches we need a memset over all
>> the memory area when the first dump is taken. However, the original idea was
>> to store even old dumps. In addition, there's the problem to catch an oops
>> after a start-up that "clean" the area before we read it. At that point we
>> lost the previous dumps. To solve this we could use a "reset" paramater, but
>> I think all of this is a little overkilling. Maybe we can only bump up the
>> record size if needed. What do you think?
> The problem with a fixed record size of 4K is that it is not very
> flexible as some setups may need more dump data (and 4K doesn't mean
> that much). Setting the record size via a module parameter or platform
> data doesn't seem as a huge problem to me if you are not using debugfs
> as you should be able to somehow export the record size (since you
> were the one who set it through the parameter in the first place) and
> get the dumps from /dev/mem.

The point here is not how to set record size, but what it does mean to 
have a variable record size compared with the current situation. 
However, if we know that there are situation where 4k are not 
sufficient, ok we can modify it.

>
> I've thought more about this problem today and I have thought of the
> following alternative solution: Have a debugfs entry which returns a
> record size chunk at a time by starting with the first entry and then
> checking each of the entries for the header (and the presence of the
> timestamp maybe to be sure). It will then return each entry that is
> valid skipping over the invalid ones and it will return an empty
> result when it reaches the end of the memory zone. It could also have
> an entry to reset to the first entry so you can start over. This way
> you wouldn't lose old entries and you could still get a pretty easy to
> parse result.
>

It seems a good strategy for me.

Marco

Re: [PATCH v2 2/3] char drivers: ramoops record_size module parameter

[Sergiu Iordache]

On Sat, Jul 2, 2011 at 1:04 AM, Marco Stornelli
<marco.stornelli@gmail.com> wrote:
> Il 01/07/2011 20:41, Sergiu Iordache ha scritto:
>>
>> On Fri, Jul 1, 2011 at 10:57 AM, Marco Stornelli
>> <marco.stornelli@gmail.com>  wrote:
>>>
>>> Hi,
>>>
>>> Il 01/07/2011 03:28, Sergiu Iordache ha scritto:
>>>>
>>>> The size of the dump is currently set using the RECORD_SIZE macro which
>>>> is set to a page size. This patch makes the record size a module
>>>> parameter and allows it to be set through platform data as well to allow
>>>> larger dumps if needed.
>>>>
>>>> Signed-off-by: Sergiu Iordache<sergiu@chromium.org>
>>>> Change-Id: Ie5a53acb50d5881d51354f5d9d13e3d6bedf6a2e
>>>> ---
>>>> The patch was built on the 2.6.38 kernel and is based on the following
>>>> patches which were applied from the mmotm tree:
>>>> ramoops-add-new-line-to-each-print
>>>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available
>>>>
>>>>
>>>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes
>>>>
>>>>  drivers/char/ramoops.c  |   33 ++++++++++++++++++++++++++-------
>>>>  include/linux/ramoops.h |    1 +
>>>>  2 files changed, 27 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
>>>> index 5349d94..f34077e 100644
>>>> --- a/drivers/char/ramoops.c
>>>> +++ b/drivers/char/ramoops.c
>>>> @@ -32,8 +32,12 @@
>>>>  #include<linux/ramoops.h>
>>>>
>>>>  #define RAMOOPS_KERNMSG_HDR "===="
>>>> +#define MIN_MEM_SIZE 4096UL
>>>>
>>>> -#define RECORD_SIZE 4096UL
>>>> +static ulong record_size = 4096UL;
>>>> +module_param(record_size, ulong, 0400);
>>>> +MODULE_PARM_DESC(record_size,
>>>> +               "size of each dump done on oops/panic");
>>>>
>>>>  static ulong mem_address;
>>>>  module_param(mem_address, ulong, 0400);
>>>> @@ -55,6 +59,7 @@ static 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;
>>>> @@ -84,10 +89,10 @@ static void ramoops_do_dump(struct kmsg_dumper
>>>> *dumper,
>>>>        if (reason == KMSG_DUMP_OOPS&&    !cxt->dump_oops)
>>>>                return;
>>>>
>>>> -       buf = cxt->virt_addr + (cxt->count * RECORD_SIZE);
>>>> +       buf = cxt->virt_addr + (cxt->count * cxt->record_size);
>>>>        buf_orig = buf;
>>>>
>>>> -       memset(buf, '\0', RECORD_SIZE);
>>>> +       memset(buf, '\0', cxt->record_size);
>>>>        res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
>>>>        buf += res;
>>>>        do_gettimeofday(&timestamp);
>>>> @@ -95,8 +100,8 @@ static void ramoops_do_dump(struct kmsg_dumper
>>>> *dumper,
>>>>        buf += res;
>>>>
>>>>        hdr_size = buf - buf_orig;
>>>> -       l2_cpy = min(l2, RECORD_SIZE - hdr_size);
>>>> -       l1_cpy = min(l1, RECORD_SIZE - hdr_size - l2_cpy);
>>>> +       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;
>>>> @@ -119,16 +124,29 @@ static int __init ramoops_probe(struct
>>>> platform_device *pdev)
>>>>        }
>>>>
>>>>        rounddown_pow_of_two(pdata->mem_size);
>>>> +       rounddown_pow_of_two(pdata->record_size);
>>>>
>>>> -       if (pdata->mem_size<    RECORD_SIZE) {
>>>> +       /* Check for the minimum memory size */
>>>> +       if (pdata->mem_size<    MIN_MEM_SIZE) {
>>>> +               pr_err("memory size too small, min %lu\n",
>>>> MIN_MEM_SIZE);
>>>> +               goto fail3;
>>>> +       }
>>>> +
>>>> +       if (pdata->mem_size<    pdata->record_size) {
>>>>                pr_err("size too small\n");
>>>>                goto fail3;
>>>>        }
>>>>
>>>> -       cxt->max_count = pdata->mem_size / RECORD_SIZE;
>>>> +       if (pdata->record_size<= 0) {
>>>> +               pr_err("record size too small\n");
>>>> +               goto fail3;
>>>> +       }
>>>
>>> There is something wrong here. First of all if record_size is unsigned it
>>> can't negative. In addition, if we are here, we know that:
>>>
>>> record_size>= mem_size>= MIN_MEM_SIZE
>>>
>>> So this check have no sense.
>>
>> The pdata->record size<= 0 check is indeed redundant and should be
>> removed.
>>
>> I didn't completely understand the second comment, the module errors
>> if mem_size<  MIN_MEM_SIZE or mem_size<  record_size, which means that
>> mem_size should be larger than MIN_MEM_SIZE and record_size (which
>> leads to record_size being between 0 and mem_size). Am I missing
>> something?
>>
>> (Resent after not reply-ing to all by mistake)
>>
>> Sergiu
>>
>
> Yes, my fault. I meant we should check that mem_size *and* record_size are
> bigger or equals than MIN_MEM_SIZE. After that, we can check that
> record_size is lesser than mem_size (I guess has no sense to use record_size
> lesser than MIN_MEM_SIZE).

That sounds fair. A check to see if record_size != 0 would probably be
needed as well so we don't divide by 0. I'll add those to the next
patch series and submit them for review.

Sergiu

Re: [PATCH v2 3/3] char drivers: ramoops debugfs entry

[Sergiu Iordache]

On Sat, Jul 2, 2011 at 4:59 AM, Marco Stornelli
<marco.stornelli@gmail.com> wrote:
> Il 02/07/2011 11:01, Sergiu Iordache ha scritto:
>>
>> On Sat, Jul 2, 2011 at 1:01 AM, Marco Stornelli
>> <marco.stornelli@gmail.com>  wrote:
>
>>>
>>>
>>> It was easy because the record size had a fixed length (4096), so maybe
>>> at
>>> this point it can be sufficient the record size information. I see a
>>> little
>>> problem however. I think we could use debugfs interface to dump the log
>>> in
>>> an easy way but we should be able to dump it even with /dev/mem.
>>> Specially
>>> on embedded systems, debugfs can be no mounted or not available at all.
>>> So
>>> maybe, as you said below, with these new patches we need a memset over
>>> all
>>> the memory area when the first dump is taken. However, the original idea
>>> was
>>> to store even old dumps. In addition, there's the problem to catch an
>>> oops
>>> after a start-up that "clean" the area before we read it. At that point
>>> we
>>> lost the previous dumps. To solve this we could use a "reset" paramater,
>>> but
>>> I think all of this is a little overkilling. Maybe we can only bump up
>>> the
>>> record size if needed. What do you think?
>>
>> The problem with a fixed record size of 4K is that it is not very
>> flexible as some setups may need more dump data (and 4K doesn't mean
>> that much). Setting the record size via a module parameter or platform
>> data doesn't seem as a huge problem to me if you are not using debugfs
>> as you should be able to somehow export the record size (since you
>> were the one who set it through the parameter in the first place) and
>> get the dumps from /dev/mem.
>
> The point here is not how to set record size, but what it does mean to have
> a variable record size compared with the current situation. However, if we
> know that there are situation where 4k are not sufficient, ok we can modify
> it.
Well right now I don't see any major disadvantages in making it
configurable as long as the present behavior is preserved (no set
record_size -> set it to 4K). Are there any other things to consider?

>> I've thought more about this problem today and I have thought of the
>> following alternative solution: Have a debugfs entry which returns a
>> record size chunk at a time by starting with the first entry and then
>> checking each of the entries for the header (and the presence of the
>> timestamp maybe to be sure). It will then return each entry that is
>> valid skipping over the invalid ones and it will return an empty
>> result when it reaches the end of the memory zone. It could also have
>> an entry to reset to the first entry so you can start over. This way
>> you wouldn't lose old entries and you could still get a pretty easy to
>> parse result.
>>
>
> It seems a good strategy for me.
I'll implement this and come back with a patch.

Sergiu