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]:" ?
> 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.) Should this first look for
/reserved-memory, then ramoops?
Testing this as-is seems to work, though I'll send a version that
looks up /reserved-memory first before ramoops.
> +
> + of_platform_default_populate(NULL, NULL, NULL);
>
> return 0;
> }
>
>
>> /* Make sure it has a compatible property */
>> if (strict && (!of_get_property(bus, "compatible", NULL))) {
>> pr_debug("%s() - skipping %s, no compatible prop\n",
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index 47516a794011..af5cee0c2156 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -486,24 +486,16 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>> struct ramoops_platform_data *pdata)
>> {
>> struct device_node *of_node = pdev->dev.of_node;
>> - struct device_node *mem_region;
>> struct resource res;
>> u32 value;
>> int ret;
>>
>> dev_dbg(&pdev->dev, "using Device Tree\n");
>>
>> - mem_region = of_parse_phandle(of_node, "memory-region", 0);
>> - if (!mem_region) {
>> - dev_err(&pdev->dev, "no memory-region phandle\n");
>> - return -ENODEV;
>> - }
>> -
>> - ret = of_address_to_resource(mem_region, 0, &res);
>> - of_node_put(mem_region);
>> + ret = of_address_to_resource(of_node, 0, &res);
>
> You can use the standard platform device resource functions now.
Okay, sounds good. I still have to parse the ramoops-specific stuff
off the of_node, so it doesn't really change things too much here, but
does look a little cleaner.
Thanks!
-Kees
>
>> if (ret) {
>> dev_err(&pdev->dev,
>> - "failed to translate memory-region to resource: %d\n",
>> + "failed to translate reserved-memory to resource: %d\n",
>> ret);
>> return ret;
>> }
>> --
>> 2.7.4
>>
>>
>> --
>> Kees Cook
>> Brillo & Chrome OS Security
--
Kees Cook
Chrome OS & Brillo Security
--
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
