ramoops: is using platform_drivers correct?

ramoops: is using platform_drivers correct?

[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.

Re: ramoops: is using platform_drivers correct?

[Américo Wang]

On Mon, May 23, 2011 at 9:27 PM, Stevie Trujillo
<stevie.trujillo@gmail.com> wrote:
> 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 <kyungmin.park@samsung.com>
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.

    [akpm@linux-foundation.org: fix ramoops_remove() return type]
    Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
    Cc: Marco Stornelli <marco.stornelli@gmail.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Re: ramoops: is using platform_drivers correct?

[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.

Thank you,
Kyungmin Park

On Mon, May 23, 2011 at 11:10 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, May 23, 2011 at 9:27 PM, Stevie Trujillo
> <stevie.trujillo@gmail.com> wrote:
>> 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 <kyungmin.park@samsung.com>
> 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.
>
>    [akpm@linux-foundation.org: fix ramoops_remove() return type]
>    Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>    Cc: Marco Stornelli <marco.stornelli@gmail.com>
>    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: ramoops: is using platform_drivers correct?

[Kyungmin Park]

On Mon, May 23, 2011 at 11:36 PM, Kyungmin Park <kmpark@infradead.org> wrote:
> 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


>
> Thank you,
> Kyungmin Park
>
> On Mon, May 23, 2011 at 11:10 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>> On Mon, May 23, 2011 at 9:27 PM, Stevie Trujillo
>> <stevie.trujillo@gmail.com> wrote:
>>> 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 <kyungmin.park@samsung.com>
>> 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.
>>
>>    [akpm@linux-foundation.org: fix ramoops_remove() return type]
>>    Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>    Cc: Marco Stornelli <marco.stornelli@gmail.com>
>>    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>>    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>

Re: ramoops: is using platform_drivers correct?

[Américo Wang]

On Tue, May 24, 2011 at 1:32 PM, Kyungmin Park <kmpark@infradead.org> wrote:
> On Mon, May 23, 2011 at 11:36 PM, Kyungmin Park <kmpark@infradead.org> wrote:
>> 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?

>>
>> 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.
>

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.

Re: ramoops: is using platform_drivers correct?

[Kyungmin Park]

On Tue, May 24, 2011 at 2:49 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, May 24, 2011 at 1:32 PM, Kyungmin Park <kmpark@infradead.org> wrote:
>> On Mon, May 23, 2011 at 11:36 PM, Kyungmin Park <kmpark@infradead.org> wrote:
>>> 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?
>
>>>
>>> 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.
>>
>
> 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.
>
> 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.

>
> Thanks.
>

Re: ramoops: is using platform_drivers correct?

[Américo Wang]

On Tue, May 24, 2011 at 2:14 PM, Kyungmin Park <kmpark@infradead.org> wrote:
> On Tue, May 24, 2011 at 2:49 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>> 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.
>>
>> 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?

Re: ramoops: is using platform_drivers correct?

[Kyungmin Park]

On Tue, May 24, 2011 at 11:12 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, May 24, 2011 at 2:14 PM, Kyungmin Park <kmpark@infradead.org> wrote:
>> On Tue, May 24, 2011 at 2:49 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>>> 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.
>>>
>>> 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?

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

Thank you,
Kyungmin Park
>

Re: ramoops: is using platform_drivers correct?

[Stevie Trujillo]

On Tuesday 24 May 2011 16:16:13 Kyungmin Park wrote:
> On Tue, May 24, 2011 at 11:12 PM, Américo Wang <xiyou.wangcong@gmail.com> 
wrote:
> > On Tue, May 24, 2011 at 2:14 PM, Kyungmin Park <kmpark@infradead.org> 
wrote:
> >> On Tue, May 24, 2011 at 2:49 PM, Américo Wang <xiyou.wangcong@gmail.com> 
wrote:
> >>> 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.
> >> 
> >>> 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?
> 
> 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.

Re: ramoops: is using platform_drivers correct?

[Marco Stornelli]

Il 23/05/2011 15:27, Stevie Trujillo ha scritto:
> 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

Re: ramoops: is using platform_drivers correct?

[Américo Wang]

On Wed, May 25, 2011 at 1:03 AM, Marco Stornelli
<marco.stornelli@gmail.com> wrote:
> Il 23/05/2011 15:27, Stevie Trujillo ha scritto:
>>
>> 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.
>

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.

Re: ramoops: is using platform_drivers correct?

[Marco Stornelli]

2011/5/25 Américo Wang <xiyou.wangcong@gmail.com>:
> On Wed, May 25, 2011 at 1:03 AM, Marco Stornelli
> <marco.stornelli@gmail.com> wrote:
>> Il 23/05/2011 15:27, Stevie Trujillo ha scritto:
>>>
>>> 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.
>>
>
> 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.

>
> 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