diff mbox

scripts/dtc: Update to upstream version 53bf130b1cdd

Message ID 1457104420-18350-1-git-send-email-robh@kernel.org
State Accepted
Commit b993734718c0106418e068f21c7be01afc12306c
Headers show

Commit Message

Rob Herring (Arm) March 4, 2016, 3:13 p.m. UTC
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(-)

-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Rob Herring (Arm) March 8, 2016, 7:37 a.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring (Arm) March 8, 2016, 8 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring (Arm) April 30, 2016, 8:48 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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"