On Mon, Aug 1, 2016 at 2:02 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Mon, Aug 1, 2016 at 8:54 AM, Rob Herring <robh+dt@xxxxxxxxxx> wrote:
>> On Fri, Jul 29, 2016 at 8:16 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>>> Instead of a ramoops-specific node, use a child node of /reserved-memory.
>>> This requires that of_platform_populate() be called for the node, though,
>>> since it does not have its own "compatible" property.
>>>
>>> Suggested-by: Rob Herring <robh@xxxxxxxxxx>
>>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>>> ---
>>> Here's what I've got for moving ramoops under /reserved-memory... still
>>> feels like a bit of a hack.
>>> ---
>>> Documentation/devicetree/bindings/misc/ramoops.txt | 48 ----------------------
>>> .../bindings/reserved-memory/ramoops.txt | 48 ++++++++++++++++++++++
>>
>> Use -M option or so you don't forget you can set in your git config:
>>
>> [diff]
>> renames = true
>
> Added, thanks.
>
>>
>>> Documentation/ramoops.txt | 2 +-
>>> drivers/of/platform.c | 5 +++
>>> fs/pstore/ram.c | 12 +-----
>>> 5 files changed, 56 insertions(+), 59 deletions(-)
>>
>> [...]
>>
>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>> index 16e8daffac06..c07adf72bb8e 100644
>>> --- a/drivers/of/platform.c
>>> +++ b/drivers/of/platform.c
>>> @@ -356,6 +356,11 @@ static int of_platform_bus_create(struct device_node *bus,
>>> void *platform_data = NULL;
>>> int rc = 0;
>>>
>>> + /* Always populate reserved-memory nodes. */
>>> + if (strict && strcmp(bus->full_name, "/reserved-memory") == 0) {
>>> + return of_platform_populate(bus, matches, lookup, parent);
>>> + }
>>> +
>>
>> This can be a lot cleaner now with the DT changes in 4.8. We could
>> make this more generic and call of_platform_populate with the
>> /reserved-memory node as the root, but that would :
>
> Is there a word missing above? "...but that would [need]:" ?
Uh, didn't finish that. ...would create devices for other nodes with
compatible strings. That's not really a problem, but not necessary
either presently.
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 765390e..4c36e06 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -499,8 +499,15 @@ EXPORT_SYMBOL_GPL(of_platform_default_populate);
>>
>> static int __init of_platform_default_populate_init(void)
>> {
>> - if (of_have_populated_dt())
>> - of_platform_default_populate(NULL, NULL, NULL);
>> + struct device_node *node;
>> +
>> + if (!of_have_populated_dt())
>> + return -ENODEV;
>> +
>> + node = of_find_compatible_node(NULL, NULL, "ramoops");
>> + of_platform_device_create(node, NULL, NULL);
>
> Does of_platform_device_create() DTRT if node is NULL here? (Looks
> like "yes", but goes through a spin lock first: I think it'd be nicer
> to check for a NULL node here.)
It does, but either way is fine with me.
> Should this first look for
> /reserved-memory, then ramoops?
No, that's not necessary. It could match if located in other places,
but it's not really the kernel's job to be a DT validator beyond what
it requires.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
