Re: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Rob Herring]

+Greg H

On Tue, Jan 5, 2016 at 7:06 PM, Kees Cook  wrote:
> [fixing devicetree mailing list]
>
> On Tue, Jan 5, 2016 at 5:04 PM, Kees Cook  wrote:
>> [thread necromancy, if you don't have the thread locally, it's here:
>> https://patchwork.kernel.org/patch/1426261/]
>>
>> We still need to solve this, and John pinged me about it today. Where
>> does this stand?

Wrong thread here as the latest version is "[PATCH v2] pstore-ram: add
Device Tree bindings" recently posted by Greg H (and reviewed by you).
I am mostly happy with it and only had a minor comment remaining. I
was expecting to see a v3.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Kees Cook]

[fixing devicetree mailing list]

On Tue, Jan 5, 2016 at 5:04 PM, Kees Cook  wrote:
> [thread necromancy, if you don't have the thread locally, it's here:
> https://patchwork.kernel.org/patch/1426261/]
>
> We still need to solve this, and John pinged me about it today. Where
> does this stand?
>
> -Kees
>
>
> On Sun, Apr 14, 2013 at 7:24 AM, Anton Vorontsov  wrote:
>> On Mon, Apr 08, 2013 at 12:54:01PM -0700, Bryan Freed wrote:
>> [...]
>>> And as a more general question, why should we try not to put
>>> configuration in the device tree?  It seems like a great (and
>>> portable) place to put this stuff.
>>> It certainly seems better to have it there than hardwired in the
>>> kernel or tacked onto the kernel command line.
>>
>> But then we have two in-kernel APIs to pass kernel parameters? So we'll
>> have to maintain two ways of passing the options for each driver. That is
>> hardly a good solution.
>>
>> If you would like to see a convenient way to pass kernel/module options
>> via the device tree, I would suggest implementing something like this:
>>
>> chosen {
>> kernel-options {
>> linux,pstore.record-size = 123;
>> linux,foo = "bar";
>> };
>> };
>>
>> And then let the kernel translate all these to module_param_*().
>>
>> I am still not sure about placing the options along with devices layout,
>> but if we go this route, then that is also viable:
>>
>> pstore-node {
>> linux,pstore.record-size = 123;
>> };
>>
>> And translate "linux,*" this to module_param_*().
>>
>> How does that sound?
>>
>> Thanks,
>> Anton
>
>
>
> --
> Kees Cook
> Chrome OS & Brillo Security



-- 
Kees Cook
Chrome OS & Brillo Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Kees Cook]

[thread necromancy, if you don't have the thread locally, it's here:
https://patchwork.kernel.org/patch/1426261/]

We still need to solve this, and John pinged me about it today. Where
does this stand?

-Kees


On Sun, Apr 14, 2013 at 7:24 AM, Anton Vorontsov  wrote:
> On Mon, Apr 08, 2013 at 12:54:01PM -0700, Bryan Freed wrote:
> [...]
>> And as a more general question, why should we try not to put
>> configuration in the device tree?  It seems like a great (and
>> portable) place to put this stuff.
>> It certainly seems better to have it there than hardwired in the
>> kernel or tacked onto the kernel command line.
>
> But then we have two in-kernel APIs to pass kernel parameters? So we'll
> have to maintain two ways of passing the options for each driver. That is
> hardly a good solution.
>
> If you would like to see a convenient way to pass kernel/module options
> via the device tree, I would suggest implementing something like this:
>
> chosen {
> kernel-options {
> linux,pstore.record-size = 123;
> linux,foo = "bar";
> };
> };
>
> And then let the kernel translate all these to module_param_*().
>
> I am still not sure about placing the options along with devices layout,
> but if we go this route, then that is also viable:
>
> pstore-node {
> linux,pstore.record-size = 123;
> };
>
> And translate "linux,*" this to module_param_*().
>
> How does that sound?
>
> Thanks,
> Anton



-- 
Kees Cook
Chrome OS & Brillo Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Kees Cook]

[fixing devicetree mailing list]

On Tue, Jan 5, 2016 at 5:04 PM, Kees Cook  wrote:
> [thread necromancy, if you don't have the thread locally, it's here:
> https://patchwork.kernel.org/patch/1426261/]
>
> We still need to solve this, and John pinged me about it today. Where
> does this stand?
>
> -Kees
>
>
> On Sun, Apr 14, 2013 at 7:24 AM, Anton Vorontsov  wrote:
>> On Mon, Apr 08, 2013 at 12:54:01PM -0700, Bryan Freed wrote:
>> [...]
>>> And as a more general question, why should we try not to put
>>> configuration in the device tree?  It seems like a great (and
>>> portable) place to put this stuff.
>>> It certainly seems better to have it there than hardwired in the
>>> kernel or tacked onto the kernel command line.
>>
>> But then we have two in-kernel APIs to pass kernel parameters? So we'll
>> have to maintain two ways of passing the options for each driver. That is
>> hardly a good solution.
>>
>> If you would like to see a convenient way to pass kernel/module options
>> via the device tree, I would suggest implementing something like this:
>>
>> chosen {
>> kernel-options {
>> linux,pstore.record-size = 123;
>> linux,foo = "bar";
>> };
>> };
>>
>> And then let the kernel translate all these to module_param_*().
>>
>> I am still not sure about placing the options along with devices layout,
>> but if we go this route, then that is also viable:
>>
>> pstore-node {
>> linux,pstore.record-size = 123;
>> };
>>
>> And translate "linux,*" this to module_param_*().
>>
>> How does that sound?
>>
>> Thanks,
>> Anton
>
>
>
> --
> Kees Cook
> Chrome OS & Brillo Security



-- 
Kees Cook
Chrome OS & Brillo Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Kees Cook]

[thread necromancy, if you don't have the thread locally, it's here:
https://patchwork.kernel.org/patch/1426261/]

We still need to solve this, and John pinged me about it today. Where
does this stand?

-Kees


On Sun, Apr 14, 2013 at 7:24 AM, Anton Vorontsov  wrote:
> On Mon, Apr 08, 2013 at 12:54:01PM -0700, Bryan Freed wrote:
> [...]
>> And as a more general question, why should we try not to put
>> configuration in the device tree?  It seems like a great (and
>> portable) place to put this stuff.
>> It certainly seems better to have it there than hardwired in the
>> kernel or tacked onto the kernel command line.
>
> But then we have two in-kernel APIs to pass kernel parameters? So we'll
> have to maintain two ways of passing the options for each driver. That is
> hardly a good solution.
>
> If you would like to see a convenient way to pass kernel/module options
> via the device tree, I would suggest implementing something like this:
>
> chosen {
> kernel-options {
> linux,pstore.record-size = 123;
> linux,foo = "bar";
> };
> };
>
> And then let the kernel translate all these to module_param_*().
>
> I am still not sure about placing the options along with devices layout,
> but if we go this route, then that is also viable:
>
> pstore-node {
> linux,pstore.record-size = 123;
> };
>
> And translate "linux,*" this to module_param_*().
>
> How does that sound?
>
> Thanks,
> Anton



-- 
Kees Cook
Chrome OS & Brillo Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Rob Herring]

+Greg H

On Tue, Jan 5, 2016 at 7:06 PM, Kees Cook  wrote:
> [fixing devicetree mailing list]
>
> On Tue, Jan 5, 2016 at 5:04 PM, Kees Cook  wrote:
>> [thread necromancy, if you don't have the thread locally, it's here:
>> https://patchwork.kernel.org/patch/1426261/]
>>
>> We still need to solve this, and John pinged me about it today. Where
>> does this stand?

Wrong thread here as the latest version is "[PATCH v2] pstore-ram: add
Device Tree bindings" recently posted by Greg H (and reviewed by you).
I am mostly happy with it and only had a minor comment remaining. I
was expecting to see a v3.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Anton Vorontsov]

On Mon, Apr 08, 2013 at 12:54:01PM -0700, Bryan Freed wrote:
[...]
> And as a more general question, why should we try not to put
> configuration in the device tree?  It seems like a great (and
> portable) place to put this stuff.
> It certainly seems better to have it there than hardwired in the
> kernel or tacked onto the kernel command line.

But then we have two in-kernel APIs to pass kernel parameters? So we'll
have to maintain two ways of passing the options for each driver. That is
hardly a good solution.

If you would like to see a convenient way to pass kernel/module options
via the device tree, I would suggest implementing something like this:

chosen {
kernel-options {
linux,pstore.record-size = 123;
linux,foo = "bar";
};
};

And then let the kernel translate all these to module_param_*().

I am still not sure about placing the options along with devices layout,
but if we go this route, then that is also viable:

pstore-node {
linux,pstore.record-size = 123;
};

And translate "linux,*" this to module_param_*().

How does that sound?

Thanks,
Anton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Anton Vorontsov]

On Mon, Apr 08, 2013 at 12:54:01PM -0700, Bryan Freed wrote:
[...]
 And as a more general question, why should we try not to put
 configuration in the device tree?  It seems like a great (and
 portable) place to put this stuff.
 It certainly seems better to have it there than hardwired in the
 kernel or tacked onto the kernel command line.

But then we have two in-kernel APIs to pass kernel parameters? So we'll
have to maintain two ways of passing the options for each driver. That is
hardly a good solution.

If you would like to see a convenient way to pass kernel/module options
via the device tree, I would suggest implementing something like this:

chosen {
kernel-options {
linux,pstore.record-size = 123;
linux,foo = bar;
};
};

And then let the kernel translate all these to module_param_*().

I am still not sure about placing the options along with devices layout,
but if we go this route, then that is also viable:

pstore-node {
linux,pstore.record-size = 123;
};

And translate linux,* this to module_param_*().

How does that sound?

Thanks,
Anton
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Rob Herring]

On 04/08/2013 02:54 PM, Bryan Freed wrote:
> Sorry for dropping the ball on this one, Anton.
> 
> Thank you for your feedback and modifications in the code.
> I gotta ask, however, why do you completely remove key ramoops fields
> like record_size and ftrace_size?
> 
> From your 9/7/2012 comments (again, sorry for the delay in getting back to 
> you):
>> Personally, I don't see how this fits into device tree. It doesn't
>> describe hardware, instead it's more a configuration stuff, which
>> usually we try to not put into the device tree.
>>
>> It would be better to have a sane defaults in ramoops, instead of
>> introducing more "virtual" stuff in the device tree. That is, feel
>> free to change defaults if they seem to be not enough for most your
>> setups.
> 
> So there are only two alternatives I can see to set these fields.
> 1. Pass kernel command line parameters, or
> 2. Modify the defaults in ram.c.
> 
> Neither of these alternatives is particularly clean to me.  The first
> is shunned to avoid clutter on the command line.
> And the second requires us to maintain our own version of ram.c or
> push our defaults on the rest of the community.
> 
> We (ok, maybe just 'I') prefer to keep our device specific values in
> the device tree, even though it is not strictly defining hardware in
> the system.
> What do you think of at least keeping the record_size, console_size
> and ftrace_size fields as optional in the ramoops device tree
> specification?
> 
> And as a more general question, why should we try not to put
> configuration in the device tree?  It seems like a great (and
> portable) place to put this stuff.
> It certainly seems better to have it there than hardwired in the
> kernel or tacked onto the kernel command line.

You have to account for the fact that you may not be able to update the
dtb which may be part of firmware. I can imagine you might want to
change some of the sizes frequently based on what you are doing and
trying to capture. So even if you put in the DT, you need some easy way
to override the settings. This could always be fixed-up by the
bootloader which is what is done for things we don't expect the firmware
to know like initrd address/size and kernel command line. This is why
the prior discussion I mentioned below suggested putting all this in the
/chosen node.

Rob

> 
> Thanks,
> 
> bryan.
> 
> On Sun, Apr 7, 2013 at 10:43 AM, Anton Vorontsov  wrote:
>> On Thu, Apr 04, 2013 at 09:03:47PM -0500, Rob Herring wrote:
>>> On Mon, Sep 17, 2012 at 1:23 AM, Anton Vorontsov  
>>> wrote:
 On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
> On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
>> When called with a non-zero of_node, fill out a new ramoops_platform_data
>> with data from the specified Flattened Device Tree node.
>> Update ramoops documentation with the new FDT interface.
>> Update devicetree/binding documentation with pstore/ramoops.txt.
>
> Thanks for your work, Bryan! There were a few issues, I fixed
> them myself but I need your confirmation if you're OK w/ all
> the changes.
 [...]
> So, the resulting patch is down below. But I have not pushed
> it out yet, I'm waiting for your sign off. If you're OK with the
> changes, please reply with
> 'Signed-off-by: Bryan Freed '

 Bryan, have you had a chance to look into this?
>>>
>>> Whatever happened to this? Olof was also looking at doing a binding
>>> back in Jan 2012:
>>>
>>> http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09905.html
>>
>> I don't know. Bryan's patch looked good, I'd happily apply it. But I still
>> need Bryan's Signed-off-by tag. :-/ Or, somebody needs to sign off on his
>> behalf (anyone from Chromium project?). Or redo the patch.
>>
>> Thanks,
>>
>> Anton

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Bryan Freed]

Sorry for dropping the ball on this one, Anton.

Thank you for your feedback and modifications in the code.
I gotta ask, however, why do you completely remove key ramoops fields
like record_size and ftrace_size?

>From your 9/7/2012 comments (again, sorry for the delay in getting back to 
>you):
> Personally, I don't see how this fits into device tree. It doesn't
> describe hardware, instead it's more a configuration stuff, which
> usually we try to not put into the device tree.
>
> It would be better to have a sane defaults in ramoops, instead of
> introducing more "virtual" stuff in the device tree. That is, feel
> free to change defaults if they seem to be not enough for most your
> setups.

So there are only two alternatives I can see to set these fields.
1. Pass kernel command line parameters, or
2. Modify the defaults in ram.c.

Neither of these alternatives is particularly clean to me.  The first
is shunned to avoid clutter on the command line.
And the second requires us to maintain our own version of ram.c or
push our defaults on the rest of the community.

We (ok, maybe just 'I') prefer to keep our device specific values in
the device tree, even though it is not strictly defining hardware in
the system.
What do you think of at least keeping the record_size, console_size
and ftrace_size fields as optional in the ramoops device tree
specification?

And as a more general question, why should we try not to put
configuration in the device tree?  It seems like a great (and
portable) place to put this stuff.
It certainly seems better to have it there than hardwired in the
kernel or tacked onto the kernel command line.

Thanks,

bryan.

On Sun, Apr 7, 2013 at 10:43 AM, Anton Vorontsov  wrote:
> On Thu, Apr 04, 2013 at 09:03:47PM -0500, Rob Herring wrote:
>> On Mon, Sep 17, 2012 at 1:23 AM, Anton Vorontsov  
>> wrote:
>> > On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
>> >> On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
>> >> > When called with a non-zero of_node, fill out a new 
>> >> > ramoops_platform_data
>> >> > with data from the specified Flattened Device Tree node.
>> >> > Update ramoops documentation with the new FDT interface.
>> >> > Update devicetree/binding documentation with pstore/ramoops.txt.
>> >>
>> >> Thanks for your work, Bryan! There were a few issues, I fixed
>> >> them myself but I need your confirmation if you're OK w/ all
>> >> the changes.
>> > [...]
>> >> So, the resulting patch is down below. But I have not pushed
>> >> it out yet, I'm waiting for your sign off. If you're OK with the
>> >> changes, please reply with
>> >> 'Signed-off-by: Bryan Freed '
>> >
>> > Bryan, have you had a chance to look into this?
>>
>> Whatever happened to this? Olof was also looking at doing a binding
>> back in Jan 2012:
>>
>> http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09905.html
>
> I don't know. Bryan's patch looked good, I'd happily apply it. But I still
> need Bryan's Signed-off-by tag. :-/ Or, somebody needs to sign off on his
> behalf (anyone from Chromium project?). Or redo the patch.
>
> Thanks,
>
> Anton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Bryan Freed]

Sorry for dropping the ball on this one, Anton.

Thank you for your feedback and modifications in the code.
I gotta ask, however, why do you completely remove key ramoops fields
like record_size and ftrace_size?

From your 9/7/2012 comments (again, sorry for the delay in getting back to 
you):
 Personally, I don't see how this fits into device tree. It doesn't
 describe hardware, instead it's more a configuration stuff, which
 usually we try to not put into the device tree.

 It would be better to have a sane defaults in ramoops, instead of
 introducing more virtual stuff in the device tree. That is, feel
 free to change defaults if they seem to be not enough for most your
 setups.

So there are only two alternatives I can see to set these fields.
1. Pass kernel command line parameters, or
2. Modify the defaults in ram.c.

Neither of these alternatives is particularly clean to me.  The first
is shunned to avoid clutter on the command line.
And the second requires us to maintain our own version of ram.c or
push our defaults on the rest of the community.

We (ok, maybe just 'I') prefer to keep our device specific values in
the device tree, even though it is not strictly defining hardware in
the system.
What do you think of at least keeping the record_size, console_size
and ftrace_size fields as optional in the ramoops device tree
specification?

And as a more general question, why should we try not to put
configuration in the device tree?  It seems like a great (and
portable) place to put this stuff.
It certainly seems better to have it there than hardwired in the
kernel or tacked onto the kernel command line.

Thanks,

bryan.

On Sun, Apr 7, 2013 at 10:43 AM, Anton Vorontsov an...@enomsg.org wrote:
 On Thu, Apr 04, 2013 at 09:03:47PM -0500, Rob Herring wrote:
 On Mon, Sep 17, 2012 at 1:23 AM, Anton Vorontsov cbouatmai...@gmail.com 
 wrote:
  On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
  On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
   When called with a non-zero of_node, fill out a new 
   ramoops_platform_data
   with data from the specified Flattened Device Tree node.
   Update ramoops documentation with the new FDT interface.
   Update devicetree/binding documentation with pstore/ramoops.txt.
 
  Thanks for your work, Bryan! There were a few issues, I fixed
  them myself but I need your confirmation if you're OK w/ all
  the changes.
  [...]
  So, the resulting patch is down below. But I have not pushed
  it out yet, I'm waiting for your sign off. If you're OK with the
  changes, please reply with
  'Signed-off-by: Bryan Freed bfr...@chromium.org'
 
  Bryan, have you had a chance to look into this?

 Whatever happened to this? Olof was also looking at doing a binding
 back in Jan 2012:

 http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09905.html

 I don't know. Bryan's patch looked good, I'd happily apply it. But I still
 need Bryan's Signed-off-by tag. :-/ Or, somebody needs to sign off on his
 behalf (anyone from Chromium project?). Or redo the patch.

 Thanks,

 Anton
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Rob Herring]

On 04/08/2013 02:54 PM, Bryan Freed wrote:
 Sorry for dropping the ball on this one, Anton.
 
 Thank you for your feedback and modifications in the code.
 I gotta ask, however, why do you completely remove key ramoops fields
 like record_size and ftrace_size?
 
 From your 9/7/2012 comments (again, sorry for the delay in getting back to 
 you):
 Personally, I don't see how this fits into device tree. It doesn't
 describe hardware, instead it's more a configuration stuff, which
 usually we try to not put into the device tree.

 It would be better to have a sane defaults in ramoops, instead of
 introducing more virtual stuff in the device tree. That is, feel
 free to change defaults if they seem to be not enough for most your
 setups.
 
 So there are only two alternatives I can see to set these fields.
 1. Pass kernel command line parameters, or
 2. Modify the defaults in ram.c.
 
 Neither of these alternatives is particularly clean to me.  The first
 is shunned to avoid clutter on the command line.
 And the second requires us to maintain our own version of ram.c or
 push our defaults on the rest of the community.
 
 We (ok, maybe just 'I') prefer to keep our device specific values in
 the device tree, even though it is not strictly defining hardware in
 the system.
 What do you think of at least keeping the record_size, console_size
 and ftrace_size fields as optional in the ramoops device tree
 specification?
 
 And as a more general question, why should we try not to put
 configuration in the device tree?  It seems like a great (and
 portable) place to put this stuff.
 It certainly seems better to have it there than hardwired in the
 kernel or tacked onto the kernel command line.

You have to account for the fact that you may not be able to update the
dtb which may be part of firmware. I can imagine you might want to
change some of the sizes frequently based on what you are doing and
trying to capture. So even if you put in the DT, you need some easy way
to override the settings. This could always be fixed-up by the
bootloader which is what is done for things we don't expect the firmware
to know like initrd address/size and kernel command line. This is why
the prior discussion I mentioned below suggested putting all this in the
/chosen node.

Rob

 
 Thanks,
 
 bryan.
 
 On Sun, Apr 7, 2013 at 10:43 AM, Anton Vorontsov an...@enomsg.org wrote:
 On Thu, Apr 04, 2013 at 09:03:47PM -0500, Rob Herring wrote:
 On Mon, Sep 17, 2012 at 1:23 AM, Anton Vorontsov cbouatmai...@gmail.com 
 wrote:
 On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
 On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
 When called with a non-zero of_node, fill out a new ramoops_platform_data
 with data from the specified Flattened Device Tree node.
 Update ramoops documentation with the new FDT interface.
 Update devicetree/binding documentation with pstore/ramoops.txt.

 Thanks for your work, Bryan! There were a few issues, I fixed
 them myself but I need your confirmation if you're OK w/ all
 the changes.
 [...]
 So, the resulting patch is down below. But I have not pushed
 it out yet, I'm waiting for your sign off. If you're OK with the
 changes, please reply with
 'Signed-off-by: Bryan Freed bfr...@chromium.org'

 Bryan, have you had a chance to look into this?

 Whatever happened to this? Olof was also looking at doing a binding
 back in Jan 2012:

 http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09905.html

 I don't know. Bryan's patch looked good, I'd happily apply it. But I still
 need Bryan's Signed-off-by tag. :-/ Or, somebody needs to sign off on his
 behalf (anyone from Chromium project?). Or redo the patch.

 Thanks,

 Anton

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Anton Vorontsov]

On Thu, Apr 04, 2013 at 09:03:47PM -0500, Rob Herring wrote:
> On Mon, Sep 17, 2012 at 1:23 AM, Anton Vorontsov  
> wrote:
> > On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
> >> On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
> >> > When called with a non-zero of_node, fill out a new ramoops_platform_data
> >> > with data from the specified Flattened Device Tree node.
> >> > Update ramoops documentation with the new FDT interface.
> >> > Update devicetree/binding documentation with pstore/ramoops.txt.
> >>
> >> Thanks for your work, Bryan! There were a few issues, I fixed
> >> them myself but I need your confirmation if you're OK w/ all
> >> the changes.
> > [...]
> >> So, the resulting patch is down below. But I have not pushed
> >> it out yet, I'm waiting for your sign off. If you're OK with the
> >> changes, please reply with
> >> 'Signed-off-by: Bryan Freed '
> >
> > Bryan, have you had a chance to look into this?
> 
> Whatever happened to this? Olof was also looking at doing a binding
> back in Jan 2012:
> 
> http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09905.html

I don't know. Bryan's patch looked good, I'd happily apply it. But I still
need Bryan's Signed-off-by tag. :-/ Or, somebody needs to sign off on his
behalf (anyone from Chromium project?). Or redo the patch.

Thanks,

Anton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Anton Vorontsov]

On Thu, Apr 04, 2013 at 09:03:47PM -0500, Rob Herring wrote:
 On Mon, Sep 17, 2012 at 1:23 AM, Anton Vorontsov cbouatmai...@gmail.com 
 wrote:
  On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
  On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
   When called with a non-zero of_node, fill out a new ramoops_platform_data
   with data from the specified Flattened Device Tree node.
   Update ramoops documentation with the new FDT interface.
   Update devicetree/binding documentation with pstore/ramoops.txt.
 
  Thanks for your work, Bryan! There were a few issues, I fixed
  them myself but I need your confirmation if you're OK w/ all
  the changes.
  [...]
  So, the resulting patch is down below. But I have not pushed
  it out yet, I'm waiting for your sign off. If you're OK with the
  changes, please reply with
  'Signed-off-by: Bryan Freed bfr...@chromium.org'
 
  Bryan, have you had a chance to look into this?
 
 Whatever happened to this? Olof was also looking at doing a binding
 back in Jan 2012:
 
 http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09905.html

I don't know. Bryan's patch looked good, I'd happily apply it. But I still
need Bryan's Signed-off-by tag. :-/ Or, somebody needs to sign off on his
behalf (anyone from Chromium project?). Or redo the patch.

Thanks,

Anton
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Rob Herring]

On Mon, Sep 17, 2012 at 1:23 AM, Anton Vorontsov  wrote:
> On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
>> On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
>> > When called with a non-zero of_node, fill out a new ramoops_platform_data
>> > with data from the specified Flattened Device Tree node.
>> > Update ramoops documentation with the new FDT interface.
>> > Update devicetree/binding documentation with pstore/ramoops.txt.
>>
>> Thanks for your work, Bryan! There were a few issues, I fixed
>> them myself but I need your confirmation if you're OK w/ all
>> the changes.
> [...]
>> So, the resulting patch is down below. But I have not pushed
>> it out yet, I'm waiting for your sign off. If you're OK with the
>> changes, please reply with
>> 'Signed-off-by: Bryan Freed '
>
> Bryan, have you had a chance to look into this?

Whatever happened to this? Olof was also looking at doing a binding
back in Jan 2012:

http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09905.html

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Rob Herring]

On Mon, Sep 17, 2012 at 1:23 AM, Anton Vorontsov cbouatmai...@gmail.com wrote:
 On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
 On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
  When called with a non-zero of_node, fill out a new ramoops_platform_data
  with data from the specified Flattened Device Tree node.
  Update ramoops documentation with the new FDT interface.
  Update devicetree/binding documentation with pstore/ramoops.txt.

 Thanks for your work, Bryan! There were a few issues, I fixed
 them myself but I need your confirmation if you're OK w/ all
 the changes.
 [...]
 So, the resulting patch is down below. But I have not pushed
 it out yet, I'm waiting for your sign off. If you're OK with the
 changes, please reply with
 'Signed-off-by: Bryan Freed bfr...@chromium.org'

 Bryan, have you had a chance to look into this?

Whatever happened to this? Olof was also looking at doing a binding
back in Jan 2012:

http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09905.html

Rob
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Anton Vorontsov]

On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
> On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
> > When called with a non-zero of_node, fill out a new ramoops_platform_data
> > with data from the specified Flattened Device Tree node.
> > Update ramoops documentation with the new FDT interface.
> > Update devicetree/binding documentation with pstore/ramoops.txt.
> 
> Thanks for your work, Bryan! There were a few issues, I fixed
> them myself but I need your confirmation if you're OK w/ all
> the changes.
[...]
> So, the resulting patch is down below. But I have not pushed
> it out yet, I'm waiting for your sign off. If you're OK with the
> changes, please reply with
> 'Signed-off-by: Bryan Freed '

Bryan, have you had a chance to look into this?

Thanks,
Anton.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Anton Vorontsov]

On Fri, Sep 07, 2012 at 10:29:10PM -0700, Anton Vorontsov wrote:
 On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
  When called with a non-zero of_node, fill out a new ramoops_platform_data
  with data from the specified Flattened Device Tree node.
  Update ramoops documentation with the new FDT interface.
  Update devicetree/binding documentation with pstore/ramoops.txt.
 
 Thanks for your work, Bryan! There were a few issues, I fixed
 them myself but I need your confirmation if you're OK w/ all
 the changes.
[...]
 So, the resulting patch is down below. But I have not pushed
 it out yet, I'm waiting for your sign off. If you're OK with the
 changes, please reply with
 'Signed-off-by: Bryan Freed bfr...@chromium.org'

Bryan, have you had a chance to look into this?

Thanks,
Anton.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Marco Stornelli]

Il 08/09/2012 10:06, Anton Vorontsov ha scritto:

On Sat, Sep 08, 2012 at 09:23:40AM +0200, Marco Stornelli wrote: [...]

+   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+   if (pdata == NULL)


I wonder why people prefer to not write !pdata, which is more natural
when reading the code.. :-)


I think it's the same for sizeof, it's much more readable
sizeof(struct ramoops_platform_data).


Well, sizeof(struct...) is against Linux coding style.  And there are
good reasons for this rule, it's all in the CodingStyle file. Thus,
it's not about personal preferences. But speaking of personal
preferences, I don't find sizeof(struct...) more readable. :-)

Thanks!
Anton.



Yes, I know, but indeed it's only a personal preference as the check 
==NULL instead of !pdata. :)


Marco
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Anton Vorontsov]

On Sat, Sep 08, 2012 at 09:23:40AM +0200, Marco Stornelli wrote: [...]
> >>+   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> >>+   if (pdata == NULL)
> >
> >I wonder why people prefer to not write !pdata, which is more natural
> >when reading the code.. :-)
> 
> I think it's the same for sizeof, it's much more readable
> sizeof(struct ramoops_platform_data).

Well, sizeof(struct...) is against Linux coding style.  And there are
good reasons for this rule, it's all in the CodingStyle file. Thus,
it's not about personal preferences. But speaking of personal
preferences, I don't find sizeof(struct...) more readable. :-)

Thanks!
Anton.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Marco Stornelli]

Il 08/09/2012 07:29, Anton Vorontsov ha scritto:

On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:

When called with a non-zero of_node, fill out a new ramoops_platform_data
with data from the specified Flattened Device Tree node.
Update ramoops documentation with the new FDT interface.
Update devicetree/binding documentation with pstore/ramoops.txt.


Thanks for your work, Bryan! There were a few issues, I fixed
them myself but I need your confirmation if you're OK w/ all
the changes.

First of, the Signed-off-by tag is missing, but I see it in v5.
I also see that v5 had an Ack from Kees Cook, you did not preserve it,
but generally it's a good idea to do so (if vX -> v(X+1) changes
aren't drastic).

[...]

+Optional properties:
+- record-size: Specifies the size of each record in preserved memory.
+- console-size: Specifies the size of the console log.
+- ftrace-size: Specifies the size of the ftrace log.
+- ecc-size: Specifies the size of each record's ECC buffer.
+- dump-oops: Specifies to dump oops in addition to panics.


Personally, I don't see how this fits into device tree. It doesn't
describe hardware, instead it's more a configuration stuff, which
usually we try to not put into the device tree.

It would be better to have a sane defaults in ramoops, instead of
introducing more "virtual" stuff in the device tree. That is, feel
free to change defaults if they seem to be not enough for most your
setups.

The only property that I see which is truly hardware-specific is
ecc-size. E.g. if we're pretty sure that a specific hw does not
need ecc, it's OK to avoid it. And some hw setups might require
bigger ECC sizes, so it's OK to have it in the device-tree.


static struct ramoops_platform_data * __init
of_ramoops_platform_data(struct device *dev)


You call this function from __devinit section, so using __init
is an error.

WARNING: fs/pstore/ramoops.o(.devinit.text+0x37): Section mismatch in reference 
from the function ramoops_probe() to the function 
.init.text:of_ramoops_platform_data()
The function __devinit ramoops_probe() references

I changed this to __devnit.

[...]

+   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+   if (pdata == NULL)


I wonder why people prefer to not write !pdata, which is more
natural when reading the code.. :-)



I think it's the same for sizeof, it's much more readable
sizeof(struct ramoops_platform_data).

Marco
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Marco Stornelli]

Il 08/09/2012 07:29, Anton Vorontsov ha scritto:

On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:

When called with a non-zero of_node, fill out a new ramoops_platform_data
with data from the specified Flattened Device Tree node.
Update ramoops documentation with the new FDT interface.
Update devicetree/binding documentation with pstore/ramoops.txt.


Thanks for your work, Bryan! There were a few issues, I fixed
them myself but I need your confirmation if you're OK w/ all
the changes.

First of, the Signed-off-by tag is missing, but I see it in v5.
I also see that v5 had an Ack from Kees Cook, you did not preserve it,
but generally it's a good idea to do so (if vX - v(X+1) changes
aren't drastic).

[...]

+Optional properties:
+- record-size: Specifies the size of each record in preserved memory.
+- console-size: Specifies the size of the console log.
+- ftrace-size: Specifies the size of the ftrace log.
+- ecc-size: Specifies the size of each record's ECC buffer.
+- dump-oops: Specifies to dump oops in addition to panics.


Personally, I don't see how this fits into device tree. It doesn't
describe hardware, instead it's more a configuration stuff, which
usually we try to not put into the device tree.

It would be better to have a sane defaults in ramoops, instead of
introducing more virtual stuff in the device tree. That is, feel
free to change defaults if they seem to be not enough for most your
setups.

The only property that I see which is truly hardware-specific is
ecc-size. E.g. if we're pretty sure that a specific hw does not
need ecc, it's OK to avoid it. And some hw setups might require
bigger ECC sizes, so it's OK to have it in the device-tree.


static struct ramoops_platform_data * __init
of_ramoops_platform_data(struct device *dev)


You call this function from __devinit section, so using __init
is an error.

WARNING: fs/pstore/ramoops.o(.devinit.text+0x37): Section mismatch in reference 
from the function ramoops_probe() to the function 
.init.text:of_ramoops_platform_data()
The function __devinit ramoops_probe() references

I changed this to __devnit.

[...]

+   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+   if (pdata == NULL)


I wonder why people prefer to not write !pdata, which is more
natural when reading the code.. :-)



I think it's the same for sizeof, it's much more readable
sizeof(struct ramoops_platform_data).

Marco
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Anton Vorontsov]

On Sat, Sep 08, 2012 at 09:23:40AM +0200, Marco Stornelli wrote: [...]
 +   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 +   if (pdata == NULL)
 
 I wonder why people prefer to not write !pdata, which is more natural
 when reading the code.. :-)
 
 I think it's the same for sizeof, it's much more readable
 sizeof(struct ramoops_platform_data).

Well, sizeof(struct...) is against Linux coding style.  And there are
good reasons for this rule, it's all in the CodingStyle file. Thus,
it's not about personal preferences. But speaking of personal
preferences, I don't find sizeof(struct...) more readable. :-)

Thanks!
Anton.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Marco Stornelli]

Il 08/09/2012 10:06, Anton Vorontsov ha scritto:

On Sat, Sep 08, 2012 at 09:23:40AM +0200, Marco Stornelli wrote: [...]

+   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+   if (pdata == NULL)


I wonder why people prefer to not write !pdata, which is more natural
when reading the code.. :-)


I think it's the same for sizeof, it's much more readable
sizeof(struct ramoops_platform_data).


Well, sizeof(struct...) is against Linux coding style.  And there are
good reasons for this rule, it's all in the CodingStyle file. Thus,
it's not about personal preferences. But speaking of personal
preferences, I don't find sizeof(struct...) more readable. :-)

Thanks!
Anton.



Yes, I know, but indeed it's only a personal preference as the check 
==NULL instead of !pdata. :)


Marco
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Anton Vorontsov]

On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
> When called with a non-zero of_node, fill out a new ramoops_platform_data
> with data from the specified Flattened Device Tree node.
> Update ramoops documentation with the new FDT interface.
> Update devicetree/binding documentation with pstore/ramoops.txt.

Thanks for your work, Bryan! There were a few issues, I fixed
them myself but I need your confirmation if you're OK w/ all
the changes.

First of, the Signed-off-by tag is missing, but I see it in v5.
I also see that v5 had an Ack from Kees Cook, you did not preserve it,
but generally it's a good idea to do so (if vX -> v(X+1) changes
aren't drastic).

[...]
> +Optional properties:
> +- record-size: Specifies the size of each record in preserved memory.
> +- console-size: Specifies the size of the console log.
> +- ftrace-size: Specifies the size of the ftrace log.
> +- ecc-size: Specifies the size of each record's ECC buffer.
> +- dump-oops: Specifies to dump oops in addition to panics.

Personally, I don't see how this fits into device tree. It doesn't
describe hardware, instead it's more a configuration stuff, which
usually we try to not put into the device tree.

It would be better to have a sane defaults in ramoops, instead of
introducing more "virtual" stuff in the device tree. That is, feel
free to change defaults if they seem to be not enough for most your
setups.

The only property that I see which is truly hardware-specific is
ecc-size. E.g. if we're pretty sure that a specific hw does not
need ecc, it's OK to avoid it. And some hw setups might require
bigger ECC sizes, so it's OK to have it in the device-tree.

> static struct ramoops_platform_data * __init
> of_ramoops_platform_data(struct device *dev)

You call this function from __devinit section, so using __init
is an error.

WARNING: fs/pstore/ramoops.o(.devinit.text+0x37): Section mismatch in reference 
from the function ramoops_probe() to the function 
.init.text:of_ramoops_platform_data()
The function __devinit ramoops_probe() references

I changed this to __devnit.

[...]
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (pdata == NULL)

I wonder why people prefer to not write !pdata, which is more
natural when reading the code.. :-)

[...]
> +#else
> +#define of_ramoops_platform_data(dev) NULL

Using static inline would be better, i.e. type-safe.

>+   .of_match_table = of_match_ptr(ramoops_of_match),

This fails to build:

fs/pstore/ram.c: At top level:
fs/pstore/ram.c:540:22: error: ‘ramoops_of_match’ undeclared here (not in a 
function)

For some reason you lost ramoops_of_match hunk from the v5.

So I made these changes on top of your patch (note, there's more
text at the end of the email):

- - - -
diff --git a/Documentation/devicetree/bindings/pstore/ramoops.txt 
b/Documentation/devicetree/bindings/pstore/ramoops.txt
index 2fddba4..1d43305 100644
--- a/Documentation/devicetree/bindings/pstore/ramoops.txt
+++ b/Documentation/devicetree/bindings/pstore/ramoops.txt
@@ -1,30 +1,17 @@
-Ramoops oops/panic/console logger
+Ramoops oops/panic/console/trace logger
 
 Required properties:
 - compatible: Must be "ramoops".
 - reg: Specifies the base physical address and size of preserved memory.
 
 Optional properties:
-- record-size: Specifies the size of each record in preserved memory.
-- console-size: Specifies the size of the console log.
-- ftrace-size: Specifies the size of the ftrace log.
 - ecc-size: Specifies the size of each record's ECC buffer.
-- dump-oops: Specifies to dump oops in addition to panics.
 
 Example:
 
 ramoops {
compatible = "ramoops";
reg = <0x41f0 0x0010>;
-   record-size = <0x0002>;
-   ftrace-size = <0x0002>;
-   dump-oops;
 };
 The "reg = <0x41f0 0x0010>" line specifies a preserved memory
 size of 1MB at physical address 0x41f0.
-The "record-size = <0x0002>" line specifies a record size of 128KB.
-The "ftrace-size = <0x0002>" line specifies an ftrace log size of 128KB.
-The optional "dump-oops" line tells ramoops to dump oopses as well as panics.
-At least one of record, console, or ftrace size must be specified.
-In this example, 128KB is allocated to the ftrace log, and 896KB
-(1024KB - 128KB) is allocated to oops and panic records.
diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
index 82d825d..a9d0c6a 100644
--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
 
 Sergiu Iordache 
 
-Updated: 5 June 2012
+Updated: 7 September 2012
 
 0. Introduction
 
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index b272473..c6e4485 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -34,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #define RAMOOPS_KERNMSG_HDR ""
@@ -354,7 +356,8 @@ static int 

[PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Bryan Freed]

When called with a non-zero of_node, fill out a new ramoops_platform_data
with data from the specified Flattened Device Tree node.
Update ramoops documentation with the new FDT interface.
Update devicetree/binding documentation with pstore/ramoops.txt.

---
I did not see any tree activity since I sent PATCH v5 on 6/26.
So I cleaned up the code a bit and added support for the new
console and ftrace logs as well.

bryan.

 .../devicetree/bindings/pstore/ramoops.txt |   30 
 Documentation/ramoops.txt  |7 ++-
 fs/pstore/ram.c|   47 
 3 files changed, 82 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pstore/ramoops.txt

diff --git a/Documentation/devicetree/bindings/pstore/ramoops.txt 
b/Documentation/devicetree/bindings/pstore/ramoops.txt
new file mode 100644
index 000..2fddba4
--- /dev/null
+++ b/Documentation/devicetree/bindings/pstore/ramoops.txt
@@ -0,0 +1,30 @@
+Ramoops oops/panic/console logger
+
+Required properties:
+- compatible: Must be "ramoops".
+- reg: Specifies the base physical address and size of preserved memory.
+
+Optional properties:
+- record-size: Specifies the size of each record in preserved memory.
+- console-size: Specifies the size of the console log.
+- ftrace-size: Specifies the size of the ftrace log.
+- ecc-size: Specifies the size of each record's ECC buffer.
+- dump-oops: Specifies to dump oops in addition to panics.
+
+Example:
+
+ramoops {
+   compatible = "ramoops";
+   reg = <0x41f0 0x0010>;
+   record-size = <0x0002>;
+   ftrace-size = <0x0002>;
+   dump-oops;
+};
+The "reg = <0x41f0 0x0010>" line specifies a preserved memory
+size of 1MB at physical address 0x41f0.
+The "record-size = <0x0002>" line specifies a record size of 128KB.
+The "ftrace-size = <0x0002>" line specifies an ftrace log size of 128KB.
+The optional "dump-oops" line tells ramoops to dump oopses as well as panics.
+At least one of record, console, or ftrace size must be specified.
+In this example, 128KB is allocated to the ftrace log, and 896KB
+(1024KB - 128KB) is allocated to oops and panic records.
diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
index 197ad59..20ab20a 100644
--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
 
 Sergiu Iordache 
 
-Updated: 17 November 2011
+Updated: 5 June 2012
 
 0. Introduction
 
@@ -37,7 +37,7 @@ corrupt, but usually it is restorable.
 
 2. Setting the parameters
 
-Setting the ramoops parameters can be done in 2 different manners:
+Setting the ramoops parameters can be done in 3 different manners:
  1. Use the module parameters (which have the names of the variables described
  as before).
  For quick debugging, you can also reserve parts of memory during boot
@@ -84,6 +84,9 @@ very early in the architecture code, e.g.:
 
 memblock_reserve(ramoops_data.mem_address, ramoops_data.mem_size);
 
+ 3. Use the Flattened Device Tree to set the platform data.
+ This is described in devicetree/bindings/pstore/ramoops.txt.
+
 3. Dump format
 
 The data dump begins with a header, currently defined as "" followed by a
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 0b311bc..876cc32 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define RAMOOPS_KERNMSG_HDR ""
 #define MIN_MEM_SIZE 4096UL
@@ -352,6 +353,43 @@ static int ramoops_init_prz(struct device *dev, struct 
ramoops_context *cxt,
return 0;
 }
 
+#ifdef CONFIG_OF
+static struct ramoops_platform_data * __init
+of_ramoops_platform_data(struct device *dev)
+{
+   struct device_node *node = dev->of_node;
+   struct ramoops_platform_data *pdata;
+   const __be32 *addrp;
+   u64 size;
+   u32 val;
+
+   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+   if (pdata == NULL)
+   return NULL;
+
+   addrp = of_get_address(node, 0, , NULL);
+   if (addrp == NULL)
+   return NULL;
+   pdata->mem_address = of_translate_address(node, addrp);
+   pdata->mem_size = size;
+
+   if (!of_property_read_u32(node, "record-size", ))
+   pdata->record_size = val;
+   if (!of_property_read_u32(node, "console-size", ))
+   pdata->console_size = val;
+   if (!of_property_read_u32(node, "ftrace-size", ))
+   pdata->ftrace_size = val;
+   if (!of_property_read_u32(node, "ecc-size", ))
+   pdata->ecc_size = val;
+   if (of_get_property(node, "dump-oops", NULL))
+   pdata->dump_oops = 1;
+
+   return pdata;
+}
+#else
+#define of_ramoops_platform_data(dev) NULL
+#endif
+
 static int __devinit ramoops_probe(struct platform_device *pdev)
 {
struct device *dev = >dev;
@@ -367,6 +405,14 @@ static int __devinit ramoops_probe(struct 

[PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Bryan Freed]

When called with a non-zero of_node, fill out a new ramoops_platform_data
with data from the specified Flattened Device Tree node.
Update ramoops documentation with the new FDT interface.
Update devicetree/binding documentation with pstore/ramoops.txt.

---
I did not see any tree activity since I sent PATCH v5 on 6/26.
So I cleaned up the code a bit and added support for the new
console and ftrace logs as well.

bryan.

 .../devicetree/bindings/pstore/ramoops.txt |   30 
 Documentation/ramoops.txt  |7 ++-
 fs/pstore/ram.c|   47 
 3 files changed, 82 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pstore/ramoops.txt

diff --git a/Documentation/devicetree/bindings/pstore/ramoops.txt 
b/Documentation/devicetree/bindings/pstore/ramoops.txt
new file mode 100644
index 000..2fddba4
--- /dev/null
+++ b/Documentation/devicetree/bindings/pstore/ramoops.txt
@@ -0,0 +1,30 @@
+Ramoops oops/panic/console logger
+
+Required properties:
+- compatible: Must be ramoops.
+- reg: Specifies the base physical address and size of preserved memory.
+
+Optional properties:
+- record-size: Specifies the size of each record in preserved memory.
+- console-size: Specifies the size of the console log.
+- ftrace-size: Specifies the size of the ftrace log.
+- ecc-size: Specifies the size of each record's ECC buffer.
+- dump-oops: Specifies to dump oops in addition to panics.
+
+Example:
+
+ramoops {
+   compatible = ramoops;
+   reg = 0x41f0 0x0010;
+   record-size = 0x0002;
+   ftrace-size = 0x0002;
+   dump-oops;
+};
+The reg = 0x41f0 0x0010 line specifies a preserved memory
+size of 1MB at physical address 0x41f0.
+The record-size = 0x0002 line specifies a record size of 128KB.
+The ftrace-size = 0x0002 line specifies an ftrace log size of 128KB.
+The optional dump-oops line tells ramoops to dump oopses as well as panics.
+At least one of record, console, or ftrace size must be specified.
+In this example, 128KB is allocated to the ftrace log, and 896KB
+(1024KB - 128KB) is allocated to oops and panic records.
diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
index 197ad59..20ab20a 100644
--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
 
 Sergiu Iordache ser...@chromium.org
 
-Updated: 17 November 2011
+Updated: 5 June 2012
 
 0. Introduction
 
@@ -37,7 +37,7 @@ corrupt, but usually it is restorable.
 
 2. Setting the parameters
 
-Setting the ramoops parameters can be done in 2 different manners:
+Setting the ramoops parameters can be done in 3 different manners:
  1. Use the module parameters (which have the names of the variables described
  as before).
  For quick debugging, you can also reserve parts of memory during boot
@@ -84,6 +84,9 @@ very early in the architecture code, e.g.:
 
 memblock_reserve(ramoops_data.mem_address, ramoops_data.mem_size);
 
+ 3. Use the Flattened Device Tree to set the platform data.
+ This is described in devicetree/bindings/pstore/ramoops.txt.
+
 3. Dump format
 
 The data dump begins with a header, currently defined as  followed by a
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 0b311bc..876cc32 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -33,6 +33,7 @@
 #include linux/platform_device.h
 #include linux/slab.h
 #include linux/pstore_ram.h
+#include linux/of_address.h
 
 #define RAMOOPS_KERNMSG_HDR 
 #define MIN_MEM_SIZE 4096UL
@@ -352,6 +353,43 @@ static int ramoops_init_prz(struct device *dev, struct 
ramoops_context *cxt,
return 0;
 }
 
+#ifdef CONFIG_OF
+static struct ramoops_platform_data * __init
+of_ramoops_platform_data(struct device *dev)
+{
+   struct device_node *node = dev-of_node;
+   struct ramoops_platform_data *pdata;
+   const __be32 *addrp;
+   u64 size;
+   u32 val;
+
+   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+   if (pdata == NULL)
+   return NULL;
+
+   addrp = of_get_address(node, 0, size, NULL);
+   if (addrp == NULL)
+   return NULL;
+   pdata-mem_address = of_translate_address(node, addrp);
+   pdata-mem_size = size;
+
+   if (!of_property_read_u32(node, record-size, val))
+   pdata-record_size = val;
+   if (!of_property_read_u32(node, console-size, val))
+   pdata-console_size = val;
+   if (!of_property_read_u32(node, ftrace-size, val))
+   pdata-ftrace_size = val;
+   if (!of_property_read_u32(node, ecc-size, val))
+   pdata-ecc_size = val;
+   if (of_get_property(node, dump-oops, NULL))
+   pdata-dump_oops = 1;
+
+   return pdata;
+}
+#else
+#define of_ramoops_platform_data(dev) NULL
+#endif
+
 static int __devinit ramoops_probe(struct platform_device *pdev)
 {
struct device *dev = pdev-dev;
@@ 

Re: [PATCH v6] pstore/ram: Add ramoops support for the Flattened Device Tree.

[Anton Vorontsov]

On Fri, Sep 07, 2012 at 11:29:36AM -0700, Bryan Freed wrote:
 When called with a non-zero of_node, fill out a new ramoops_platform_data
 with data from the specified Flattened Device Tree node.
 Update ramoops documentation with the new FDT interface.
 Update devicetree/binding documentation with pstore/ramoops.txt.

Thanks for your work, Bryan! There were a few issues, I fixed
them myself but I need your confirmation if you're OK w/ all
the changes.

First of, the Signed-off-by tag is missing, but I see it in v5.
I also see that v5 had an Ack from Kees Cook, you did not preserve it,
but generally it's a good idea to do so (if vX - v(X+1) changes
aren't drastic).

[...]
 +Optional properties:
 +- record-size: Specifies the size of each record in preserved memory.
 +- console-size: Specifies the size of the console log.
 +- ftrace-size: Specifies the size of the ftrace log.
 +- ecc-size: Specifies the size of each record's ECC buffer.
 +- dump-oops: Specifies to dump oops in addition to panics.

Personally, I don't see how this fits into device tree. It doesn't
describe hardware, instead it's more a configuration stuff, which
usually we try to not put into the device tree.

It would be better to have a sane defaults in ramoops, instead of
introducing more virtual stuff in the device tree. That is, feel
free to change defaults if they seem to be not enough for most your
setups.

The only property that I see which is truly hardware-specific is
ecc-size. E.g. if we're pretty sure that a specific hw does not
need ecc, it's OK to avoid it. And some hw setups might require
bigger ECC sizes, so it's OK to have it in the device-tree.

 static struct ramoops_platform_data * __init
 of_ramoops_platform_data(struct device *dev)

You call this function from __devinit section, so using __init
is an error.

WARNING: fs/pstore/ramoops.o(.devinit.text+0x37): Section mismatch in reference 
from the function ramoops_probe() to the function 
.init.text:of_ramoops_platform_data()
The function __devinit ramoops_probe() references

I changed this to __devnit.

[...]
 + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 + if (pdata == NULL)

I wonder why people prefer to not write !pdata, which is more
natural when reading the code.. :-)

[...]
 +#else
 +#define of_ramoops_platform_data(dev) NULL

Using static inline would be better, i.e. type-safe.

+   .of_match_table = of_match_ptr(ramoops_of_match),

This fails to build:

fs/pstore/ram.c: At top level:
fs/pstore/ram.c:540:22: error: ‘ramoops_of_match’ undeclared here (not in a 
function)

For some reason you lost ramoops_of_match hunk from the v5.

So I made these changes on top of your patch (note, there's more
text at the end of the email):

- - - -
diff --git a/Documentation/devicetree/bindings/pstore/ramoops.txt 
b/Documentation/devicetree/bindings/pstore/ramoops.txt
index 2fddba4..1d43305 100644
--- a/Documentation/devicetree/bindings/pstore/ramoops.txt
+++ b/Documentation/devicetree/bindings/pstore/ramoops.txt
@@ -1,30 +1,17 @@
-Ramoops oops/panic/console logger
+Ramoops oops/panic/console/trace logger
 
 Required properties:
 - compatible: Must be ramoops.
 - reg: Specifies the base physical address and size of preserved memory.
 
 Optional properties:
-- record-size: Specifies the size of each record in preserved memory.
-- console-size: Specifies the size of the console log.
-- ftrace-size: Specifies the size of the ftrace log.
 - ecc-size: Specifies the size of each record's ECC buffer.
-- dump-oops: Specifies to dump oops in addition to panics.
 
 Example:
 
 ramoops {
compatible = ramoops;
reg = 0x41f0 0x0010;
-   record-size = 0x0002;
-   ftrace-size = 0x0002;
-   dump-oops;
 };
 The reg = 0x41f0 0x0010 line specifies a preserved memory
 size of 1MB at physical address 0x41f0.
-The record-size = 0x0002 line specifies a record size of 128KB.
-The ftrace-size = 0x0002 line specifies an ftrace log size of 128KB.
-The optional dump-oops line tells ramoops to dump oopses as well as panics.
-At least one of record, console, or ftrace size must be specified.
-In this example, 128KB is allocated to the ftrace log, and 896KB
-(1024KB - 128KB) is allocated to oops and panic records.
diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt
index 82d825d..a9d0c6a 100644
--- a/Documentation/ramoops.txt
+++ b/Documentation/ramoops.txt
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
 
 Sergiu Iordache ser...@chromium.org
 
-Updated: 5 June 2012
+Updated: 7 September 2012
 
 0. Introduction
 
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index b272473..c6e4485 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -25,6 +25,7 @@
 #include linux/kernel.h
 #include linux/err.h
 #include linux/module.h
+#include linux/mod_devicetable.h
 #include linux/version.h
 #include linux/pstore.h
 #include linux/time.h
@@ -34,6 +35,7 @@
 #include linux/slab.h
 #include linux/compiler.h
 #include