[PATCH 1/2] checks: add phandle with arg property checks

[PATCH 1/2] checks: add phandle with arg property checks

[Rob Herring]

Many common bindings follow the same pattern of client properties
containing a phandle and N arg cells where N is defined in the provider
with a '#<specifier>-cells' property. Add a checks for properties
following this pattern.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)

diff --git a/checks.c b/checks.c
index afabf64337d5..c0450e118043 100644
--- a/checks.c
+++ b/checks.c
@@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
 WARNING(obsolete_chosen_interrupt_controller,
 	check_obsolete_chosen_interrupt_controller, NULL);
 
+struct provider {
+	const char *prop_name;
+	const char *cell_name;
+	bool optional;
+};
+
+static void check_property_phandle_args(struct check *c,
+					  struct dt_info *dti,
+				          struct node *node,
+				          struct property *prop,
+				          const struct provider *provider)
+{
+	struct marker *m = prop->val.markers;
+
+	if (!m) {
+		FAIL(c, dti, "Missing phandles in %s:%s",
+		     node->fullpath, prop->name);
+		return;
+	}
+	for_each_marker_of_type(m, REF_PHANDLE) {
+		int cellsize;
+		struct node *root = dti->dt;
+		struct node *provider_node;
+		struct property *cellprop;
+
+		provider_node = get_node_by_ref(root, m->ref);
+		if (!provider_node) {
+			FAIL(c, dti, "Could not get provider for %s:%s",
+			     node->fullpath, prop->name);
+			break;
+		}
+
+		cellprop = get_property(provider_node, provider->cell_name);
+		if (cellprop) {
+			cellsize = propval_cell(cellprop);
+		} else if (provider->optional) {
+			cellsize = 0;
+		} else {
+			FAIL(c, dti, "Missing %s in provider %s for %s",
+			     provider->cell_name,
+		     	     provider_node->fullpath,
+	     		     node->fullpath);
+			break;
+		}
+
+		if (prop->val.len < ((cellsize + 1) * sizeof(cell_t))) {
+			FAIL(c, dti, "%s property size (%d) too small for cell size %d in %s",
+			     prop->name, prop->val.len, cellsize, node->fullpath);
+		}
+	}
+}
+
+static const struct provider providers[] = {
+	{ "clocks", "#clock-cells" },
+	{ "phys", "#phy-cells" },
+	{ "interrupts-extended", "#interrupt-cells" },
+	{ "mboxes", "#mbox-cells" },
+	{ "pwms", "#pwm-cells" },
+	{ "dmas", "#dma-cells" },
+	{ "resets", "#reset-cells" },
+	{ "hwlocks", "#hwlock-cells" },
+	{ "power-domains", "#power-domain-cells" },
+	{ "io-channels", "#io-channel-cells" },
+	{ "iommus", "#iommu-cells" },
+	{ "mux-controls", "#mux-control-cells" },
+	{ "cooling-device", "#cooling-cells" },
+	{ "thermal-sensors", "#thermal-sensor-cells" },
+	{ "sound-dais", "#sound-dai-cells" },
+	{ "msi-parent", "#msi-cells", true },
+	{ NULL },
+};
+
+static void check_provider_cells_property(struct check *c,
+					  struct dt_info *dti,
+				          struct node *node)
+{
+	int i;
+
+	for (i = 0; providers[i].prop_name; i++) {
+		struct property *prop = get_property(node, providers[i].prop_name);
+		if (!prop)
+			continue;
+		check_property_phandle_args(c, dti, node, prop, &providers[i]);
+	}
+}
+WARNING(provider_cells_property, check_provider_cells_property, NULL);
+
+static void check_gpio_cells_property(struct check *c,
+					  struct dt_info *dti,
+				          struct node *node)
+{
+	struct property *prop;
+
+	for_each_property(node, prop) {
+		char *str;
+		struct provider provider;
+
+		/* Skip over false matches */
+		if (strstr(prop->name, "nr-gpio"))
+			continue;
+
+		str = strrchr(prop->name, '-');
+		if (!str || !(streq(str, "-gpio") || streq(str, "-gpios")))
+			continue;
+
+		provider.prop_name = prop->name;
+		provider.cell_name = "#gpio-cells";
+		provider.optional = false;
+		check_property_phandle_args(c, dti, node, prop, &provider);
+	}
+
+}
+WARNING(gpio_cells_property, check_gpio_cells_property, NULL);
+
 static struct check *check_table[] = {
 	&duplicate_node_names, &duplicate_property_names,
 	&node_name_chars, &node_name_format, &property_name_chars,
@@ -987,6 +1101,9 @@ static struct check *check_table[] = {
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
 
+	&provider_cells_property,
+	&gpio_cells_property,
+
 	&always_fail,
 };
 
-- 
2.11.0

[PATCH 2/2] checks: add interrupts property check

[Rob Herring]

Add a check for nodes with interrupts property that they have a valid
parent, the parent has #interrupt-cells property, and the size is a
valid multiple of #interrupt-cells.

This may not handle every possible case and doesn't deal with
translation thru interrupt-map properties, but should be enough for
modern dts files.

Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 checks.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/checks.c b/checks.c
index c0450e118043..0d452bf8e674 100644
--- a/checks.c
+++ b/checks.c
@@ -1070,6 +1070,63 @@ static void check_gpio_cells_property(struct check *c,
 }
 WARNING(gpio_cells_property, check_gpio_cells_property, NULL);
 
+static void check_interrupts_property(struct check *c,
+				      struct dt_info *dti,
+				      struct node *node)
+{
+	struct node *root = dti->dt;
+	struct node *irq_node = NULL, *parent = node;
+	struct property *irq_prop, *prop = NULL;
+	int irq_cells, phandle;
+
+	irq_prop = get_property(node, "interrupts");
+	if (!irq_prop)
+		return;
+
+	while (parent && !prop) {
+		if (parent != node) {
+			prop = get_property(parent, "interrupt-controller");
+			if (prop) {
+				irq_node = parent;
+				break;
+			}
+		}
+
+		prop = get_property(parent, "interrupt-parent");
+		if (prop) {
+			phandle = propval_cell(prop);
+			irq_node = get_node_by_phandle(root, phandle);
+			if (!irq_node) {
+				FAIL(c, dti, "Bad interrupt-parent phandle for %s",
+				     node->fullpath);
+				return;
+			}
+			break;
+		}
+
+		parent = parent->parent;
+	}
+
+	if (!irq_node) {
+		FAIL(c, dti, "Missing interrupt-parent for %s", node->fullpath);
+		return;
+	}
+
+	prop = get_property(irq_node, "#interrupt-cells");
+	if (!prop) {
+		FAIL(c, dti, "Missing #interrupt-cells in interrupt-parent %s",
+		     irq_node->fullpath);
+		return;
+	}
+
+	irq_cells = propval_cell(prop);
+	if (irq_prop->val.len % (irq_cells * sizeof(cell_t))) {
+		FAIL(c, dti, "interrupts size is (%d), expected multiple of %d in %s",
+		     irq_prop->val.len, (int)(irq_cells * sizeof(cell_t)), node->fullpath);
+	}
+}
+WARNING(interrupts_property, check_interrupts_property, &phandle_references);
+
 static struct check *check_table[] = {
 	&duplicate_node_names, &duplicate_property_names,
 	&node_name_chars, &node_name_format, &property_name_chars,
@@ -1103,6 +1160,7 @@ static struct check *check_table[] = {
 
 	&provider_cells_property,
 	&gpio_cells_property,
+	&interrupts_property,
 
 	&always_fail,
 };
-- 
2.11.0

Re: [PATCH 2/2] checks: add interrupts property check

[Rob Herring]

On Mon, Aug 14, 2017 at 4:48 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Add a check for nodes with interrupts property that they have a valid
> parent, the parent has #interrupt-cells property, and the size is a
> valid multiple of #interrupt-cells.
>
> This may not handle every possible case and doesn't deal with
> translation thru interrupt-map properties, but should be enough for
> modern dts files.
>
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>
> diff --git a/checks.c b/checks.c
> index c0450e118043..0d452bf8e674 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1070,6 +1070,63 @@ static void check_gpio_cells_property(struct check *c,
>  }
>  WARNING(gpio_cells_property, check_gpio_cells_property, NULL);
>
> +static void check_interrupts_property(struct check *c,
> +                                     struct dt_info *dti,
> +                                     struct node *node)
> +{
> +       struct node *root = dti->dt;
> +       struct node *irq_node = NULL, *parent = node;
> +       struct property *irq_prop, *prop = NULL;
> +       int irq_cells, phandle;
> +
> +       irq_prop = get_property(node, "interrupts");
> +       if (!irq_prop)
> +               return;
> +
> +       while (parent && !prop) {
> +               if (parent != node) {
> +                       prop = get_property(parent, "interrupt-controller");
> +                       if (prop) {
> +                               irq_node = parent;
> +                               break;
> +                       }

This also needs a similar interrupt-map check here which I somehow
dropped in rebasing.

> +               }
> +
> +               prop = get_property(parent, "interrupt-parent");
> +               if (prop) {
> +                       phandle = propval_cell(prop);
> +                       irq_node = get_node_by_phandle(root, phandle);
> +                       if (!irq_node) {
> +                               FAIL(c, dti, "Bad interrupt-parent phandle for %s",
> +                                    node->fullpath);
> +                               return;
> +                       }
> +                       break;
> +               }
> +
> +               parent = parent->parent;
> +       }
> +
> +       if (!irq_node) {
> +               FAIL(c, dti, "Missing interrupt-parent for %s", node->fullpath);
> +               return;
> +       }
> +
> +       prop = get_property(irq_node, "#interrupt-cells");
> +       if (!prop) {
> +               FAIL(c, dti, "Missing #interrupt-cells in interrupt-parent %s",
> +                    irq_node->fullpath);
> +               return;
> +       }
> +
> +       irq_cells = propval_cell(prop);
> +       if (irq_prop->val.len % (irq_cells * sizeof(cell_t))) {
> +               FAIL(c, dti, "interrupts size is (%d), expected multiple of %d in %s",
> +                    irq_prop->val.len, (int)(irq_cells * sizeof(cell_t)), node->fullpath);
> +       }
> +}
> +WARNING(interrupts_property, check_interrupts_property, &phandle_references);
> +
>  static struct check *check_table[] = {
>         &duplicate_node_names, &duplicate_property_names,
>         &node_name_chars, &node_name_format, &property_name_chars,
> @@ -1103,6 +1160,7 @@ static struct check *check_table[] = {
>
>         &provider_cells_property,
>         &gpio_cells_property,
> +       &interrupts_property,
>
>         &always_fail,
>  };
> --
> 2.11.0
>

Re: [PATCH 1/2] checks: add phandle with arg property checks

[David Gibson]

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

On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:

Yay! Someone actually implementing checks.

> Many common bindings follow the same pattern of client properties
> containing a phandle and N arg cells where N is defined in the provider
> with a '#<specifier>-cells' property. Add a checks for properties
> following this pattern.

I think this description would be easier to follow if you led with an example.

> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Looks pretty good, though I have some suggestions.

> ---
>  checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 117 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index afabf64337d5..c0450e118043 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
>  WARNING(obsolete_chosen_interrupt_controller,
>  	check_obsolete_chosen_interrupt_controller, NULL);
>  
> +struct provider {
> +	const char *prop_name;
> +	const char *cell_name;
> +	bool optional;
> +};
> +
> +static void check_property_phandle_args(struct check *c,
> +					  struct dt_info *dti,
> +				          struct node *node,
> +				          struct property *prop,
> +				          const struct provider *provider)
> +{
> +	struct marker *m = prop->val.markers;
> +
> +	if (!m) {
> +		FAIL(c, dti, "Missing phandles in %s:%s",
> +		     node->fullpath, prop->name);
> +		return;
> +	}
> +	for_each_marker_of_type(m, REF_PHANDLE) {

So going through the markers I think is not the best approach.
That'll work if the source contains a reference here, which it usually
will, but there are some circumstances where it could contain a "raw"
phandle value (the most obvious being when you're decompiling an
existing dtb).

But I don't think you really need to.  Instead you should be able to
read the actual value, look it up with get_node_by_phandle().  You can
make this check dependent on fixup_phandle_references to make sure
it's executed after the references are resolved.

That should also let you more strictly verify the property in the
client node, including checking for misaligned entries of extraneous bits.

> +		int cellsize;
> +		struct node *root = dti->dt;
> +		struct node *provider_node;
> +		struct property *cellprop;
> +
> +		provider_node = get_node_by_ref(root, m->ref);
> +		if (!provider_node) {
> +			FAIL(c, dti, "Could not get provider for %s:%s",
> +			     node->fullpath, prop->name);
> +			break;
> +		}

AFAIK the "provider" terminology isn't standardized somewhere.  It's a
reasonable term internally, but if can rephrase to it in the displayed
messages that would be a bonus.

> +		cellprop = get_property(provider_node, provider->cell_name);
> +		if (cellprop) {
> +			cellsize = propval_cell(cellprop);
> +		} else if (provider->optional) {
> +			cellsize = 0;
> +		} else {
> +			FAIL(c, dti, "Missing %s in provider %s for %s",
> +			     provider->cell_name,
> +		     	     provider_node->fullpath,
> +	     		     node->fullpath);
> +			break;
> +		}
> +
> +		if (prop->val.len < ((cellsize + 1) * sizeof(cell_t))) {
> +			FAIL(c, dti, "%s property size (%d) too small for cell size %d in %s",
> +			     prop->name, prop->val.len, cellsize, node->fullpath);
> +		}
> +	}
> +}
> +
> +static const struct provider providers[] = {
> +	{ "clocks", "#clock-cells" },
> +	{ "phys", "#phy-cells" },
> +	{ "interrupts-extended", "#interrupt-cells" },
> +	{ "mboxes", "#mbox-cells" },
> +	{ "pwms", "#pwm-cells" },
> +	{ "dmas", "#dma-cells" },
> +	{ "resets", "#reset-cells" },
> +	{ "hwlocks", "#hwlock-cells" },
> +	{ "power-domains", "#power-domain-cells" },
> +	{ "io-channels", "#io-channel-cells" },
> +	{ "iommus", "#iommu-cells" },
> +	{ "mux-controls", "#mux-control-cells" },
> +	{ "cooling-device", "#cooling-cells" },
> +	{ "thermal-sensors", "#thermal-sensor-cells" },
> +	{ "sound-dais", "#sound-dai-cells" },
> +	{ "msi-parent", "#msi-cells", true },
> +	{ NULL },
> +};

How hard would it be to use macros to make each of these providers
different check structures.  I think the output would be a bit more
useful if errors in different types of these things was reported as a
different check failure rather than one big generic one.

It does mean listing everything in the check_table which is a pain.  I
would really like to change things so that a single macro can both
declare the check and add it to the master list, but I haven't thought
of a portable way to do that so far.

If you do need the providers array, ARRAY_SIZE() is preferred over a
terminator NULL.

> +static void check_provider_cells_property(struct check *c,
> +					  struct dt_info *dti,
> +				          struct node *node)
> +{
> +	int i;
> +
> +	for (i = 0; providers[i].prop_name; i++) {
> +		struct property *prop = get_property(node, providers[i].prop_name);
> +		if (!prop)
> +			continue;
> +		check_property_phandle_args(c, dti, node, prop, &providers[i]);
> +	}
> +}
> +WARNING(provider_cells_property, check_provider_cells_property, NULL);
> +
> +static void check_gpio_cells_property(struct check *c,
> +					  struct dt_info *dti,
> +				          struct node *node)
> +{
> +	struct property *prop;
> +
> +	for_each_property(node, prop) {
> +		char *str;
> +		struct provider provider;
> +
> +		/* Skip over false matches */
> +		if (strstr(prop->name, "nr-gpio"))
> +			continue;
> +
> +		str = strrchr(prop->name, '-');
> +		if (!str || !(streq(str, "-gpio") || streq(str, "-gpios")))
> +			continue;
> +
> +		provider.prop_name = prop->name;
> +		provider.cell_name = "#gpio-cells";
> +		provider.optional = false;
> +		check_property_phandle_args(c, dti, node, prop, &provider);
> +	}
> +
> +}
> +WARNING(gpio_cells_property, check_gpio_cells_property, NULL);

Since the gpio stuff is a bit tweaker, it would be good to go into a
separate patch.  Then the commit message can explain what the logic
here is for (I'm having a little trouble following it).

>  static struct check *check_table[] = {
>  	&duplicate_node_names, &duplicate_property_names,
>  	&node_name_chars, &node_name_format, &property_name_chars,
> @@ -987,6 +1101,9 @@ static struct check *check_table[] = {
>  	&avoid_default_addr_size,
>  	&obsolete_chosen_interrupt_controller,
>  
> +	&provider_cells_property,
> +	&gpio_cells_property,
> +
>  	&always_fail,
>  };
>  

-- 
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: 833 bytes --]

Re: [PATCH 2/2] checks: add interrupts property check

[David Gibson]

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

On Mon, Aug 14, 2017 at 04:48:07PM -0500, Rob Herring wrote:
> Add a check for nodes with interrupts property that they have a valid
> parent, the parent has #interrupt-cells property, and the size is a
> valid multiple of #interrupt-cells.
> 
> This may not handle every possible case and doesn't deal with
> translation thru interrupt-map properties, but should be enough for
> modern dts files.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index c0450e118043..0d452bf8e674 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1070,6 +1070,63 @@ static void check_gpio_cells_property(struct check *c,
>  }
>  WARNING(gpio_cells_property, check_gpio_cells_property, NULL);
>  
> +static void check_interrupts_property(struct check *c,
> +				      struct dt_info *dti,
> +				      struct node *node)
> +{
> +	struct node *root = dti->dt;
> +	struct node *irq_node = NULL, *parent = node;
> +	struct property *irq_prop, *prop = NULL;
> +	int irq_cells, phandle;
> +
> +	irq_prop = get_property(node, "interrupts");
> +	if (!irq_prop)
> +		return;
> +
> +	while (parent && !prop) {
> +		if (parent != node) {

So, it's kind of academic, but is it actually disallowed for an
interrupt-controller node to itself have interrupts which are
implicityly routed to itself?

> +			prop = get_property(parent, "interrupt-controller");
> +			if (prop) {
> +				irq_node = parent;
> +				break;
> +			}
> +		}
> +
> +		prop = get_property(parent, "interrupt-parent");
> +		if (prop) {
> +			phandle = propval_cell(prop);
> +			irq_node = get_node_by_phandle(root, phandle);
> +			if (!irq_node) {
> +				FAIL(c, dti, "Bad interrupt-parent phandle for %s",
> +				     node->fullpath);
> +				return;
> +			}
> +			break;
> +		}

As noted you also need a check for interrupt-map.

> +
> +		parent = parent->parent;
> +	}
> +
> +	if (!irq_node) {
> +		FAIL(c, dti, "Missing interrupt-parent for %s", node->fullpath);
> +		return;
> +	}
> +
> +	prop = get_property(irq_node, "#interrupt-cells");
> +	if (!prop) {
> +		FAIL(c, dti, "Missing #interrupt-cells in interrupt-parent %s",
> +		     irq_node->fullpath);
> +		return;
> +	}
> +
> +	irq_cells = propval_cell(prop);

So this is unsafe if #interrupt-cells is misformatted in the interrupt
controller.  There's already a test for that (using
WARNING_IF_NOT_CELL), so you just need to make this check dependent on
that one.

> +	if (irq_prop->val.len % (irq_cells * sizeof(cell_t))) {
> +		FAIL(c, dti, "interrupts size is (%d), expected multiple of %d in %s",
> +		     irq_prop->val.len, (int)(irq_cells * sizeof(cell_t)), node->fullpath);
> +	}
> +}
> +WARNING(interrupts_property, check_interrupts_property, &phandle_references);
> +
>  static struct check *check_table[] = {
>  	&duplicate_node_names, &duplicate_property_names,
>  	&node_name_chars, &node_name_format, &property_name_chars,
> @@ -1103,6 +1160,7 @@ static struct check *check_table[] = {
>  
>  	&provider_cells_property,
>  	&gpio_cells_property,
> +	&interrupts_property,
>  
>  	&always_fail,
>  };

With both these patches testcases to make sure the checks actually
trip on a bad example would be good.


-- 
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: 833 bytes --]

Re: [PATCH 2/2] checks: add interrupts property check

[Rob Herring]

On Thu, Aug 17, 2017 at 11:43 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Mon, Aug 14, 2017 at 04:48:07PM -0500, Rob Herring wrote:
>> Add a check for nodes with interrupts property that they have a valid
>> parent, the parent has #interrupt-cells property, and the size is a
>> valid multiple of #interrupt-cells.
>>
>> This may not handle every possible case and doesn't deal with
>> translation thru interrupt-map properties, but should be enough for
>> modern dts files.
>>
>> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>>  checks.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 58 insertions(+)
>>
>> diff --git a/checks.c b/checks.c
>> index c0450e118043..0d452bf8e674 100644
>> --- a/checks.c
>> +++ b/checks.c
>> @@ -1070,6 +1070,63 @@ static void check_gpio_cells_property(struct check *c,
>>  }
>>  WARNING(gpio_cells_property, check_gpio_cells_property, NULL);
>>
>> +static void check_interrupts_property(struct check *c,
>> +                                   struct dt_info *dti,
>> +                                   struct node *node)
>> +{
>> +     struct node *root = dti->dt;
>> +     struct node *irq_node = NULL, *parent = node;
>> +     struct property *irq_prop, *prop = NULL;
>> +     int irq_cells, phandle;
>> +
>> +     irq_prop = get_property(node, "interrupts");
>> +     if (!irq_prop)
>> +             return;
>> +
>> +     while (parent && !prop) {
>> +             if (parent != node) {
>
> So, it's kind of academic, but is it actually disallowed for an
> interrupt-controller node to itself have interrupts which are
> implicityly routed to itself?

I think so, yes. Because otherwise how do you describe a chained
interrupt controller? "interrupts" in the chained controller's node
are the parent's interrupts. I guess you have to rely on the
compatible implying any interrupts connected to itself. It could work
with interrupts-extended though.

[...]

>> @@ -1103,6 +1160,7 @@ static struct check *check_table[] = {
>>
>>       &provider_cells_property,
>>       &gpio_cells_property,
>> +     &interrupts_property,
>>
>>       &always_fail,
>>  };
>
> With both these patches testcases to make sure the checks actually
> trip on a bad example would be good.

Yeah, I wanted to get some initial feedback first.

Rob

Re: [PATCH 1/2] checks: add phandle with arg property checks

[Rob Herring]

On Thu, Aug 17, 2017 at 11:35 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
>
> Yay! Someone actually implementing checks.

Not like it is my first. I'll celebrate when someone else does.


>> Many common bindings follow the same pattern of client properties
>> containing a phandle and N arg cells where N is defined in the provider
>> with a '#<specifier>-cells' property. Add a checks for properties
>> following this pattern.
>
> I think this description would be easier to follow if you led with an example.
>
>>
>> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> Looks pretty good, though I have some suggestions.
>
>> ---
>>  checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 117 insertions(+)
>>
>> diff --git a/checks.c b/checks.c
>> index afabf64337d5..c0450e118043 100644
>> --- a/checks.c
>> +++ b/checks.c
>> @@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
>>  WARNING(obsolete_chosen_interrupt_controller,
>>       check_obsolete_chosen_interrupt_controller, NULL);
>>
>> +struct provider {
>> +     const char *prop_name;
>> +     const char *cell_name;
>> +     bool optional;
>> +};
>> +
>> +static void check_property_phandle_args(struct check *c,
>> +                                       struct dt_info *dti,
>> +                                       struct node *node,
>> +                                       struct property *prop,
>> +                                       const struct provider *provider)
>> +{
>> +     struct marker *m = prop->val.markers;
>> +
>> +     if (!m) {
>> +             FAIL(c, dti, "Missing phandles in %s:%s",
>> +                  node->fullpath, prop->name);
>> +             return;
>> +     }
>> +     for_each_marker_of_type(m, REF_PHANDLE) {
>
> So going through the markers I think is not the best approach.
> That'll work if the source contains a reference here, which it usually
> will, but there are some circumstances where it could contain a "raw"
> phandle value (the most obvious being when you're decompiling an
> existing dtb).
>
> But I don't think you really need to.  Instead you should be able to
> read the actual value, look it up with get_node_by_phandle().  You can
> make this check dependent on fixup_phandle_references to make sure
> it's executed after the references are resolved.

That's how I implemented it initially...

> That should also let you more strictly verify the property in the
> client node, including checking for misaligned entries of extraneous bits.
>
>> +             int cellsize;
>> +             struct node *root = dti->dt;
>> +             struct node *provider_node;
>> +             struct property *cellprop;
>> +
>> +             provider_node = get_node_by_ref(root, m->ref);
>> +             if (!provider_node) {
>> +                     FAIL(c, dti, "Could not get provider for %s:%s",
>> +                          node->fullpath, prop->name);
>> +                     break;
>> +             }
>
> AFAIK the "provider" terminology isn't standardized somewhere.  It's a
> reasonable term internally, but if can rephrase to it in the displayed
> messages that would be a bonus.

The kernel uses that at least. I guess maybe "phandle node" will work.

>> +             cellprop = get_property(provider_node, provider->cell_name);
>> +             if (cellprop) {
>> +                     cellsize = propval_cell(cellprop);
>> +             } else if (provider->optional) {
>> +                     cellsize = 0;
>> +             } else {
>> +                     FAIL(c, dti, "Missing %s in provider %s for %s",
>> +                          provider->cell_name,
>> +                          provider_node->fullpath,
>> +                          node->fullpath);
>> +                     break;
>> +             }
>> +
>> +             if (prop->val.len < ((cellsize + 1) * sizeof(cell_t))) {
>> +                     FAIL(c, dti, "%s property size (%d) too small for cell size %d in %s",
>> +                          prop->name, prop->val.len, cellsize, node->fullpath);
>> +             }
>> +     }
>> +}
>> +
>> +static const struct provider providers[] = {
>> +     { "clocks", "#clock-cells" },
>> +     { "phys", "#phy-cells" },
>> +     { "interrupts-extended", "#interrupt-cells" },
>> +     { "mboxes", "#mbox-cells" },
>> +     { "pwms", "#pwm-cells" },
>> +     { "dmas", "#dma-cells" },
>> +     { "resets", "#reset-cells" },
>> +     { "hwlocks", "#hwlock-cells" },
>> +     { "power-domains", "#power-domain-cells" },
>> +     { "io-channels", "#io-channel-cells" },
>> +     { "iommus", "#iommu-cells" },
>> +     { "mux-controls", "#mux-control-cells" },
>> +     { "cooling-device", "#cooling-cells" },
>> +     { "thermal-sensors", "#thermal-sensor-cells" },
>> +     { "sound-dais", "#sound-dai-cells" },
>> +     { "msi-parent", "#msi-cells", true },
>> +     { NULL },
>> +};
>
> How hard would it be to use macros to make each of these providers
> different check structures.  I think the output would be a bit more
> useful if errors in different types of these things was reported as a
> different check failure rather than one big generic one.

Should be doable.

> It does mean listing everything in the check_table which is a pain.  I
> would really like to change things so that a single macro can both
> declare the check and add it to the master list, but I haven't thought
> of a portable way to do that so far.

This is done in the kernel frequently using linker sections. Each
check entry would get put into a specific section, then you just
iterate through the entries. Would that work? I could imagine that
linker magic may not be all the portable.

> If you do need the providers array, ARRAY_SIZE() is preferred over a
> terminator NULL.

Okay.

>> +static void check_provider_cells_property(struct check *c,
>> +                                       struct dt_info *dti,
>> +                                       struct node *node)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; providers[i].prop_name; i++) {
>> +             struct property *prop = get_property(node, providers[i].prop_name);
>> +             if (!prop)
>> +                     continue;
>> +             check_property_phandle_args(c, dti, node, prop, &providers[i]);
>> +     }
>> +}
>> +WARNING(provider_cells_property, check_provider_cells_property, NULL);
>> +
>> +static void check_gpio_cells_property(struct check *c,
>> +                                       struct dt_info *dti,
>> +                                       struct node *node)
>> +{
>> +     struct property *prop;
>> +
>> +     for_each_property(node, prop) {
>> +             char *str;
>> +             struct provider provider;
>> +
>> +             /* Skip over false matches */
>> +             if (strstr(prop->name, "nr-gpio"))
>> +                     continue;
>> +
>> +             str = strrchr(prop->name, '-');
>> +             if (!str || !(streq(str, "-gpio") || streq(str, "-gpios")))
>> +                     continue;
>> +
>> +             provider.prop_name = prop->name;
>> +             provider.cell_name = "#gpio-cells";
>> +             provider.optional = false;
>> +             check_property_phandle_args(c, dti, node, prop, &provider);
>> +     }
>> +
>> +}
>> +WARNING(gpio_cells_property, check_gpio_cells_property, NULL);
>
> Since the gpio stuff is a bit tweaker, it would be good to go into a
> separate patch.  Then the commit message can explain what the logic
> here is for (I'm having a little trouble following it).

Okay.

Rob

Re: [PATCH 1/2] checks: add phandle with arg property checks

[David Gibson]

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

On Fri, Aug 18, 2017 at 11:02:01AM -0500, Rob Herring wrote:
> On Thu, Aug 17, 2017 at 11:35 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
> >
> > Yay! Someone actually implementing checks.
> 
> Not like it is my first. I'll celebrate when someone else does.
> 
> 
> >> Many common bindings follow the same pattern of client properties
> >> containing a phandle and N arg cells where N is defined in the provider
> >> with a '#<specifier>-cells' property. Add a checks for properties
> >> following this pattern.
> >
> > I think this description would be easier to follow if you led with an example.
> >
> >>
> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >
> > Looks pretty good, though I have some suggestions.
> >
> >> ---
> >>  checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 117 insertions(+)
> >>
> >> diff --git a/checks.c b/checks.c
> >> index afabf64337d5..c0450e118043 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
> >>  WARNING(obsolete_chosen_interrupt_controller,
> >>       check_obsolete_chosen_interrupt_controller, NULL);
> >>
> >> +struct provider {
> >> +     const char *prop_name;
> >> +     const char *cell_name;
> >> +     bool optional;
> >> +};
> >> +
> >> +static void check_property_phandle_args(struct check *c,
> >> +                                       struct dt_info *dti,
> >> +                                       struct node *node,
> >> +                                       struct property *prop,
> >> +                                       const struct provider *provider)
> >> +{
> >> +     struct marker *m = prop->val.markers;
> >> +
> >> +     if (!m) {
> >> +             FAIL(c, dti, "Missing phandles in %s:%s",
> >> +                  node->fullpath, prop->name);
> >> +             return;
> >> +     }
> >> +     for_each_marker_of_type(m, REF_PHANDLE) {
> >
> > So going through the markers I think is not the best approach.
> > That'll work if the source contains a reference here, which it usually
> > will, but there are some circumstances where it could contain a "raw"
> > phandle value (the most obvious being when you're decompiling an
> > existing dtb).
> >
> > But I don't think you really need to.  Instead you should be able to
> > read the actual value, look it up with get_node_by_phandle().  You can
> > make this check dependent on fixup_phandle_references to make sure
> > it's executed after the references are resolved.
> 
> That's how I implemented it initially...

Ok.. why did you change?

> > That should also let you more strictly verify the property in the
> > client node, including checking for misaligned entries of extraneous bits.
> >
> >> +             int cellsize;
> >> +             struct node *root = dti->dt;
> >> +             struct node *provider_node;
> >> +             struct property *cellprop;
> >> +
> >> +             provider_node = get_node_by_ref(root, m->ref);
> >> +             if (!provider_node) {
> >> +                     FAIL(c, dti, "Could not get provider for %s:%s",
> >> +                          node->fullpath, prop->name);
> >> +                     break;
> >> +             }
> >
> > AFAIK the "provider" terminology isn't standardized somewhere.  It's a
> > reasonable term internally, but if can rephrase to it in the displayed
> > messages that would be a bonus.
> 
> The kernel uses that at least. I guess maybe "phandle node" will work.

Hm, if the kernel uses the term, that's probably enough, actually.

> >> +             cellprop = get_property(provider_node, provider->cell_name);
> >> +             if (cellprop) {
> >> +                     cellsize = propval_cell(cellprop);
> >> +             } else if (provider->optional) {
> >> +                     cellsize = 0;
> >> +             } else {
> >> +                     FAIL(c, dti, "Missing %s in provider %s for %s",
> >> +                          provider->cell_name,
> >> +                          provider_node->fullpath,
> >> +                          node->fullpath);
> >> +                     break;
> >> +             }
> >> +
> >> +             if (prop->val.len < ((cellsize + 1) * sizeof(cell_t))) {
> >> +                     FAIL(c, dti, "%s property size (%d) too small for cell size %d in %s",
> >> +                          prop->name, prop->val.len, cellsize, node->fullpath);
> >> +             }
> >> +     }
> >> +}
> >> +
> >> +static const struct provider providers[] = {
> >> +     { "clocks", "#clock-cells" },
> >> +     { "phys", "#phy-cells" },
> >> +     { "interrupts-extended", "#interrupt-cells" },
> >> +     { "mboxes", "#mbox-cells" },
> >> +     { "pwms", "#pwm-cells" },
> >> +     { "dmas", "#dma-cells" },
> >> +     { "resets", "#reset-cells" },
> >> +     { "hwlocks", "#hwlock-cells" },
> >> +     { "power-domains", "#power-domain-cells" },
> >> +     { "io-channels", "#io-channel-cells" },
> >> +     { "iommus", "#iommu-cells" },
> >> +     { "mux-controls", "#mux-control-cells" },
> >> +     { "cooling-device", "#cooling-cells" },
> >> +     { "thermal-sensors", "#thermal-sensor-cells" },
> >> +     { "sound-dais", "#sound-dai-cells" },
> >> +     { "msi-parent", "#msi-cells", true },
> >> +     { NULL },
> >> +};
> >
> > How hard would it be to use macros to make each of these providers
> > different check structures.  I think the output would be a bit more
> > useful if errors in different types of these things was reported as a
> > different check failure rather than one big generic one.
> 
> Should be doable.

Ok.  Not essential, but I think it would improve the output.

> > It does mean listing everything in the check_table which is a pain.  I
> > would really like to change things so that a single macro can both
> > declare the check and add it to the master list, but I haven't thought
> > of a portable way to do that so far.
> 
> This is done in the kernel frequently using linker sections. Each
> check entry would get put into a specific section, then you just
> iterate through the entries. Would that work? I could imagine that
> linker magic may not be all the portable.

Right.  Linker sections are the usual way to do this, but that
requires lower level knowledge of how the toolchain works than I
really want to put into dtc.  At heart dtc is really a very
straightforward standard C program, so I don't want to introduce
dependencies on a specific compiler.

> 
> > If you do need the providers array, ARRAY_SIZE() is preferred over a
> > terminator NULL.
> 
> Okay.
> 
> >> +static void check_provider_cells_property(struct check *c,
> >> +                                       struct dt_info *dti,
> >> +                                       struct node *node)
> >> +{
> >> +     int i;
> >> +
> >> +     for (i = 0; providers[i].prop_name; i++) {
> >> +             struct property *prop = get_property(node, providers[i].prop_name);
> >> +             if (!prop)
> >> +                     continue;
> >> +             check_property_phandle_args(c, dti, node, prop, &providers[i]);
> >> +     }
> >> +}
> >> +WARNING(provider_cells_property, check_provider_cells_property, NULL);
> >> +
> >> +static void check_gpio_cells_property(struct check *c,
> >> +                                       struct dt_info *dti,
> >> +                                       struct node *node)
> >> +{
> >> +     struct property *prop;
> >> +
> >> +     for_each_property(node, prop) {
> >> +             char *str;
> >> +             struct provider provider;
> >> +
> >> +             /* Skip over false matches */
> >> +             if (strstr(prop->name, "nr-gpio"))
> >> +                     continue;
> >> +
> >> +             str = strrchr(prop->name, '-');
> >> +             if (!str || !(streq(str, "-gpio") || streq(str, "-gpios")))
> >> +                     continue;
> >> +
> >> +             provider.prop_name = prop->name;
> >> +             provider.cell_name = "#gpio-cells";
> >> +             provider.optional = false;
> >> +             check_property_phandle_args(c, dti, node, prop, &provider);
> >> +     }
> >> +
> >> +}
> >> +WARNING(gpio_cells_property, check_gpio_cells_property, NULL);
> >
> > Since the gpio stuff is a bit tweaker, it would be good to go into a
> > separate patch.  Then the commit message can explain what the logic
> > here is for (I'm having a little trouble following it).
> 
> Okay.
> 
> 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: 833 bytes --]

Re: [PATCH 2/2] checks: add interrupts property check

[David Gibson]

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

On Fri, Aug 18, 2017 at 10:53:56AM -0500, Rob Herring wrote:
> On Thu, Aug 17, 2017 at 11:43 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Mon, Aug 14, 2017 at 04:48:07PM -0500, Rob Herring wrote:
> >> Add a check for nodes with interrupts property that they have a valid
> >> parent, the parent has #interrupt-cells property, and the size is a
> >> valid multiple of #interrupt-cells.
> >>
> >> This may not handle every possible case and doesn't deal with
> >> translation thru interrupt-map properties, but should be enough for
> >> modern dts files.
> >>
> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> ---
> >>  checks.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 58 insertions(+)
> >>
> >> diff --git a/checks.c b/checks.c
> >> index c0450e118043..0d452bf8e674 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -1070,6 +1070,63 @@ static void check_gpio_cells_property(struct check *c,
> >>  }
> >>  WARNING(gpio_cells_property, check_gpio_cells_property, NULL);
> >>
> >> +static void check_interrupts_property(struct check *c,
> >> +                                   struct dt_info *dti,
> >> +                                   struct node *node)
> >> +{
> >> +     struct node *root = dti->dt;
> >> +     struct node *irq_node = NULL, *parent = node;
> >> +     struct property *irq_prop, *prop = NULL;
> >> +     int irq_cells, phandle;
> >> +
> >> +     irq_prop = get_property(node, "interrupts");
> >> +     if (!irq_prop)
> >> +             return;
> >> +
> >> +     while (parent && !prop) {
> >> +             if (parent != node) {
> >
> > So, it's kind of academic, but is it actually disallowed for an
> > interrupt-controller node to itself have interrupts which are
> > implicityly routed to itself?
> 
> I think so, yes. Because otherwise how do you describe a chained
> interrupt controller? "interrupts" in the chained controller's node
> are the parent's interrupts. I guess you have to rely on the
> compatible implying any interrupts connected to itself. It could work
> with interrupts-extended though.

Ah.. yes.  My guess would be that you could do it by using an explicit
interrupt parent - which would mean that interrupt-parent would need
to take precedence over an interrupt controller in the same node.  I
guess we should probably check the old interrupt binding document.

Note that a possible, though strange case is for a node to have
interrupt-parent pointing at itself, with itself being an interrupt
nexus redirecting the interrupts elsewhere.  I've used this in the
past to handle a device with interrupts going to multiple interrupt
controllers before interrupts-extended was invented.

> 
> [...]
> 
> >> @@ -1103,6 +1160,7 @@ static struct check *check_table[] = {
> >>
> >>       &provider_cells_property,
> >>       &gpio_cells_property,
> >> +     &interrupts_property,
> >>
> >>       &always_fail,
> >>  };
> >
> > With both these patches testcases to make sure the checks actually
> > trip on a bad example would be good.
> 
> Yeah, I wanted to get some initial feedback first.

Fair enough.

-- 
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: 833 bytes --]

Re: [PATCH 1/2] checks: add phandle with arg property checks

[Rob Herring]

On Sat, Aug 19, 2017 at 2:33 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Aug 18, 2017 at 11:02:01AM -0500, Rob Herring wrote:
>> On Thu, Aug 17, 2017 at 11:35 PM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
>> >
>> > Yay! Someone actually implementing checks.
>>
>> Not like it is my first. I'll celebrate when someone else does.
>>
>>
>> >> Many common bindings follow the same pattern of client properties
>> >> containing a phandle and N arg cells where N is defined in the provider
>> >> with a '#<specifier>-cells' property. Add a checks for properties
>> >> following this pattern.
>> >
>> > I think this description would be easier to follow if you led with an example.
>> >
>> >>
>> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> >
>> > Looks pretty good, though I have some suggestions.
>> >
>> >> ---
>> >>  checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 117 insertions(+)
>> >>
>> >> diff --git a/checks.c b/checks.c
>> >> index afabf64337d5..c0450e118043 100644
>> >> --- a/checks.c
>> >> +++ b/checks.c
>> >> @@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
>> >>  WARNING(obsolete_chosen_interrupt_controller,
>> >>       check_obsolete_chosen_interrupt_controller, NULL);
>> >>
>> >> +struct provider {
>> >> +     const char *prop_name;
>> >> +     const char *cell_name;
>> >> +     bool optional;
>> >> +};
>> >> +
>> >> +static void check_property_phandle_args(struct check *c,
>> >> +                                       struct dt_info *dti,
>> >> +                                       struct node *node,
>> >> +                                       struct property *prop,
>> >> +                                       const struct provider *provider)
>> >> +{
>> >> +     struct marker *m = prop->val.markers;
>> >> +
>> >> +     if (!m) {
>> >> +             FAIL(c, dti, "Missing phandles in %s:%s",
>> >> +                  node->fullpath, prop->name);
>> >> +             return;
>> >> +     }
>> >> +     for_each_marker_of_type(m, REF_PHANDLE) {
>> >
>> > So going through the markers I think is not the best approach.
>> > That'll work if the source contains a reference here, which it usually
>> > will, but there are some circumstances where it could contain a "raw"
>> > phandle value (the most obvious being when you're decompiling an
>> > existing dtb).
>> >
>> > But I don't think you really need to.  Instead you should be able to
>> > read the actual value, look it up with get_node_by_phandle().  You can
>> > make this check dependent on fixup_phandle_references to make sure
>> > it's executed after the references are resolved.
>>
>> That's how I implemented it initially...
>
> Ok.. why did you change?

Just because the code is a bit more compact using markers. But
decompiling a dtb is a good reason I didn't think of.

Rob

Re: [PATCH 1/2] checks: add phandle with arg property checks

[David Gibson]

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

On Tue, Aug 22, 2017 at 09:38:59AM -0500, Rob Herring wrote:
> On Sat, Aug 19, 2017 at 2:33 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Aug 18, 2017 at 11:02:01AM -0500, Rob Herring wrote:
> >> On Thu, Aug 17, 2017 at 11:35 PM, David Gibson
> >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> > On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
> >> >
> >> > Yay! Someone actually implementing checks.
> >>
> >> Not like it is my first. I'll celebrate when someone else does.
> >>
> >>
> >> >> Many common bindings follow the same pattern of client properties
> >> >> containing a phandle and N arg cells where N is defined in the provider
> >> >> with a '#<specifier>-cells' property. Add a checks for properties
> >> >> following this pattern.
> >> >
> >> > I think this description would be easier to follow if you led with an example.
> >> >
> >> >>
> >> >> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> >
> >> > Looks pretty good, though I have some suggestions.
> >> >
> >> >> ---
> >> >>  checks.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >>  1 file changed, 117 insertions(+)
> >> >>
> >> >> diff --git a/checks.c b/checks.c
> >> >> index afabf64337d5..c0450e118043 100644
> >> >> --- a/checks.c
> >> >> +++ b/checks.c
> >> >> @@ -956,6 +956,120 @@ static void check_obsolete_chosen_interrupt_controller(struct check *c,
> >> >>  WARNING(obsolete_chosen_interrupt_controller,
> >> >>       check_obsolete_chosen_interrupt_controller, NULL);
> >> >>
> >> >> +struct provider {
> >> >> +     const char *prop_name;
> >> >> +     const char *cell_name;
> >> >> +     bool optional;
> >> >> +};
> >> >> +
> >> >> +static void check_property_phandle_args(struct check *c,
> >> >> +                                       struct dt_info *dti,
> >> >> +                                       struct node *node,
> >> >> +                                       struct property *prop,
> >> >> +                                       const struct provider *provider)
> >> >> +{
> >> >> +     struct marker *m = prop->val.markers;
> >> >> +
> >> >> +     if (!m) {
> >> >> +             FAIL(c, dti, "Missing phandles in %s:%s",
> >> >> +                  node->fullpath, prop->name);
> >> >> +             return;
> >> >> +     }
> >> >> +     for_each_marker_of_type(m, REF_PHANDLE) {
> >> >
> >> > So going through the markers I think is not the best approach.
> >> > That'll work if the source contains a reference here, which it usually
> >> > will, but there are some circumstances where it could contain a "raw"
> >> > phandle value (the most obvious being when you're decompiling an
> >> > existing dtb).
> >> >
> >> > But I don't think you really need to.  Instead you should be able to
> >> > read the actual value, look it up with get_node_by_phandle().  You can
> >> > make this check dependent on fixup_phandle_references to make sure
> >> > it's executed after the references are resolved.
> >>
> >> That's how I implemented it initially...
> >
> > Ok.. why did you change?
> 
> Just because the code is a bit more compact using markers. But
> decompiling a dtb is a good reason I didn't think of.

Ok.  Well, if you can revert to the first way, that should be good.

-- 
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: 833 bytes --]

Re: [PATCH 1/2] checks: add phandle with arg property checks

[Rob Herring]

On Sat, Aug 19, 2017 at 2:33 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Aug 18, 2017 at 11:02:01AM -0500, Rob Herring wrote:
>> On Thu, Aug 17, 2017 at 11:35 PM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
>> >

[...]

>> > It does mean listing everything in the check_table which is a pain.  I
>> > would really like to change things so that a single macro can both
>> > declare the check and add it to the master list, but I haven't thought
>> > of a portable way to do that so far.
>>
>> This is done in the kernel frequently using linker sections. Each
>> check entry would get put into a specific section, then you just
>> iterate through the entries. Would that work? I could imagine that
>> linker magic may not be all the portable.
>
> Right.  Linker sections are the usual way to do this, but that
> requires lower level knowledge of how the toolchain works than I
> really want to put into dtc.  At heart dtc is really a very
> straightforward standard C program, so I don't want to introduce
> dependencies on a specific compiler.

To follow up on this, what about using dlopen/dlsym and having
libraries with checks? That should be fairly portable. dtc could load
checks libs from some known or env path. Each checks library would
provide some known entry point we can call for it to register its
checks.

Rob

Re: [PATCH 1/2] checks: add phandle with arg property checks

[David Gibson]

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

On Tue, Nov 21, 2017 at 03:06:46PM -0600, Rob Herring wrote:
> On Sat, Aug 19, 2017 at 2:33 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Aug 18, 2017 at 11:02:01AM -0500, Rob Herring wrote:
> >> On Thu, Aug 17, 2017 at 11:35 PM, David Gibson
> >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> > On Mon, Aug 14, 2017 at 04:48:06PM -0500, Rob Herring wrote:
> >> >
> 
> [...]
> 
> >> > It does mean listing everything in the check_table which is a pain.  I
> >> > would really like to change things so that a single macro can both
> >> > declare the check and add it to the master list, but I haven't thought
> >> > of a portable way to do that so far.
> >>
> >> This is done in the kernel frequently using linker sections. Each
> >> check entry would get put into a specific section, then you just
> >> iterate through the entries. Would that work? I could imagine that
> >> linker magic may not be all the portable.
> >
> > Right.  Linker sections are the usual way to do this, but that
> > requires lower level knowledge of how the toolchain works than I
> > really want to put into dtc.  At heart dtc is really a very
> > straightforward standard C program, so I don't want to introduce
> > dependencies on a specific compiler.
> 
> To follow up on this, what about using dlopen/dlsym and having
> libraries with checks? That should be fairly portable. dtc could load
> checks libs from some known or env path. Each checks library would
> provide some known entry point we can call for it to register its
> checks.

I don't immediately see how this addresses the problem.  Either we'd
have to have every check in a separate library, which would be pretty
painful, or each library would need a list of checks, and we'd have
the same issue generating it as we do already.

-- 
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: 833 bytes --]