ramoops: use module parameters instead of platform data if not available

PATCH Superseded Marco Stornelli 4 participants 12 messages May 28, 2011 – Jun 18, 2011 Char and misc drivers

lore

Share & tools

lore

Marco Stornelli

author

2011-05-28 09:07 UTC #0 lore raw

PATCH 1/1 ramoops: use module parameters instead of platform data if not available

From: Marco Stornelli <[email protected]>

Use generic module parameters instead of platform data, if platform data are not available. This limitation has been introduced with commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0.

Signed-off-by Marco Stornelli <[email protected]>

CC Kyungmin Park <[email protected]>

Reported-by Stevie Trujillo <[email protected]>

---

drivers/char/ramoops.c +33 ~~-12~~

index 1a9f5f6..a201f81 100644
@@ -26,6 +26,7 @@
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 #include <linux/ramoops.h>
 
 #define RAMOOPS_KERNMSG_HDR "===="

@@ -56,6 +57,9 @@ static struct ramoops_context {
 	int max_count;
 } oops_cxt;
 
+static struct platform_device *dummy;
+static struct ramoops_platform_data *dummy_data;
+
 static void ramoops_do_dump(struct kmsg_dumper *dumper,
 		enum kmsg_dump_reason reason, const char *s1, unsigned long l1,
 		const char *s2, unsigned long l2)

@@ -106,27 +110,22 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	struct ramoops_context *cxt = &oops_cxt;
 	int err = -EINVAL;
 
-	if (pdata) {
-		mem_size = pdata->mem_size;
-		mem_address = pdata->mem_address;
-	}
-
-	if (!mem_size) {
+	if (!pdata->mem_size) {
 		printk(KERN_ERR "ramoops: invalid size specification");
 		goto fail3;
 	}
 
-	rounddown_pow_of_two(mem_size);
+	rounddown_pow_of_two(pdata->mem_size);
 
-	if (mem_size < RECORD_SIZE) {
+	if (pdata->mem_size < RECORD_SIZE) {
 		printk(KERN_ERR "ramoops: size too small");
 		goto fail3;
 	}
 
-	cxt->max_count = mem_size / RECORD_SIZE;
+	cxt->max_count = pdata->mem_size / RECORD_SIZE;
 	cxt->count = 0;
-	cxt->size = mem_size;
-	cxt->phys_addr = mem_address;
+	cxt->size = pdata->mem_size;
+	cxt->phys_addr = pdata->mem_address;
 
 	if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
 		printk(KERN_ERR "ramoops: request mem region failed");

@@ -179,12 +178,34 @@ static struct platform_driver ramoops_driver = {
 
 static int __init ramoops_init(void)
 {
-	return platform_driver_probe(&ramoops_driver, ramoops_probe);
+	int ret;
+	ret = platform_driver_probe(&ramoops_driver, ramoops_probe);
+	if (ret == -ENODEV)
+	{
+	   /* 
+	    * if we didn't find a platform device, we use module parameters
+	    * building platform data on the fly.
+	    */
+	   dummy_data = (struct ramoops_platform_data *) 
+		     kzalloc(sizeof(struct ramoops_platform_data), GFP_KERNEL);
+	   dummy_data->mem_size = mem_size;
+	   dummy_data->mem_address = mem_address;
+	   dummy = platform_create_bundle(&ramoops_driver, ramoops_probe, NULL,
+			0, dummy_data, sizeof(struct ramoops_platform_data));
+
+	   if (IS_ERR(dummy)) 
+	     ret = PTR_ERR(dummy);
+	   else
+	     ret = 0;
+	}
+	
+	return ret;
 }
 
 static void __exit ramoops_exit(void)
 {
 	platform_driver_unregister(&ramoops_driver);
+	kfree(dummy_data);
 }
 
 module_init(ramoops_init);

Stevie Trujillo

2011-05-28 10:05 UTC #1 lore raw

Quoted 9 lines · On Saturday 28 May 2011 11:01:12 you wrote:

From: Marco Stornelli <[email protected]>
Use generic module parameters instead of platform data, if platform data are not available. This limitation has been introduced with commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0.
Signed-off-by Marco Stornelli <[email protected]>
CC Kyungmin Park <[email protected]>
Reported-by Stevie Trujillo <[email protected]>

Nice work, I think this will fix my problems :) I have some comments - not sure how many of them are sane.

I think the indent is wrong (mixed tabs + spaces) in ramoops_init. Tried to fix it, but my email client just made it worse :p

With this patch, ramoops_platform_data takes precedence over module parameters. Should it maybe be the other way?

I think you can just statically allocate ramoops_platform_data, since it's only 2x(unsigned long)? You will use one more long in .data, but less in .text?

Not related to the patch: Should the printks end with "\n"? If i do printk(KERN_ERR "a"); printk(KERN_ERR "b"); I get two lines, but with printk(KERN_ERR "a"); printk("b"); they end up on the same line. So if another driver did printk without KERN_ after ramoops, they would end up on same line?

--
Stevie Trujillo

Marco Stornelli

author

2011-05-28 14:24 UTC #2 lore raw

Il 28/05/2011 12:05, Stevie Trujillo ha scritto:

Quoted 11 lines · On Saturday 28 May 2011 11:01:12 you wrote:

Quoted 9 lines
From: Marco Stornelli<[email protected]>
Use generic module parameters instead of platform data, if platform data are not available. This limitation has been introduced with commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0.
Signed-off-by Marco Stornelli<[email protected]>
CC Kyungmin Park<[email protected]>
Reported-by Stevie Trujillo<[email protected]>

Quoted 5 lines

Nice work, I think this will fix my problems :) I have some comments - not sure how many of them are sane.
I think the indent is wrong (mixed tabs + spaces) in ramoops_init. Tried to fix it, but my email client just made it worse :p

Oops, my fault, I'll resend the patch.

Quoted 4 lines

With this patch, ramoops_platform_data takes precedence over module parameters. Should it maybe be the other way?

I don't like the "user overwrite kernel configuration" pattern :) At the end, for archs with a device tree source it's possible to change the value there.

Quoted 3 lines

I think you can just statically allocate ramoops_platform_data, since it's only 2x(unsigned long)? You will use one more long in .data, but less in .text?

If some other field it's added to the struct, we already use the right policy.

Quoted 6 lines

Not related to the patch: Should the printks end with "\n"? If i do printk(KERN_ERR "a"); printk(KERN_ERR "b"); I get two lines, but with printk(KERN_ERR "a"); printk("b"); they end up on the same line. So if another driver did printk without KERN_ after ramoops, they would end up on same line?

I'll add the \n with a separate patch.

Marco

Marco Stornelli

author

2011-05-28 14:54 UTC #3 lore raw

PATCH v2 1/2 ramoops: use module parameters instead of platform data if not available

In reply to Marco Stornelli

From: Marco Stornelli <[email protected]>

Use generic module parameters instead of platform data, if platform data are not available. This limitation has been introduced with commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0.

Signed-off-by Marco Stornelli <[email protected]>

CC Kyungmin Park <[email protected]>

Reported-by Stevie Trujillo <[email protected]>

---
ChangeLog
v2: fixed tab/space problem

drivers/char/ramoops.c +34 ~~-12~~

index 1a9f5f6..bf5f9f6 100644
@@ -26,6 +26,7 @@
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 #include <linux/ramoops.h>
 
 #define RAMOOPS_KERNMSG_HDR "===="

@@ -56,6 +57,9 @@ static struct ramoops_context {
 	int max_count;
 } oops_cxt;
 
+static struct platform_device *dummy;
+static struct ramoops_platform_data *dummy_data;
+
 static void ramoops_do_dump(struct kmsg_dumper *dumper,
 		enum kmsg_dump_reason reason, const char *s1, unsigned long l1,
 		const char *s2, unsigned long l2)

@@ -106,27 +110,22 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	struct ramoops_context *cxt = &oops_cxt;
 	int err = -EINVAL;
 
-	if (pdata) {
-		mem_size = pdata->mem_size;
-		mem_address = pdata->mem_address;
-	}
-
-	if (!mem_size) {
+	if (!pdata->mem_size) {
 		printk(KERN_ERR "ramoops: invalid size specification");
 		goto fail3;
 	}
 
-	rounddown_pow_of_two(mem_size);
+	rounddown_pow_of_two(pdata->mem_size);
 
-	if (mem_size < RECORD_SIZE) {
+	if (pdata->mem_size < RECORD_SIZE) {
 		printk(KERN_ERR "ramoops: size too small");
 		goto fail3;
 	}
 
-	cxt->max_count = mem_size / RECORD_SIZE;
+	cxt->max_count = pdata->mem_size / RECORD_SIZE;
 	cxt->count = 0;
-	cxt->size = mem_size;
-	cxt->phys_addr = mem_address;
+	cxt->size = pdata->mem_size;
+	cxt->phys_addr = pdata->mem_address;
 
 	if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
 		printk(KERN_ERR "ramoops: request mem region failed");

@@ -179,12 +178,35 @@ static struct platform_driver ramoops_driver = {
 
 static int __init ramoops_init(void)
 {
-	return platform_driver_probe(&ramoops_driver, ramoops_probe);
+	int ret;
+	ret = platform_driver_probe(&ramoops_driver, ramoops_probe);
+	if (ret == -ENODEV)
+	{
+		/* 
+		 * if we didn't find a platform device, we use module parameters
+		 * building platform data on the fly.
+		 */
+		dummy_data = (struct ramoops_platform_data *) 
+		     kzalloc(sizeof(struct ramoops_platform_data), GFP_KERNEL);
+		dummy_data->mem_size = mem_size;
+		dummy_data->mem_address = mem_address;
+		dummy = platform_create_bundle(&ramoops_driver, ramoops_probe,
+			NULL, 0, dummy_data,
+			sizeof(struct ramoops_platform_data));
+
+		if (IS_ERR(dummy)) 
+			ret = PTR_ERR(dummy);
+		else
+			ret = 0;
+	}
+	
+	return ret;
 }
 
 static void __exit ramoops_exit(void)
 {
 	platform_driver_unregister(&ramoops_driver);
+	kfree(dummy_data);
 }
 
 module_init(ramoops_init);

Américo Wang

2011-06-06 16:02 UTC #4 lore raw

Quoted 70 lines · Marco Stornelli

From: Marco Stornelli <[email protected]>

Use generic module parameters instead of platform data, if platform data are not available. This limitation has been introduced with commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0.

Signed-off-by Marco Stornelli <[email protected]>

CC Kyungmin Park <[email protected]>

Reported-by Stevie Trujillo <[email protected]>

---
ChangeLog
v2: fixed tab/space problem

drivers/char/ramoops.c +10 ~~-11~~

index 1a9f5f6..bf5f9f6 100644
@@ -26,6 +26,7 @@
  #include <linux/io.h>
  #include <linux/ioport.h>
  #include <linux/platform_device.h>
+#include <linux/slab.h>
  #include <linux/ramoops.h>
 
  #define RAMOOPS_KERNMSG_HDR "===="

@@ -56,6 +57,9 @@ static struct ramoops_context {
        int max_count;
  } oops_cxt;
 
+static struct platform_device *dummy;
+static struct ramoops_platform_data *dummy_data;
+
  static void ramoops_do_dump(struct kmsg_dumper *dumper,
                enum kmsg_dump_reason reason, const char *s1, unsigned long l1,
                const char *s2, unsigned long l2)

@@ -106,27 +110,22 @@ static int __init ramoops_probe(struct platform_device *pdev)
        struct ramoops_context *cxt = &oops_cxt;
        int err = -EINVAL;
 
-       if (pdata) {
-               mem_size = pdata->mem_size;
-               mem_address = pdata->mem_address;
-       }
-
-       if (!mem_size) {
+       if (!pdata->mem_size) {
                printk(KERN_ERR "ramoops: invalid size specification");
                goto fail3;
        }
 
-       rounddown_pow_of_two(mem_size);
+       rounddown_pow_of_two(pdata->mem_size);
 
-       if (mem_size < RECORD_SIZE) {
+       if (pdata->mem_size < RECORD_SIZE) {
                printk(KERN_ERR "ramoops: size too small");
                goto fail3;
        }
 
-       cxt->max_count = mem_size / RECORD_SIZE;
+       cxt->max_count = pdata->mem_size / RECORD_SIZE;
        cxt->count = 0;
-       cxt->size = mem_size;
-       cxt->phys_addr = mem_address;
+       cxt->size = pdata->mem_size;
+       cxt->phys_addr = pdata->mem_address;
 
        if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
                printk(KERN_ERR "ramoops: request mem region failed");

drivers/char/ramoops.c static struct platform_driver ramoops_driver = { · line 184

Quoted 8 lines

drivers/char/ramoops.c +4 -1

@@ -179,12 +178,35 @@ static struct platform_driver ramoops_driver = {
 
  static int __init ramoops_init(void)
  {
-       return platform_driver_probe(&ramoops_driver, ramoops_probe);
+       int ret;
+       ret = platform_driver_probe(&ramoops_driver, ramoops_probe);
+       if (ret == -ENODEV)
+       {

Wrong coding style...

drivers/char/ramoops.c static struct platform_driver ramoops_driver = { · line 190

Quoted 6 lines

drivers/char/ramoops.c +6 -0

+               /*
+                * if we didn't find a platform device, we use module parameters
+                * building platform data on the fly.
+                */
+               dummy_data = (struct ramoops_platform_data *)
+                    kzalloc(sizeof(struct ramoops_platform_data), GFP_KERNEL);

Please check the return value of kzalloc(), and, you don't need to cast it.

Quoted 24 lines

drivers/char/ramoops.c +14 -0

+               dummy_data->mem_size = mem_size;
+               dummy_data->mem_address = mem_address;
+               dummy = platform_create_bundle(&ramoops_driver, ramoops_probe,
+                       NULL, 0, dummy_data,
+                       sizeof(struct ramoops_platform_data));
+
+               if (IS_ERR(dummy))
+                       ret = PTR_ERR(dummy);
+               else
+                       ret = 0;
+       }
+
+       return ret;
  }
 
  static void __exit ramoops_exit(void)
  {
        platform_driver_unregister(&ramoops_driver);
+       kfree(dummy_data);
  }
 
  module_init(ramoops_init);

Marco Stornelli

author

2011-06-07 16:55 UTC #5 lore raw

PATCH v3 1/2 ramoops: use module parameters instead of platform data if not available

In reply to Marco Stornelli

From: Marco Stornelli <[email protected]>

Use generic module parameters instead of platform data, if platform data are not available. This limitation has been introduced with commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0.

Signed-off-by Marco Stornelli <[email protected]>

CC Kyungmin Park <[email protected]>

CC Américo Wang <[email protected]>

Reported-by Stevie Trujillo <[email protected]>

---
ChangeLog
- v3: add a check for kzalloc return value;
removed cast for kzalloc return value
- v2: fixed tab/space problem

drivers/char/ramoops.c +36 ~~-12~~

index 1a9f5f6..df092e1 100644
@@ -26,6 +26,7 @@
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 #include <linux/ramoops.h>
 
 #define RAMOOPS_KERNMSG_HDR "===="

@@ -56,6 +57,9 @@ static struct ramoops_context {
 	int max_count;
 } oops_cxt;
 
+static struct platform_device *dummy;
+static struct ramoops_platform_data *dummy_data;
+
 static void ramoops_do_dump(struct kmsg_dumper *dumper,
 		enum kmsg_dump_reason reason, const char *s1, unsigned long l1,
 		const char *s2, unsigned long l2)

@@ -106,27 +110,22 @@ static int __init ramoops_probe(struct platform_device *pdev)
 	struct ramoops_context *cxt = &oops_cxt;
 	int err = -EINVAL;
 
-	if (pdata) {
-		mem_size = pdata->mem_size;
-		mem_address = pdata->mem_address;
-	}
-
-	if (!mem_size) {
+	if (!pdata->mem_size) {
 		printk(KERN_ERR "ramoops: invalid size specification");
 		goto fail3;
 	}
 
-	rounddown_pow_of_two(mem_size);
+	rounddown_pow_of_two(pdata->mem_size);
 
-	if (mem_size < RECORD_SIZE) {
+	if (pdata->mem_size < RECORD_SIZE) {
 		printk(KERN_ERR "ramoops: size too small");
 		goto fail3;
 	}
 
-	cxt->max_count = mem_size / RECORD_SIZE;
+	cxt->max_count = pdata->mem_size / RECORD_SIZE;
 	cxt->count = 0;
-	cxt->size = mem_size;
-	cxt->phys_addr = mem_address;
+	cxt->size = pdata->mem_size;
+	cxt->phys_addr = pdata->mem_address;
 
 	if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
 		printk(KERN_ERR "ramoops: request mem region failed");

@@ -179,12 +178,37 @@ static struct platform_driver ramoops_driver = {
 
 static int __init ramoops_init(void)
 {
-	return platform_driver_probe(&ramoops_driver, ramoops_probe);
+	int ret;
+	ret = platform_driver_probe(&ramoops_driver, ramoops_probe);
+	if (ret == -ENODEV)
+	{
+		/* 
+		 * if we didn't find a platform device, we use module parameters
+		 * building platform data on the fly.
+		 */
+		dummy_data = kzalloc(sizeof(struct ramoops_platform_data),
+				     GFP_KERNEL);
+		if (!dummy_data)
+			return -ENOMEM;
+		dummy_data->mem_size = mem_size;
+		dummy_data->mem_address = mem_address;
+		dummy = platform_create_bundle(&ramoops_driver, ramoops_probe,
+			NULL, 0, dummy_data,
+			sizeof(struct ramoops_platform_data));
+
+		if (IS_ERR(dummy)) 
+			ret = PTR_ERR(dummy);
+		else
+			ret = 0;
+	}
+	
+	return ret;
 }
 
 static void __exit ramoops_exit(void)
 {
 	platform_driver_unregister(&ramoops_driver);
+	kfree(dummy_data);
 }
 
 module_init(ramoops_init);

Américo Wang

2011-06-08 16:01 UTC #6 lore raw

Quoted 10 lines · Marco Stornelli

From: Marco Stornelli <[email protected]>
Use generic module parameters instead of platform data, if platform data are not available. This limitation has been introduced with commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0.
Signed-off-by Marco Stornelli <[email protected]>
CC Kyungmin Park <[email protected]>
CC Américo Wang <[email protected]>
Reported-by Stevie Trujillo <[email protected]>

Quoted 5 lines

---
ChangeLog
- v3: add a check for kzalloc return value;
removed cast for kzalloc return value
- v2: fixed tab/space problem

Looks good, thanks!

Marco Stornelli

author

2011-06-08 19:58 UTC #7 lore raw

In reply to Marco Stornelli

Il 07/06/2011 18:49, Marco Stornelli ha scritto:

Quoted 11 lines

From: Marco Stornelli<[email protected]>
Use generic module parameters instead of platform data, if platform data are not available. This limitation has been introduced with commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0.
Signed-off-by Marco Stornelli<[email protected]>
CC Kyungmin Park<[email protected]>
CC Américo Wang<[email protected]>
Reported-by Stevie Trujillo<[email protected]>
---

Stephen,

can you add this patch and "[PATCH 2/2 v3] ramoops: add a new line for each print" to the linux-next? Thanks.

Regards,

Marco

Stephen Rothwell

2011-06-09 00:59 UTC #8 lore raw

Hi Marco,

Quoted 8 lines · On Wed, 08 Jun 2011 21:52:21 +0200 Marco Stornelli <[email protected]>…

Il 07/06/2011 18:49, Marco Stornelli ha scritto:
Quoted 6 lines
From: Marco Stornelli<[email protected]>
Use generic module parameters instead of platform data, if platform data are not available. This limitation has been introduced with commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0.

Quoted 8 lines

Quoted 5 lines
Signed-off-by Marco Stornelli<[email protected]>
CC Kyungmin Park<[email protected]>
CC Américo Wang<[email protected]>
Reported-by Stevie Trujillo<[email protected]>
---
can you add this patch and "[PATCH 2/2 v3] ramoops: add a new line for each print" to the linux-next? Thanks.

I don't know what "this patch" is. Also, I don't add patches to linux-next normally, they should all come through some tree that has been included in linux-next.

Signature · 3 lines

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/

Américo Wang

2011-06-09 01:02 UTC #9 lore raw

Quoted 17 lines · Stephen Rothwell

Hi Marco,
Quoted 14 lines · On Wed, 08 Jun 2011 21:52:21 +0200 Marco Stornelli <[email protected]>…
Il 07/06/2011 18:49, Marco Stornelli ha scritto:
Quoted 11 lines
From: Marco Stornelli<[email protected]>
Use generic module parameters instead of platform data, if platform data are not available. This limitation has been introduced with commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0.
Signed-off-by Marco Stornelli<[email protected]>
CC Kyungmin Park<[email protected]>
CC Américo Wang<[email protected]>
Reported-by Stevie Trujillo<[email protected]>
---

Quoted 7 lines

Quoted 2 lines
can you add this patch and "[PATCH 2/2 v3] ramoops: add a new line for each print" to the linux-next? Thanks.
I don't know what "this patch" is. Also, I don't add patches to linux-next normally, they should all come through some tree that has been included in linux-next.

Right, the patch should be merged into some tree first, in this case, I think Andrew Morton will take it.

Thanks.

Marco Stornelli

author

2011-06-09 06:46 UTC #10 lore raw

2011/6/9 Américo Wang <[email protected]>:

Quoted 26 lines · Stephen Rothwell

Quoted 17 lines
Hi Marco,
Quoted 14 lines · On Wed, 08 Jun 2011 21:52:21 +0200 Marco Stornelli <[email protected]>…
Il 07/06/2011 18:49, Marco Stornelli ha scritto:
Quoted 11 lines
From: Marco Stornelli<[email protected]>
Use generic module parameters instead of platform data, if platform data are not available. This limitation has been introduced with commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0.
Signed-off-by Marco Stornelli<[email protected]>
CC Kyungmin Park<[email protected]>
CC Américo Wang<[email protected]>
Reported-by Stevie Trujillo<[email protected]>
---
Quoted 7 lines
Quoted 2 lines
can you add this patch and "[PATCH 2/2 v3] ramoops: add a new line for each print" to the linux-next? Thanks.
I don't know what "this patch" is. Also, I don't add patches to linux-next normally, they should all come through some tree that has been included in linux-next.

Quoted 5 lines

Right, the patch should be merged into some tree first, in this case, I think Andrew Morton will take it.
Thanks.

Hi Andrew,

can you add these patches into your tree? Thanks.

Regards,

Marco

Marco Stornelli

author

2011-06-18 14:07 UTC #11 lore raw

In reply to Marco Stornelli

Andrew,

Il 07/06/2011 18:49, Marco Stornelli ha scritto:

Quoted 10 lines

From: Marco Stornelli<[email protected]>
Use generic module parameters instead of platform data, if platform data are not available. This limitation has been introduced with commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0.
Signed-off-by Marco Stornelli<[email protected]>
CC Kyungmin Park<[email protected]>
CC Américo Wang<[email protected]>
Reported-by Stevie Trujillo<[email protected]>

can you give me a feedback about this patch series? Can you add it into your tree?

Marco