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

[Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

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.

>
>> +             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