[PATCH v3 0/3] char drivers: rammops improvements

[PATCH v3 0/3] char drivers: rammops 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 two by being built on the ramoops modifications present in the mmotm tree and by using a different
approach regarding the debugfs entry.

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  |  151 +++++++++++++++++++++++++++++++++++++++++++----
 include/linux/ramoops.h |    2 +
 2 files changed, 142 insertions(+), 11 deletions(-)

-- 
1.7.3.1

[PATCH v3 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>
Acked-by: Marco Stornelli <marco.stornelli@gmail.com>
---
 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 v3 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
---
 drivers/char/ramoops.c  |   37 +++++++++++++++++++++++++++----------
 include/linux/ramoops.h |    1 +
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index 5349d94..bd9b94b 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 = MIN_MEM_SIZE;
+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;
@@ -113,22 +118,33 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	struct ramoops_context *cxt = &oops_cxt;
 	int err = -EINVAL;
 
-	if (!pdata->mem_size) {
-		pr_err("invalid size specification\n");
+	if (!pdata->mem_size || !pdata->record_size) {
+		pr_err("The memory size and the record size must be "
+			"non-zero\n");
 		goto fail3;
 	}
 
 	rounddown_pow_of_two(pdata->mem_size);
+	rounddown_pow_of_two(pdata->record_size);
 
-	if (pdata->mem_size < RECORD_SIZE) {
-		pr_err("size too small\n");
+	/* Check for the minimum memory size */
+	if (pdata->mem_size < MIN_MEM_SIZE &&
+			pdata->record_size < MIN_MEM_SIZE) {
+		pr_err("memory size too small, minium is %lu\n", MIN_MEM_SIZE);
 		goto fail3;
 	}
 
-	cxt->max_count = pdata->mem_size / RECORD_SIZE;
+	if (pdata->mem_size < pdata->record_size) {
+		pr_err("The memory size must be larger than the "
+			"records size\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 +212,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 v3 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.

The entry added is /sys/kernel/debug/ramoops/next

The entry returns a dump of size record_size each time, skipping invalid
dumps. When it reaches the end of the memory area reserved for dumps it
returns an empty record and resets the current record count.

Change-Id: Ifbab9af833be9ee0bc1c33bc9871f2fc0eb9d228
Signed-off-by: Sergiu Iordache <sergiu@chromium.org>
---
 drivers/char/ramoops.c |  113 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 111 insertions(+), 2 deletions(-)

diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index bd9b94b..791084c 100644
--- a/drivers/char/ramoops.c
+++ b/drivers/char/ramoops.c
@@ -30,9 +30,13 @@
 #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_NEXT "next"
 
 static ulong record_size = MIN_MEM_SIZE;
 module_param(record_size, ulong, 0400);
@@ -61,12 +65,81 @@ static struct ramoops_context {
 	unsigned long size;
 	unsigned long record_size;
 	int dump_oops;
+	int current_entry;
 	int count;
 	int max_count;
 } oops_cxt;
 
 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_next_entry;
+
+/*
+ * Entry to have access to the memory logged by ramoops. The way the data
+ * is returned is as follows:
+ * Data records are checked one by one for the existence of the header.
+ * If a valid record is found data is returned from it, otherwise it is
+ * skipped.
+ * Once all the records are checked the next call after the last record
+ * will return an empty buffer and the counter is reset.
+ */
+static ssize_t ramoops_read_next(struct file *file, char __user *buf,
+					size_t count, loff_t *ppos)
+{
+	struct ramoops_context *cxt = &oops_cxt;
+
+	mutex_lock(&ramoops_mutex);
+
+	/* Do this check here so it returns an empty file once it resets */
+	if (cxt->current_entry >= cxt->max_count) {
+		cxt->current_entry = 0;
+		count = 0;
+		goto out;
+	}
+
+	/* Check to see if the current dump is valid */
+	while (cxt->current_entry < cxt->max_count
+		&& memcmp(cxt->virt_addr + cxt->record_size *
+			  cxt->current_entry, RAMOOPS_KERNMSG_HDR,
+			  strlen(RAMOOPS_KERNMSG_HDR)))
+		cxt->current_entry++;
+
+	/* In case a dump was not found return 0 */
+	if (cxt->current_entry >= cxt->max_count) {
+		count = 0;
+		goto out;
+	}
+
+	/* Otherwise proceed as normal to return the data */
+	if (*ppos + count > cxt->record_size)
+		count = cxt->record_size - *ppos;
+	if (*ppos > cxt->record_size) {
+		count = 0;
+		cxt->current_entry++;
+		goto out;
+	}
+	if (copy_to_user(buf, cxt->virt_addr + cxt->record_size *
+			 cxt->current_entry + *ppos, count)) {
+		count = -EFAULT;
+		goto out;
+	}
+	*ppos += count;
+
+	/* completed reading this part, go to the next one */
+	if (*ppos == cxt->record_size)
+		cxt->current_entry++;
+
+out:
+	mutex_unlock(&ramoops_mutex);
+	return count;
+}
+
+static const struct file_operations ramoops_next_fops = {
+	.read = ramoops_read_next,
+};
 
 static void ramoops_do_dump(struct kmsg_dumper *dumper,
 		enum kmsg_dump_reason reason, const char *s1, unsigned long l1,
@@ -89,6 +162,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 +184,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)
@@ -128,8 +203,8 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	rounddown_pow_of_two(pdata->record_size);
 
 	/* Check for the minimum memory size */
-	if (pdata->mem_size < MIN_MEM_SIZE &&
-			pdata->record_size < MIN_MEM_SIZE) {
+	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;
 	}
@@ -145,6 +220,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	cxt->size = pdata->mem_size;
 	cxt->phys_addr = pdata->mem_address;
 	cxt->record_size = pdata->record_size;
+	cxt->current_entry = 0;
 	cxt->dump_oops = pdata->dump_oops;
 
 	if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
@@ -166,6 +242,30 @@ 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;
+	}
+
+	/*
+	 * This interface exposes the next valid entry from ramoops, starting
+	 * from the start of memory area. Once there are no more entries it
+	 * returns an empty buffer. Reading the entry again afterwards starts
+	 * from the begining.
+	 */
+	ramoops_next_entry = debugfs_create_file(RAMOOPS_NEXT, 0444,
+					ramoops_dir, NULL, &ramoops_next_fops);
+
+	if (ramoops_next_entry == NULL) {
+		err = -ENOMEM;
+		pr_err("debugfs next entry creation failed\n");
+		goto no_ramoops_next;
+	}
+
+
 	return 0;
 
 fail1:
@@ -174,6 +274,11 @@ fail2:
 	release_mem_region(cxt->phys_addr, cxt->size);
 fail3:
 	return err;
+
+no_ramoops_next:
+	debugfs_remove(ramoops_dir);
+out:
+	return err;
 }
 
 static int __exit ramoops_remove(struct platform_device *pdev)
@@ -231,6 +336,10 @@ static void __exit ramoops_exit(void)
 {
 	platform_driver_unregister(&ramoops_driver);
 	kfree(dummy_data);
+
+	/* Clean up debugfs entries */
+	debugfs_remove(ramoops_next_entry);
+	debugfs_remove(ramoops_dir);
 }
 
 module_init(ramoops_init);
-- 
1.7.3.1

Re: [PATCH v3 0/3] char drivers: rammops improvements

[Marco Stornelli]

Il 07/07/2011 01:29, Sergiu Iordache ha scritto:
> 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 two by being built on the ramoops modifications present in the mmotm tree and by using a different
> approach regarding the debugfs entry.
>
> 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  |  151 +++++++++++++++++++++++++++++++++++++++++++----
>   include/linux/ramoops.h |    2 +
>   2 files changed, 142 insertions(+), 11 deletions(-)
>

The patch series is ok for me.

Marco

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

[Andrew Morton]

On Wed,  6 Jul 2011 16:29:50 -0700
Sergiu Iordache <sergiu@chromium.org> wrote:

> 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.
> 
> The entry added is /sys/kernel/debug/ramoops/next
> 
> The entry returns a dump of size record_size each time, skipping invalid
> dumps. When it reaches the end of the memory area reserved for dumps it
> returns an empty record and resets the current record count.

The patch increases the size of ramoops.o text&data from 1725 bytes to
2282 even when CONFIG_DEBUG_FS=n.  That's quite a lot of bloat!

Furthermore, I think debugfs is the wrong place to put this.  debugfs
is, umm, for debugging.  It's a place in which to put transitory junk
which may or may not be there and where interfaces may change without
notice and whose presence userspace should not assume.  But in this
patch you're proposing a permanent and formal new interface into the
ramoops driver.  It should go into a permanent well-known place where
it will be maintained with the formality and care of all the other
parts of the kernel API.

Also, you're proposing a new kernel/userspace API but it wasn't
documented anywhere.  We don't want to have to reverse-engineer your
proposed API from the implementation for review purposes.  That API
should have been exhaustively documented in the changelog so others can
decide whether they like it.

Finally, we should provide user documentation for the new interface so
others can find out that it exists and can use it correctly.  We seem
not to have bothered documenting ramoops so we don't currently have a
context in which to do this.

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

[Sergiu Iordache]

On Thu, Jul 7, 2011 at 1:01 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed,  6 Jul 2011 16:29:50 -0700
> Sergiu Iordache <sergiu@chromium.org> wrote:
>
>> 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.
>>
>> The entry added is /sys/kernel/debug/ramoops/next
>>
>> The entry returns a dump of size record_size each time, skipping invalid
>> dumps. When it reaches the end of the memory area reserved for dumps it
>> returns an empty record and resets the current record count.
>
> The patch increases the size of ramoops.o text&data from 1725 bytes to
> 2282 even when CONFIG_DEBUG_FS=n.  That's quite a lot of bloat!
>
> Furthermore, I think debugfs is the wrong place to put this.  debugfs
> is, umm, for debugging.  It's a place in which to put transitory junk
> which may or may not be there and where interfaces may change without
> notice and whose presence userspace should not assume.  But in this
> patch you're proposing a permanent and formal new interface into the
> ramoops driver.  It should go into a permanent well-known place where
> it will be maintained with the formality and care of all the other
> parts of the kernel API.
How about not using the debugfs entry and adding a char driver? There
are 2 possibilities here as well: using read operations to return the
data like the current patch does or using ioctl to return the data(and
maybe return other info as well such as the records size and the
memory size). Any suggestions regarding a valid approach?

> Also, you're proposing a new kernel/userspace API but it wasn't
> documented anywhere.  We don't want to have to reverse-engineer your
> proposed API from the implementation for review purposes.  That API
> should have been exhaustively documented in the changelog so others can
> decide whether they like it.
>
> Finally, we should provide user documentation for the new interface so
> others can find out that it exists and can use it correctly.  We seem
> not to have bothered documenting ramoops so we don't currently have a
> context in which to do this.
I'll create some documentation for both the (future) API and the ramoops module.

Sergiu

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

[Andrew Morton]

On Thu, 7 Jul 2011 15:34:26 -0700
Sergiu Iordache <sergiu@chromium.org> wrote:

> On Thu, Jul 7, 2011 at 1:01 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Wed, __6 Jul 2011 16:29:50 -0700
> > Sergiu Iordache <sergiu@chromium.org> wrote:
> >
> >> 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.
> >>
> >> The entry added is /sys/kernel/debug/ramoops/next
> >>
> >> The entry returns a dump of size record_size each time, skipping invalid
> >> dumps. When it reaches the end of the memory area reserved for dumps it
> >> returns an empty record and resets the current record count.
> >
> > The patch increases the size of ramoops.o text&data from 1725 bytes to
> > 2282 even when CONFIG_DEBUG_FS=n. __That's quite a lot of bloat!
> >
> > Furthermore, I think debugfs is the wrong place to put this. __debugfs
> > is, umm, for debugging. __It's a place in which to put transitory junk
> > which may or may not be there and where interfaces may change without
> > notice and whose presence userspace should not assume. __But in this
> > patch you're proposing a permanent and formal new interface into the
> > ramoops driver. __It should go into a permanent well-known place where
> > it will be maintained with the formality and care of all the other
> > parts of the kernel API.
> How about not using the debugfs entry and adding a char driver? There
> are 2 possibilities here as well: using read operations to return the
> data like the current patch does or using ioctl to return the data(and
> maybe return other info as well such as the records size and the
> memory size). Any suggestions regarding a valid approach?

Well.  I haven't seen a description of what the interfaces *does* (and
I'm too obstinate to work that out from the patch) so it's hard to
advise.

ioctls are unpopular.

A char driver always seems weird and fake to me - there's no underlying
device.  /dev/zero considered harmful!

Perhaps switch it to sysfs?

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

[Sergiu Iordache]

On Thu, Jul 7, 2011 at 3:54 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 7 Jul 2011 15:34:26 -0700
> Sergiu Iordache <sergiu@chromium.org> wrote:
>
>> On Thu, Jul 7, 2011 at 1:01 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>> > On Wed, __6 Jul 2011 16:29:50 -0700
>> > Sergiu Iordache <sergiu@chromium.org> wrote:
>> >
>> >> 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.
>> >>
>> >> The entry added is /sys/kernel/debug/ramoops/next
>> >>
>> >> The entry returns a dump of size record_size each time, skipping invalid
>> >> dumps. When it reaches the end of the memory area reserved for dumps it
>> >> returns an empty record and resets the current record count.
>> >
>> > The patch increases the size of ramoops.o text&data from 1725 bytes to
>> > 2282 even when CONFIG_DEBUG_FS=n. __That's quite a lot of bloat!
>> >
>> > Furthermore, I think debugfs is the wrong place to put this. __debugfs
>> > is, umm, for debugging. __It's a place in which to put transitory junk
>> > which may or may not be there and where interfaces may change without
>> > notice and whose presence userspace should not assume. __But in this
>> > patch you're proposing a permanent and formal new interface into the
>> > ramoops driver. __It should go into a permanent well-known place where
>> > it will be maintained with the formality and care of all the other
>> > parts of the kernel API.
>> How about not using the debugfs entry and adding a char driver? There
>> are 2 possibilities here as well: using read operations to return the
>> data like the current patch does or using ioctl to return the data(and
>> maybe return other info as well such as the records size and the
>> memory size). Any suggestions regarding a valid approach?
>
> Well.  I haven't seen a description of what the interfaces *does* (and
> I'm too obstinate to work that out from the patch) so it's hard to
> advise.
>
> ioctls are unpopular.
>
> A char driver always seems weird and fake to me - there's no underlying
> device.  /dev/zero considered harmful!
>
> Perhaps switch it to sysfs?
I tried to explain it in the patch message, sorry if I was not clear enough.

Ramoops currently dumps the log of a panic/oops in a memory area which
is known not to be overwritten on restart (for example 1MB starting at
15MB). The way it works is by dividing the memory area in records of a
set size (fixed at 4K before my patches, configurable after) and by
dumping a record there for each oops/panic. The problem is that right
now you have to access that memory area through other means, such as
/dev/mem, which is not always possible.

What my patch did was to add a debugfs entry which returns a valid
record each time (a single dump done by ramoops). The first call
returns the first dump. The first call after the last valid dump
returns an empty buffer. . After it has returned nothing, the next
calls return records from the start again. The validity of a dump is
checked by looking after the header. Any comments on this approach are
welcome.

Changing the entry from debugfs to sysfs wouldn't be a problem. If
sysfs is a valid solution I'll come with a patch that updates the
documentation as well along with the sysfs entry.

Sergiu

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

[Andrew Morton]

On Thu, 7 Jul 2011 16:16:43 -0700
Sergiu Iordache <sergiu@google.com> wrote:

> Ramoops currently dumps the log of a panic/oops in a memory area which
> is known not to be overwritten on restart (for example 1MB starting at
> 15MB). The way it works is by dividing the memory area in records of a
> set size (fixed at 4K before my patches, configurable after) and by
> dumping a record there for each oops/panic. The problem is that right
> now you have to access that memory area through other means, such as
> /dev/mem, which is not always possible.
> 
> What my patch did was to add a debugfs entry which returns a valid
> record each time (a single dump done by ramoops). The first call
> returns the first dump. The first call after the last valid dump
> returns an empty buffer. .

Please fully describe this "record" in the v2 patch changelog.  We'll
want to review it for endianness, 32/64-bit compat issues,
maintainability, extensibility, etc.

> After it has returned nothing, the next
> calls return records from the start again.

That sounds a bit weird.  One would expect it to keep returning zero,
requiring userspace to lseek or close/open.

> The validity of a dump is
> checked by looking after the header. Any comments on this approach are
> welcome.
> 
> Changing the entry from debugfs to sysfs wouldn't be a problem. If
> sysfs is a valid solution I'll come with a patch that updates the
> documentation as well along with the sysfs entry.

sysfs sounds OK to me.  Then again, sysfs is supposed to be
one-value-per-file, so using it would be naughty.

I dunno, I'd be inclined to abuse the sysfs rule and hope that nobody
notices rather than create a fake char device.  But there's certainly
plenty of precedent for the fake char driver.

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

[Greg KH]

On Thu, Jul 07, 2011 at 04:27:27PM -0700, Andrew Morton wrote:
> On Thu, 7 Jul 2011 16:16:43 -0700
> Sergiu Iordache <sergiu@google.com> wrote:
> 
> > Ramoops currently dumps the log of a panic/oops in a memory area which
> > is known not to be overwritten on restart (for example 1MB starting at
> > 15MB). The way it works is by dividing the memory area in records of a
> > set size (fixed at 4K before my patches, configurable after) and by
> > dumping a record there for each oops/panic. The problem is that right
> > now you have to access that memory area through other means, such as
> > /dev/mem, which is not always possible.
> > 
> > What my patch did was to add a debugfs entry which returns a valid
> > record each time (a single dump done by ramoops). The first call
> > returns the first dump. The first call after the last valid dump
> > returns an empty buffer. .
> 
> Please fully describe this "record" in the v2 patch changelog.  We'll
> want to review it for endianness, 32/64-bit compat issues,
> maintainability, extensibility, etc.
> 
> > After it has returned nothing, the next
> > calls return records from the start again.
> 
> That sounds a bit weird.  One would expect it to keep returning zero,
> requiring userspace to lseek or close/open.
> 
> > The validity of a dump is
> > checked by looking after the header. Any comments on this approach are
> > welcome.
> > 
> > Changing the entry from debugfs to sysfs wouldn't be a problem. If
> > sysfs is a valid solution I'll come with a patch that updates the
> > documentation as well along with the sysfs entry.
> 
> sysfs sounds OK to me.  Then again, sysfs is supposed to be
> one-value-per-file, so using it would be naughty.
> 
> I dunno, I'd be inclined to abuse the sysfs rule and hope that nobody
> notices rather than create a fake char device.  But there's certainly
> plenty of precedent for the fake char driver.

No, please don't abuse sysfs that way.

Use debugfs or a char device node.

thanks,

greg k-h

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

[Marco Stornelli]

2011/7/8 Greg KH <greg@kroah.com>:
> On Thu, Jul 07, 2011 at 04:27:27PM -0700, Andrew Morton wrote:
>> On Thu, 7 Jul 2011 16:16:43 -0700
>> Sergiu Iordache <sergiu@google.com> wrote:
>>
>> > Ramoops currently dumps the log of a panic/oops in a memory area which
>> > is known not to be overwritten on restart (for example 1MB starting at
>> > 15MB). The way it works is by dividing the memory area in records of a
>> > set size (fixed at 4K before my patches, configurable after) and by
>> > dumping a record there for each oops/panic. The problem is that right
>> > now you have to access that memory area through other means, such as
>> > /dev/mem, which is not always possible.
>> >
>> > What my patch did was to add a debugfs entry which returns a valid
>> > record each time (a single dump done by ramoops). The first call
>> > returns the first dump. The first call after the last valid dump
>> > returns an empty buffer. .
>>
>> Please fully describe this "record" in the v2 patch changelog.  We'll
>> want to review it for endianness, 32/64-bit compat issues,
>> maintainability, extensibility, etc.
>>
>> > After it has returned nothing, the next
>> > calls return records from the start again.
>>
>> That sounds a bit weird.  One would expect it to keep returning zero,
>> requiring userspace to lseek or close/open.
>>
>> > The validity of a dump is
>> > checked by looking after the header. Any comments on this approach are
>> > welcome.
>> >
>> > Changing the entry from debugfs to sysfs wouldn't be a problem. If
>> > sysfs is a valid solution I'll come with a patch that updates the
>> > documentation as well along with the sysfs entry.
>>
>> sysfs sounds OK to me.  Then again, sysfs is supposed to be
>> one-value-per-file, so using it would be naughty.
>>
>> I dunno, I'd be inclined to abuse the sysfs rule and hope that nobody
>> notices rather than create a fake char device.  But there's certainly
>> plenty of precedent for the fake char driver.
>
> No, please don't abuse sysfs that way.
>
> Use debugfs or a char device node.
>
> thanks,
>
> greg k-h
>

I agree with Greg. I asked to not break the existent way to read data
via /dev/mem because for me it's the right way to do this thing.
However to do an easy *debug* a debugfs entry can be useful. IMHO, a
"production" script/application that use debugfs instead of /dev/mem
in this case is simply broken because the debugfs can't be like a
system call or other kernel interaction mechanism. Debugfs should be
used only for debug.

Marco

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

[Sergiu Iordache]

On Thu, Jul 7, 2011 at 11:43 PM, Marco Stornelli
<marco.stornelli@gmail.com> wrote:
> 2011/7/8 Greg KH <greg@kroah.com>:
>> On Thu, Jul 07, 2011 at 04:27:27PM -0700, Andrew Morton wrote:
>>> On Thu, 7 Jul 2011 16:16:43 -0700
>>> Sergiu Iordache <sergiu@google.com> wrote:
>>>
>>> > Ramoops currently dumps the log of a panic/oops in a memory area which
>>> > is known not to be overwritten on restart (for example 1MB starting at
>>> > 15MB). The way it works is by dividing the memory area in records of a
>>> > set size (fixed at 4K before my patches, configurable after) and by
>>> > dumping a record there for each oops/panic. The problem is that right
>>> > now you have to access that memory area through other means, such as
>>> > /dev/mem, which is not always possible.
>>> >
>>> > What my patch did was to add a debugfs entry which returns a valid
>>> > record each time (a single dump done by ramoops). The first call
>>> > returns the first dump. The first call after the last valid dump
>>> > returns an empty buffer. .
>>>
>>> Please fully describe this "record" in the v2 patch changelog.  We'll
>>> want to review it for endianness, 32/64-bit compat issues,
>>> maintainability, extensibility, etc.
>>>
>>> > After it has returned nothing, the next
>>> > calls return records from the start again.
>>>
>>> That sounds a bit weird.  One would expect it to keep returning zero,
>>> requiring userspace to lseek or close/open.
>>>
>>> > The validity of a dump is
>>> > checked by looking after the header. Any comments on this approach are
>>> > welcome.
>>> >
>>> > Changing the entry from debugfs to sysfs wouldn't be a problem. If
>>> > sysfs is a valid solution I'll come with a patch that updates the
>>> > documentation as well along with the sysfs entry.
>>>
>>> sysfs sounds OK to me.  Then again, sysfs is supposed to be
>>> one-value-per-file, so using it would be naughty.
>>>
>>> I dunno, I'd be inclined to abuse the sysfs rule and hope that nobody
>>> notices rather than create a fake char device.  But there's certainly
>>> plenty of precedent for the fake char driver.
>>
>> No, please don't abuse sysfs that way.
>>
>> Use debugfs or a char device node.
>>
>> thanks,
>>
>> greg k-h
>>
>
> I agree with Greg. I asked to not break the existent way to read data
> via /dev/mem because for me it's the right way to do this thing.
> However to do an easy *debug* a debugfs entry can be useful. IMHO, a
> "production" script/application that use debugfs instead of /dev/mem
> in this case is simply broken because the debugfs can't be like a
> system call or other kernel interaction mechanism. Debugfs should be
> used only for debug.
>
> Marco

Any consensus/decision on how to go on with this patch idea?

The options that I see right now are:
- keep access through /dev/mem only (but access to /dev/mem is
sometimes restricted);
- keep the debugfs entry as well(as in the patch);
- remove the debugfs entry and add a char driver to access the memory
using read and seek operations.

+ the rejected(?) options from before

Sergiu

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

[Marco Stornelli]

2011/7/11 Sergiu Iordache <sergiu@chromium.org>:
> On Thu, Jul 7, 2011 at 11:43 PM, Marco Stornelli
> <marco.stornelli@gmail.com> wrote:
>> 2011/7/8 Greg KH <greg@kroah.com>:
>>> On Thu, Jul 07, 2011 at 04:27:27PM -0700, Andrew Morton wrote:
>>>> On Thu, 7 Jul 2011 16:16:43 -0700
>>>> Sergiu Iordache <sergiu@google.com> wrote:
>>>>
>>>> > Ramoops currently dumps the log of a panic/oops in a memory area which
>>>> > is known not to be overwritten on restart (for example 1MB starting at
>>>> > 15MB). The way it works is by dividing the memory area in records of a
>>>> > set size (fixed at 4K before my patches, configurable after) and by
>>>> > dumping a record there for each oops/panic. The problem is that right
>>>> > now you have to access that memory area through other means, such as
>>>> > /dev/mem, which is not always possible.
>>>> >
>>>> > What my patch did was to add a debugfs entry which returns a valid
>>>> > record each time (a single dump done by ramoops). The first call
>>>> > returns the first dump. The first call after the last valid dump
>>>> > returns an empty buffer. .
>>>>
>>>> Please fully describe this "record" in the v2 patch changelog.  We'll
>>>> want to review it for endianness, 32/64-bit compat issues,
>>>> maintainability, extensibility, etc.
>>>>
>>>> > After it has returned nothing, the next
>>>> > calls return records from the start again.
>>>>
>>>> That sounds a bit weird.  One would expect it to keep returning zero,
>>>> requiring userspace to lseek or close/open.
>>>>
>>>> > The validity of a dump is
>>>> > checked by looking after the header. Any comments on this approach are
>>>> > welcome.
>>>> >
>>>> > Changing the entry from debugfs to sysfs wouldn't be a problem. If
>>>> > sysfs is a valid solution I'll come with a patch that updates the
>>>> > documentation as well along with the sysfs entry.
>>>>
>>>> sysfs sounds OK to me.  Then again, sysfs is supposed to be
>>>> one-value-per-file, so using it would be naughty.
>>>>
>>>> I dunno, I'd be inclined to abuse the sysfs rule and hope that nobody
>>>> notices rather than create a fake char device.  But there's certainly
>>>> plenty of precedent for the fake char driver.
>>>
>>> No, please don't abuse sysfs that way.
>>>
>>> Use debugfs or a char device node.
>>>
>>> thanks,
>>>
>>> greg k-h
>>>
>>
>> I agree with Greg. I asked to not break the existent way to read data
>> via /dev/mem because for me it's the right way to do this thing.
>> However to do an easy *debug* a debugfs entry can be useful. IMHO, a
>> "production" script/application that use debugfs instead of /dev/mem
>> in this case is simply broken because the debugfs can't be like a
>> system call or other kernel interaction mechanism. Debugfs should be
>> used only for debug.
>>
>> Marco
>
> Any consensus/decision on how to go on with this patch idea?
>
> The options that I see right now are:
> - keep access through /dev/mem only (but access to /dev/mem is
> sometimes restricted);
> - keep the debugfs entry as well(as in the patch);
> - remove the debugfs entry and add a char driver to access the memory
> using read and seek operations.
>
> + the rejected(?) options from before
>
> Sergiu
>

For me the best option it's to use a sysfs/proc entry to export
(read-only) the memory address, record size etc. At that point we can
use a generic script/program to access via /dev/mem. However I let
Andrew/Greg say the last word.

Marco

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

[Sergiu Iordache]

On Mon, Jul 11, 2011 at 11:41 PM, Marco Stornelli
<marco.stornelli@gmail.com> wrote:
> 2011/7/11 Sergiu Iordache <sergiu@chromium.org>:
>> On Thu, Jul 7, 2011 at 11:43 PM, Marco Stornelli
>> <marco.stornelli@gmail.com> wrote:
>>> 2011/7/8 Greg KH <greg@kroah.com>:
>>>> On Thu, Jul 07, 2011 at 04:27:27PM -0700, Andrew Morton wrote:
>>>>> On Thu, 7 Jul 2011 16:16:43 -0700
>>>>> Sergiu Iordache <sergiu@google.com> wrote:
>>>>>
>>>>> > Ramoops currently dumps the log of a panic/oops in a memory area which
>>>>> > is known not to be overwritten on restart (for example 1MB starting at
>>>>> > 15MB). The way it works is by dividing the memory area in records of a
>>>>> > set size (fixed at 4K before my patches, configurable after) and by
>>>>> > dumping a record there for each oops/panic. The problem is that right
>>>>> > now you have to access that memory area through other means, such as
>>>>> > /dev/mem, which is not always possible.
>>>>> >
>>>>> > What my patch did was to add a debugfs entry which returns a valid
>>>>> > record each time (a single dump done by ramoops). The first call
>>>>> > returns the first dump. The first call after the last valid dump
>>>>> > returns an empty buffer. .
>>>>>
>>>>> Please fully describe this "record" in the v2 patch changelog.  We'll
>>>>> want to review it for endianness, 32/64-bit compat issues,
>>>>> maintainability, extensibility, etc.
>>>>>
>>>>> > After it has returned nothing, the next
>>>>> > calls return records from the start again.
>>>>>
>>>>> That sounds a bit weird.  One would expect it to keep returning zero,
>>>>> requiring userspace to lseek or close/open.
>>>>>
>>>>> > The validity of a dump is
>>>>> > checked by looking after the header. Any comments on this approach are
>>>>> > welcome.
>>>>> >
>>>>> > Changing the entry from debugfs to sysfs wouldn't be a problem. If
>>>>> > sysfs is a valid solution I'll come with a patch that updates the
>>>>> > documentation as well along with the sysfs entry.
>>>>>
>>>>> sysfs sounds OK to me.  Then again, sysfs is supposed to be
>>>>> one-value-per-file, so using it would be naughty.
>>>>>
>>>>> I dunno, I'd be inclined to abuse the sysfs rule and hope that nobody
>>>>> notices rather than create a fake char device.  But there's certainly
>>>>> plenty of precedent for the fake char driver.
>>>>
>>>> No, please don't abuse sysfs that way.
>>>>
>>>> Use debugfs or a char device node.
>>>>
>>>> thanks,
>>>>
>>>> greg k-h
>>>>
>>>
>>> I agree with Greg. I asked to not break the existent way to read data
>>> via /dev/mem because for me it's the right way to do this thing.
>>> However to do an easy *debug* a debugfs entry can be useful. IMHO, a
>>> "production" script/application that use debugfs instead of /dev/mem
>>> in this case is simply broken because the debugfs can't be like a
>>> system call or other kernel interaction mechanism. Debugfs should be
>>> used only for debug.
>>>
>>> Marco
>>
>> Any consensus/decision on how to go on with this patch idea?
>>
>> The options that I see right now are:
>> - keep access through /dev/mem only (but access to /dev/mem is
>> sometimes restricted);
>> - keep the debugfs entry as well(as in the patch);
>> - remove the debugfs entry and add a char driver to access the memory
>> using read and seek operations.
>>
>> + the rejected(?) options from before
>>
>> Sergiu
>>
>
> For me the best option it's to use a sysfs/proc entry to export
> (read-only) the memory address, record size etc. At that point we can
> use a generic script/program to access via /dev/mem. However I let
> Andrew/Greg say the last word.

Well, since the only method to read the dump data is /dev/mem,
exporting the record size/address/etc is needed in order to parse it
properly. But as far as I can see the data is already exported through
sysfs in /sys/module/ramoops/parameters/.
The current module still needs a patch to write the variables of the
module parameters from the platform data in case that is used, but is
there any reason why we would need other sysfs entries except these?

Sergiu

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

[Marco Stornelli]

Il 29/07/2011 02:15, Sergiu Iordache ha scritto:
> On Mon, Jul 11, 2011 at 11:41 PM, Marco Stornelli
> <marco.stornelli@gmail.com>  wrote:
>> 2011/7/11 Sergiu Iordache<sergiu@chromium.org>:
>>> On Thu, Jul 7, 2011 at 11:43 PM, Marco Stornelli
>>> <marco.stornelli@gmail.com>  wrote:
>>>> 2011/7/8 Greg KH<greg@kroah.com>:
>>>>> On Thu, Jul 07, 2011 at 04:27:27PM -0700, Andrew Morton wrote:
>>>>>> On Thu, 7 Jul 2011 16:16:43 -0700
>>>>>> Sergiu Iordache<sergiu@google.com>  wrote:
>>>>>>
>>>>>>> Ramoops currently dumps the log of a panic/oops in a memory area which
>>>>>>> is known not to be overwritten on restart (for example 1MB starting at
>>>>>>> 15MB). The way it works is by dividing the memory area in records of a
>>>>>>> set size (fixed at 4K before my patches, configurable after) and by
>>>>>>> dumping a record there for each oops/panic. The problem is that right
>>>>>>> now you have to access that memory area through other means, such as
>>>>>>> /dev/mem, which is not always possible.
>>>>>>>
>>>>>>> What my patch did was to add a debugfs entry which returns a valid
>>>>>>> record each time (a single dump done by ramoops). The first call
>>>>>>> returns the first dump. The first call after the last valid dump
>>>>>>> returns an empty buffer. .
>>>>>>
>>>>>> Please fully describe this "record" in the v2 patch changelog.  We'll
>>>>>> want to review it for endianness, 32/64-bit compat issues,
>>>>>> maintainability, extensibility, etc.
>>>>>>
>>>>>>> After it has returned nothing, the next
>>>>>>> calls return records from the start again.
>>>>>>
>>>>>> That sounds a bit weird.  One would expect it to keep returning zero,
>>>>>> requiring userspace to lseek or close/open.
>>>>>>
>>>>>>> The validity of a dump is
>>>>>>> checked by looking after the header. Any comments on this approach are
>>>>>>> welcome.
>>>>>>>
>>>>>>> Changing the entry from debugfs to sysfs wouldn't be a problem. If
>>>>>>> sysfs is a valid solution I'll come with a patch that updates the
>>>>>>> documentation as well along with the sysfs entry.
>>>>>>
>>>>>> sysfs sounds OK to me.  Then again, sysfs is supposed to be
>>>>>> one-value-per-file, so using it would be naughty.
>>>>>>
>>>>>> I dunno, I'd be inclined to abuse the sysfs rule and hope that nobody
>>>>>> notices rather than create a fake char device.  But there's certainly
>>>>>> plenty of precedent for the fake char driver.
>>>>>
>>>>> No, please don't abuse sysfs that way.
>>>>>
>>>>> Use debugfs or a char device node.
>>>>>
>>>>> thanks,
>>>>>
>>>>> greg k-h
>>>>>
>>>>
>>>> I agree with Greg. I asked to not break the existent way to read data
>>>> via /dev/mem because for me it's the right way to do this thing.
>>>> However to do an easy *debug* a debugfs entry can be useful. IMHO, a
>>>> "production" script/application that use debugfs instead of /dev/mem
>>>> in this case is simply broken because the debugfs can't be like a
>>>> system call or other kernel interaction mechanism. Debugfs should be
>>>> used only for debug.
>>>>
>>>> Marco
>>>
>>> Any consensus/decision on how to go on with this patch idea?
>>>
>>> The options that I see right now are:
>>> - keep access through /dev/mem only (but access to /dev/mem is
>>> sometimes restricted);
>>> - keep the debugfs entry as well(as in the patch);
>>> - remove the debugfs entry and add a char driver to access the memory
>>> using read and seek operations.
>>>
>>> + the rejected(?) options from before
>>>
>>> Sergiu
>>>
>>
>> For me the best option it's to use a sysfs/proc entry to export
>> (read-only) the memory address, record size etc. At that point we can
>> use a generic script/program to access via /dev/mem. However I let
>> Andrew/Greg say the last word.
>
> Well, since the only method to read the dump data is /dev/mem,
> exporting the record size/address/etc is needed in order to parse it
> properly. But as far as I can see the data is already exported through
> sysfs in /sys/module/ramoops/parameters/.
> The current module still needs a patch to write the variables of the
> module parameters from the platform data in case that is used, but is
> there any reason why we would need other sysfs entries except these?

I'd say no. I think it's sufficient.

Marco