[PATCH] scripts/dtc: Update to upstream version 53bf130b1cdd

[PATCH] scripts/dtc: Update to upstream version 53bf130b1cdd

[Rob Herring]

Sync to upstream dtc commit 53bf130b1cdd ("libfdt: simplify
fdt_node_check_compatible()"). This adds the following commits from
upstream:

53bf130 libfdt: simplify fdt_node_check_compatible()
c9d9121 Warn on node name unit-address presence/absence mismatch
2e53f9d Catch unsigned 32bit overflow when parsing flattened device tree offsets

Signed-off-by: Rob Herring <robh@kernel.org>
---
As usual, this is just an automated copy of upstream dtc into the kernel 
tree. The changeset is small enough that I have left it here.

The main reason for this sync is to pick-up the new unit-address 
warnings.

Rob

 scripts/dtc/checks.c        | 26 ++++++++++++++++++++++++++
 scripts/dtc/flattree.c      |  4 ++--
 scripts/dtc/libfdt/fdt_ro.c |  6 ++----
 scripts/dtc/version_gen.h   |  2 +-
 4 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/scripts/dtc/checks.c b/scripts/dtc/checks.c
index 0c03ac9..386f956 100644
--- a/scripts/dtc/checks.c
+++ b/scripts/dtc/checks.c
@@ -294,6 +294,30 @@ static void check_node_name_format(struct check *c, struct node *dt,
 }
 NODE_ERROR(node_name_format, NULL, &node_name_chars);
 
+static void check_unit_address_vs_reg(struct check *c, struct node *dt,
+			     struct node *node)
+{
+	const char *unitname = get_unitname(node);
+	struct property *prop = get_property(node, "reg");
+
+	if (!prop) {
+		prop = get_property(node, "ranges");
+		if (prop && !prop->val.len)
+			prop = NULL;
+	}
+
+	if (prop) {
+		if (!unitname[0])
+			FAIL(c, "Node %s has a reg or ranges property, but no unit name",
+			    node->fullpath);
+	} else {
+		if (unitname[0])
+			FAIL(c, "Node %s has a unit name, but no reg property",
+			    node->fullpath);
+	}
+}
+NODE_WARNING(unit_address_vs_reg, NULL);
+
 static void check_property_name_chars(struct check *c, struct node *dt,
 				      struct node *node, struct property *prop)
 {
@@ -667,6 +691,8 @@ static struct check *check_table[] = {
 
 	&addr_size_cells, &reg_format, &ranges_format,
 
+	&unit_address_vs_reg,
+
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
 
diff --git a/scripts/dtc/flattree.c b/scripts/dtc/flattree.c
index bd99fa2..ec14954 100644
--- a/scripts/dtc/flattree.c
+++ b/scripts/dtc/flattree.c
@@ -889,7 +889,7 @@ struct boot_info *dt_from_blob(const char *fname)
 
 	if (version >= 3) {
 		uint32_t size_str = fdt32_to_cpu(fdt->size_dt_strings);
-		if (off_str+size_str > totalsize)
+		if ((off_str+size_str < off_str) || (off_str+size_str > totalsize))
 			die("String table extends past total size\n");
 		inbuf_init(&strbuf, blob + off_str, blob + off_str + size_str);
 	} else {
@@ -898,7 +898,7 @@ struct boot_info *dt_from_blob(const char *fname)
 
 	if (version >= 17) {
 		size_dt = fdt32_to_cpu(fdt->size_dt_struct);
-		if (off_dt+size_dt > totalsize)
+		if ((off_dt+size_dt < off_dt) || (off_dt+size_dt > totalsize))
 			die("Structure block extends past total size\n");
 	}
 
diff --git a/scripts/dtc/libfdt/fdt_ro.c b/scripts/dtc/libfdt/fdt_ro.c
index e5b3136..50cce86 100644
--- a/scripts/dtc/libfdt/fdt_ro.c
+++ b/scripts/dtc/libfdt/fdt_ro.c
@@ -647,10 +647,8 @@ int fdt_node_check_compatible(const void *fdt, int nodeoffset,
 	prop = fdt_getprop(fdt, nodeoffset, "compatible", &len);
 	if (!prop)
 		return len;
-	if (fdt_stringlist_contains(prop, len, compatible))
-		return 0;
-	else
-		return 1;
+
+	return !fdt_stringlist_contains(prop, len, compatible);
 }
 
 int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
diff --git a/scripts/dtc/version_gen.h b/scripts/dtc/version_gen.h
index 11d93e6..ad9b05a 100644
--- a/scripts/dtc/version_gen.h
+++ b/scripts/dtc/version_gen.h
@@ -1 +1 @@
-#define DTC_VERSION "DTC 1.4.1-gb06e55c8"
+#define DTC_VERSION "DTC 1.4.1-g53bf130b"
-- 
2.5.0

Re: [PATCH] scripts/dtc: Update to upstream version 53bf130b1cdd

[Geert Uytterhoeven]

Hi Rob,

On Fri, Mar 4, 2016 at 4:13 PM, Rob Herring <robh@kernel.org> wrote:
> Sync to upstream dtc commit 53bf130b1cdd ("libfdt: simplify
> fdt_node_check_compatible()"). This adds the following commits from
> upstream:
>
> 53bf130 libfdt: simplify fdt_node_check_compatible()
> c9d9121 Warn on node name unit-address presence/absence mismatch
> 2e53f9d Catch unsigned 32bit overflow when parsing flattened device tree offsets
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> As usual, this is just an automated copy of upstream dtc into the kernel
> tree. The changeset is small enough that I have left it here.
>
> The main reason for this sync is to pick-up the new unit-address
> warnings.

I gave this a try. Obviously it finds many abuses that should be fixed.

However, I'm wondering about the following, where the unit-address is just
used to distinguish between multiple instances:

    Warning (unit_address_vs_reg): Node /cache-controller@0 has a unit
name, but no reg property
        compatible = "cache";
    Warning (unit_address_vs_reg): Node /regulator@1 has a unit name,
but no reg property
        compatible = "regulator-fixed"
    Warning (unit_address_vs_reg): Node /i2c@2 has a unit name, but no
reg property
        compatible = "i2c-gpio"

How should these be fixed?

BTW, there seems to be a missing dependency of the DTBs on the dtc itself.
Applying your patch and running "make dtbs" didn't rebuilt any DTBs.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] scripts/dtc: Update to upstream version 53bf130b1cdd

[Rob Herring]

On Mon, Mar 7, 2016 at 5:27 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Rob,
>
> On Fri, Mar 4, 2016 at 4:13 PM, Rob Herring <robh@kernel.org> wrote:
>> Sync to upstream dtc commit 53bf130b1cdd ("libfdt: simplify
>> fdt_node_check_compatible()"). This adds the following commits from
>> upstream:
>>
>> 53bf130 libfdt: simplify fdt_node_check_compatible()
>> c9d9121 Warn on node name unit-address presence/absence mismatch
>> 2e53f9d Catch unsigned 32bit overflow when parsing flattened device tree offsets
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>> As usual, this is just an automated copy of upstream dtc into the kernel
>> tree. The changeset is small enough that I have left it here.
>>
>> The main reason for this sync is to pick-up the new unit-address
>> warnings.
>
> I gave this a try. Obviously it finds many abuses that should be fixed.
>
> However, I'm wondering about the following, where the unit-address is just
> used to distinguish between multiple instances:
>
>     Warning (unit_address_vs_reg): Node /cache-controller@0 has a unit
> name, but no reg property
>         compatible = "cache";

Just add a reg property. The values should probably match the MPIDR in
some way (e.g. 0 and 100).

>     Warning (unit_address_vs_reg): Node /regulator@1 has a unit name,
> but no reg property
>         compatible = "regulator-fixed"

Regulators are oddball in that the node names are generally supposed
to be the regulator name not generic.

>     Warning (unit_address_vs_reg): Node /i2c@2 has a unit name, but no
> reg property
>         compatible = "i2c-gpio"

You all should have all the on-chip devices under a simple-bus, then
you would not have this namespace collision here. Still you could have
2 i2c-gpio devices. We can add reg in those cases.

>
> How should these be fixed?
>
> BTW, there seems to be a missing dependency of the DTBs on the dtc itself.
> Applying your patch and running "make dtbs" didn't rebuilt any DTBs.

Should probably fix, but It is rare that that would actually matter.

Rob

Re: [PATCH] scripts/dtc: Update to upstream version 53bf130b1cdd

[Geert Uytterhoeven]

Hi Rob,

On Tue, Mar 8, 2016 at 9:00 AM, Rob Herring <robh@kernel.org> wrote:
> On Mon, Mar 7, 2016 at 5:27 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Fri, Mar 4, 2016 at 4:13 PM, Rob Herring <robh@kernel.org> wrote:
>>> Sync to upstream dtc commit 53bf130b1cdd ("libfdt: simplify
>>> fdt_node_check_compatible()"). This adds the following commits from
>>> upstream:
>>>
>>> 53bf130 libfdt: simplify fdt_node_check_compatible()
>>> c9d9121 Warn on node name unit-address presence/absence mismatch
>>> 2e53f9d Catch unsigned 32bit overflow when parsing flattened device tree offsets
>>>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>> As usual, this is just an automated copy of upstream dtc into the kernel
>>> tree. The changeset is small enough that I have left it here.
>>>
>>> The main reason for this sync is to pick-up the new unit-address
>>> warnings.
>>
>> I gave this a try. Obviously it finds many abuses that should be fixed.
>>
>> However, I'm wondering about the following, where the unit-address is just
>> used to distinguish between multiple instances:
>>
>>     Warning (unit_address_vs_reg): Node /cache-controller@0 has a unit
>> name, but no reg property
>>         compatible = "cache";
>
> Just add a reg property. The values should probably match the MPIDR in
> some way (e.g. 0 and 100).

it's not "just" adding a reg property.
It's also introducing a "cache" subnode with "#address-cells = <1>" and
"#size-cells = <0>", and perhaps adding code to make the subnode work.
Or moving them under the cpu node, which is actually what the example in
ePAPR shows.

>>     Warning (unit_address_vs_reg): Node /regulator@1 has a unit name,
>> but no reg property
>>         compatible = "regulator-fixed"
>
> Regulators are oddball in that the node names are generally supposed
> to be the regulator name not generic.

OK.

>>     Warning (unit_address_vs_reg): Node /i2c@2 has a unit name, but no
>> reg property
>>         compatible = "i2c-gpio"
>
> You all should have all the on-chip devices under a simple-bus, then
> you would not have this namespace collision here. Still you could have

Fair enough.

> 2 i2c-gpio devices. We can add reg in those cases.

Indeed.
And it's not "just" adding a reg property. More "#address-cells = <1>" and
"#size-cells = <0>" glue needed here, too.
And an update of Documentation/devicetree/bindings/i2c/i2c-gpio.txt

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] scripts/dtc: Update to upstream version 53bf130b1cdd

[Geert Uytterhoeven]

Hi Rob,

On Tue, Mar 8, 2016 at 9:00 AM, Rob Herring <robh@kernel.org> wrote:
> On Mon, Mar 7, 2016 at 5:27 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Fri, Mar 4, 2016 at 4:13 PM, Rob Herring <robh@kernel.org> wrote:
>>> Sync to upstream dtc commit 53bf130b1cdd ("libfdt: simplify
>>> fdt_node_check_compatible()"). This adds the following commits from
>>> upstream:
>>>
>>> 53bf130 libfdt: simplify fdt_node_check_compatible()
>>> c9d9121 Warn on node name unit-address presence/absence mismatch
>>> 2e53f9d Catch unsigned 32bit overflow when parsing flattened device tree offsets
>>>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>> As usual, this is just an automated copy of upstream dtc into the kernel
>>> tree. The changeset is small enough that I have left it here.
>>>
>>> The main reason for this sync is to pick-up the new unit-address
>>> warnings.
>>
>> I gave this a try. Obviously it finds many abuses that should be fixed.
>>
>> However, I'm wondering about the following, where the unit-address is just
>> used to distinguish between multiple instances:
>>
>>     Warning (unit_address_vs_reg): Node /cache-controller@0 has a unit
>> name, but no reg property
>>         compatible = "cache";
>
> Just add a reg property. The values should probably match the MPIDR in
> some way (e.g. 0 and 100).

Is it correct to move the cache-controller nodes under the cpus node?
Else the reg properties don't match #address/size-cells?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] scripts/dtc: Update to upstream version 53bf130b1cdd

[Rob Herring]

On Fri, Apr 29, 2016 at 3:51 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Rob,
>
> On Tue, Mar 8, 2016 at 9:00 AM, Rob Herring <robh@kernel.org> wrote:
>> On Mon, Mar 7, 2016 at 5:27 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Fri, Mar 4, 2016 at 4:13 PM, Rob Herring <robh@kernel.org> wrote:
>>>> Sync to upstream dtc commit 53bf130b1cdd ("libfdt: simplify
>>>> fdt_node_check_compatible()"). This adds the following commits from
>>>> upstream:
>>>>
>>>> 53bf130 libfdt: simplify fdt_node_check_compatible()
>>>> c9d9121 Warn on node name unit-address presence/absence mismatch
>>>> 2e53f9d Catch unsigned 32bit overflow when parsing flattened device tree offsets
>>>>
>>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>>> ---
>>>> As usual, this is just an automated copy of upstream dtc into the kernel
>>>> tree. The changeset is small enough that I have left it here.
>>>>
>>>> The main reason for this sync is to pick-up the new unit-address
>>>> warnings.
>>>
>>> I gave this a try. Obviously it finds many abuses that should be fixed.
>>>
>>> However, I'm wondering about the following, where the unit-address is just
>>> used to distinguish between multiple instances:
>>>
>>>     Warning (unit_address_vs_reg): Node /cache-controller@0 has a unit
>>> name, but no reg property
>>>         compatible = "cache";
>>
>> Just add a reg property. The values should probably match the MPIDR in
>> some way (e.g. 0 and 100).
>
> Is it correct to move the cache-controller nodes under the cpus node?

IIRC, the ePAPR^W DTSpec says that is valid.

> Else the reg properties don't match #address/size-cells?

If there's no mmio access then yes, I think under /cpus makes sense.
The ARM /cpus code may throw a warning on this, but we should quiet it
down.

Rob

Re: [PATCH] scripts/dtc: Update to upstream version 53bf130b1cdd

[Olof Johansson]

Hi,

On Fri, Mar 4, 2016 at 7:13 AM, Rob Herring <robh@kernel.org> wrote:
> Sync to upstream dtc commit 53bf130b1cdd ("libfdt: simplify
> fdt_node_check_compatible()"). This adds the following commits from
> upstream:
>
> 53bf130 libfdt: simplify fdt_node_check_compatible()
> c9d9121 Warn on node name unit-address presence/absence mismatch
> 2e53f9d Catch unsigned 32bit overflow when parsing flattened device tree offsets
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> As usual, this is just an automated copy of upstream dtc into the kernel
> tree. The changeset is small enough that I have left it here.
>
> The main reason for this sync is to pick-up the new unit-address
> warnings.

This spews a crazy amount of warnings on a multi_v7_defconfig build.

I'd prefer to see most of those warnings fixed _before_ we introduce
it by default. Otherwise we just add a huge amount of noise that will
hide any real valid warnings that are now brought up.


-Olof

Re: [PATCH] scripts/dtc: Update to upstream version 53bf130b1cdd

[Rob Herring]

On Mon, Mar 7, 2016 at 5:10 PM, Olof Johansson <olof@lixom.net> wrote:
> Hi,
>
> On Fri, Mar 4, 2016 at 7:13 AM, Rob Herring <robh@kernel.org> wrote:
>> Sync to upstream dtc commit 53bf130b1cdd ("libfdt: simplify
>> fdt_node_check_compatible()"). This adds the following commits from
>> upstream:
>>
>> 53bf130 libfdt: simplify fdt_node_check_compatible()
>> c9d9121 Warn on node name unit-address presence/absence mismatch
>> 2e53f9d Catch unsigned 32bit overflow when parsing flattened device tree offsets
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>> As usual, this is just an automated copy of upstream dtc into the kernel
>> tree. The changeset is small enough that I have left it here.
>>
>> The main reason for this sync is to pick-up the new unit-address
>> warnings.
>
> This spews a crazy amount of warnings on a multi_v7_defconfig build.

Shocking, huh? And I've got more checks in the works. :)

> I'd prefer to see most of those warnings fixed _before_ we introduce
> it by default. Otherwise we just add a huge amount of noise that will
> hide any real valid warnings that are now brought up.

How do you propose to do that? If it is not enabled, then no one will
see them nor care. I don't intend to fix everyone's stuff myself. We
could hide the check behind COMPILE_TEST perhaps.

Rob

Re: [PATCH] scripts/dtc: Update to upstream version 53bf130b1cdd

[Olof Johansson]

Hi,

On Mon, Mar 7, 2016 at 11:37 PM, Rob Herring <robh@kernel.org> wrote:
> On Mon, Mar 7, 2016 at 5:10 PM, Olof Johansson <olof@lixom.net> wrote:
>> Hi,
>>
>> On Fri, Mar 4, 2016 at 7:13 AM, Rob Herring <robh@kernel.org> wrote:
>>> Sync to upstream dtc commit 53bf130b1cdd ("libfdt: simplify
>>> fdt_node_check_compatible()"). This adds the following commits from
>>> upstream:
>>>
>>> 53bf130 libfdt: simplify fdt_node_check_compatible()
>>> c9d9121 Warn on node name unit-address presence/absence mismatch
>>> 2e53f9d Catch unsigned 32bit overflow when parsing flattened device tree offsets
>>>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>> As usual, this is just an automated copy of upstream dtc into the kernel
>>> tree. The changeset is small enough that I have left it here.
>>>
>>> The main reason for this sync is to pick-up the new unit-address
>>> warnings.
>>
>> This spews a crazy amount of warnings on a multi_v7_defconfig build.
>
> Shocking, huh? And I've got more checks in the works. :)
>
>> I'd prefer to see most of those warnings fixed _before_ we introduce
>> it by default. Otherwise we just add a huge amount of noise that will
>> hide any real valid warnings that are now brought up.
>
> How do you propose to do that? If it is not enabled, then no one will
> see them nor care. I don't intend to fix everyone's stuff myself.

Right, but pushing a change on everyone that makes the build
near-unusable as a tool to see if you're introducing a new error is
also not a good way to do it. However:

> We could hide the check behind COMPILE_TEST perhaps.

Putting it behind an option sounds like a suitable approach. Would it
be possible to plumb in under "make C=1" / "make C=2" instead? That's
closer in meaning to what you're doing here than COMPILE_TEST, which
is more about turning on drivers that might not make sense on your
platform to get compile coverage.

This would make people aware of the tool, give them an easy way to run
it to do the cleanups. Nag maintainers when they post new code without
having paid attention to the old sources/cleanup, and with some amount
of time most of it might be fixed.


-Olof

Re: [PATCH] scripts/dtc: Update to upstream version 53bf130b1cdd

[Rob Herring]

On Tue, Mar 8, 2016 at 10:22 AM, Olof Johansson <olof@lixom.net> wrote:
> Hi,
>
> On Mon, Mar 7, 2016 at 11:37 PM, Rob Herring <robh@kernel.org> wrote:
>> On Mon, Mar 7, 2016 at 5:10 PM, Olof Johansson <olof@lixom.net> wrote:
>>> Hi,
>>>
>>> On Fri, Mar 4, 2016 at 7:13 AM, Rob Herring <robh@kernel.org> wrote:
>>>> Sync to upstream dtc commit 53bf130b1cdd ("libfdt: simplify
>>>> fdt_node_check_compatible()"). This adds the following commits from
>>>> upstream:
>>>>
>>>> 53bf130 libfdt: simplify fdt_node_check_compatible()
>>>> c9d9121 Warn on node name unit-address presence/absence mismatch
>>>> 2e53f9d Catch unsigned 32bit overflow when parsing flattened device tree offsets
>>>>
>>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>>> ---
>>>> As usual, this is just an automated copy of upstream dtc into the kernel
>>>> tree. The changeset is small enough that I have left it here.
>>>>
>>>> The main reason for this sync is to pick-up the new unit-address
>>>> warnings.
>>>
>>> This spews a crazy amount of warnings on a multi_v7_defconfig build.
>>
>> Shocking, huh? And I've got more checks in the works. :)
>>
>>> I'd prefer to see most of those warnings fixed _before_ we introduce
>>> it by default. Otherwise we just add a huge amount of noise that will
>>> hide any real valid warnings that are now brought up.
>>
>> How do you propose to do that? If it is not enabled, then no one will
>> see them nor care. I don't intend to fix everyone's stuff myself.
>
> Right, but pushing a change on everyone that makes the build
> near-unusable as a tool to see if you're introducing a new error is
> also not a good way to do it. However:
>
>> We could hide the check behind COMPILE_TEST perhaps.
>
> Putting it behind an option sounds like a suitable approach. Would it
> be possible to plumb in under "make C=1" / "make C=2" instead? That's
> closer in meaning to what you're doing here than COMPILE_TEST, which
> is more about turning on drivers that might not make sense on your
> platform to get compile coverage.

C=1/2 is only designed to run on C files, so it would be a bit of work
to extend it. However, "W=1" seems like a better match and easy to
add:

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index ad50d58..417d8c2 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -271,6 +271,10 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -n
-f -9 > $@) || \
 # ---------------------------------------------------------------------------
 DTC ?= $(objtree)/scripts/dtc/dtc

+ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),)
+DTC_FLAGS += -Wno-unit_address_vs_reg
+endif
+
 # Generate an assembly file to wrap the output of the device tree compiler
 quiet_cmd_dt_S_dtb= DTB     $@
 cmd_dt_S_dtb=                                          \


> This would make people aware of the tool, give them an easy way to run
> it to do the cleanups. Nag maintainers when they post new code without
> having paid attention to the old sources/cleanup, and with some amount
> of time most of it might be fixed.

I added the ability to do "make DTC=my/dtc", so there already is a way
for people to run it if they want to. Of course, they'd have to fetch
and build dtc compared to the above. Still, with either option it is
dependent on people running something they don't likely run normally.

Rob