[PATCH 0/3] char drivers: ramoops improvements

[PATCH 0/3] char drivers: ramoops improvements

[Sergiu Iordache]

This series of patches improves ramoops by allowing it to be used only with
module parameters (without having to set a platform device externally),
adding a new parameter for the dump size in order to be able to save larger
logs and updating the platform data structure to allow setting all the
options through it.

Sergiu Iordache (3):
  char drivers: ramoops default platform device for module parameter
    use
  char drivers: ramoops dump_oops platform data
  char drivers: ramoops record_size module parameter

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

-- 
1.7.3.1

[PATCH 1/3] char drivers: ramoops default platform device for module parameter use

[Sergiu Iordache]

The introduction of platform data in ramoops resulted in the need to
define a platform device in order to use the ramoops module with module
parameters (without setting any platform data). Sadly this is not clear or
properly documented.

This patch checks if the module parameters are set and declares a platform
device if they are so that the module can be used with module parameters
without any aditional declarations out of the module.

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

diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index 1a9f5f6..0c3cb42 100644
--- a/drivers/char/ramoops.c
+++ b/drivers/char/ramoops.c
@@ -177,13 +177,33 @@ static struct platform_driver ramoops_driver = {
 	},
 };
 
+struct platform_device *ramoops_device;
+
 static int __init ramoops_init(void)
 {
+	/*
+	 * If mem_address and mem_size are set (i.e. the module parameters
+	 * are used) then a default platform driver is created
+	 * as there is no need to configure the platform data elsewhere.
+	 */
+	if (mem_address && mem_size) {
+		ramoops_device = platform_device_register_simple("ramoops",
+								 -1, NULL, 0);
+
+		if (IS_ERR(ramoops_device)) {
+			printk(KERN_ERR "Unable to register platform device\n");
+			return PTR_ERR(ramoops_device);
+		}
+	}
+
 	return platform_driver_probe(&ramoops_driver, ramoops_probe);
 }
 
 static void __exit ramoops_exit(void)
 {
+	/* Unregister the device if it was set */
+	if (ramoops_device)
+		platform_device_unregister(ramoops_device);
 	platform_driver_unregister(&ramoops_driver);
 }
 
-- 
1.7.3.1

[PATCH 2/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.

Signed-off-by: Sergiu Iordache <sergiu@chromium.org>
---
 drivers/char/ramoops.c  |    1 +
 include/linux/ramoops.h |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index 0c3cb42..56338a3 100644
--- a/drivers/char/ramoops.c
+++ b/drivers/char/ramoops.c
@@ -109,6 +109,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	if (pdata) {
 		mem_size = pdata->mem_size;
 		mem_address = pdata->mem_address;
+		dump_oops = pdata->dump_oops;
 	}
 
 	if (!mem_size) {
diff --git a/include/linux/ramoops.h b/include/linux/ramoops.h
index 0ae68a2..1d05505 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 char	dump_oops;
 };
 
 #endif
-- 
1.7.3.1

[PATCH 3/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.

Change-Id: Ie6bd28a898dab476abf34decb0eecc42122f17ce
Signed-off-by: Sergiu Iordache <sergiu@chromium.org>
---
 drivers/char/ramoops.c  |   18 +++++++++++-------
 include/linux/ramoops.h |    1 +
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index 56338a3..b16cf07 100644
--- a/drivers/char/ramoops.c
+++ b/drivers/char/ramoops.c
@@ -30,7 +30,10 @@
 
 #define RAMOOPS_KERNMSG_HDR "===="
 
-#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);
@@ -77,10 +80,10 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
 	if (reason == KMSG_DUMP_OOPS && !dump_oops)
 		return;
 
-	buf = cxt->virt_addr + (cxt->count * RECORD_SIZE);
+	buf = cxt->virt_addr + (cxt->count * record_size);
 	buf_orig = buf;
 
-	memset(buf, '\0', RECORD_SIZE);
+	memset(buf, '\0', record_size);
 	res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
 	buf += res;
 	do_gettimeofday(&timestamp);
@@ -88,8 +91,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, record_size - hdr_size);
+	l1_cpy = min(l1, record_size - hdr_size - l2_cpy);
 
 	s2_start = l2 - l2_cpy;
 	s1_start = l1 - l1_cpy;
@@ -110,6 +113,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
 		mem_size = pdata->mem_size;
 		mem_address = pdata->mem_address;
 		dump_oops = pdata->dump_oops;
+		record_size = pdata->record_size;
 	}
 
 	if (!mem_size) {
@@ -119,12 +123,12 @@ static int __init ramoops_probe(struct platform_device *pdev)
 
 	rounddown_pow_of_two(mem_size);
 
-	if (mem_size < RECORD_SIZE) {
+	if (mem_size < record_size) {
 		printk(KERN_ERR "ramoops: size too small");
 		goto fail3;
 	}
 
-	cxt->max_count = mem_size / RECORD_SIZE;
+	cxt->max_count = mem_size / record_size;
 	cxt->count = 0;
 	cxt->size = mem_size;
 	cxt->phys_addr = mem_address;
diff --git a/include/linux/ramoops.h b/include/linux/ramoops.h
index 1d05505..cfa2623 100644
--- a/include/linux/ramoops.h
+++ b/include/linux/ramoops.h
@@ -11,6 +11,7 @@ struct ramoops_platform_data {
 	unsigned long	mem_size;
 	unsigned long	mem_address;
 	unsigned char	dump_oops;
+	unsigned long	record_size;
 };
 
 #endif
-- 
1.7.3.1

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

[Marco Stornelli]

Hi,

2011/6/28 Sergiu Iordache <sergiu@chromium.org>:
> This series of patches improves ramoops by allowing it to be used only with
> module parameters (without having to set a platform device externally),
> adding a new parameter for the dump size in order to be able to save larger
> logs and updating the platform data structure to allow setting all the
> options through it.
>

there are already a couple of patches in the Andrew's tree, you can
look at them.

Marco

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

[Marco Stornelli]

2011/6/28 Sergiu Iordache <sergiu@chromium.org>:
> 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.
>
> Signed-off-by: Sergiu Iordache <sergiu@chromium.org>
> ---

It makes sense.

Marco

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

[Marco Stornelli]

Hi,

2011/6/28 Sergiu Iordache <sergiu@chromium.org>:
> 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.
>
> Change-Id: Ie6bd28a898dab476abf34decb0eecc42122f17ce
> Signed-off-by: Sergiu Iordache <sergiu@chromium.org>
> ---

the idea can be valid, but you have to add some check to set the
record size. It'd be better to add a lower and upper bound and to
check for it's power of two.

Marco

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

[Sergiu Iordache]

On Mon, Jun 27, 2011 at 11:39 PM, Marco Stornelli
<marco.stornelli@gmail.com> wrote:
> Hi,
>
> 2011/6/28 Sergiu Iordache <sergiu@chromium.org>:
>> 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.
>>
>> Change-Id: Ie6bd28a898dab476abf34decb0eecc42122f17ce
>> Signed-off-by: Sergiu Iordache <sergiu@chromium.org>
>> ---
>
> the idea can be valid, but you have to add some check to set the
> record size. It'd be better to add a lower and upper bound and to
> check for it's power of two.

That sounds like a good idea. Since the memory size gets rounded to a
power of two it would probably be more consistent to round down the
record size as well. This way you would be sure that mem_size is a
multiple of record size as well. The upper bound would be the memory
size, which is already checked. I'm not sure whether it would be a
good idea to add lower bound different from record_size != 0 (I don't
know why someone would need to dump 8 bytes for example but is there a
reason to limit it?)

Sergiu

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

[Marco Stornelli]

Il 28/06/2011 18:38, Sergiu Iordache ha scritto:
> On Mon, Jun 27, 2011 at 11:39 PM, Marco Stornelli
> <marco.stornelli@gmail.com>  wrote:
>> Hi,
>>
>> 2011/6/28 Sergiu Iordache<sergiu@chromium.org>:
>>> 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.
>>>
>>> Change-Id: Ie6bd28a898dab476abf34decb0eecc42122f17ce
>>> Signed-off-by: Sergiu Iordache<sergiu@chromium.org>
>>> ---
>>
>> the idea can be valid, but you have to add some check to set the
>> record size. It'd be better to add a lower and upper bound and to
>> check for it's power of two.
>
> That sounds like a good idea. Since the memory size gets rounded to a
> power of two it would probably be more consistent to round down the
> record size as well. This way you would be sure that mem_size is a
> multiple of record size as well. The upper bound would be the memory
> size, which is already checked. I'm not sure whether it would be a
> good idea to add lower bound different from record_size != 0 (I don't
> know why someone would need to dump 8 bytes for example but is there a
> reason to limit it?)
>
> Sergiu
>

I meant to use (as at the moment) min 4k for record size and so min 4k 
for mem_size.

Marco

Re: [PATCH 1/3] char drivers: ramoops default platform device for module parameter use

[Daniel Baluta]

> +struct platform_device *ramoops_device;

This should be static.

> +
>  static int __init ramoops_init(void)
>  {
> +       /*
> +        * If mem_address and mem_size are set (i.e. the module parameters
> +        * are used) then a default platform driver is created
> +        * as there is no need to configure the platform data elsewhere.
> +        */
> +       if (mem_address && mem_size) {
You should get back to this check after introducing record_size.

thanks,
Daniel.