diff mbox series

scripts: dtc: fix a false alarm for node_name_chars_strict

Message ID 20220531053358.19003-1-qun-wei.lin@mediatek.com (mailing list archive)
State New, archived
Headers show
Series scripts: dtc: fix a false alarm for node_name_chars_strict | expand

Commit Message

Qun-wei Lin (林群崴) May 31, 2022, 5:33 a.m. UTC
The function check_node_name_chars_strict issues a false alarm when
compiling an overlay dts.

/fragment@0/__overlay__: Character '_' not recommended in node name

This workaround will fix it by skip checking for node named __overlay__.

Signed-off-by: Qun-Wei Lin <qun-wei.lin@mediatek.com>
---
 scripts/dtc/checks.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Rob Herring May 31, 2022, 1:06 p.m. UTC | #1
On Tue, May 31, 2022 at 12:34 AM Qun-Wei Lin <qun-wei.lin@mediatek.com> wrote:
>
> The function check_node_name_chars_strict issues a false alarm when
> compiling an overlay dts.
>
> /fragment@0/__overlay__: Character '_' not recommended in node name
>
> This workaround will fix it by skip checking for node named __overlay__.
>
> Signed-off-by: Qun-Wei Lin <qun-wei.lin@mediatek.com>
> ---
>  scripts/dtc/checks.c | 5 +++++

We don't take patches for dtc. You must send them to upstream dtc and
then it will be sync'ed to the kernel tree.

>  1 file changed, 5 insertions(+)
>
> diff --git a/scripts/dtc/checks.c b/scripts/dtc/checks.c
> index 781ba1129a8e..6ef4f2cd67b9 100644
> --- a/scripts/dtc/checks.c
> +++ b/scripts/dtc/checks.c
> @@ -325,6 +325,11 @@ static void check_node_name_chars_strict(struct check *c, struct dt_info *dti,
>  {
>         int n = strspn(node->name, c->data);
>
> +       if (streq(node->name, "__overlay__")) {
> +               /* HACK: Overlay fragments are a special case */

Not a hack IMO.

However, this should be checking for any node name starting with '__'.

Also, doesn't 'fragment@0' cause a warning about missing 'reg'?
Frank Rowand May 31, 2022, 8:43 p.m. UTC | #2
On 5/31/22 01:33, Qun-Wei Lin wrote:
> The function check_node_name_chars_strict issues a false alarm when
> compiling an overlay dts.
> 
> /fragment@0/__overlay__: Character '_' not recommended in node name
> 
> This workaround will fix it by skip checking for node named __overlay__.

This is not a false alarm.

Do not special case node name "__overlay__".  This node name should never
occur in a modern overlay source file.

For details, see "Overlay Source Format" in the "Overlays" section of:
https://elinux.org/Device_Tree_Reference#Overlays

That paragraph also has a pointer to the correct format for overlay
source files, which is slides 29-34 of:
https://elinux.org/Device_Tree_Reference#Overlays

-Frank

> 
> Signed-off-by: Qun-Wei Lin <qun-wei.lin@mediatek.com>
> ---
>  scripts/dtc/checks.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/scripts/dtc/checks.c b/scripts/dtc/checks.c
> index 781ba1129a8e..6ef4f2cd67b9 100644
> --- a/scripts/dtc/checks.c
> +++ b/scripts/dtc/checks.c
> @@ -325,6 +325,11 @@ static void check_node_name_chars_strict(struct check *c, struct dt_info *dti,
>  {
>  	int n = strspn(node->name, c->data);
>  
> +	if (streq(node->name, "__overlay__")) {
> +		/* HACK: Overlay fragments are a special case */
> +		return;
> +	}
> +
>  	if (n < node->basenamelen)
>  		FAIL(c, dti, node, "Character '%c' not recommended in node name",
>  		     node->name[n]);
Rob Herring (Arm) May 31, 2022, 9:49 p.m. UTC | #3
On Tue, May 31, 2022 at 04:43:18PM -0400, Frank Rowand wrote:
> On 5/31/22 01:33, Qun-Wei Lin wrote:
> > The function check_node_name_chars_strict issues a false alarm when
> > compiling an overlay dts.
> > 
> > /fragment@0/__overlay__: Character '_' not recommended in node name
> > 
> > This workaround will fix it by skip checking for node named __overlay__.
> 
> This is not a false alarm.
> 
> Do not special case node name "__overlay__".  This node name should never
> occur in a modern overlay source file.

A dtbo -> dts pass will give warnings, so handling these nodes is 
worthwhile. Though thinking a bit more about it, I think this one is off 
by default, but W=2 turns it on.

Rob
Frank Rowand June 1, 2022, 1:45 a.m. UTC | #4
On 5/31/22 17:49, Rob Herring wrote:
> On Tue, May 31, 2022 at 04:43:18PM -0400, Frank Rowand wrote:
>> On 5/31/22 01:33, Qun-Wei Lin wrote:
>>> The function check_node_name_chars_strict issues a false alarm when
>>> compiling an overlay dts.
>>>
>>> /fragment@0/__overlay__: Character '_' not recommended in node name
>>>
>>> This workaround will fix it by skip checking for node named __overlay__.
>>
>> This is not a false alarm.
>>
>> Do not special case node name "__overlay__".  This node name should never
>> occur in a modern overlay source file.
> 
> A dtbo -> dts pass will give warnings, so handling these nodes is 
> worthwhile. Though thinking a bit more about it, I think this one is off 
> by default, but W=2 turns it on.

Yes, at least as of 5.18-rc1 the warning is only if
'-W node_name_chars_strict', so a dtbo -> dts pass will not give the
warning for node __overlay__ by default.

-Frank

> 
> Rob
Qun-wei Lin (林群崴) June 1, 2022, 1:58 p.m. UTC | #5
On Tue, 2022-05-31 at 08:06 -0500, Rob Herring wrote:
> On Tue, May 31, 2022 at 12:34 AM Qun-Wei Lin <
> qun-wei.lin@mediatek.com> wrote:
> > 
> > The function check_node_name_chars_strict issues a false alarm when
> > compiling an overlay dts.
> > 
> > /fragment@0/__overlay__: Character '_' not recommended in node name
> > 
> > This workaround will fix it by skip checking for node named
> > __overlay__.
> > 
> > Signed-off-by: Qun-Wei Lin <qun-wei.lin@mediatek.com>
> > ---
> >  scripts/dtc/checks.c | 5 +++++
> 
> We don't take patches for dtc. You must send them to upstream dtc and
> then it will be sync'ed to the kernel tree.

Ok, I'll send this patch to the correct upstream.

> 
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/scripts/dtc/checks.c b/scripts/dtc/checks.c
> > index 781ba1129a8e..6ef4f2cd67b9 100644
> > --- a/scripts/dtc/checks.c
> > +++ b/scripts/dtc/checks.c
> > @@ -325,6 +325,11 @@ static void
> > check_node_name_chars_strict(struct check *c, struct dt_info *dti,
> >  {
> >         int n = strspn(node->name, c->data);
> > 
> > +       if (streq(node->name, "__overlay__")) {
> > +               /* HACK: Overlay fragments are a special case */
> 
> Not a hack IMO.
> 
> However, this should be checking for any node name starting with
> '__'.

Did you mean it needs to skip the check for all nodes whose names start
with "_", such as __overlay__, __fixup__, __local_fixup__, and
__symbols__?

> 
> Also, doesn't 'fragment@0' cause a warning about missing 'reg'?

No, it doesn't.
Actually, the check_unit_address_vs_reg function suppress the common
warnings about the fragment names [1].

[1] 
https://github.com/dgibson/dtc/commit/afbddcd418fbf70467419b91f73cce972482449c
Qun-wei Lin (林群崴) June 1, 2022, 1:59 p.m. UTC | #6
On Tue, 2022-05-31 at 20:45 -0500, Frank Rowand wrote:
> On 5/31/22 17:49, Rob Herring wrote:
> > On Tue, May 31, 2022 at 04:43:18PM -0400, Frank Rowand wrote:
> > > On 5/31/22 01:33, Qun-Wei Lin wrote:
> > > > The function check_node_name_chars_strict issues a false alarm
> > > > when
> > > > compiling an overlay dts.
> > > > 
> > > > /fragment@0/__overlay__: Character '_' not recommended in node
> > > > name
> > > > 
> > > > This workaround will fix it by skip checking for node named
> > > > __overlay__.
> > > 
> > > This is not a false alarm.
> > > 
> > > Do not special case node name "__overlay__".  This node name
> > > should never
> > > occur in a modern overlay source file.
> > 
> > A dtbo -> dts pass will give warnings, so handling these nodes is 
> > worthwhile. Though thinking a bit more about it, I think this one
> > is off 
> > by default, but W=2 turns it on.
> 

When DTC detects an overlay target, it will generate a node name
"__overlay__" in function add_orphan_node [1]

Use the example on page 34 of the slide [2]. When the parser reads this
line in overlay source file:

&{/soc/base_fpga_region} {

It will build two nodes, one with no name and the other one is a
subnode __overlay__ [2].

So if we set W=2, the following warning will be issued:

Warning (node_name_chars_strict): /fragment@0/__overlay__: Character
'_' not recommended in node name

I think this is a false alarm because this __overlay__ is automatically
generated by DTC.

> Yes, at least as of 5.18-rc1 the warning is only if
> '-W node_name_chars_strict', so a dtbo -> dts pass will not give the
> warning for node __overlay__ by default.
> 
> -Frank
> 
> > 
> > Rob
> 
> 

We want to enable all warnings in DTC to ensure that all of our DTS are
compliant with the conventions.

[1] 
https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/dtc-parser.y#n169
[2] https://elinux.org/images/0/03/Elce_2018_dt_bof.pdf
[3] 
https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/livetree.c#n228
Frank Rowand June 2, 2022, 3:07 a.m. UTC | #7
On 6/1/22 09:59, qun-wei.lin wrote:
> On Tue, 2022-05-31 at 20:45 -0500, Frank Rowand wrote:
>> On 5/31/22 17:49, Rob Herring wrote:
>>> On Tue, May 31, 2022 at 04:43:18PM -0400, Frank Rowand wrote:
>>>> On 5/31/22 01:33, Qun-Wei Lin wrote:
>>>>> The function check_node_name_chars_strict issues a false alarm
>>>>> when
>>>>> compiling an overlay dts.
>>>>>
>>>>> /fragment@0/__overlay__: Character '_' not recommended in node
>>>>> name
>>>>>
>>>>> This workaround will fix it by skip checking for node named
>>>>> __overlay__.
>>>>
>>>> This is not a false alarm.
>>>>
>>>> Do not special case node name "__overlay__".  This node name
>>>> should never
>>>> occur in a modern overlay source file.
>>>
>>> A dtbo -> dts pass will give warnings, so handling these nodes is 
>>> worthwhile. Though thinking a bit more about it, I think this one
>>> is off 
>>> by default, but W=2 turns it on.
>>
> 
> When DTC detects an overlay target, it will generate a node name
> "__overlay__" in function add_orphan_node [1]
> 
> Use the example on page 34 of the slide [2]. When the parser reads this
> line in overlay source file:
> 
> &{/soc/base_fpga_region} {
> 
> It will build two nodes, one with no name and the other one is a
> subnode __overlay__ [2].
> 
> So if we set W=2, the following warning will be issued:
> 
> Warning (node_name_chars_strict): /fragment@0/__overlay__: Character
> '_' not recommended in node name

Thanks for digging into the internals of dtc to determine how a properly
formatted overlay source file is processed in a way that the check for
a leading "_" in a node name is after dtc creating the __overlay__ node
name.

I have looked at dtc several times and have never educated myself enough
to be anything other than dangerous.  It seems that you might be more
educated about how it works.  If so, is there a reasonable way to tell
check_node_name_chars_strict() when the node name is one of the special
ones that dtc creates for overlays instead of a node name that appears
in a source file?

-Frank

> 
> I think this is a false alarm because this __overlay__ is automatically
> generated by DTC.
> 
>> Yes, at least as of 5.18-rc1 the warning is only if
>> '-W node_name_chars_strict', so a dtbo -> dts pass will not give the
>> warning for node __overlay__ by default.
>>
>> -Frank
>>
>>>
>>> Rob
>>
>>
> 
> We want to enable all warnings in DTC to ensure that all of our DTS are
> compliant with the conventions.
> 
> [1] 
> https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/dtc-parser.y#n169
> [2] https://elinux.org/images/0/03/Elce_2018_dt_bof.pdf
> [3] 
> https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/livetree.c#n228
> 
> 
> 
>
diff mbox series

Patch

diff --git a/scripts/dtc/checks.c b/scripts/dtc/checks.c
index 781ba1129a8e..6ef4f2cd67b9 100644
--- a/scripts/dtc/checks.c
+++ b/scripts/dtc/checks.c
@@ -325,6 +325,11 @@  static void check_node_name_chars_strict(struct check *c, struct dt_info *dti,
 {
 	int n = strspn(node->name, c->data);
 
+	if (streq(node->name, "__overlay__")) {
+		/* HACK: Overlay fragments are a special case */
+		return;
+	}
+
 	if (n < node->basenamelen)
 		FAIL(c, dti, node, "Character '%c' not recommended in node name",
 		     node->name[n]);