Date:	Tue, 10 May 2016 16:56:43 +0300
From:	Pantelis Antoniou <panto@...oniou-consulting.com>
To:	Rob Herring <robherring2@...il.com>
Cc:	Frank Rowand <frowand.list@...il.com>,
	Matt Porter <mporter@...sulko.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Koen Kooi <koen@...inion.thruhere.net>,
	Guenter Roeck <linux@...ck-us.net>,
	Marek Vasut <marex@...x.de>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] of: overlay: Pick up label symbols from overlays.

Hi Rob,

> On May 10, 2016, at 00:47 , Rob Herring <robherring2@...il.com> wrote:
> 
> On Mon, May 9, 2016 at 1:11 PM, Pantelis Antoniou
> <pantelis.antoniou@...sulko.com> wrote:
>> Insert overlay symbols to the base tree when applied.
>> This makes it possible to apply an overlay that references a label
>> that a previously inserted overlay had.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@...sulko.com>
>> Tested-by: Geert Uytterhoeven <geert+renesas@...der.be>
>> 
>> ---
>> drivers/of/overlay.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 102 insertions(+)
>> 
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index a274d65..a7956a2 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -514,6 +514,101 @@ static int of_free_overlay_info(struct of_overlay *ov)
>>        return 0;
>> }
>> 
>> +static int of_overlay_add_symbols(
>> +               struct device_node *tree,
>> +               struct of_overlay *ov)
>> +{
>> +       struct of_overlay_info *ovinfo;
>> +       struct device_node *root_sym = NULL;
>> +       struct device_node *child = NULL;
>> +       struct property *prop;
>> +       const char *path, *s;
>> +       char *new_path;
>> +       int i, len, err;
>> +
>> +       /* this may fail (if no fixups are required) */
>> +       root_sym = of_find_node_by_path("/__symbols__");
>> +
>> +       /* do nothing if no root symbols */
>> +       if (!root_sym)
>> +               return 0;
>> +
>> +       /* locate the symbols & fixups nodes on resolve */
>> +       for_each_child_of_node(tree, child) {
>> +               if (of_node_cmp(child->name, "__symbols__") == 0)
>> +                       goto found;
> 
> Doesn't of_get_child_by_name work here?
> 

No, we’re holding of_mutex.

>> +       }
>> +       /* no symbols, no problem */
>> +       of_node_put(root_sym);
>> +       return 0;
>> +
>> +found:
> 
> As Marek said, get rid of the goto.
> 
>> +       err = -EINVAL;
>> +       for_each_property_of_node(child, prop) {
>> +
>> +               /* skip properties added automatically */
>> +               if (of_prop_cmp(prop->name, "name") == 0)
>> +                       continue;
>> +
>> +               err = of_property_read_string(child,
>> +                               prop->name, &path);
>> +               if (err != 0) {
>> +                       pr_err("%s: Could not find symbol '%s'\n",
>> +                                       __func__, prop->name);
> 
> Can you introduce and use a common pr_fmt prefix here and elsewhere
> (just what you are adding).

OK

> 
>> +                       continue;
>> +               }
>> +
>> +
> 
> Extra blank line.
> 
>> +               /* now find fragment index */
>> +               s = path;
>> +
>> +               /* compare paths to find fragment index */
>> +               ovinfo = NULL;
>> +               len = -1;
> 
> Move these into the for init.
> 
>> +               for (i = 0; i < ov->count; i++) {
>> +                       ovinfo = &ov->ovinfo_tab[i];
>> +
>> +                       pr_debug("%s: #%d: overlay->name=%s target->name=%s\n",
>> +                                       __func__, i, ovinfo->overlay->full_name,
>> +                                       ovinfo->target->full_name);
>> +
>> +                       len = strlen(ovinfo->overlay->full_name);
>> +                       if (strncasecmp(path, ovinfo->overlay->full_name,
>> +                                               len) == 0 && path[len] == '/')
>> +                               break;
>> +               }
>> +
>> +               if (i >= ov->count)
>> +                       continue;
>> +
>> +               pr_debug("%s: found target at #%d\n", __func__, i);
>> +               new_path = kasprintf(GFP_KERNEL, "%s%s",
>> +                               ovinfo->target->full_name,
>> +                               path + len);
>> +               if (!new_path) {
>> +                       pr_err("%s: Failed to allocate propname for \"%s\"\n",
>> +                                       __func__, prop->name);
>> +                       continue;
> 
> If this fails, is there much point in continuing?
> 

No

>> +               }
>> +
>> +               err = of_changeset_add_property_string(&ov->cset, root_sym,
>> +                               prop->name, new_path);
>> +
>> +               /* free always */
>> +               kfree(new_path);
>> +
>> +               if (err) {
> 
> No need for brackets.
> 
>> +                       pr_err("%s: Failed to add property for \"%s\"\n",
>> +                                       __func__, prop->name);
>> +               }
>> +       }
>> +
>> +       of_node_put(child);
>> +       of_node_put(root_sym);
>> +
>> +       return 0;
>> +}
>> +
>> static LIST_HEAD(ov_list);
>> static DEFINE_IDR(ov_idr);
>> 
>> @@ -642,6 +737,13 @@ static int __of_overlay_create(struct device_node *tree,
>>                goto err_abort_trans;
>>        }
>> 
>> +       err = of_overlay_add_symbols(tree, ov);
>> +       if (err) {
>> +               pr_err("%s: of_overlay_add_symbols() failed for tree@%s\n",
>> +                               __func__, tree->full_name);
>> +               goto err_abort_trans;
>> +       }
>> +
>>        /* apply the changeset */
>>        err = __of_changeset_apply(&ov->cset);
>>        if (err) {
>> --
>> 1.7.12
>> 

Regards

— Pantelis

Powered by blists - more mailing lists