Re: [RFC][PATCH] pstore: use DT reserved-memory bindings

[Rob Herring]

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

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

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);
+
+       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.

>         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