From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752139AbcEJOHB (ORCPT ); Tue, 10 May 2016 10:07:01 -0400 Received: from li42-95.members.linode.com ([209.123.162.95]:58963 "EHLO li42-95.members.linode.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751231AbcEJOG7 convert rfc822-to-8bit (ORCPT ); Tue, 10 May 2016 10:06:59 -0400 Subject: Re: [PATCH 3/3] of: overlay: Pick up label symbols from overlays. Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\)) Content-Type: text/plain; charset=utf-8 From: Pantelis Antoniou In-Reply-To: Date: Tue, 10 May 2016 16:56:43 +0300 Cc: Frank Rowand , Matt Porter , Grant Likely , Koen Kooi , Guenter Roeck , Marek Vasut , Geert Uytterhoeven , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" Content-Transfer-Encoding: 8BIT Message-Id: <2B34FBA8-A9DB-4DB5-BF95-4FE752EAF128@antoniou-consulting.com> References: <1462817488-8370-1-git-send-email-pantelis.antoniou@konsulko.com> <1462817488-8370-4-git-send-email-pantelis.antoniou@konsulko.com> To: Rob Herring X-Mailer: Apple Mail (2.3124) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Rob, > On May 10, 2016, at 00:47 , Rob Herring wrote: > > On Mon, May 9, 2016 at 1:11 PM, Pantelis Antoniou > 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 >> Tested-by: Geert Uytterhoeven >> >> --- >> 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