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

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

[Andre Przywara]

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

While the first patch is quite clean, I am not so confident about the
second one, hence the RFC tags. It adds more output in case a map is
considered broken, so the issue can be found much easier. But it will
increase the amount of warnings, beyond the usual one-problem-per-line,
and lacks the usual prefixes.

Please let me know what you think.

Cheers,
Andre

Andre Przywara (2):
  checks: Validate interrupt-map properties
  checks: interrupt-map: Dump entries on error

 checks.c                    | 165 ++++++++++++++++++++++++++++++++++++++++++++
 tests/bad-interrupt-map.dts |  21 ++++++
 tests/run_tests.sh          |   2 +
 3 files changed, 188 insertions(+)
 create mode 100644 tests/bad-interrupt-map.dts

-- 
2.14.1

[RFC PATCH 1/2] 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                    | 86 +++++++++++++++++++++++++++++++++++++++++++++
 tests/bad-interrupt-map.dts | 21 +++++++++++
 tests/run_tests.sh          |  2 ++
 3 files changed, 109 insertions(+)
 create mode 100644 tests/bad-interrupt-map.dts

diff --git a/checks.c b/checks.c
index 4b3c486..12518db 100644
--- a/checks.c
+++ b/checks.c
@@ -924,6 +924,90 @@ 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, &pci_bridge);
 
+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, "invalid length of interrupt-map");
+		return;
+	}
+	cells = map->val.len / sizeof(cell_t);
+
+	prop = get_property(node, "#interrupt-cells");
+	if (!prop) {
+		FAIL(c, dti, node, "missing #interrupt-cells in nexus\n");
+		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);
+		/* 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;
+		}
+
+		prop = get_property(intc, "interrupt-controller");
+		if (!prop) {
+			FAIL_PROP(c,dti, node, map,
+				  "interrupt-map phandle does not point to interrupt controller");
+			return;
+		}
+
+		prop = get_property(intc, "#address-cells");
+		if (!prop) {
+			FAIL_PROP(c,dti, node, map,
+				  "interrupt-controller misses #address-cells property");
+			/*
+			 * Linux treats non-existing #address-cells in the
+			 * interrupt parent as 0, and not 2, as the spec
+			 * suggests. Deal with that, but print the warning,
+			 * since we should have an explicit #a-c = 0 in the
+			 * controller node in this case.
+			 */
+			intc_addr_cells = 0;
+		} else
+			intc_addr_cells = propval_cell(prop);
+
+		prop = get_property(intc, "#interrupt-cells");
+		if (!prop) {
+			FAIL_PROP(c,dti, node, map,
+				  "interrupt-controller misses #interrupt-cells property");
+			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 simple_bus = {
 	.name = "simple-bus",
 };
@@ -1792,6 +1876,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 eccb85d..aec92fb 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -732,6 +732,8 @@ dtc_tests () {
     check_tests "$SRCDIR/pci-bridge-bad1.dts" pci_bridge
     check_tests "$SRCDIR/pci-bridge-bad2.dts" pci_bridge
 
+    check_tests "$SRCDIR/bad-interrupt-map.dts" interrupt_map
+
     check_tests "$SRCDIR/unit-addr-simple-bus-reg-mismatch.dts" simple_bus_reg
     check_tests "$SRCDIR/unit-addr-simple-bus-compatible.dts" simple_bus_reg
 
-- 
2.14.1

[RFC PATCH 2/2] 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 12518db..5a63c20 100644
--- a/checks.c
+++ b/checks.c
@@ -924,6 +924,80 @@ 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, &pci_bridge);
 
+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)
 {
@@ -956,6 +1030,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);
@@ -965,6 +1040,7 @@ 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;
 		}
 
@@ -972,6 +1048,7 @@ static void check_interrupt_map(struct check *c, struct dt_info *dti,
 		if (!prop) {
 			FAIL_PROP(c,dti, node, map,
 				  "interrupt-map phandle does not point to interrupt controller");
+			dump_interrupt_map(dti->dt, node, map, irq_cells);
 			return;
 		}
 
@@ -994,6 +1071,7 @@ static void check_interrupt_map(struct check *c, struct dt_info *dti,
 		if (!prop) {
 			FAIL_PROP(c,dti, node, map,
 				  "interrupt-controller misses #interrupt-cells property");
+			dump_interrupt_map(dti->dt, node, map, irq_cells);
 			return;
 		}
 		intc_irq_cells = propval_cell(prop);
@@ -1001,6 +1079,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: [RFC PATCH 1/2] checks: Validate interrupt-map properties

[Rob Herring]

On Wed, May 13, 2020 at 11:35 AM Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> 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                    | 86 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/bad-interrupt-map.dts | 21 +++++++++++
>  tests/run_tests.sh          |  2 ++
>  3 files changed, 109 insertions(+)
>  create mode 100644 tests/bad-interrupt-map.dts
>
> diff --git a/checks.c b/checks.c
> index 4b3c486..12518db 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -924,6 +924,90 @@ 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, &pci_bridge);
>
> +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, "invalid length of interrupt-map");

It's good to say what size you found and what was expected.

> +               return;
> +       }
> +       cells = map->val.len / sizeof(cell_t);
> +
> +       prop = get_property(node, "#interrupt-cells");
> +       if (!prop) {
> +               FAIL(c, dti, node, "missing #interrupt-cells in nexus\n");
> +               return;
> +       }
> +       irq_cells = propval_cell(prop);
> +
> +       for (i = 0; i < cells;) {
> +               int phandle_idx = i + node_addr_cells(node) + irq_cells;

IIRC, node_addr_cells() will give you a default if not found which is
not really what you want.

> +               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);
> +               /* 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;
> +               }
> +
> +               prop = get_property(intc, "interrupt-controller");
> +               if (!prop) {
> +                       FAIL_PROP(c,dti, node, map,
> +                                 "interrupt-map phandle does not point to interrupt controller");

interrupt-map can point to another interrupt-map.

> +                       return;
> +               }
> +
> +               prop = get_property(intc, "#address-cells");
> +               if (!prop) {
> +                       FAIL_PROP(c,dti, node, map,
> +                                 "interrupt-controller misses #address-cells property");
> +                       /*
> +                        * Linux treats non-existing #address-cells in the
> +                        * interrupt parent as 0, and not 2, as the spec
> +                        * suggests. Deal with that, but print the warning,
> +                        * since we should have an explicit #a-c = 0 in the
> +                        * controller node in this case.

IMO, we should not print a warning. Or make it separately enabled.

> +                        */
> +                       intc_addr_cells = 0;
> +               } else
> +                       intc_addr_cells = propval_cell(prop);
> +
> +               prop = get_property(intc, "#interrupt-cells");
> +               if (!prop) {
> +                       FAIL_PROP(c,dti, node, map,
> +                                 "interrupt-controller misses #interrupt-cells property");
> +                       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 simple_bus = {
>         .name = "simple-bus",
>  };
> @@ -1792,6 +1876,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 eccb85d..aec92fb 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -732,6 +732,8 @@ dtc_tests () {
>      check_tests "$SRCDIR/pci-bridge-bad1.dts" pci_bridge
>      check_tests "$SRCDIR/pci-bridge-bad2.dts" pci_bridge
>
> +    check_tests "$SRCDIR/bad-interrupt-map.dts" interrupt_map
> +
>      check_tests "$SRCDIR/unit-addr-simple-bus-reg-mismatch.dts" simple_bus_reg
>      check_tests "$SRCDIR/unit-addr-simple-bus-compatible.dts" simple_bus_reg
>
> --
> 2.14.1
>

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

[Rob Herring]

On Wed, May 13, 2020 at 11:35 AM Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
>
> 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 12518db..5a63c20 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -924,6 +924,80 @@ 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, &pci_bridge);
>
> +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);

Printing like this doesn't work well when you have parallel calls to
dtc. You have to print everything to a buffer and then write the
buffer to stderr in one call.

> +}
> +
> +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)
>  {
> @@ -956,6 +1030,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);
> @@ -965,6 +1040,7 @@ 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;
>                 }
>
> @@ -972,6 +1048,7 @@ static void check_interrupt_map(struct check *c, struct dt_info *dti,
>                 if (!prop) {
>                         FAIL_PROP(c,dti, node, map,
>                                   "interrupt-map phandle does not point to interrupt controller");
> +                       dump_interrupt_map(dti->dt, node, map, irq_cells);
>                         return;
>                 }
>
> @@ -994,6 +1071,7 @@ static void check_interrupt_map(struct check *c, struct dt_info *dti,
>                 if (!prop) {
>                         FAIL_PROP(c,dti, node, map,
>                                   "interrupt-controller misses #interrupt-cells property");
> +                       dump_interrupt_map(dti->dt, node, map, irq_cells);
>                         return;
>                 }
>                 intc_irq_cells = propval_cell(prop);
> @@ -1001,6 +1079,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: [RFC PATCH 1/2] checks: Validate interrupt-map properties

[David Gibson]

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

On Wed, May 13, 2020 at 10:03:14PM -0500, Rob Herring wrote:
> On Wed, May 13, 2020 at 11:35 AM Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> 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                    | 86 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/bad-interrupt-map.dts | 21 +++++++++++
> >  tests/run_tests.sh          |  2 ++
> >  3 files changed, 109 insertions(+)
> >  create mode 100644 tests/bad-interrupt-map.dts
> >
> > diff --git a/checks.c b/checks.c
> > index 4b3c486..12518db 100644
> > --- a/checks.c
> > +++ b/checks.c
> > @@ -924,6 +924,90 @@ 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, &pci_bridge);
> >
> > +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, "invalid length of interrupt-map");
> 
> It's good to say what size you found and what was expected.
> 
> > +               return;
> > +       }
> > +       cells = map->val.len / sizeof(cell_t);
> > +
> > +       prop = get_property(node, "#interrupt-cells");
> > +       if (!prop) {
> > +               FAIL(c, dti, node, "missing #interrupt-cells in nexus\n");
> > +               return;
> > +       }
> > +       irq_cells = propval_cell(prop);
> > +
> > +       for (i = 0; i < cells;) {
> > +               int phandle_idx = i + node_addr_cells(node) + irq_cells;
> 
> IIRC, node_addr_cells() will give you a default if not found which is
> not really what you want.

Using the default seems right to me.  We might want a warning in that
case, but I don't think it belongs in this test.

> > +               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);
> > +               /* Avoid the assert in get_node_by_phandle(). */
> > +               if (intc_phandle != 0)
> > +                       intc = get_node_by_phandle(dti->dt, intc_phandle);

This will always fail the check if the phandle is an unresolved
reference, which is likely for an overlay dt.

> > +               if (!intc) {
> > +                       FAIL_PROP(c, dti, node, map,
> > +                                 "invalid phandle for interrupt-map entry");
> > +                       return;
> > +               }
> > +
> > +               prop = get_property(intc, "interrupt-controller");
> > +               if (!prop) {
> > +                       FAIL_PROP(c,dti, node, map,
> > +                                 "interrupt-map phandle does not point to interrupt controller");
> 
> interrupt-map can point to another interrupt-map.

Right.

> > +                       return;
> > +               }
> > +
> > +               prop = get_property(intc, "#address-cells");
> > +               if (!prop) {
> > +                       FAIL_PROP(c,dti, node, map,
> > +                                 "interrupt-controller misses #address-cells property");

"is missing", or simply "missing" would be more normal english than
"misses" (here and elsewhere).

> > +                       /*
> > +                        * Linux treats non-existing #address-cells in the
> > +                        * interrupt parent as 0, and not 2, as the spec
> > +                        * suggests. Deal with that, but print the warning,
> > +                        * since we should have an explicit #a-c = 0 in the
> > +                        * controller node in this case.
> 
> IMO, we should not print a warning. Or make it separately enabled.

I tend to agree.  A separate check to warn for an interrupt controller
(or nexus) without #address-cells seems like a good idea.

> > +                        */
> > +                       intc_addr_cells = 0;
> > +               } else
> > +                       intc_addr_cells = propval_cell(prop);
> > +
> > +               prop = get_property(intc, "#interrupt-cells");
> > +               if (!prop) {
> > +                       FAIL_PROP(c,dti, node, map,
> > +                                 "interrupt-controller misses #interrupt-cells property");
> > +                       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 simple_bus = {
> >         .name = "simple-bus",
> >  };
> > @@ -1792,6 +1876,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 eccb85d..aec92fb 100755
> > --- a/tests/run_tests.sh
> > +++ b/tests/run_tests.sh
> > @@ -732,6 +732,8 @@ dtc_tests () {
> >      check_tests "$SRCDIR/pci-bridge-bad1.dts" pci_bridge
> >      check_tests "$SRCDIR/pci-bridge-bad2.dts" pci_bridge
> >
> > +    check_tests "$SRCDIR/bad-interrupt-map.dts" interrupt_map
> > +
> >      check_tests "$SRCDIR/unit-addr-simple-bus-reg-mismatch.dts" simple_bus_reg
> >      check_tests "$SRCDIR/unit-addr-simple-bus-compatible.dts" simple_bus_reg
> >
> >
> 

-- 
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: [RFC PATCH 1/2] checks: Validate interrupt-map properties

[André Przywara]

On 14/05/2020 04:03, Rob Herring wrote:

Hi Rob,

thanks for having a look.

> On Wed, May 13, 2020 at 11:35 AM Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> 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                    | 86 +++++++++++++++++++++++++++++++++++++++++++++
>>  tests/bad-interrupt-map.dts | 21 +++++++++++
>>  tests/run_tests.sh          |  2 ++
>>  3 files changed, 109 insertions(+)
>>  create mode 100644 tests/bad-interrupt-map.dts
>>
>> diff --git a/checks.c b/checks.c
>> index 4b3c486..12518db 100644
>> --- a/checks.c
>> +++ b/checks.c
>> @@ -924,6 +924,90 @@ 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, &pci_bridge);
>>
>> +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, "invalid length of interrupt-map");
> 
> It's good to say what size you found and what was expected.

OK.

>> +               return;
>> +       }
>> +       cells = map->val.len / sizeof(cell_t);
>> +
>> +       prop = get_property(node, "#interrupt-cells");
>> +       if (!prop) {
>> +               FAIL(c, dti, node, "missing #interrupt-cells in nexus\n");
>> +               return;
>> +       }
>> +       irq_cells = propval_cell(prop);
>> +
>> +       for (i = 0; i < cells;) {
>> +               int phandle_idx = i + node_addr_cells(node) + irq_cells;
> 
> IIRC, node_addr_cells() will give you a default if not found which is
> not really what you want.

Can you elaborate why not? I think this is exactly what I want - after
all we seem to have this function for exactly this purpose, don't we?

Or do you want to emit a warning if the there is no explicit #a-c?

> 
>> +               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);
>> +               /* 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;
>> +               }
>> +
>> +               prop = get_property(intc, "interrupt-controller");
>> +               if (!prop) {
>> +                       FAIL_PROP(c,dti, node, map,
>> +                                 "interrupt-map phandle does not point to interrupt controller");
> 
> interrupt-map can point to another interrupt-map.

True, good point. And we even have node_is_interrupt_provider() for that.

>> +                       return;
>> +               }
>> +
>> +               prop = get_property(intc, "#address-cells");
>> +               if (!prop) {
>> +                       FAIL_PROP(c,dti, node, map,
>> +                                 "interrupt-controller misses #address-cells property");
>> +                       /*
>> +                        * Linux treats non-existing #address-cells in the
>> +                        * interrupt parent as 0, and not 2, as the spec
>> +                        * suggests. Deal with that, but print the warning,
>> +                        * since we should have an explicit #a-c = 0 in the
>> +                        * controller node in this case.
> 
> IMO, we should not print a warning. Or make it separately enabled.

OK, I will make it silent here, and introduce (or amend) another test to
check and warn about missing #a-c in interrupt-controller nodes.

Cheers,
Andre

> 
>> +                        */
>> +                       intc_addr_cells = 0;
>> +               } else
>> +                       intc_addr_cells = propval_cell(prop);
>> +
>> +               prop = get_property(intc, "#interrupt-cells");
>> +               if (!prop) {
>> +                       FAIL_PROP(c,dti, node, map,
>> +                                 "interrupt-controller misses #interrupt-cells property");
>> +                       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 simple_bus = {
>>         .name = "simple-bus",
>>  };
>> @@ -1792,6 +1876,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 eccb85d..aec92fb 100755
>> --- a/tests/run_tests.sh
>> +++ b/tests/run_tests.sh
>> @@ -732,6 +732,8 @@ dtc_tests () {
>>      check_tests "$SRCDIR/pci-bridge-bad1.dts" pci_bridge
>>      check_tests "$SRCDIR/pci-bridge-bad2.dts" pci_bridge
>>
>> +    check_tests "$SRCDIR/bad-interrupt-map.dts" interrupt_map
>> +
>>      check_tests "$SRCDIR/unit-addr-simple-bus-reg-mismatch.dts" simple_bus_reg
>>      check_tests "$SRCDIR/unit-addr-simple-bus-compatible.dts" simple_bus_reg
>>
>> --
>> 2.14.1
>>

Re: [RFC PATCH 1/2] checks: Validate interrupt-map properties

[André Przywara]

On 14/05/2020 08:06, David Gibson wrote:

Hi,

> On Wed, May 13, 2020 at 10:03:14PM -0500, Rob Herring wrote:
>> On Wed, May 13, 2020 at 11:35 AM Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> 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                    | 86 +++++++++++++++++++++++++++++++++++++++++++++
>>>  tests/bad-interrupt-map.dts | 21 +++++++++++
>>>  tests/run_tests.sh          |  2 ++
>>>  3 files changed, 109 insertions(+)
>>>  create mode 100644 tests/bad-interrupt-map.dts
>>>
>>> diff --git a/checks.c b/checks.c
>>> index 4b3c486..12518db 100644
>>> --- a/checks.c
>>> +++ b/checks.c
>>> @@ -924,6 +924,90 @@ 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, &pci_bridge);
>>>
>>> +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, "invalid length of interrupt-map");
>>
>> It's good to say what size you found and what was expected.
>>
>>> +               return;
>>> +       }
>>> +       cells = map->val.len / sizeof(cell_t);
>>> +
>>> +       prop = get_property(node, "#interrupt-cells");
>>> +       if (!prop) {
>>> +               FAIL(c, dti, node, "missing #interrupt-cells in nexus\n");
>>> +               return;
>>> +       }
>>> +       irq_cells = propval_cell(prop);
>>> +
>>> +       for (i = 0; i < cells;) {
>>> +               int phandle_idx = i + node_addr_cells(node) + irq_cells;
>>
>> IIRC, node_addr_cells() will give you a default if not found which is
>> not really what you want.
> 
> Using the default seems right to me.  We might want a warning in that
> case, but I don't think it belongs in this test.
> 
>>> +               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);
>>> +               /* Avoid the assert in get_node_by_phandle(). */
>>> +               if (intc_phandle != 0)
>>> +                       intc = get_node_by_phandle(dti->dt, intc_phandle);
> 
> This will always fail the check if the phandle is an unresolved
> reference, which is likely for an overlay dt.

I don't know much about overlays, but is this a valid use case for an
interrupt controller? Or don't we make any assumptions about what
devices can be unresolved?

And without that the assert in get_node_by_phandle() fires if the
phandle is 0 (which is a common case if the interrupt map is wrong). But
that will kill dtc, and not translate the DT. I don't think this is
desirable.

Any ideas what I could do instead?

>>> +               if (!intc) {
>>> +                       FAIL_PROP(c, dti, node, map,
>>> +                                 "invalid phandle for interrupt-map entry");
>>> +                       return;
>>> +               }
>>> +
>>> +               prop = get_property(intc, "interrupt-controller");
>>> +               if (!prop) {
>>> +                       FAIL_PROP(c,dti, node, map,
>>> +                                 "interrupt-map phandle does not point to interrupt controller");
>>
>> interrupt-map can point to another interrupt-map.
> 
> Right.
> 
>>> +                       return;
>>> +               }
>>> +
>>> +               prop = get_property(intc, "#address-cells");
>>> +               if (!prop) {
>>> +                       FAIL_PROP(c,dti, node, map,
>>> +                                 "interrupt-controller misses #address-cells property");
> 
> "is missing", or simply "missing" would be more normal english than
> "misses" (here and elsewhere).

Yeah, I was miserly over the 80 characters ;-)

>>> +                       /*
>>> +                        * Linux treats non-existing #address-cells in the
>>> +                        * interrupt parent as 0, and not 2, as the spec
>>> +                        * suggests. Deal with that, but print the warning,
>>> +                        * since we should have an explicit #a-c = 0 in the
>>> +                        * controller node in this case.
>>
>> IMO, we should not print a warning. Or make it separately enabled.
> 
> I tend to agree.  A separate check to warn for an interrupt controller
> (or nexus) without #address-cells seems like a good idea.

Yes, will do that.

Many thanks for having a look!

Cheers,
Andre

>>> +                        */
>>> +                       intc_addr_cells = 0;
>>> +               } else
>>> +                       intc_addr_cells = propval_cell(prop);
>>> +
>>> +               prop = get_property(intc, "#interrupt-cells");
>>> +               if (!prop) {
>>> +                       FAIL_PROP(c,dti, node, map,
>>> +                                 "interrupt-controller misses #interrupt-cells property");
>>> +                       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 simple_bus = {
>>>         .name = "simple-bus",
>>>  };
>>> @@ -1792,6 +1876,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 eccb85d..aec92fb 100755
>>> --- a/tests/run_tests.sh
>>> +++ b/tests/run_tests.sh
>>> @@ -732,6 +732,8 @@ dtc_tests () {
>>>      check_tests "$SRCDIR/pci-bridge-bad1.dts" pci_bridge
>>>      check_tests "$SRCDIR/pci-bridge-bad2.dts" pci_bridge
>>>
>>> +    check_tests "$SRCDIR/bad-interrupt-map.dts" interrupt_map
>>> +
>>>      check_tests "$SRCDIR/unit-addr-simple-bus-reg-mismatch.dts" simple_bus_reg
>>>      check_tests "$SRCDIR/unit-addr-simple-bus-compatible.dts" simple_bus_reg
>>>
>>>
>>
>