[PATCH 0/3] dtc: checks: Validate interrupt-map properties

[PATCH 0/3] dtc: checks: Validate interrupt-map properties

[Andre Przywara]

Compared to the RFC, this take addresses Rob's and David's comments.
I added an extra patch to check for an explicit #address-cells property
in all interrupt provider nodes. Also I tried to filter out unresolved
phandle references, so that it just gives up in case this is an overlay
DT. Changelog below.
===============

The interrupt-map properties can be quite tricky, as they are already
hard to read in their source form, and depend on at least two nodes.

The first patch adds a separate test for interrupt providers, to avoid
redundant warnings for missing properties later.
Patch 2/3 is the actual check routine, to verify some features
of the map. In particular it should be able to spot missing fields or
fields using the wrong number of cells. Most of the fields cannot easily
be verified, but at the least the phandle to the interrupt controller and
the total number of cells in the property should be valid.
It actually find problems in the Linux kernel tree's .dts files.

The third patch is more a proof of concept, not sure it should be merged
as is, since it deviates from the current output scheme: It adds more
output in case a map is considered broken, so the issue can be found
much easier.
I would be happy with it living on the list for now.

Please let me know what you think.

Cheers,
Andre

Changelog RFC .. v1:
- add extra test for #interrupt-cells and #address-cells
- report expected and actual interrupt-map property length
- drop (now) redundant warnings
- explicitly check for overlay DT
- consider nested interrupt-map properties

Andre Przywara (3):
  checks: Add interrupt provider test
  checks: Validate interrupt-map properties
  checks: interrupt-map: Dump entries on error

 checks.c                           | 192 ++++++++++++++++++++++++++++++++++++-
 tests/bad-interrupt-controller.dts |   7 ++
 tests/bad-interrupt-map.dts        |  21 ++++
 tests/run_tests.sh                 |   2 +
 4 files changed, 221 insertions(+), 1 deletion(-)
 create mode 100644 tests/bad-interrupt-controller.dts
 create mode 100644 tests/bad-interrupt-map.dts

-- 
2.14.1

[PATCH 1/3] checks: Add interrupt provider test

[Andre Przywara]

An interrupt provider (an actual interrupt-controller node or an
interrupt nexus) should have both #address-cells and #interrupt-cells
properties explicitly defined.

Add an extra test for this. We check for the #interrupt-cells property
already, but this does not cover every controller so far, only those that
get referenced by an interrupts property in some node. Also we miss
interrupt nexus nodes.

A missing #address-cells property is less critical, but creates
ambiguities when used in interrupt-map properties, so warn about this as
well now.
This removes the now redundant warning in the existing interrupts test.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 checks.c                           | 25 ++++++++++++++++++++++++-
 tests/bad-interrupt-controller.dts |  7 +++++++
 tests/run_tests.sh                 |  1 +
 3 files changed, 32 insertions(+), 1 deletion(-)
 create mode 100644 tests/bad-interrupt-controller.dts

diff --git a/checks.c b/checks.c
index 4b3c486..a8213c0 100644
--- a/checks.c
+++ b/checks.c
@@ -1547,6 +1547,28 @@ static bool node_is_interrupt_provider(struct node *node)
 
 	return false;
 }
+
+static void check_interrupt_provider(struct check *c,
+				     struct dt_info *dti,
+				     struct node *node)
+{
+	struct property *prop;
+
+	if (!node_is_interrupt_provider(node))
+		return;
+
+	prop = get_property(node, "#interrupt-cells");
+	if (!prop)
+		FAIL(c, dti, node,
+		     "Missing #interrupt-cells in interrupt provider");
+
+	prop = get_property(node, "#address-cells");
+	if (!prop)
+		FAIL(c, dti, node,
+		     "Missing #address-cells in interrupt provider");
+}
+WARNING(interrupt_provider, check_interrupt_provider, NULL);
+
 static void check_interrupts_property(struct check *c,
 				      struct dt_info *dti,
 				      struct node *node)
@@ -1604,7 +1626,7 @@ static void check_interrupts_property(struct check *c,
 
 	prop = get_property(irq_node, "#interrupt-cells");
 	if (!prop) {
-		FAIL(c, dti, irq_node, "Missing #interrupt-cells in interrupt-parent");
+		/* We warn about that already in another test. */
 		return;
 	}
 
@@ -1828,6 +1850,7 @@ static struct check *check_table[] = {
 	&deprecated_gpio_property,
 	&gpios_property,
 	&interrupts_property,
+	&interrupt_provider,
 
 	&alias_paths,
 
diff --git a/tests/bad-interrupt-controller.dts b/tests/bad-interrupt-controller.dts
new file mode 100644
index 0000000..62fa118
--- /dev/null
+++ b/tests/bad-interrupt-controller.dts
@@ -0,0 +1,7 @@
+/dts-v1/;
+
+/ {
+	intc: interrupt-controller {
+		interrupt-controller;
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index eccb85d..294585b 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -714,6 +714,7 @@ dtc_tests () {
     check_tests "$SRCDIR/bad-graph.dts" graph_endpoint
     run_sh_test "$SRCDIR/dtc-checkfails.sh" deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb "$SRCDIR/bad-gpio.dts"
     check_tests "$SRCDIR/bad-interrupt-cells.dts" interrupts_property
+    check_tests "$SRCDIR/bad-interrupt-controller.dts" interrupt_provider
     run_sh_test "$SRCDIR/dtc-checkfails.sh" node_name_chars -- -I dtb -O dtb bad_node_char.dtb
     run_sh_test "$SRCDIR/dtc-checkfails.sh" node_name_format -- -I dtb -O dtb bad_node_format.dtb
     run_sh_test "$SRCDIR/dtc-checkfails.sh" property_name_chars -- -I dtb -O dtb bad_prop_char.dtb
-- 
2.14.1

[PATCH 2/3] checks: Validate interrupt-map properties

[Andre Przywara]

The interrupt-map in an interrupt nexus is quite a tricky property: Each
entry contains five fields, the size of four of those depending on some
*-cells entries from two different nodes. This is even hard to validate
in a .dts file, especially when the associated interrupt controller is
described in a separate (included) file.

Add checks to validate those entries, by:
- Checking some basic properties of the interrupt nexus node.
- Checking that a map entry contains at least enough cells to point to
  the associated interrupt controller.
- Checking that the phandle points to an actual interrupt controller.
- Checking that there are enough entries to describe an interrupt in
  that interrupt controller's domain.

If each iteration passes and we exhaust exactly all the cells in the
interrupt-map property, the check passes.
Report errors on the way, and abort the check if that happens.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 checks.c                    | 88 +++++++++++++++++++++++++++++++++++++++++++++
 tests/bad-interrupt-map.dts | 21 +++++++++++
 tests/run_tests.sh          |  1 +
 3 files changed, 110 insertions(+)
 create mode 100644 tests/bad-interrupt-map.dts

diff --git a/checks.c b/checks.c
index a8213c0..d01df01 100644
--- a/checks.c
+++ b/checks.c
@@ -1639,6 +1639,92 @@ static void check_interrupts_property(struct check *c,
 }
 WARNING(interrupts_property, check_interrupts_property, &phandle_references);
 
+static void check_interrupt_map(struct check *c, struct dt_info *dti,
+				struct node *node)
+{
+	struct property *map = get_property(node, "interrupt-map");
+	struct property *prop;
+	int i, cells, irq_cells;
+
+	/* We are only interested in interrupt nexus nodes. */
+	if (!map)
+		return;
+
+	if (map->val.len % sizeof(cell_t)) {
+		FAIL_PROP(c, dti, node, map,
+			  "size (%d) is invalid, expected multiple of %zu",
+			  map->val.len, sizeof(cell_t));
+		return;
+	}
+	cells = map->val.len / sizeof(cell_t);
+
+	prop = get_property(node, "#interrupt-cells");
+	if (!prop) {
+		/* We warn about that already in another test. */
+		return;
+	}
+	irq_cells = propval_cell(prop);
+
+	for (i = 0; i < cells;) {
+		int phandle_idx = i + node_addr_cells(node) + irq_cells;
+		cell_t intc_phandle, intc_irq_cells, intc_addr_cells;
+		struct node *intc = NULL;
+
+		if (phandle_idx + 1 >= cells) {
+			FAIL_PROP(c, dti, node, map,
+				"insufficient cells for interrupt-map entry");
+			return;
+		}
+		intc_phandle = propval_cell_n(map, phandle_idx);
+		/* Give up if this is an overlay with external references */
+		if ((intc_phandle == 0 || intc_phandle == -1) &&
+		    dti->dtsflags & DTSF_PLUGIN)
+			break;
+		/* Avoid the assert in get_node_by_phandle() */
+		if (intc_phandle != 0)
+			intc = get_node_by_phandle(dti->dt, intc_phandle);
+		if (!intc) {
+			FAIL_PROP(c, dti, node, map,
+				  "invalid phandle for interrupt-map entry");
+			return;
+		}
+
+		if (!node_is_interrupt_provider(intc)) {
+			FAIL_PROP(c,dti, node, map,
+				  "interrupt-map phandle does not point to interrupt provider");
+			return;
+		}
+
+		prop = get_property(intc, "#address-cells");
+		if (!prop) {
+			/*
+			 * Linux treats non-existing #address-cells in the
+			 * interrupt parent as 0, and not 2, as the spec
+			 * suggests. Deal with that here.
+			 * We have a separate check for an explicit #a-c
+			 * in an interrupt provider that warns already.
+			 */
+			intc_addr_cells = 0;
+		} else
+			intc_addr_cells = propval_cell(prop);
+
+		prop = get_property(intc, "#interrupt-cells");
+		if (!prop) {
+			/* We warn about that already in another test. */
+			return;
+		}
+		intc_irq_cells = propval_cell(prop);
+
+		if (phandle_idx + intc_addr_cells + intc_irq_cells >= cells) {
+			FAIL_PROP(c, dti, node, map,
+				"insufficient cells for interrupt-map entry");
+			return;
+		}
+		i = phandle_idx + 1 + intc_addr_cells + intc_irq_cells;
+	}
+}
+WARNING(interrupt_map, check_interrupt_map, NULL);
+
 static const struct bus_type graph_port_bus = {
 	.name = "graph-port",
 };
@@ -1814,6 +1900,8 @@ static struct check *check_table[] = {
 	&pci_device_reg,
 	&pci_device_bus_num,
 
+	&interrupt_map,
+
 	&simple_bus_bridge,
 	&simple_bus_reg,
 
diff --git a/tests/bad-interrupt-map.dts b/tests/bad-interrupt-map.dts
new file mode 100644
index 0000000..cf9618f
--- /dev/null
+++ b/tests/bad-interrupt-map.dts
@@ -0,0 +1,21 @@
+/dts-v1/;
+
+/ {
+	intc: interrupt-controller {
+		interrupt-controller;
+		#address-cells = <2>;
+		#interrupt-cells = <3>;
+	};
+
+	nexus-node {
+		#address-cells = <1>;
+		#interrupt-cells = <1>;
+/*
+ * The cells after the phandle are the address in the interrupt controller's
+ * domain. This here encodes 0 cells , but the actual number is 2 above.
+ */
+		interrupt-map = <0 0 &intc 1 42 4>,
+				<0 1 &intc 1 43 4>,
+				<0 2 &intc 1 44 4>;
+	};
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 294585b..7c149d9 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -715,6 +715,7 @@ dtc_tests () {
     run_sh_test "$SRCDIR/dtc-checkfails.sh" deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb "$SRCDIR/bad-gpio.dts"
     check_tests "$SRCDIR/bad-interrupt-cells.dts" interrupts_property
     check_tests "$SRCDIR/bad-interrupt-controller.dts" interrupt_provider
+    check_tests "$SRCDIR/bad-interrupt-map.dts" interrupt_map
     run_sh_test "$SRCDIR/dtc-checkfails.sh" node_name_chars -- -I dtb -O dtb bad_node_char.dtb
     run_sh_test "$SRCDIR/dtc-checkfails.sh" node_name_format -- -I dtb -O dtb bad_node_format.dtb
     run_sh_test "$SRCDIR/dtc-checkfails.sh" property_name_chars -- -I dtb -O dtb bad_prop_char.dtb
-- 
2.14.1

[PATCH 3/3] RFC: checks: interrupt-map: Dump entries on error

[Andre Przywara]

While the interrupt-map property is now validated, the current error
messages will probably leave people scratching their head on what the
actual problem is.

To help with the diagnosis, dump the map property formatted in a way
that corresponds to the actual interpretation from an IRQ consumer's
point of view. While this can't pinpoint the actual problem, it's much
easier to see what went wrong, especially when comparing it to the .dts
source.

As this will generate one line per mapped interrupt, it can be
potentially long on those larger maps.
A report would look like this:
Warning (interrupt_map): /soc/pci@1c00000:interrupt-map: invalid phandle for interrupt-map entry
        < <0x0 0x0 0x0>, <0x1>, <&gic@17a00000>, <0x0>, <0x87 0x4 0x0> >,
        < <0x0 0x0 0x2>, <0x1>, <&invalid>, >

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 checks.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/checks.c b/checks.c
index d01df01..d814e8d 100644
--- a/checks.c
+++ b/checks.c
@@ -1639,6 +1639,80 @@ static void check_interrupts_property(struct check *c,
 }
 WARNING(interrupts_property, check_interrupts_property, &phandle_references);
 
+static void dump_cell_group(struct property *prop, int index, int cells,
+			    int nr_cells)
+{
+	int limit, i;
+
+	if (index + cells > nr_cells)
+		limit = nr_cells - index;
+	else
+		limit = cells;
+
+	fputs("<", stderr);
+	for (i = 0; i < limit; i++) {
+		cell_t v = propval_cell_n(prop, index + i);
+		fprintf(stderr, "%s0x%x", i == 0 ? "" : " ", v);
+	}
+	if (limit < cells)
+		fputs(", MISSING", stderr);
+	fputs(">", stderr);
+}
+
+static void dump_interrupt_map(struct node *root, struct node *nexus,
+			       struct property *map, int irq_cells)
+{
+	int cells = map->val.len / 4;
+	struct property *prop;
+	int value;
+	int i;
+
+	for (i = 0; i < cells;) {
+		struct node *intc = NULL;
+
+		fprintf(stderr, "%s\t< ", i == 0 ? "" : ",\n");
+		dump_cell_group(map, i, node_addr_cells(nexus), cells);
+		fputs(", ", stderr);
+		dump_cell_group(map, i + node_addr_cells(nexus), irq_cells, cells);
+		fputs(", <&", stderr);
+		i += node_addr_cells(nexus) + irq_cells;
+		if (i >= cells)
+			break;
+		value = propval_cell_n(map, i);
+		if (value != 0)
+			intc = get_node_by_phandle(root, value);
+		fprintf(stderr, "%s>, ",
+			intc && intc->name ? intc->name : "invalid");
+		if (!intc) {
+			fprintf(stderr, ">\n");
+			return;
+		}
+		/*
+		 * Linux treats non-existing #address-cells in the interrupt
+		 * parent as 0 (and not 2, as the spec says).
+		 * Deal with that, since many DTs rely on that.
+		 */
+		prop = get_property(intc, "#address-cells");
+		if (prop)
+			value = propval_cell(prop);
+		else
+			value = 0;
+		dump_cell_group(map, i + 1, value, cells);
+		fputs(", ", stderr);
+		i += 1 + value;
+
+		prop = get_property(intc, "#interrupt-cells");
+		if (prop) {
+			value = propval_cell(prop);
+			dump_cell_group(map, i, value, cells);
+		} else
+			fputs("<?>", stderr);
+		fputs(" >", stderr);
+		i += value;
+	}
+	fputs("\n", stderr);
+}
+
 static void check_interrupt_map(struct check *c, struct dt_info *dti,
 				struct node *node)
 {
@@ -1673,6 +1747,7 @@ static void check_interrupt_map(struct check *c, struct dt_info *dti,
 		if (phandle_idx + 1 >= cells) {
 			FAIL_PROP(c, dti, node, map,
 				"insufficient cells for interrupt-map entry");
+			dump_interrupt_map(dti->dt, node, map, irq_cells);
 			return;
 		}
 		intc_phandle = propval_cell_n(map, phandle_idx);
@@ -1686,12 +1761,14 @@ static void check_interrupt_map(struct check *c, struct dt_info *dti,
 		if (!intc) {
 			FAIL_PROP(c, dti, node, map,
 				  "invalid phandle for interrupt-map entry");
+			dump_interrupt_map(dti->dt, node, map, irq_cells);
 			return;
 		}
 
 		if (!node_is_interrupt_provider(intc)) {
 			FAIL_PROP(c,dti, node, map,
 				  "interrupt-map phandle does not point to interrupt provider");
+			dump_interrupt_map(dti->dt, node, map, irq_cells);
 			return;
 		}
 
@@ -1711,6 +1788,7 @@ static void check_interrupt_map(struct check *c, struct dt_info *dti,
 		prop = get_property(intc, "#interrupt-cells");
 		if (!prop) {
 			/* We warn about that already in another test. */
+			dump_interrupt_map(dti->dt, node, map, irq_cells);
 			return;
 		}
 		intc_irq_cells = propval_cell(prop);
@@ -1718,6 +1796,7 @@ static void check_interrupt_map(struct check *c, struct dt_info *dti,
 		if (phandle_idx + intc_addr_cells + intc_irq_cells >= cells) {
 			FAIL_PROP(c, dti, node, map,
 				"insufficient cells for interrupt-map entry");
+			dump_interrupt_map(dti->dt, node, map, irq_cells);
 			return;
 		}
 		i = phandle_idx + 1 + intc_addr_cells + intc_irq_cells;
-- 
2.14.1

Re: [PATCH 1/3] checks: Add interrupt provider test

[David Gibson]

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

On Fri, May 15, 2020 at 03:18:25PM +0100, Andre Przywara wrote:
> An interrupt provider (an actual interrupt-controller node or an
> interrupt nexus) should have both #address-cells and #interrupt-cells
> properties explicitly defined.
> 
> Add an extra test for this. We check for the #interrupt-cells property
> already, but this does not cover every controller so far, only those that
> get referenced by an interrupts property in some node. Also we miss
> interrupt nexus nodes.
> 
> A missing #address-cells property is less critical, but creates
> ambiguities when used in interrupt-map properties, so warn about this as
> well now.
> This removes the now redundant warning in the existing interrupts test.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  checks.c                           | 25 ++++++++++++++++++++++++-
>  tests/bad-interrupt-controller.dts |  7 +++++++
>  tests/run_tests.sh                 |  1 +
>  3 files changed, 32 insertions(+), 1 deletion(-)
>  create mode 100644 tests/bad-interrupt-controller.dts
> 
> diff --git a/checks.c b/checks.c
> index 4b3c486..a8213c0 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1547,6 +1547,28 @@ static bool node_is_interrupt_provider(struct node *node)
>  
>  	return false;
>  }
> +
> +static void check_interrupt_provider(struct check *c,
> +				     struct dt_info *dti,
> +				     struct node *node)
> +{
> +	struct property *prop;
> +
> +	if (!node_is_interrupt_provider(node))
> +		return;
> +
> +	prop = get_property(node, "#interrupt-cells");
> +	if (!prop)
> +		FAIL(c, dti, node,
> +		     "Missing #interrupt-cells in interrupt provider");
> +
> +	prop = get_property(node, "#address-cells");
> +	if (!prop)
> +		FAIL(c, dti, node,
> +		     "Missing #address-cells in interrupt provider");
> +}
> +WARNING(interrupt_provider, check_interrupt_provider, NULL);
> +
>  static void check_interrupts_property(struct check *c,
>  				      struct dt_info *dti,
>  				      struct node *node)
> @@ -1604,7 +1626,7 @@ static void check_interrupts_property(struct check *c,
>  
>  	prop = get_property(irq_node, "#interrupt-cells");
>  	if (!prop) {
> -		FAIL(c, dti, irq_node, "Missing #interrupt-cells in interrupt-parent");
> +		/* We warn about that already in another test. */
>  		return;
>  	}
>  
> @@ -1828,6 +1850,7 @@ static struct check *check_table[] = {
>  	&deprecated_gpio_property,
>  	&gpios_property,
>  	&interrupts_property,
> +	&interrupt_provider,
>  
>  	&alias_paths,
>  
> diff --git a/tests/bad-interrupt-controller.dts b/tests/bad-interrupt-controller.dts
> new file mode 100644
> index 0000000..62fa118
> --- /dev/null
> +++ b/tests/bad-interrupt-controller.dts
> @@ -0,0 +1,7 @@
> +/dts-v1/;
> +
> +/ {
> +	intc: interrupt-controller {
> +		interrupt-controller;
> +	};
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index eccb85d..294585b 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -714,6 +714,7 @@ dtc_tests () {
>      check_tests "$SRCDIR/bad-graph.dts" graph_endpoint
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb "$SRCDIR/bad-gpio.dts"
>      check_tests "$SRCDIR/bad-interrupt-cells.dts" interrupts_property
> +    check_tests "$SRCDIR/bad-interrupt-controller.dts" interrupt_provider
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" node_name_chars -- -I dtb -O dtb bad_node_char.dtb
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" node_name_format -- -I dtb -O dtb bad_node_format.dtb
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" property_name_chars -- -I dtb -O dtb bad_prop_char.dtb

-- 
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/3] checks: Validate interrupt-map properties

[David Gibson]

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

On Fri, May 15, 2020 at 03:18:26PM +0100, Andre Przywara wrote:
> The interrupt-map in an interrupt nexus is quite a tricky property: Each
> entry contains five fields, the size of four of those depending on some
> *-cells entries from two different nodes. This is even hard to validate
> in a .dts file, especially when the associated interrupt controller is
> described in a separate (included) file.
> 
> Add checks to validate those entries, by:
> - Checking some basic properties of the interrupt nexus node.
> - Checking that a map entry contains at least enough cells to point to
>   the associated interrupt controller.
> - Checking that the phandle points to an actual interrupt controller.
> - Checking that there are enough entries to describe an interrupt in
>   that interrupt controller's domain.
> 
> If each iteration passes and we exhaust exactly all the cells in the
> interrupt-map property, the check passes.
> Report errors on the way, and abort the check if that happens.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> ---
>  checks.c                    | 88 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/bad-interrupt-map.dts | 21 +++++++++++
>  tests/run_tests.sh          |  1 +
>  3 files changed, 110 insertions(+)
>  create mode 100644 tests/bad-interrupt-map.dts
> 
> diff --git a/checks.c b/checks.c
> index a8213c0..d01df01 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1639,6 +1639,92 @@ static void check_interrupts_property(struct check *c,
>  }
>  WARNING(interrupts_property, check_interrupts_property, &phandle_references);
>  
> +static void check_interrupt_map(struct check *c, struct dt_info *dti,
> +				struct node *node)
> +{
> +	struct property *map = get_property(node, "interrupt-map");
> +	struct property *prop;
> +	int i, cells, irq_cells;
> +
> +	/* We are only interested in interrupt nexus nodes. */
> +	if (!map)
> +		return;
> +
> +	if (map->val.len % sizeof(cell_t)) {
> +		FAIL_PROP(c, dti, node, map,
> +			  "size (%d) is invalid, expected multiple of %zu",
> +			  map->val.len, sizeof(cell_t));
> +		return;
> +	}
> +	cells = map->val.len / sizeof(cell_t);
> +
> +	prop = get_property(node, "#interrupt-cells");
> +	if (!prop) {
> +		/* We warn about that already in another test. */

It would probably be neater to make the other test a prerequisite of
this one, then you could just make this an assert().

> +		return;
> +	}
> +	irq_cells = propval_cell(prop);

Likewise you should make the test that #interrupt-cells has the
correct format a prereq, or this line could do bad things.

> +
> +	for (i = 0; i < cells;) {
> +		int phandle_idx = i + node_addr_cells(node) + irq_cells;
> +		cell_t intc_phandle, intc_irq_cells, intc_addr_cells;
> +		struct node *intc = NULL;
> +
> +		if (phandle_idx + 1 >= cells) {
> +			FAIL_PROP(c, dti, node, map,
> +				"insufficient cells for interrupt-map entry");
> +			return;
> +		}
> +		intc_phandle = propval_cell_n(map, phandle_idx);
> +		/* Give up if this is an overlay with external references */
> +		if ((intc_phandle == 0 || intc_phandle == -1) &&
> +		    dti->dtsflags & DTSF_PLUGIN)
> +			break;
> +		/* Avoid the assert in get_node_by_phandle() */
> +		if (intc_phandle != 0)
> +			intc = get_node_by_phandle(dti->dt, intc_phandle);
> +		if (!intc) {
> +			FAIL_PROP(c, dti, node, map,
> +				  "invalid phandle for interrupt-map entry");
> +			return;
> +		}
> +
> +		if (!node_is_interrupt_provider(intc)) {
> +			FAIL_PROP(c,dti, node, map,
> +				  "interrupt-map phandle does not point to interrupt provider");
> +			return;
> +		}
> +
> +		prop = get_property(intc, "#address-cells");
> +		if (!prop) {
> +			/*
> +			 * Linux treats non-existing #address-cells in the
> +			 * interrupt parent as 0, and not 2, as the spec
> +			 * suggests. Deal with that here.
> +			 * We have a separate check for an explicit #a-c
> +			 * in an interrupt provider that warns already.
> +			 */
> +			intc_addr_cells = 0;
> +		} else
> +			intc_addr_cells = propval_cell(prop);
> +
> +		prop = get_property(intc, "#interrupt-cells");
> +		if (!prop) {
> +			/* We warn about that already in another test. */
> +			return;
> +		}
> +		intc_irq_cells = propval_cell(prop);
> +
> +		if (phandle_idx + intc_addr_cells + intc_irq_cells >= cells) {
> +			FAIL_PROP(c, dti, node, map,
> +				"insufficient cells for interrupt-map entry");
> +			return;
> +		}
> +		i = phandle_idx + 1 + intc_addr_cells + intc_irq_cells;
> +	}
> +}
> +WARNING(interrupt_map, check_interrupt_map, NULL);

After that NULL you can list prerequisite checks for this one.  I
think you want &interrupt_provider, &address_cells_is_cell,
&size_cells_is_cell and &interrupt_cells_is_cell.

>  static const struct bus_type graph_port_bus = {
>  	.name = "graph-port",
>  };
> @@ -1814,6 +1900,8 @@ static struct check *check_table[] = {
>  	&pci_device_reg,
>  	&pci_device_bus_num,
>  
> +	&interrupt_map,
> +
>  	&simple_bus_bridge,
>  	&simple_bus_reg,
>  
> diff --git a/tests/bad-interrupt-map.dts b/tests/bad-interrupt-map.dts
> new file mode 100644
> index 0000000..cf9618f
> --- /dev/null
> +++ b/tests/bad-interrupt-map.dts
> @@ -0,0 +1,21 @@
> +/dts-v1/;
> +
> +/ {
> +	intc: interrupt-controller {
> +		interrupt-controller;
> +		#address-cells = <2>;
> +		#interrupt-cells = <3>;
> +	};
> +
> +	nexus-node {
> +		#address-cells = <1>;
> +		#interrupt-cells = <1>;
> +/*
> + * The cells after the phandle are the address in the interrupt controller's
> + * domain. This here encodes 0 cells , but the actual number is 2 above.
> + */
> +		interrupt-map = <0 0 &intc 1 42 4>,
> +				<0 1 &intc 1 43 4>,
> +				<0 2 &intc 1 44 4>;
> +	};
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 294585b..7c149d9 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -715,6 +715,7 @@ dtc_tests () {
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" deprecated_gpio_property -- -Wdeprecated_gpio_property -I dts -O dtb "$SRCDIR/bad-gpio.dts"
>      check_tests "$SRCDIR/bad-interrupt-cells.dts" interrupts_property
>      check_tests "$SRCDIR/bad-interrupt-controller.dts" interrupt_provider
> +    check_tests "$SRCDIR/bad-interrupt-map.dts" interrupt_map
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" node_name_chars -- -I dtb -O dtb bad_node_char.dtb
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" node_name_format -- -I dtb -O dtb bad_node_format.dtb
>      run_sh_test "$SRCDIR/dtc-checkfails.sh" property_name_chars -- -I dtb -O dtb bad_prop_char.dtb

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