Re: [PATCH v3 2/3] checks: Add bus checks for simple-bus buses

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3138 bytes --]

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-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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, &reg_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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]