From: Frank Rowand <frowand.list@gmail.com>
To: Joe Perches <joe@perches.com>, Rob Herring <robh+dt@kernel.org>,
Pantelis Antoniou <pantelis.antoniou@konsulko.com>,
Michael Ellerman <mpe@ellerman.id.au>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>, Alan Tull <atull@kernel.org>,
Moritz Fischer <mdf@kernel.org>
Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
devicetree@vger.kernel.org, linux-fpga@vger.kernel.org
Subject: Re: [PATCH v2 12/18] of: overlay: check prevents multiple fragments add or delete same node
Date: Sat, 13 Oct 2018 11:21:46 -0700 [thread overview]
Message-ID: <651a8488-caec-b153-43c7-1fb81f641f1a@gmail.com> (raw)
In-Reply-To: <97b26203e6792795bdc0a66ce4cdb47571ff16c1.camel@perches.com>
On 10/13/18 05:51, Joe Perches wrote:
> On Fri, 2018-10-12 at 21:53 -0700, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> Multiple overlay fragments adding or deleting the same node is not
>> supported. Replace code comment of such, with check to detect the
>> attempt and fail the overlay apply.
>>
>> Devicetree unittest where multiple fragments added the same node was
>> added in the previous patch in the series. After applying this patch
>> the unittest messages will no longer include:
>>
>> Duplicate name in motor-1, renamed to "controller#1"
>> OF: overlay: of_overlay_apply() err=0
>> ### dt-test ### of_overlay_fdt_apply() expected -22, ret=0, overlay_bad_add_dup_node
>> ### dt-test ### FAIL of_unittest_overlay_high_level():2419 Adding overlay 'overlay_bad_add_dup_node' failed
>>
>> ...
>>
>> ### dt-test ### end of unittest - 210 passed, 1 failed
>>
>> but will instead include:
>>
>> OF: overlay: ERROR: multiple overlay fragments add and/or delete node /testcase-data-2/substation@100/motor-1/controller
>>
>> ...
>>
>> ### dt-test ### end of unittest - 211 passed, 0 failed
> []
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> []
>> @@ -523,6 +515,54 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
>> }
>>
>> /**
>> + * check_changeset_dup_add_node() - changeset validation: duplicate add node
>> + * @ovcs: Overlay changeset
>> + *
>> + * Check changeset @ovcs->cset for multiple add node entries for the same
>> + * node.
>> + *
>> + * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if
>> + * invalid overlay in @ovcs->fragments[].
>> + */
>> +static int check_changeset_dup_add_node(struct overlay_changeset *ovcs)
>> +{
>> + struct of_changeset_entry *ce_1, *ce_2;
>> + char *fn_1, *fn_2;
>> + int name_match;
>> +
>> + list_for_each_entry(ce_1, &ovcs->cset.entries, node) {
>> +
>> + if (ce_1->action == OF_RECONFIG_ATTACH_NODE ||
>> + ce_1->action == OF_RECONFIG_DETACH_NODE) {
>> +
>> + ce_2 = ce_1;
>> + list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) {
>> + if (ce_2->action == OF_RECONFIG_ATTACH_NODE ||
>> + ce_2->action == OF_RECONFIG_DETACH_NODE) {
>> + /* inexpensive name compare */
>> + if (!of_node_cmp(ce_1->np->full_name,
>> + ce_2->np->full_name)) {
>
> A bit of odd indentation here.
> This line is normally aligned to the second ( on the line above.
Yes, thanks.
>
>> + /* expensive full path name compare */
>> + fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np);
>> + fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np);
>> + name_match = !strcmp(fn_1, fn_2);
>> + kfree(fn_1);
>> + kfree(fn_2);
>> + if (name_match) {
>> + pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n",
>> + ce_1->np);
>> + return -EINVAL;
>> + }
>> + }
>> + }
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>
> Style trivia:
>
> Using inverted tests and continue would reduce indentation.
Yes, thanks.
-Frank
>
> list_for_each_entry(ce_1, &ovcs->cset.entries, node) {
> if (ce_1->action != OF_RECONFIG_ATTACH_NODE &&
> ce_1->action != OF_RECONFIG_DETACH_NODE)
> continue;
>
> ce_2 = ce_1;
> list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) {
> if (ce_2->action != OF_RECONFIG_ATTACH_NODE &&
> ce_2->action != OF_RECONFIG_DETACH_NODE)
> continue;
>
> /* inexpensive name compare */
> if (of_node_cmp(ce_1->np->full_name, ce_2->np->full_name))
> continue;
>
> /* expensive full path name compare */
> fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np);
> fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np);
> name_match = !strcmp(fn_1, fn_2);
> kfree(fn_1);
> kfree(fn_2);
> if (name_match) {
> pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n",
> ce_1->np);
> return -EINVAL;
> }
> }
> }
>
>
>
next prev parent reply other threads:[~2018-10-13 18:21 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-13 4:53 [PATCH v2 00/18] of: overlay: validation checks, subsequent fixes frowand.list
2018-10-13 4:53 ` [PATCH v2 01/18] of: overlay: add tests to validate kfrees from overlay removal frowand.list
2018-10-13 4:53 ` [PATCH v2 02/18] of: overlay: add missing of_node_put() after add new node to changeset frowand.list
2018-10-13 4:53 ` [PATCH v2 03/18] of: overlay: add missing of_node_get() in __of_attach_node_sysfs frowand.list
2018-10-13 4:53 ` [PATCH v2 04/18] powerpc/pseries: add of_node_put() in dlpar_detach_node() frowand.list
2018-10-13 4:53 ` [PATCH v2 05/18] of: overlay: use prop add changeset entry for property in new nodes frowand.list
2018-10-13 4:53 ` [PATCH v2 06/18] of: overlay: do not duplicate properties from overlay for " frowand.list
2018-10-13 4:53 ` [PATCH v2 07/18] of: dynamic: change type of of_{at,de}tach_node() to void frowand.list
2018-10-13 4:53 ` [PATCH v2 08/18] of: overlay: reorder fields in struct fragment frowand.list
2018-10-13 4:53 ` [PATCH v2 09/18] of: overlay: validate overlay properties #address-cells and #size-cells frowand.list
2018-10-13 4:53 ` [PATCH v2 10/18] of: overlay: make all pr_debug() and pr_err() messages unique frowand.list
2018-10-13 4:53 ` [PATCH v2 11/18] of: overlay: test case of two fragments adding same node frowand.list
2018-10-13 4:53 ` [PATCH v2 12/18] of: overlay: check prevents multiple fragments add or delete " frowand.list
2018-10-13 12:51 ` Joe Perches
2018-10-13 18:21 ` Frank Rowand [this message]
2018-10-15 0:29 ` Frank Rowand
2018-10-13 4:53 ` [PATCH v2 13/18] of: overlay: check prevents multiple fragments touching same property frowand.list
2018-10-13 4:53 ` [PATCH v2 14/18] of: unittest: remove unused of_unittest_apply_overlay() argument frowand.list
2018-10-13 4:53 ` [PATCH v2 15/18] of: overlay: set node fields from properties when add new overlay node frowand.list
2018-10-13 4:53 ` [PATCH v2 16/18] of: unittest: allow base devicetree to have symbol metadata frowand.list
2018-10-13 4:53 ` [PATCH v2 17/18] of: unittest: find overlays[] entry by name instead of index frowand.list
2018-10-13 4:53 ` [PATCH v2 18/18] of: unittest: initialize args before calling of_*parse_*() frowand.list
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=651a8488-caec-b153-43c7-1fb81f641f1a@gmail.com \
--to=frowand.list@gmail.com \
--cc=atull@kernel.org \
--cc=benh@kernel.crashing.org \
--cc=devicetree@vger.kernel.org \
--cc=joe@perches.com \
--cc=linux-fpga@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mdf@kernel.org \
--cc=mpe@ellerman.id.au \
--cc=pantelis.antoniou@konsulko.com \
--cc=paulus@samba.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).