| Message ID | 1457104420-18350-1-git-send-email-robh@kernel.org |
|---|---|
| State | Accepted |
| Commit | b993734718c0106418e068f21c7be01afc12306c |
| Headers | show |
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
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
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 --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, ®_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"
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