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]>
---
diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index 1a9f5f6..a201f81 100644
--- a/drivers/char/ramoops.c
+++ b/drivers/char/ramoops.c
@@ -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);
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
Il 28/05/2011 12:05, Stevie Trujillo ha scritto:
> 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
Oops, my fault, I'll resend the patch.
>
> 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.
> 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.
>
> 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
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
diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index 1a9f5f6..bf5f9f6 100644
--- a/drivers/char/ramoops.c
+++ b/drivers/char/ramoops.c
@@ -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);
On Sat, May 28, 2011 at 10:48 PM, Marco Stornelli
<[email protected]> 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]>
> ---
> ChangeLog
> v2: fixed tab/space problem
>
> diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
> index 1a9f5f6..bf5f9f6 100644
> --- a/drivers/char/ramoops.c
> +++ b/drivers/char/ramoops.c
> @@ -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)
> + {
Wrong coding style...
> + /*
> + * 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.
> + 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);
>
>
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
diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index 1a9f5f6..df092e1 100644
--- a/drivers/char/ramoops.c
+++ b/drivers/char/ramoops.c
@@ -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);
On Wed, Jun 8, 2011 at 12:49 AM, Marco Stornelli
<[email protected]> 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]>
> 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
Looks good, thanks!
Il 07/06/2011 18:49, Marco Stornelli ha scritto:
> 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
Hi Marco,
On Wed, 08 Jun 2011 21:52:21 +0200 Marco Stornelli <[email protected]> wrote:
>
> Il 07/06/2011 18:49, Marco Stornelli ha scritto:
> > 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 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.
--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/
On Thu, Jun 9, 2011 at 8:59 AM, Stephen Rothwell <[email protected]> wrote:
> Hi Marco,
>
> On Wed, 08 Jun 2011 21:52:21 +0200 Marco Stornelli <[email protected]> wrote:
>>
>> Il 07/06/2011 18:49, Marco Stornelli ha scritto:
>> > 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 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.
2011/6/9 Am?rico Wang <[email protected]>:
> On Thu, Jun 9, 2011 at 8:59 AM, Stephen Rothwell <[email protected]> wrote:
>> Hi Marco,
>>
>> On Wed, 08 Jun 2011 21:52:21 +0200 Marco Stornelli <[email protected]> wrote:
>>>
>>> Il 07/06/2011 18:49, Marco Stornelli ha scritto:
>>> > 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 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.
>
Hi Andrew,
can you add these patches into your tree? Thanks.
Regards,
Marco
Andrew,
Il 07/06/2011 18:49, Marco Stornelli ha scritto:
> 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