| Message ID | 20170228224310.14162-3-robh@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | dtc bus and unit address checks | expand |
On Tue, Feb 28, 2017 at 04:43:09PM -0600, Rob Herring wrote: > Add checks to identify simple-bus bus types and checks for child > devices. Simple-bus type is generally identified by "simple-bus" > compatible string. We also treat the root as a simple-bus, but only for > child nodes with reg property. > > Signed-off-by: Rob Herring <robh@kernel.org> > --- > v2: > - new patch > > checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/checks.c b/checks.c > index 5ed91ac50a10..c4865b4c8da0 100644 > --- a/checks.c > +++ b/checks.c > @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no > } > WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); > > +static const struct bus_type simple_bus = { > + .name = "simple-bus", > +}; > + > +static bool node_is_compatible(struct node *node, const char *compat) > +{ > + struct property *prop; > + const char *str; > + > + prop = get_property(node, "compatible"); > + if (!prop) > + return false; > + > + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { This isn't safe if the compatible property is filled with garbage (not '\0' terminated) - the strlen() could access beyond the end of the property value. > + if (streq(str, compat)) > + return true; > + } > + return false; > +} > + > +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) > +{ > + if (node_is_compatible(node, "simple-bus") || !node->parent) > + node->bus = &simple_bus; I don't think it's correct to assume the root bus is always a simple-bus. If it is, it really should be listed explicitly in the root node's compatible property. > +} > +WARNING(simple_bus_bridge, check_simple_bus_bridge, NULL, &addr_size_cells); > + > +static void check_simple_bus_reg(struct check *c, struct dt_info *dti, struct node *node) > +{ > + struct property *prop; > + const char *unitname = get_unitname(node); > + char unit_addr[17]; > + unsigned int size; > + uint64_t reg = 0; > + cell_t *cells = NULL; > + > + if (!node->parent || (node->parent->bus != &simple_bus)) > + return; > + > + prop = get_property(node, "reg"); > + if (prop) > + cells = (cell_t *)prop->val.val; > + else { > + prop = get_property(node, "ranges"); > + if (prop && prop->val.len) > + /* skip of child address */ > + cells = ((cell_t *)prop->val.val) + node_addr_cells(node); > + } > + > + if (!cells) { > + if (node->parent->parent && !(node->bus == &simple_bus)) > + FAIL(c, dti, "Node %s missing or empty reg/ranges property", node->fullpath); > + return; > + } > + > + size = node_addr_cells(node->parent); > + while (size--) > + reg = (reg << 32) | fdt32_to_cpu(*(cells++)); > + > + snprintf(unit_addr, sizeof(unit_addr), "%lx", reg); > + if (!streq(unitname, unit_addr)) > + FAIL(c, dti, "Node %s simple-bus unit address format error, expected \"%s\"", > + node->fullpath, unit_addr); > +} > +WARNING(simple_bus_reg, check_simple_bus_reg, NULL, ®_format, &simple_bus_bridge); > + > /* > * Style checks > */ > @@ -893,6 +959,9 @@ static struct check *check_table[] = { > &pci_device_reg, > &pci_device_bus_num, > > + &simple_bus_bridge, > + &simple_bus_reg, > + > &avoid_default_addr_size, > &obsolete_chosen_interrupt_controller, > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Thu, Mar 2, 2017 at 8:12 PM, David Gibson <david@gibson.dropbear.id.au> wrote: > On Tue, Feb 28, 2017 at 04:43:09PM -0600, Rob Herring wrote: >> Add checks to identify simple-bus bus types and checks for child >> devices. Simple-bus type is generally identified by "simple-bus" >> compatible string. We also treat the root as a simple-bus, but only for >> child nodes with reg property. >> >> Signed-off-by: Rob Herring <robh@kernel.org> >> --- >> v2: >> - new patch >> >> checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 69 insertions(+) >> >> diff --git a/checks.c b/checks.c >> index 5ed91ac50a10..c4865b4c8da0 100644 >> --- a/checks.c >> +++ b/checks.c >> @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no >> } >> WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); >> >> +static const struct bus_type simple_bus = { >> + .name = "simple-bus", >> +}; >> + >> +static bool node_is_compatible(struct node *node, const char *compat) >> +{ >> + struct property *prop; >> + const char *str; >> + >> + prop = get_property(node, "compatible"); >> + if (!prop) >> + return false; >> + >> + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { > > This isn't safe if the compatible property is filled with garbage (not > '\0' terminated) - the strlen() could access beyond the end of the > property value. Okay, I guess I can check that prop->val.val[prop->val.len - 1] == 0 up front. > >> + if (streq(str, compat)) >> + return true; >> + } >> + return false; >> +} >> + >> +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) >> +{ >> + if (node_is_compatible(node, "simple-bus") || !node->parent) >> + node->bus = &simple_bus; > > I don't think it's correct to assume the root bus is always a > simple-bus. If it is, it really should be listed explicitly in the > root node's compatible property. It is in the sense that Linux treats the root the same and creates devices for top level children and then descends for nodes with "simple-bus". And since unit addresses are a single address space for a given level, we can't have any other bus type. IOW, any other defined bus type like I2C is going to be a child of its controller. 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 06, 2017 at 04:48:18AM -0600, Rob Herring wrote: > On Thu, Mar 2, 2017 at 8:12 PM, David Gibson > <david@gibson.dropbear.id.au> wrote: > > On Tue, Feb 28, 2017 at 04:43:09PM -0600, Rob Herring wrote: > >> Add checks to identify simple-bus bus types and checks for child > >> devices. Simple-bus type is generally identified by "simple-bus" > >> compatible string. We also treat the root as a simple-bus, but only for > >> child nodes with reg property. > >> > >> Signed-off-by: Rob Herring <robh@kernel.org> > >> --- > >> v2: > >> - new patch > >> > >> checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 69 insertions(+) > >> > >> diff --git a/checks.c b/checks.c > >> index 5ed91ac50a10..c4865b4c8da0 100644 > >> --- a/checks.c > >> +++ b/checks.c > >> @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no > >> } > >> WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); > >> > >> +static const struct bus_type simple_bus = { > >> + .name = "simple-bus", > >> +}; > >> + > >> +static bool node_is_compatible(struct node *node, const char *compat) > >> +{ > >> + struct property *prop; > >> + const char *str; > >> + > >> + prop = get_property(node, "compatible"); > >> + if (!prop) > >> + return false; > >> + > >> + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { > > > > This isn't safe if the compatible property is filled with garbage (not > > '\0' terminated) - the strlen() could access beyond the end of the > > property value. > > Okay, I guess I can check that prop->val.val[prop->val.len - 1] == 0 up front. Sure. Or use strnlen. > > > > >> + if (streq(str, compat)) > >> + return true; > >> + } > >> + return false; > >> +} > >> + > >> +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) > >> +{ > >> + if (node_is_compatible(node, "simple-bus") || !node->parent) > >> + node->bus = &simple_bus; > > > > I don't think it's correct to assume the root bus is always a > > simple-bus. If it is, it really should be listed explicitly in the > > root node's compatible property. > > It is in the sense that Linux treats the root the same and creates > devices for top level children and then descends for nodes with > "simple-bus". Hmm.. where in Linux is that? I think that's a bug, technically speaking, traversing the root node's children without regard to the type of the root node. > And since unit addresses are a single address space for > a given level, we can't have any other bus type. IOW, any other > defined bus type like I2C is going to be a child of its controller. > > Rob -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Mon, Mar 6, 2017 at 9:41 PM, David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Mar 06, 2017 at 04:48:18AM -0600, Rob Herring wrote: >> On Thu, Mar 2, 2017 at 8:12 PM, David Gibson >> <david@gibson.dropbear.id.au> wrote: >> > On Tue, Feb 28, 2017 at 04:43:09PM -0600, Rob Herring wrote: >> >> Add checks to identify simple-bus bus types and checks for child >> >> devices. Simple-bus type is generally identified by "simple-bus" >> >> compatible string. We also treat the root as a simple-bus, but only for >> >> child nodes with reg property. >> >> >> >> Signed-off-by: Rob Herring <robh@kernel.org> >> >> --- >> >> v2: >> >> - new patch >> >> >> >> checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> >> 1 file changed, 69 insertions(+) >> >> >> >> diff --git a/checks.c b/checks.c >> >> index 5ed91ac50a10..c4865b4c8da0 100644 >> >> --- a/checks.c >> >> +++ b/checks.c >> >> @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no >> >> } >> >> WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); >> >> >> >> +static const struct bus_type simple_bus = { >> >> + .name = "simple-bus", >> >> +}; >> >> + >> >> +static bool node_is_compatible(struct node *node, const char *compat) >> >> +{ >> >> + struct property *prop; >> >> + const char *str; >> >> + >> >> + prop = get_property(node, "compatible"); >> >> + if (!prop) >> >> + return false; >> >> + >> >> + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { >> > >> > This isn't safe if the compatible property is filled with garbage (not >> > '\0' terminated) - the strlen() could access beyond the end of the >> > property value. >> >> Okay, I guess I can check that prop->val.val[prop->val.len - 1] == 0 up front. > > Sure. Or use strnlen. >> >> > >> >> + if (streq(str, compat)) >> >> + return true; >> >> + } >> >> + return false; >> >> +} >> >> + >> >> +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) >> >> +{ >> >> + if (node_is_compatible(node, "simple-bus") || !node->parent) >> >> + node->bus = &simple_bus; >> > >> > I don't think it's correct to assume the root bus is always a >> > simple-bus. If it is, it really should be listed explicitly in the >> > root node's compatible property. >> >> It is in the sense that Linux treats the root the same and creates >> devices for top level children and then descends for nodes with >> "simple-bus". > > Hmm.. where in Linux is that? I think that's a bug, technically > speaking, traversing the root node's children without regard to the > type of the root node. > >> And since unit addresses are a single address space for >> a given level, we can't have any other bus type. IOW, any other >> defined bus type like I2C is going to be a child of its controller. >> >> Rob > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson -- 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 6, 2017 at 9:41 PM, David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Mar 06, 2017 at 04:48:18AM -0600, Rob Herring wrote: >> On Thu, Mar 2, 2017 at 8:12 PM, David Gibson >> <david@gibson.dropbear.id.au> wrote: >> > On Tue, Feb 28, 2017 at 04:43:09PM -0600, Rob Herring wrote: >> >> Add checks to identify simple-bus bus types and checks for child >> >> devices. Simple-bus type is generally identified by "simple-bus" >> >> compatible string. We also treat the root as a simple-bus, but only for >> >> child nodes with reg property. >> >> >> >> Signed-off-by: Rob Herring <robh@kernel.org> >> >> --- >> >> v2: >> >> - new patch >> >> >> >> checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> >> 1 file changed, 69 insertions(+) >> >> >> >> diff --git a/checks.c b/checks.c >> >> index 5ed91ac50a10..c4865b4c8da0 100644 >> >> --- a/checks.c >> >> +++ b/checks.c >> >> @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no >> >> } >> >> WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); >> >> >> >> +static const struct bus_type simple_bus = { >> >> + .name = "simple-bus", >> >> +}; >> >> + >> >> +static bool node_is_compatible(struct node *node, const char *compat) >> >> +{ >> >> + struct property *prop; >> >> + const char *str; >> >> + >> >> + prop = get_property(node, "compatible"); >> >> + if (!prop) >> >> + return false; >> >> + >> >> + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { >> > >> > This isn't safe if the compatible property is filled with garbage (not >> > '\0' terminated) - the strlen() could access beyond the end of the >> > property value. >> >> Okay, I guess I can check that prop->val.val[prop->val.len - 1] == 0 up front. > > Sure. Or use strnlen. Duh... >> >> + if (streq(str, compat)) >> >> + return true; >> >> + } >> >> + return false; >> >> +} >> >> + >> >> +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) >> >> +{ >> >> + if (node_is_compatible(node, "simple-bus") || !node->parent) >> >> + node->bus = &simple_bus; >> > >> > I don't think it's correct to assume the root bus is always a >> > simple-bus. If it is, it really should be listed explicitly in the >> > root node's compatible property. >> >> It is in the sense that Linux treats the root the same and creates >> devices for top level children and then descends for nodes with >> "simple-bus". > > Hmm.. where in Linux is that? I think that's a bug, technically > speaking, traversing the root node's children without regard to the > type of the root node. drivers/of/platform.c:of_platform_populate() which is called on the root node with "simple-bus" in the match table. Plus I know we have some DT's like Tegra that didn't put all their devices under a bus (but should have). Maybe I should warn on that (i.e. warn on having unit-addresses without a bus type set on root). 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 Tue, Mar 07, 2017 at 07:51:15AM -0600, Rob Herring wrote: > On Mon, Mar 6, 2017 at 9:41 PM, David Gibson > <david@gibson.dropbear.id.au> wrote: > > On Mon, Mar 06, 2017 at 04:48:18AM -0600, Rob Herring wrote: > >> On Thu, Mar 2, 2017 at 8:12 PM, David Gibson > >> <david@gibson.dropbear.id.au> wrote: > >> > On Tue, Feb 28, 2017 at 04:43:09PM -0600, Rob Herring wrote: > >> >> Add checks to identify simple-bus bus types and checks for child > >> >> devices. Simple-bus type is generally identified by "simple-bus" > >> >> compatible string. We also treat the root as a simple-bus, but only for > >> >> child nodes with reg property. > >> >> > >> >> Signed-off-by: Rob Herring <robh@kernel.org> > >> >> --- > >> >> v2: > >> >> - new patch > >> >> > >> >> checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> >> 1 file changed, 69 insertions(+) > >> >> > >> >> diff --git a/checks.c b/checks.c > >> >> index 5ed91ac50a10..c4865b4c8da0 100644 > >> >> --- a/checks.c > >> >> +++ b/checks.c > >> >> @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no > >> >> } > >> >> WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); > >> >> > >> >> +static const struct bus_type simple_bus = { > >> >> + .name = "simple-bus", > >> >> +}; > >> >> + > >> >> +static bool node_is_compatible(struct node *node, const char *compat) > >> >> +{ > >> >> + struct property *prop; > >> >> + const char *str; > >> >> + > >> >> + prop = get_property(node, "compatible"); > >> >> + if (!prop) > >> >> + return false; > >> >> + > >> >> + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { > >> > > >> > This isn't safe if the compatible property is filled with garbage (not > >> > '\0' terminated) - the strlen() could access beyond the end of the > >> > property value. > >> > >> Okay, I guess I can check that prop->val.val[prop->val.len - 1] == 0 up front. > > > > Sure. Or use strnlen. > > Duh... > > >> >> + if (streq(str, compat)) > >> >> + return true; > >> >> + } > >> >> + return false; > >> >> +} > >> >> + > >> >> +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) > >> >> +{ > >> >> + if (node_is_compatible(node, "simple-bus") || !node->parent) > >> >> + node->bus = &simple_bus; > >> > > >> > I don't think it's correct to assume the root bus is always a > >> > simple-bus. If it is, it really should be listed explicitly in the > >> > root node's compatible property. > >> > >> It is in the sense that Linux treats the root the same and creates > >> devices for top level children and then descends for nodes with > >> "simple-bus". > > > > Hmm.. where in Linux is that? I think that's a bug, technically > > speaking, traversing the root node's children without regard to the > > type of the root node. > > drivers/of/platform.c:of_platform_populate() which is called on the > root node with "simple-bus" in the match table. It's called on the root node *by platform code*. And the platform is selected on properties of the root node, so it already knows something about what format the root node should have on that particular platform. dtc does not know that. > Plus I know we have some DT's like Tegra that didn't put all their > devices under a bus (but should have). Maybe I should warn on that > (i.e. warn on having unit-addresses without a bus type set on root). There's nothing inherently wrong with having devices on the root bus. Really platforms that want this should be putting something like: compatible = "vendor,myboard", "simple-bus"; in their root node. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
diff --git a/checks.c b/checks.c index 5ed91ac50a10..c4865b4c8da0 100644 --- a/checks.c +++ b/checks.c @@ -817,6 +817,72 @@ static void check_pci_device_reg(struct check *c, struct dt_info *dti, struct no } WARNING(pci_device_reg, check_pci_device_reg, NULL, ®_format); +static const struct bus_type simple_bus = { + .name = "simple-bus", +}; + +static bool node_is_compatible(struct node *node, const char *compat) +{ + struct property *prop; + const char *str; + + prop = get_property(node, "compatible"); + if (!prop) + return false; + + for (str = prop->val.val; str < prop->val.val + prop->val.len; str += strlen(str) + 1) { + if (streq(str, compat)) + return true; + } + return false; +} + +static void check_simple_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) +{ + if (node_is_compatible(node, "simple-bus") || !node->parent) + node->bus = &simple_bus; +} +WARNING(simple_bus_bridge, check_simple_bus_bridge, NULL, &addr_size_cells); + +static void check_simple_bus_reg(struct check *c, struct dt_info *dti, struct node *node) +{ + struct property *prop; + const char *unitname = get_unitname(node); + char unit_addr[17]; + unsigned int size; + uint64_t reg = 0; + cell_t *cells = NULL; + + if (!node->parent || (node->parent->bus != &simple_bus)) + return; + + prop = get_property(node, "reg"); + if (prop) + cells = (cell_t *)prop->val.val; + else { + prop = get_property(node, "ranges"); + if (prop && prop->val.len) + /* skip of child address */ + cells = ((cell_t *)prop->val.val) + node_addr_cells(node); + } + + if (!cells) { + if (node->parent->parent && !(node->bus == &simple_bus)) + FAIL(c, dti, "Node %s missing or empty reg/ranges property", node->fullpath); + return; + } + + size = node_addr_cells(node->parent); + while (size--) + reg = (reg << 32) | fdt32_to_cpu(*(cells++)); + + snprintf(unit_addr, sizeof(unit_addr), "%lx", reg); + if (!streq(unitname, unit_addr)) + FAIL(c, dti, "Node %s simple-bus unit address format error, expected \"%s\"", + node->fullpath, unit_addr); +} +WARNING(simple_bus_reg, check_simple_bus_reg, NULL, ®_format, &simple_bus_bridge); + /* * Style checks */ @@ -893,6 +959,9 @@ static struct check *check_table[] = { &pci_device_reg, &pci_device_bus_num, + &simple_bus_bridge, + &simple_bus_reg, + &avoid_default_addr_size, &obsolete_chosen_interrupt_controller,
Add checks to identify simple-bus bus types and checks for child devices. Simple-bus type is generally identified by "simple-bus" compatible string. We also treat the root as a simple-bus, but only for child nodes with reg property. Signed-off-by: Rob Herring <robh@kernel.org> --- v2: - new patch checks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) -- 2.10.1 -- 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