ramoops: is using platform_drivers correct?

DISCUSSION Stevie Trujillo 4 participants 12 messages May 23, 2011 – May 25, 2011

Share & tools

Stevie Trujillo

author

2011-05-23 13:28 UTC #0 lore raw

Hello,

ramoops (drivers/char/ramoops.c) is for "all" computers right? When I try to load it on my laptop, I get ENODEV. This is caused by platform_driver_probe()
- the code never reaches the ramoops probe callback.

After I removed the platform_driver stuff, moving everything into ramoops_init and ramoops_exit it worked.

Américo Wang

2011-05-23 14:10 UTC #1 lore raw

Quoted 8 lines · Stevie Trujillo

Hello,
ramoops (drivers/char/ramoops.c) is for "all" computers right? When I try to load it on my laptop, I get ENODEV. This is caused by platform_driver_probe()
- the code never reaches the ramoops probe callback.
After I removed the platform_driver stuff, moving everything into ramoops_init and ramoops_exit it worked.

Actually that was changed by Kyungmin, Cc'ing...

commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0

Author: Kyungmin Park <[email protected]>
Date:   Wed Oct 27 15:34:52 2010 -0700

ramoops: use the platform data structure instead of module params

As each board and system has different memory for ramoops. It's better to define the platform data instead of module params.

[[email protected]: fix ramoops_remove() return type]

Signed-off-by Kyungmin Park <[email protected]>

Cc Marco Stornelli <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

Kyungmin Park

2011-05-23 14:36 UTC #2 lore raw

Hi,

You have to define the ramoops platform data at your board file and pass it to the platform device init.
As these address is different for each SoCs. e.g., x86, and Samsung ARM SoCs and so on.

I think maybe you use the x86 so define the default x86 ram address for ramoops and pass it to platform structures.

At office, I will send the sample usage.

Thank you,
Kyungmin Park

Quoted 33 lines · Américo Wang

Quoted 8 lines · Stevie Trujillo
Hello,
ramoops (drivers/char/ramoops.c) is for "all" computers right? When I try to load it on my laptop, I get ENODEV. This is caused by platform_driver_probe()
- the code never reaches the ramoops probe callback.
After I removed the platform_driver stuff, moving everything into ramoops_init and ramoops_exit it worked.
Actually that was changed by Kyungmin, Cc'ing...
commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0
Author: Kyungmin Park <[email protected]>
Date: Wed Oct 27 15:34:52 2010 -0700
ramoops: use the platform data structure instead of module params
As each board and system has different memory for ramoops. It's better to define the platform data instead of module params.
[[email protected]: fix ramoops_remove() return type]
Signed-off-by Kyungmin Park <[email protected]>
Cc Marco Stornelli <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Kyungmin Park

2011-05-24 05:32 UTC #3 lore raw

Quoted 11 lines · Kyungmin Park

Hi,
You have to define the ramoops platform data at your board file and pass it to the platform device init.
As these address is different for each SoCs. e.g., x86, and Samsung ARM SoCs and so on.
I think maybe you use the x86 so define the default x86 ram address for ramoops and pass it to platform structures.
At office, I will send the sample usage.

+static struct ramoops_platform_data goni_ramoops_data = {

+       .mem_size               = SZ_16K,
+       .mem_address            = 0xED000000,   /* SRAM */

+};
+
+static struct platform_device goni_ramoops = {
+ .name = "ramoops",
+ .dev = {
+ .platform_data = &goni_ramoops_data,
+ },
+};

and register the goni_rammoops. then you can find a rammops.

Thank you,
Kyungmin Park

Quoted 39 lines

Thank you,
Kyungmin Park
Quoted 33 lines · Américo Wang
Quoted 8 lines · Stevie Trujillo
Hello,
ramoops (drivers/char/ramoops.c) is for "all" computers right? When I try to load it on my laptop, I get ENODEV. This is caused by platform_driver_probe()
- the code never reaches the ramoops probe callback.
After I removed the platform_driver stuff, moving everything into ramoops_init and ramoops_exit it worked.
Actually that was changed by Kyungmin, Cc'ing...
commit c3b92ce9e75f6353104fc7f8e32fb9fdb2550ad0
Author: Kyungmin Park <[email protected]>
Date: Wed Oct 27 15:34:52 2010 -0700
ramoops: use the platform data structure instead of module params
As each board and system has different memory for ramoops. It's better to define the platform data instead of module params.
[[email protected]: fix ramoops_remove() return type]
Signed-off-by Kyungmin Park <[email protected]>
Cc Marco Stornelli <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Américo Wang

2011-05-24 05:49 UTC #4 lore raw

Quoted 10 lines · Kyungmin Park

Quoted 9 lines · Kyungmin Park
Hi,
You have to define the ramoops platform data at your board file and pass it to the platform device init.
As these address is different for each SoCs. e.g., x86, and Samsung ARM SoCs and so on.
I think maybe you use the x86 so define the default x86 ram address for ramoops and pass it to platform structures.

Why not document this?

Quoted 15 lines

Quoted 2 lines

At office, I will send the sample usage.

(quoted diff) +11 -0

+static struct ramoops_platform_data goni_ramoops_data = {
+       .mem_size               = SZ_16K,
+       .mem_address            = 0xED000000,   /* SRAM */
+};
+
+static struct platform_device goni_ramoops = {
+       .name = "ramoops",
+       .dev = {
+               .platform_data = &goni_ramoops_data,
+       },
+};

Quoted 2 lines

and register the goni_rammoops. then you can find a rammops.

Huh? Is this for x86 too? Why so unfriendly for end-users?

I think we need some kernel parameter like 'crashkernel=' (or memmap=) to reserve memory for ramoops, right?

Thanks.

Kyungmin Park

2011-05-24 06:14 UTC #5 lore raw

Quoted 29 lines · Américo Wang

Quoted 10 lines · Kyungmin Park
Quoted 9 lines · Kyungmin Park
Hi,
You have to define the ramoops platform data at your board file and pass it to the platform device init.
As these address is different for each SoCs. e.g., x86, and Samsung ARM SoCs and so on.
I think maybe you use the x86 so define the default x86 ram address for ramoops and pass it to platform structures.
Why not document this?
Quoted 15 lines
Quoted 2 lines
At office, I will send the sample usage.
(quoted diff) +11 -0
+static struct ramoops_platform_data goni_ramoops_data = {
+       .mem_size               = SZ_16K,
+       .mem_address            = 0xED000000,   /* SRAM */
+};
+
+static struct platform_device goni_ramoops = {
+       .name = "ramoops",
+       .dev = {
+               .platform_data = &goni_ramoops_data,
+       },
+};

Quoted 4 lines

Quoted 2 lines
and register the goni_rammoops. then you can find a rammops.
Huh? Is this for x86 too? Why so unfriendly for end-users?

I don't know which address is acceptable for x86, in case of ARM, each SoCs has different SRAM address. so it's not good to define for all SoCs and ARM.

Quoted 3 lines

I think we need some kernel parameter like 'crashkernel=' (or memmap=) to reserve memory for ramoops, right?

The first implementation is just module parameters. ramoops.address=0x??????? ramoops.size=0x????. So I patched it as using platform devices.
and the reason use the platform is it's dependent on each SoCs and board usage.

Quoted 3 lines

Thanks.

Américo Wang

2011-05-24 14:12 UTC #6 lore raw

Quoted 6 lines · Kyungmin Park

Quoted 1 line · Américo Wang
Huh? Is this for x86 too? Why so unfriendly for end-users?
I don't know which address is acceptable for x86, in case of ARM, each SoCs has different SRAM address. so it's not good to define for all SoCs and ARM.
Quoted 1 line

Quoted 8 lines

Quoted 2 lines
I think we need some kernel parameter like 'crashkernel=' (or memmap=) to reserve memory for ramoops, right?
The first implementation is just module parameters. ramoops.address=0x??????? ramoops.size=0x????. So I patched it as using platform devices.
and the reason use the platform is it's dependent on each SoCs and board usage.

But the result is that this makes end-users harder to use it.

Using platform API still relies on a hard-code address, at least in your example,
so, why not leave it as a module parameter to let user to find the correct address?

Kyungmin Park

2011-05-24 14:16 UTC #7 lore raw

Quoted 16 lines · Américo Wang

Quoted 6 lines · Kyungmin Park
Quoted 1 line · Américo Wang
Huh? Is this for x86 too? Why so unfriendly for end-users?
I don't know which address is acceptable for x86, in case of ARM, each SoCs has different SRAM address. so it's not good to define for all SoCs and ARM.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
I think we need some kernel parameter like 'crashkernel=' (or memmap=) to reserve memory for ramoops, right?
The first implementation is just module parameters. ramoops.address=0x??????? ramoops.size=0x????. So I patched it as using platform devices.
and the reason use the platform is it's dependent on each SoCs and board usage.

Quoted 6 lines

But the result is that this makes end-users harder to use it.
Using platform API still relies on a hard-code address, at least in your example,
so, why not leave it as a module parameter to let user to find the correct address?

It's possible. I just make it possible to use the platform driver. you can specify the original method.

Thank you,
Kyungmin Park

Quoted 1 line

Stevie Trujillo

author

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

Quoted 1 line · On Tuesday 24 May 2011 16:16:13 Kyungmin Park wrote:

On Tue, May 24, 2011 at 11:12 PM, Américo Wang <[email protected]>

Quoted 1 line · wrote:

Quoted 1 line
On Tue, May 24, 2011 at 2:14 PM, Kyungmin Park <[email protected]>

Quoted 1 line · wrote:

Quoted 1 line
Quoted 1 line
On Tue, May 24, 2011 at 2:49 PM, Américo Wang <[email protected]>

Quoted 17 lines · wrote:

Quoted 17 lines
Quoted 6 lines
Quoted 1 line
Huh? Is this for x86 too? Why so unfriendly for end-users?
I don't know which address is acceptable for x86, in case of ARM, each SoCs has different SRAM address. so it's not good to define for all SoCs and ARM.
Quoted 8 lines
Quoted 2 lines
I think we need some kernel parameter like 'crashkernel=' (or memmap=) to reserve memory for ramoops, right?
The first implementation is just module parameters. ramoops.address=0x??????? ramoops.size=0x????. So I patched it as using platform devices.
and the reason use the platform is it's dependent on each SoCs and board usage.
But the result is that this makes end-users harder to use it.

Quoted 7 lines

Quoted 4 lines
Using platform API still relies on a hard-code address, at least in your example,
so, why not leave it as a module parameter to let user to find the correct address?
It's possible. I just make it possible to use the platform driver. you can specify the original method.

I don't think it's possible without also having a platform_device. ramoops_probe is never called here, I think platform_driver_probe returns - ENODEV.

static int __init ramoops_init(void)

{
    return platform_driver_probe(&ramoops_driver, ramoops_probe);
}

Maybe one could run platform_driver_probe() only if (!mem_address && !mem_size)?

Unrelated question: should the printk()s end with "\n"? I see they do that other places in the kernel.

Marco Stornelli

2011-05-24 17:09 UTC #9 lore raw

In reply to Stevie Trujillo

Il 23/05/2011 15:27, Stevie Trujillo ha scritto:

Quoted 3 lines

Hello,
ramoops (drivers/char/ramoops.c) is for "all" computers right? When I try to

Yes, or at least was my intention. It's true that this drivers is useful when you use an NVRAM, this is typical for the embedded world where the platform driver approach is more diffused. Sure at this point the module parameters are not useful. In addition the platform data struct doesn't define a way to select if to dump only oops. At the end I think a patch it's needed here. I have to look at the code to see if it's possible to use the platform data OR module parameters. I'll submit a patch.

Marco

Américo Wang

2011-05-25 13:24 UTC #10 lore raw

Quoted 7 lines · Marco Stornelli

Il 23/05/2011 15:27, Stevie Trujillo ha scritto:
Quoted 5 lines
Hello,
ramoops (drivers/char/ramoops.c) is for "all" computers right? When I try to

Quoted 8 lines

Yes, or at least was my intention. It's true that this drivers is useful when you use an NVRAM, this is typical for the embedded world where the platform driver approach is more diffused. Sure at this point the module parameters are not useful. In addition the platform data struct doesn't define a way to select if to dump only oops. At the end I think a patch it's needed here. I have to look at the code to see if it's possible to use the platform data OR module parameters. I'll submit a patch.

But we have the following code:

        if (reason != KMSG_DUMP_OOPS &&
            reason != KMSG_DUMP_PANIC &&
            reason != KMSG_DUMP_KEXEC)
                return;

which is what you meant by saying "only oops" ?

I still don't think that is a correct way to tell people not to use ramoops, we need to document this rather than prevent using it in the code.

Thanks.

Marco Stornelli

2011-05-25 14:08 UTC #11 lore raw

2011/5/25 Américo Wang <[email protected]>:

Quoted 18 lines

Quoted 7 lines · Marco Stornelli
Il 23/05/2011 15:27, Stevie Trujillo ha scritto:
Quoted 5 lines
Hello,
ramoops (drivers/char/ramoops.c) is for "all" computers right? When I try to
Quoted 8 lines
Yes, or at least was my intention. It's true that this drivers is useful when you use an NVRAM, this is typical for the embedded world where the platform driver approach is more diffused. Sure at this point the module parameters are not useful. In addition the platform data struct doesn't define a way to select if to dump only oops. At the end I think a patch it's needed here. I have to look at the code to see if it's possible to use the platform data OR module parameters. I'll submit a patch.

Quoted 8 lines

But we have the following code:
if (reason != KMSG_DUMP_OOPS &&
reason != KMSG_DUMP_PANIC &&
reason != KMSG_DUMP_KEXEC)
return;
which is what you meant by saying "only oops" ?

I meant a way to set dump_oops parameter via platform data.

Quoted 5 lines

I still don't think that is a correct way to tell people not to use ramoops, we need to document this rather than prevent using it in the code.

I really don't understand what you mean here.

Marco